operator: upgrade all control plane nodes first #3444

3u13r · 2024-10-20T20:48:55Z

Context

Allow #3396. Since kubelets must not communicate with a KubeAPI Server that has an older version than the kubelet itself, we need to upgrade all control planes, before upgrading the worker nodes. Control plane nodes are configured in a way that they only talk to the local KubeAPI server that matches the kubelet version.

Proposed change(s)

Allow increasing the node budget: This is technically a full feature that I thought we already had and I needed it for the env test that tries to upgrade the worker nodes so I can verify that the control plane pending node is created but not the worker node. This should fail because of our explicit check and node the missing node budget.
Generally improve test coverage of the nodeversion env test since we are upgrading 2 nodes in different scaling groups now.
Don't call out to the cloud provider API to create new worker nodes if there are still control planes in the outdated or donor category, even if we would have enough node budget to do so.
Since the operator code gen make target didn't work for me, I bumped the version (this seemingly was needed so that this plays nicely with go workspaces), reverted the make target scripts back to their original form and manually copied over the newly generated file to the cli helm embedding (we might want to automate this in the future when we use bazel for all parts of the operator).

How to test:

bazel test --test_output=all --cache_test_results=no //operators/constellation-node-operator/controllers:controllers_test

Related issue

kubernetes/kubernetes#127316

Checklist

Run the E2E tests that are relevant to this PR's changes
- ~~gcp-snp, 3:2, v1.30.4 -> v1.31.1 K8s, v1.18.0 -> head of this PR: https://github.com/edgelesssys/constellation/actions/runs/11431369883~~
- gcp-snp, 3:2, v1.30.4 -> v1.31.1 K8s, v1.18.0 -> head of this PR, image to upgrade to: ref/euler-feat-operator-upgrade-control-planes-first/stream/console/v2.19.0-pre.0.20241021015702-d0e8a385bb2f: https://github.com/edgelesssys/constellation/actions/runs/11455280858
Add labels (e.g., for changelog category)

Link to Milestone

Only use feature flag on K8s versions that support that feature flag. Otherwise Constellation throws an error during init. Also fixup some tests.

This bump the controller gen version and also adjusts the generate commands (back to the original ones). This allows correct generation of CRDs and go code.

daniel-weisse · 2024-10-21T06:25:59Z

...ation/helm/charts/edgeless/operators/charts/constellation-operator/crds/nodeversion-crd.yaml

+              maxNodeBudget:
+                description: MaxNodeBudget is the maximum number of nodes that can
+                  be created simultaneously.
+                format: int32
+                type: integer


Is this problematic for upgrades, since we never upgrade the installed CRDs through helm?

No, I think helm has some upgrade magic build in regarding CRDs. But this time we only add one field that is optional, we are fine. Proof: https://github.com/edgelesssys/constellation/actions/runs/11455280858/job/31871905456.
Or do you have a concrete error in mind, I'm missing?

As far as I know, Helm simply doesn't do anything if a CRD already exists: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

So if we change the CRDs, running helm apply won't actually apply these changes.
From what I can tell, this effectively makes the maxNodeBudget option in the nodeversion crd non-functional.
I'm assuming everything still works fine because we default to 1 if the value is not set in the CR

Oh, thanks for the hint. Yes, the behavior is (hopefully) exactly the same. I think we are good for now then (also since the e2e test passed). I didn't want to advertise this feature anyway, but even if we do we should add this constraint.
In the future(tm), we likely want to do what others do (e.g., istio: https://istio.io/latest/docs/setup/upgrade/helm/#canary-upgrade-recommended).

operators/constellation-node-operator/controllers/nodeversion_controller.go

operators/constellation-node-operator/controllers/scalinggroup_controller.go

Before we call out ot the cloud provider we check if there are still control plane nodes that are outdated (or donors). If there are, we don't create any worker nodes, even if we have the budget to do so.

github-actions · 2024-10-22T11:06:46Z

Coverage report

Package	Old	New	Trend
bootstrapper/internal/kubernetes/k8sapi	13.10%	12.60%	↘️
internal/constellation/kubecmd	62.40%	24.90%	↘️
internal/versions	9.70%	8.70%	↘️
operators/constellation-node-operator	0.00%	0.00%	🚧
operators/constellation-node-operator/api/v1alpha1	0.00%	0.00%	🚧
operators/constellation-node-operator/controllers	30.80%	26.20%	↘️
operators/constellation-node-operator/internal/constants	[no test files]	[no test files]	🚧
operators/constellation-node-operator/internal/controlplane	100.00%	5.70%	↘️
operators/constellation-node-operator/internal/etcd	65.80%	9.70%	↘️
operators/constellation-node-operator/internal/node	100.00%	9.20%	↘️

burgerdev · 2024-10-23T09:17:36Z

operators/constellation-node-operator/internal/constants/constants.go

@@ -19,4 +19,6 @@ const (
 	PlaceholderControlPlaneScalingGroupName = "control-planes-id"
 	// PlaceholderWorkerScalingGroupName name of the worker scaling group used if upgrades are not yet supported.
 	PlaceholderWorkerScalingGroupName = "workers-id"
+	// ControlPlaneRoleLabel label used to identify control plane nodes.


Suggested change

// ControlPlaneRoleLabel label used to identify control plane nodes.

// ControlPlaneRoleLabel label used to identify control plane nodes.

// https://kubernetes.io/docs/reference/labels-annotations-taints/#node-role-kubernetes-io-control-plane

Nit, just to document that this is canonical.

burgerdev · 2024-10-23T09:24:07Z

operators/constellation-node-operator/controllers/nodeversion_controller_test.go

 	r.RLock()
 	defer r.RUnlock()


Does this need a write lock now?

burgerdev · 2024-10-23T10:41:21Z

operators/constellation-node-operator/api/v1alpha1/nodeversion_types.go

@@ -21,6 +21,8 @@ type NodeVersionSpec struct {
 	KubernetesComponentsReference string `json:"kubernetesComponentsReference,omitempty"`
 	// KubernetesClusterVersion is the advertised Kubernetes version of the cluster.
 	KubernetesClusterVersion string `json:"kubernetesClusterVersion,omitempty"`
+	// MaxNodeBudget is the maximum number of nodes that can be created simultaneously.
+	MaxNodeBudget uint32 `json:"maxNodeBudget,omitempty"`


I'm against making this a user-facing feature for now, because we did not discuss its semantics sufficiently. What if the user sets it to 1000 - do we replace all control-plane nodes at once? Should we have different budgets for control planes and for workers? Could this be relative? Should we rather implement a different upgrade algorithm (3533 etc)? I'm also reminded of the evolution of https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#rollingupdatedeployment-v1-apps.

Afaict this PR would be much smaller if we removed this feature and tried to find a different way to test it.

do we replace all control-plane nodes at once?

Yes, I also tested it setting the budget to 5 when I had 3:2 nodes. But "at-once" only applies to the world how the operator sees it. The join service still forbids multiple control-plane nodes joining at the same time. Note that this is the scenario right after initializing a Constellation with >=3 control planes. Also replace means, adding the nodes first. So in theory you could go from 3 control planes to 6 since the operator only removes a node once the hand over is finished.

Should we have different budgets for control planes and for workers?

We might do that in the future, if a customer requires it or we think we need it.

Could this be relative?

I assume as in a percentage value. Sure, but this is more difficult/complex then setting the number of nodes.

Should we rather implement a different upgrade algorithm

I think the bug is orthogonal to this proposal, since I'm not reworking the operator replacement algorithm, which would be a large undertaking in my opinion.

I'm also reminded of the evolution of https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#rollingupdatedeployment-v1-apps

I don't know the evolution, is there a summary somewhere of what happened?

Note that also the operator API is on version v1alpha1, so in my opinion, we don't have to provide any API stability guarantees between Constellation versions and we can completely change the whole upgrade process and APIs between Constellation versions.

Some of the current fields are not really user-facing in a sense that the user should directly use them. Changing the image reference requires 1. the image reference to be one of our images references 2. the measurements in the join config to match the image. Changing the k8s version requires a config map under that name and for the Constellation to upgrade correctly it has to contain the right set of components and patches.

Afaict this PR would be much smaller if we removed this feature and tried to find a different way to test it.

Then I'll have another try at the test for this, but this might take a bit of time before I get back to this.

3u13r added the no changelog Change won't be listed in release changelog label Oct 20, 2024

3u13r requested a review from derpsteb as a code owner October 20, 2024 20:48

3u13r requested a review from burgerdev October 20, 2024 20:49

3u13r force-pushed the euler/feat/operator/upgrade-control-planes-first branch from 39f28da to 5000cc9 Compare October 20, 2024 20:54

3u13r changed the title ~~Euler/feat/operator/upgrade control planes first~~ operator: upgrade all control plane node first Oct 20, 2024

3u13r force-pushed the euler/feat/operator/upgrade-control-planes-first branch from 5000cc9 to 14e650b Compare October 20, 2024 21:27

3u13r added 7 commits October 21, 2024 00:14

kubernetes: fixup feature flag

d5bb722

Only use feature flag on K8s versions that support that feature flag. Otherwise Constellation throws an error during init. Also fixup some tests.

operator: bump controller-gen version

01af37a

This bump the controller gen version and also adjusts the generate commands (back to the original ones). This allows correct generation of CRDs and go code.

operator: allow increasing number of node upgrade budget

15d7de5

operator: requeue scaling group on conflict

5a59d10

operator: clean up after joining node env test

49ef340

operator: move control plane label to constants

ce2e48c

operator: stub etcd remove calls in env tests

bf7fc6f

3u13r force-pushed the euler/feat/operator/upgrade-control-planes-first branch from 14e650b to 5fcf88f Compare October 20, 2024 22:14

3u13r requested a review from daniel-weisse as a code owner October 20, 2024 22:14

3u13r force-pushed the euler/feat/operator/upgrade-control-planes-first branch 2 times, most recently from a612103 to d0e8a38 Compare October 20, 2024 23:57

daniel-weisse reviewed Oct 21, 2024

View reviewed changes

3u13r force-pushed the euler/feat/operator/upgrade-control-planes-first branch from d0e8a38 to a8ec800 Compare October 22, 2024 09:37

3u13r added 2 commits October 22, 2024 12:47

operator: upgrade control plane nodes first

eecd971

Before we call out ot the cloud provider we check if there are still control plane nodes that are outdated (or donors). If there are, we don't create any worker nodes, even if we have the budget to do so.

operator: add test that tries to upgrade worker nodes first and fails

72f85f3

3u13r force-pushed the euler/feat/operator/upgrade-control-planes-first branch from a8ec800 to 72f85f3 Compare October 22, 2024 10:47

3u13r changed the title ~~operator: upgrade all control plane node first~~ operator: upgrade all control plane nodes first Oct 22, 2024

burgerdev reviewed Oct 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: upgrade all control plane nodes first #3444

operator: upgrade all control plane nodes first #3444

3u13r commented Oct 20, 2024 •

edited

Loading

daniel-weisse Oct 21, 2024

3u13r Oct 22, 2024

daniel-weisse Oct 22, 2024

3u13r Oct 22, 2024

github-actions bot commented Oct 22, 2024

burgerdev Oct 23, 2024

burgerdev Oct 23, 2024

burgerdev Oct 23, 2024

3u13r Oct 24, 2024

	// ControlPlaneRoleLabel label used to identify control plane nodes.
	// ControlPlaneRoleLabel label used to identify control plane nodes.
	// https://kubernetes.io/docs/reference/labels-annotations-taints/#node-role-kubernetes-io-control-plane

operator: upgrade all control plane nodes first #3444

Are you sure you want to change the base?

operator: upgrade all control plane nodes first #3444

Conversation

3u13r commented Oct 20, 2024 • edited Loading

Context

Proposed change(s)

Related issue

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2024

Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3u13r commented Oct 20, 2024 •

edited

Loading