Allow deprecated beta topology labels to be applied for those not ready to migrate #3685

JoelSpeed · 2023-04-03T11:28:12Z

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

#2653 removed the deprecated labels completely, however, other cloud providers have not yet removed these and there are a lot of people out there who are probably not ready to migrate. For example, the Azure Red Hat OpenShift offering has not announced removal of the labels and still supports the labels, even though Microsoft and AKS have removed them.

This PR introduces a softer change, allowing users to maintain the old behaviour should they wish to.

Commit wise, it does:

Reverts 2653 to put things back to how they were
Refactors the cloud node modifier code to clean that up ahead of later changes
Switch priority from beta labels to GA labels, so that GA labels become the source of truth
Adds --deprecated-apply-beta-topology-labels as a flag, off by default, that allows users to opt-in to the old behaviour

I think this should maintain the direction of the Azure provider, while allowing those who aren't yet ready to drop the labels, to continue using them until they have migrated.

My suggestion would be to keep these labels and this flag in-place until the wider cloud-provider community has sunset the labels completely and worked out the migration path for everyone.

Which issue(s) this PR fixes:

Fixes #2453

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The deprecated beta topology labels are no longer applied by default, to maintain the legacy behaviour use `--deprecated-apply-beta-topology-labels`

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify · 2023-04-03T11:28:19Z

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

Name	Link
🔨 Latest commit	`dba7928`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/64672f6c2d8a0700082a8f7c

coveralls · 2023-04-03T11:32:30Z

Coverage: 79.692% (+0.1%) from 79.589% when pulling 5416552 on JoelSpeed:optional-deprecated-labels into 6a378e6 on kubernetes-sigs:master.

bridgetkromhout · 2023-04-03T14:15:55Z

/retest

feiskyer · 2023-04-04T05:09:42Z

cmd/cloud-node-manager/app/config/config.go

+	// DeprecatedApplyBetaTopologyLabels indicates whether the node should apply beta topology labels.
+	// If true, the node will apply beta topology labels.
+	// DEPRECATED: This flag will be removed in a future release.
+	DeprecatedApplyBetaTopologyLabels bool


I don't think we should change the behavior for the versions that have already been released, it may break some existing v1.26+ clusters.

Could we change this to something like "EnableDepecatedBetaTopologyLabels" and default it to false?

I don't think we should change the behavior for the versions that have already been released, it may break some existing v1.26+ clusters.

Agreed. The current behaviour is that the behaviour stays the same as master now, unless you opt in, ie the flag defaults to false and we only apply the labels when the flag is explicitly specified

Could we change this to something like "EnableDepecatedBetaTopologyLabels" and default it to false?

I can go through and rename everything, sure

feiskyer · 2023-04-04T05:11:05Z

pkg/nodemanager/nodemanager.go

 	}

 	return nodeModifiers, nil
 }

+// addCloudNodeLabel creates a nodeModifier that adds a label to a node.
+func addCloudNodeLabel(key, value string) func(*v1.Node) {


Thanks for the refactoring, it looks much better!

JoelSpeed · 2023-04-04T08:56:13Z

Renamed as suggested, and as per my comment, this is off by default to persist 1.26 behaviour anyway, so should be good to go if we are happy with the new naming

JoelSpeed · 2023-04-12T10:46:23Z

/retest

@feiskyer Are you happy with the new naming? Any further feedback or can we get this merged?

nilo19 · 2023-04-22T14:18:05Z

Can the user who wants to use these labels continue using the old versions of cloud provider azure, and change their app behavior before upgrading to the newer version?

nilo19 · 2023-04-22T14:19:07Z

The kubernetes has decided to deprecate these labels so I think it would be better if we keep aligned with the kubernetes upstream. By the way, if we want to merge this, don't forget to squash the commits.

JoelSpeed · 2023-04-24T09:29:16Z

Can the user who wants to use these labels continue using the old versions of cloud provider azure, and change their app behavior before upgrading to the newer version?

IMO this is undesirable since it blocks users from upgrading. Imagine I am running a platform for my large company, I want to keep Kubernetes up to date so need the latest CCM. I have 1000s of developers running applications on there, it will likely take some time to get all of their applications migrated.
Yes, some will have started this process, I suspect many will not have done.

The kubernetes has decided to deprecate these labels so I think it would be better if we keep aligned with the kubernetes upstream. By the way, if we want to merge this, don't forget to squash the commits.

Other providers that use the core logic still sync beta to stable, they have deprecated the label but are far from removing the sync logic from their providers as far as I understand, Azure is way ahead here in this regard.

I had kept the commits as separate logical commits but can squash, (or maybe tide can?) if you prefer it that way, LMK

JoelSpeed · 2023-05-09T10:44:28Z

@nilo19 Would you like me to squash this so we can merge? Or can we use the tide squash method?

JoelSpeed · 2023-05-19T07:19:35Z

@nilo19 any further comments? Hoping to get this merged soon

feiskyer · 2023-05-19T08:00:45Z

@JoelSpeed sorry for the delay. Could you squash the commits?

…dy to migrate This reverts commit 67c2407. It then introduces a new flag to allow users to opt-in to syncing older, beta topology labels based on the presence of the GA topolgoy labels.

JoelSpeed · 2023-05-19T08:12:43Z

@JoelSpeed sorry for the delay. Could you squash the commits?

Sure, just pushed a squashed commit and rebased on the latest master

feiskyer · 2023-05-20T02:59:19Z

/lgtm
/approve

k8s-ci-robot · 2023-05-20T02:59:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, JoelSpeed

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [feiskyer]

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

feiskyer · 2023-05-22T00:32:58Z

/retest

nilo19 · 2023-06-06T00:28:57Z

/cherrypick release-1.26

nilo19 · 2023-06-06T00:29:02Z

/cherrypick release-1.27

k8s-infra-cherrypick-robot · 2023-06-06T00:30:00Z

@nilo19: new pull request created: #4040

In response to this:

/cherrypick release-1.26

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/test-infra repository.

k8s-infra-cherrypick-robot · 2023-06-06T00:30:02Z

@nilo19: new pull request created: #4041

In response to this:

/cherrypick release-1.27

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/test-infra repository.

…e-manager, not cloud-controller-manager gardener#716 added the flag to cloud-controller-manager. This fails with: ``` Error: unknown flag: --deprecated-apply-beta-topology-labels ``` The flag is supported for cloud-node-manager. See kubernetes-sigs/cloud-provider-azure#3685.

…e-manager, not cloud-controller-manager (#717) #716 added the flag to cloud-controller-manager. This fails with: ``` Error: unknown flag: --deprecated-apply-beta-topology-labels ``` The flag is supported for cloud-node-manager. See kubernetes-sigs/cloud-provider-azure#3685.

…e-manager, not cloud-controller-manager (gardener#717) gardener#716 added the flag to cloud-controller-manager. This fails with: ``` Error: unknown flag: --deprecated-apply-beta-topology-labels ``` The flag is supported for cloud-node-manager. See kubernetes-sigs/cloud-provider-azure#3685.

* Configure CCM to keep setting deprecated labes on the nodes (#716) * Pass the `--enable-deprecated-beta-topology-labels` flag to cloud-node-manager, not cloud-controller-manager (#717) #716 added the flag to cloud-controller-manager. This fails with: ``` Error: unknown flag: --deprecated-apply-beta-topology-labels ``` The flag is supported for cloud-node-manager. See kubernetes-sigs/cloud-provider-azure#3685. --------- Co-authored-by: Vladimir Nachev <vladimir.nachev@sap.com> Co-authored-by: Ismail Alidzhikov <i.alidjikov@gmail.com>

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 3, 2023

k8s-ci-robot requested review from jwtty and MartinForReal April 3, 2023 11:28

feiskyer reviewed Apr 4, 2023

View reviewed changes

JoelSpeed force-pushed the optional-deprecated-labels branch from ab4089e to 5416552 Compare April 4, 2023 08:55

Allow deprecated beta topology labels to be applied for those not rea…

dba7928

…dy to migrate This reverts commit 67c2407. It then introduces a new flag to allow users to opt-in to syncing older, beta topology labels based on the presence of the GA topolgoy labels.

JoelSpeed force-pushed the optional-deprecated-labels branch from 5416552 to dba7928 Compare May 19, 2023 08:12

k8s-ci-robot assigned feiskyer May 20, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2023

k8s-ci-robot merged commit bf80827 into kubernetes-sigs:master May 22, 2023
20 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Jun 6, 2023

[release-1.26] Allow deprecated beta topology labels to be applied for those not ready to migrate #4040

Merged

k8s-infra-cherrypick-robot mentioned this pull request Jun 6, 2023

[release-1.27] Allow deprecated beta topology labels to be applied for those not ready to migrate #4041

Merged

This was referenced Aug 4, 2023

Automated cherry pick of #115391: allow to mutate pv nodeaffinity label key kubernetes/kubernetes#119756

Closed

Configure CCM to keep setting deprecated labes on the nodes gardener/gardener-extension-provider-azure#716

Merged

ialidzhikov mentioned this pull request Aug 4, 2023

Pass the --enable-deprecated-beta-topology-labels flag to cloud-node-manager, not cloud-controller-manager gardener/gardener-extension-provider-azure#717

Merged

kon-angelo mentioned this pull request Mar 27, 2024

increase period of deprecated node labels until k8s v1.29 gardener/gardener-extension-provider-azure#811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow deprecated beta topology labels to be applied for those not ready to migrate #3685

Allow deprecated beta topology labels to be applied for those not ready to migrate #3685

JoelSpeed commented Apr 3, 2023

netlify bot commented Apr 3, 2023 •

edited

coveralls commented Apr 3, 2023 •

edited

bridgetkromhout commented Apr 3, 2023

feiskyer Apr 4, 2023

JoelSpeed Apr 4, 2023

feiskyer Apr 4, 2023

JoelSpeed commented Apr 4, 2023

JoelSpeed commented Apr 12, 2023

nilo19 commented Apr 22, 2023

nilo19 commented Apr 22, 2023

JoelSpeed commented Apr 24, 2023

JoelSpeed commented May 9, 2023

JoelSpeed commented May 19, 2023

feiskyer commented May 19, 2023

JoelSpeed commented May 19, 2023

feiskyer commented May 20, 2023

k8s-ci-robot commented May 20, 2023

feiskyer commented May 22, 2023

nilo19 commented Jun 6, 2023

nilo19 commented Jun 6, 2023

k8s-infra-cherrypick-robot commented Jun 6, 2023

k8s-infra-cherrypick-robot commented Jun 6, 2023

Allow deprecated beta topology labels to be applied for those not ready to migrate #3685

Allow deprecated beta topology labels to be applied for those not ready to migrate #3685

Conversation

JoelSpeed commented Apr 3, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify bot commented Apr 3, 2023 • edited

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

coveralls commented Apr 3, 2023 • edited

bridgetkromhout commented Apr 3, 2023

feiskyer Apr 4, 2023

Choose a reason for hiding this comment

JoelSpeed Apr 4, 2023

Choose a reason for hiding this comment

feiskyer Apr 4, 2023

Choose a reason for hiding this comment

JoelSpeed commented Apr 4, 2023

JoelSpeed commented Apr 12, 2023

nilo19 commented Apr 22, 2023

nilo19 commented Apr 22, 2023

JoelSpeed commented Apr 24, 2023

JoelSpeed commented May 9, 2023

JoelSpeed commented May 19, 2023

feiskyer commented May 19, 2023

JoelSpeed commented May 19, 2023

feiskyer commented May 20, 2023

k8s-ci-robot commented May 20, 2023

feiskyer commented May 22, 2023

nilo19 commented Jun 6, 2023

nilo19 commented Jun 6, 2023

k8s-infra-cherrypick-robot commented Jun 6, 2023

k8s-infra-cherrypick-robot commented Jun 6, 2023

netlify bot commented Apr 3, 2023 •

edited

coveralls commented Apr 3, 2023 •

edited