Only support default partition in server cluster #721

thisisnotashwin · 2021-09-10T20:48:45Z

Changes proposed in this PR:

Fail if the user attempts to update the admin partition name on the server cluster to a value that isn't "default"
Create an immutable configmap with the partition name which will cause helm upgrade to fail if the admin partition name is updated between the install and the upgrade.

Questions:
Even though the helm upgrade fails when the partition name is changed, the partition-init job runs and the agents get reassigned. I could make the partition-init job a pre-install job and not a pre-upgrade job, BUT the drawback of doing so would be anyone trying to upgrade an existing non-partition cluster to a partitioned cluster cannot. Should we ignore this use case? Ignore the upgrade use-case will ensure that the partition-init job will not run thereby not creating a new partition.

FOLLOW UP:
After conversations with the team, it was decided that in the near term, we will not support updating to a partition that is non-default. Removing the pre-upgrade hook from partition-init job. This will ensure we don't create a partition when a user errantly updates the partition-name and performs a helm upgrade.

How I've tested this PR:
Manual testing.
Bats tests

How I expect reviewers to test this PR:
Code review
Would love to hear thoughts on the above questions.

Checklist:

Tests added

In almost all cases, users want to set bootstrapExpect to the number of server replicas. This change defaults it to null in values.yaml and then in the template if it's left as null, then we set the -bootstrap-expect flag to the number of server replicas. This is backwards compatible, if users have been setting this it will continue to be set. Also error out if bootstrapExpect is less than the number of replicas because this is definitely a misconfiguration as the servers won't wait until the proper number have started before electing a leader.

kschoche · 2021-09-14T19:05:53Z

charts/consul/test/unit/partition-name-configmap.bats

+  cd `chart_dir`
+  assert_empty helm template \
+      -s templates/partition-init-role.yaml  \
+      --set 'global.adminPartitions.enabled=true' \


I think the description or the test is wrong here on whether or not adminPartitions.enabled is true or false!

Good catch!

charts/consul/test/unit/partition-name-configmap.bats

kschoche · 2021-09-14T19:07:25Z

charts/consul/test/unit/client-daemonset.bats

+      --set 'global.adminPartitions.name=test' \
+      .
+
+  [ "$status" -eq 1 ]


I like this test!

kschoche

Great work.
I left a couple comments before you merge but otherwise it's good to go!
On a meta note : I wish there was a better way to make the $serverEnabled := easier to read, thx Helm!

"default"

lkysow

The docs should be updated for the values to state that:

Only default is allowed for server cluster.
Can't change the partition name
Only supported on initial installation, can't enable partitions after initial installation.

lkysow · 2021-09-14T20:09:03Z

charts/consul/templates/client-daemonset.yaml

@@ -1,5 +1,7 @@
 {{- if (or (and (ne (.Values.client.enabled | toString) "-") .Values.client.enabled) (and (eq (.Values.client.enabled | toString) "-") .Values.global.enabled)) }}
 {{- if (and (and .Values.global.tls.enabled .Values.global.tls.httpsOnly) (and .Values.global.metrics.enabled .Values.global.metrics.enableAgentMetrics))}}{{ fail "global.metrics.enableAgentMetrics cannot be enabled if TLS (HTTPS only) is enabled" }}{{ end -}}
+{{- $serverEnabled := (or (and (ne (.Values.server.enabled | toString) "-") .Values.server.enabled) (and (eq (.Values.server.enabled | toString) "-") .Values.global.enabled)) -}}
+{{- if (and .Values.global.adminPartitions.enabled $serverEnabled (ne (.Values.global.adminPartitions.name | toString) "default"))}}{{ fail "global.adminPartitions.name has to be \"default\" in the server cluster" }}{{ end -}}


Suggested change

{{- if (and .Values.global.adminPartitions.enabled $serverEnabled (ne (.Values.global.adminPartitions.name | toString) "default"))}}{{ fail "global.adminPartitions.name has to be \"default\" in the server cluster" }}{{ end -}}

{{- if (and .Values.global.adminPartitions.enabled $serverEnabled (ne .Values.global.adminPartitions.name "default"))}}{{ fail "global.adminPartitions.name has to be \"default\" in the server cluster" }}{{ end -}}

Does it need toString?

lkysow · 2021-09-14T20:10:58Z

charts/consul/templates/partition-name-configmap.yaml

+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ template "consul.fullname" . }}-partition-configmap


Can we drop the configmap suffix since we don't need the type as it will be implied by the kind of the resource.

charts/consul/test/unit/partition-name-configmap.bats

lkysow · 2021-09-14T20:14:56Z

charts/consul/values.yaml

+    # The name of the Admin Partition. Must be "default" in the server cluster ie the Kubernetes cluster that
+    # the Consul server pods are deployed onto.
+    name: "default"


Should note that:

Can be something other than default in "non-server" clusters

Can't be changed after initial installation. If they wish to change it they need to uninstall Consul.

- This does not cause the parition-init job from running or the agents from attempting to join the new partition, but the helm upgrade stays in a failed state until an upgrade is run with the partition name reverted.

* Fail if agents in server cluster are created in a partition not "default" * Fail helm upgrades if partition name is updated. - This does not cause the parition-init job from running or the agents from attempting to join the new partition, but the helm upgrade stays in a failed state until an upgrade is run with the partition name reverted.

thisisnotashwin requested review from lkysow, a team, sadjamz and kschoche and removed request for a team and sadjamz September 10, 2021 20:48

thisisnotashwin force-pushed the default-partitions branch from 74a17a0 to 5a882a8 Compare September 14, 2021 14:32

thisisnotashwin force-pushed the partitions branch from 8073aee to 5ac36a8 Compare September 14, 2021 18:30

kschoche reviewed Sep 14, 2021

View reviewed changes

charts/consul/test/unit/partition-name-configmap.bats Show resolved Hide resolved

kschoche reviewed Sep 14, 2021

View reviewed changes

kschoche approved these changes Sep 14, 2021

View reviewed changes

thisisnotashwin changed the base branch from partitions to main September 14, 2021 20:32

thisisnotashwin changed the base branch from main to partitions September 14, 2021 20:32

thisisnotashwin force-pushed the partitions branch from 5ac36a8 to 30126ae Compare September 14, 2021 20:37

Fail if agents in server cluster are created in a partition not

8910fd3

"default"

thisisnotashwin force-pushed the default-partitions branch 2 times, most recently from 171379f to 7bb77a2 Compare September 14, 2021 21:05

lkysow approved these changes Sep 14, 2021

View reviewed changes

Fail helm upgrades if partition name is updated.

112ef52

- This does not cause the parition-init job from running or the agents from attempting to join the new partition, but the helm upgrade stays in a failed state until an upgrade is run with the partition name reverted.

thisisnotashwin force-pushed the default-partitions branch from 7bb77a2 to 112ef52 Compare September 14, 2021 23:07

thisisnotashwin merged commit ea23808 into partitions Sep 14, 2021

thisisnotashwin deleted the default-partitions branch September 14, 2021 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only support default partition in server cluster #721

Only support default partition in server cluster #721

thisisnotashwin commented Sep 10, 2021 •

edited

Loading

kschoche Sep 14, 2021

thisisnotashwin Sep 14, 2021

kschoche Sep 14, 2021

kschoche left a comment

lkysow left a comment

lkysow Sep 14, 2021

lkysow Sep 14, 2021

lkysow Sep 14, 2021

	{{- if (and .Values.global.adminPartitions.enabled $serverEnabled (ne (.Values.global.adminPartitions.name \| toString) "default"))}}{{ fail "global.adminPartitions.name has to be \"default\" in the server cluster" }}{{ end -}}
	{{- if (and .Values.global.adminPartitions.enabled $serverEnabled (ne .Values.global.adminPartitions.name "default"))}}{{ fail "global.adminPartitions.name has to be \"default\" in the server cluster" }}{{ end -}}

Only support default partition in server cluster #721

Only support default partition in server cluster #721

Conversation

thisisnotashwin commented Sep 10, 2021 • edited Loading

kschoche Sep 14, 2021

Choose a reason for hiding this comment

thisisnotashwin Sep 14, 2021

Choose a reason for hiding this comment

kschoche Sep 14, 2021

Choose a reason for hiding this comment

kschoche left a comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

lkysow Sep 14, 2021

Choose a reason for hiding this comment

lkysow Sep 14, 2021

Choose a reason for hiding this comment

lkysow Sep 14, 2021

Choose a reason for hiding this comment

thisisnotashwin commented Sep 10, 2021 •

edited

Loading