Fix version upgrade kubelet support #16932

rsafonseca · 2024-11-04T16:07:58Z

Automatically preserve kubelet supported version skew on worker nodes while control plane is being updated

…, while control plane is being updated

k8s-ci-robot · 2024-11-04T16:08:08Z

Hi @rsafonseca. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-11-04T16:08:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johngmyers for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rsafonseca · 2024-11-04T16:09:18Z

cc @danports @hakman @justinsb as discussed in the office hours
I've only had time to do some basic testing, but haven't gone through an actual upgrade, though the outputs look as expected :)

danports · 2024-11-04T17:26:40Z

Thanks @rsafonseca! On the one hand, I can't say I'm the biggest fan of the ergonomics of this approach, because it basically means we have to run the identical kops update cluster and kops rolling-update cluster commands twice in our CD script - and not having a command line switch like --instance-group-role makes the script less self-documenting. But on the other hand, I am very happy to have any solution that works and this also has the advantage of being safe by default in terms of automatically enforcing the version skew policy. An "uber command" as @rifelpet called it would still be great, and while I haven't looked at all at the code, I wondered whether one could implement it by making a rolling update (of CP or worker nodes) just another type of task that needs to be run in the update dependency tree.

rsafonseca · 2024-11-04T18:22:21Z

Yeah, it's not perfect for all use cases, but it's a first step and we can build upon this for the so called "uber" command :)
Like you said, it's safe by default, but the user can override this behaviour to keep the old one if they so wish by using the --ignore-version-skew command line switch.

In addition, this will produce no behaviour change for any kops update cluster other than specifically when the kubernetes version is being increased (e.g. kops updates will still procede normally and will update things like the containerRuntime, etc without the need for 2 interactions)

It also allows the instancegroups to continue to be managed while the controlplane isn't updated. E.g. if i need to make a change to an instancegroup but haven't finished rolling out all control planes.

As for the CD script, here I'm probably the least impacted as we're running a kops update loop on all clusters to reconcile with CRDs from our operator, so this change will (in our scenario) make the upgrades seamless and safe without having to change anything :)

Edit: Optionally an easy way to maintain behaviour in current CD workflows, is to simply ensure api-server nodes are rolled out first in the rolling-update command, after the masters have rolled out, before moving on to the other instance groups trigger an update cluster :)

justinsb · 2024-11-05T02:29:50Z

cmd/kops/update_cluster.go

+		if err != nil {
+			klog.Warningf("error checking control plane running verion: %v", err)
+		} else {
+			klog.Warningf("successfully checked control plane running version: %v", minControlPlaneRunningVersion)


Nit: any reason to make this a warning (as opposed to Infof)?

oh no... that was just for my testing i fully intended to make it info or debug and then forgot :)

justinsb · 2024-11-05T02:30:41Z

cmd/kops/update_cluster.go

+	if !c.IgnoreVersionSkew {
+		minControlPlaneRunningVersion, err = checkControlPlaneRunningVersion(ctx, cluster.ObjectMeta.Name, minControlPlaneRunningVersion)
+		if err != nil {
+			klog.Warningf("error checking control plane running verion: %v", err)


I think we should just return the error, unless there's some reason to expect this error to happen which we need to tolerate

Yes, it will error out for example if the cluster isn't up (e.g. new clusters, testing, etc) in which case we need to tolerate it.

justinsb · 2024-11-05T02:31:45Z

cmd/kops/update_cluster.go

+
+func checkControlPlaneRunningVersion(ctx context.Context, clusterName string, version string) (string, error) {
+
+	configLoadingRules := clientcmd.NewDefaultClientConfigLoadingRules()


Nit: I think we should try to share this "get kubeconfig" code with the rolling-update logic (I think that also connects to the cluster). Not a blocker though.

justinsb · 2024-11-05T02:33:26Z

cmd/kops/update_cluster.go

+		LabelSelector: "node-role.kubernetes.io/control-plane",
+	})
+	if err != nil {
+		return version, fmt.Errorf("cannot list nodes in cluster %q: %v", clusterName, err)


Ah - this is tricky. If the control plane is offline, we presumably don't want to update the nodes (?)

If the control plane is offline, this is the least of our worries lol. But it might be an expected situation (e.g. creating a new cluster) so we just log the error and continue as normal

justinsb · 2024-11-05T02:35:48Z

pkg/nodemodel/fileassets.go

+		return err
+	}
+	if len(assetBuilder.KubeletSupportedVersion) > 0 && assetBuilder.KubeletSupportedVersion != c.Cluster.Spec.KubernetesVersion {
+		c.AssetsSupportedKubelet, err = c.addFileAssets(assetBuilder, assetBuilder.KubeletSupportedVersion)


Ah - great point - we need different assets. Although I'm starting to worry about complexity now...

Yes, the complexity here worried me a bit as well as I dug a bit into how to achieve this. The code isn't perfect but i tried to do it without having to refactor many things, if you have any better ideas we can either improve this down the line or i can make any adjustments :)

justinsb · 2024-11-05T02:40:18Z

Thanks for this! A few random comments on the code but I'm going to mull on the overall approach.

k8s-ci-robot · 2024-11-09T22:06:28Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

justinsb · 2024-11-12T18:31:38Z

So we have added the ability to update only some of the instance groups, and in #16939 I am proposing to make it easy to drive those operations in a predefined sequence.

One thing I don't like about that approach is that we do have to predefine the sequence, and one of the things I like in this PR is that we're being more reasoned about the sequence: rather than hard-coding the sequence we effectively infer the sequence at run-time.

It might be that the predefined approach is good enough to unblock 1.31, but I would like to pursue this direction more. I am wondering if we should make it more explicit, adding a field for the Kube version on each instance groups and managing that.

danports · 2024-11-13T03:00:30Z

@justinsb Could we combine the two approaches by having the anneal/reconcile cluster command call update & rolling-update repeatedly, until the update command reports that there are no tasks left that it had to skip due to version skew or other dependencies? Then the anneal/reconcile command wouldn't have to hard-code any sequences, and that structure might also be able to incorporate running the --prune logic as well.

Automatically preserve kubelet supported version skew on worker nodes…

a454adb

…, while control plane is being updated

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2024

k8s-ci-robot requested review from hakman and johngmyers November 4, 2024 16:08

update cli-docs

fdc76a7

k8s-ci-robot added the area/documentation label Nov 4, 2024

justinsb reviewed Nov 5, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix version upgrade kubelet support #16932

Fix version upgrade kubelet support #16932

rsafonseca commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

rsafonseca commented Nov 4, 2024

danports commented Nov 4, 2024

rsafonseca commented Nov 4, 2024 •

edited

Loading

justinsb Nov 5, 2024

rsafonseca Nov 5, 2024

justinsb Nov 5, 2024

rsafonseca Nov 5, 2024

justinsb Nov 5, 2024

justinsb Nov 5, 2024

rsafonseca Nov 5, 2024

justinsb Nov 5, 2024

rsafonseca Nov 5, 2024

justinsb commented Nov 5, 2024

k8s-ci-robot commented Nov 9, 2024

justinsb commented Nov 12, 2024

danports commented Nov 13, 2024


		func checkControlPlaneRunningVersion(ctx context.Context, clusterName string, version string) (string, error) {

		configLoadingRules := clientcmd.NewDefaultClientConfigLoadingRules()

Fix version upgrade kubelet support #16932

Are you sure you want to change the base?

Fix version upgrade kubelet support #16932

Conversation

rsafonseca commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

rsafonseca commented Nov 4, 2024

danports commented Nov 4, 2024

rsafonseca commented Nov 4, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

justinsb commented Nov 5, 2024

k8s-ci-robot commented Nov 9, 2024

justinsb commented Nov 12, 2024

danports commented Nov 13, 2024

rsafonseca commented Nov 4, 2024 •

edited

Loading