✨ Propagate labels and annotations from ClusterClass and Cluster topology to KCP and MD #7088

MaxFedotov · 2022-08-18T14:26:28Z

What this PR does / why we need it:
It is possible to specify labels and annotations in cluster topology spec, but they are propagated only to Machines created by MachineDeployment and KubeadmControlPlane. This prevents users from setting annotations for cluster-autoscaler in Cluster topology spec.

This PR would allow to propagate labels and annotations from ClusterClass and Cluster topology spec not only to Machines, but also to KubeadmControlPlane and MachineDeployments

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #7006

linux-foundation-easycla · 2022-08-18T14:26:32Z

The committers listed above are authorized under a signed CLA.

✅ login: MaxFedotov / name: Max Fedotov (66aacfb)

k8s-ci-robot · 2022-08-18T14:26:35Z

@MaxFedotov: This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-ci-robot · 2022-08-18T14:26:36Z

Welcome @MaxFedotov!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-08-18T14:26:37Z

Hi @MaxFedotov. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/test-infra repository.

k8s-ci-robot · 2022-08-18T14:26:40Z

[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 neolit123 for approval by writing /assign @neolit123 in a comment. 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

killianmuldoon

/ok-to-test

…gy to KCP and MD

MaxFedotov · 2022-08-25T18:07:10Z

@killianmuldoon, hi! Is there anything I need to do in order to proceed with this PR?
Thanks!

killianmuldoon · 2022-08-29T09:33:49Z

I think you just need some reviews now to see if people are happy with the implementation and direction.

sbueringer · 2022-08-31T17:35:42Z

@MaxFedotov I'll take a look soon

fabriziopandini

@MaxFedotov thanks for pushing forward this work, this is a great start!
I will try to figure out the second part of this effort so we are sure that with this change we are not creating a situation that creates problems to the users

fabriziopandini · 2022-09-08T10:09:18Z

api/v1beta1/cluster_types.go

@@ -101,7 +101,7 @@ type Topology struct {

 // ControlPlaneTopology specifies the parameters for the control plane nodes in the cluster.
 type ControlPlaneTopology struct {
-	// Metadata is the metadata applied to the machines of the ControlPlane.
+	// Metadata is the metadata applied to ControlPlane and the machines of the ControlPlane.


I'm still mulling about this, but if we are going to propagate labels/annotations to objects down the chain I think we should propagate labels/annotations to bootstrap and infrastructure template as well; this implies also keeping such labels and annotations in sync (TBD how given that this should work no matter is using a cluster class or not)

do you consider it a part of this PR, or is it better to create a separate issue\PR to track this activity?

fabriziopandini · 2022-09-08T10:17:46Z

internal/controllers/topology/cluster/desired_state.go

+	machineLabels := mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels)
+	machineAnnotations := mergeMap(topologyMetadata.Annotations, clusterClassMetadata.Annotations)


nit: should this be controlPlaneLabels and controlPlaneAnnotations, so we can compute machineLabels/machineAnnotations down in the for loop?

q: should we apply the cluster label and the topology-owned label to the CP as well (I don't remember if this is already done somewhere else)

Will rename it.

Regarding labels - they were added there:

cluster-api/internal/controllers/topology/cluster/desired_state.go

Lines 733 to 740 in 66aacfb

func templateToObject(in templateToInput) (*unstructured.Unstructured, error) {

// NOTE: The cluster label is added at creation time so this object could be read by the ClusterTopology

// controller immediately after creation, even before other controllers are going to add the label (if missing).

labels := map[string]string{}

labels[clusterv1.ClusterLabelName] = in.cluster.Name

labels[clusterv1.ClusterTopologyOwnedLabel] = ""

templateLabels := mergeMap(labels, in.labels)

But I will move all labels-related code to computeControlPlane func, to keep all in one place

fabriziopandini · 2022-09-08T11:36:17Z

internal/controllers/topology/cluster/desired_state.go

+	machineLabels := mergeMap(machineDeploymentTopology.Metadata.Labels, machineDeploymentBlueprint.Metadata.Labels)
+	machineAnnotations := mergeMap(machineDeploymentTopology.Metadata.Annotations, machineDeploymentBlueprint.Metadata.Annotations)


similar comments as above: machineDeploymentLabels/annotations + should we add the cluster and topology-owner annotation?

Will rename it too.

All code, related to cluster and topology-owned annotations is a few lines down, together with selectors:

cluster-api/internal/controllers/topology/cluster/desired_state.go

Lines 566 to 594 in 66aacfb

// Apply Labels

// NOTE: On top of all the labels applied to managed objects we are applying the ClusterTopologyMachineDeploymentLabel

// keeping track of the MachineDeployment name from the Topology; this will be used to identify the object in next reconcile loops.

labels := map[string]string{}

labels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name

labels[clusterv1.ClusterTopologyOwnedLabel] = ""

labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name

desiredMachineDeploymentObj.SetLabels(mergeMap(labels, machineLabels))

// Apply Annotations

desiredMachineDeploymentObj.SetAnnotations(machineAnnotations)

// Set the selector with the subset of labels identifying controlled machines.

// NOTE: this prevents the web hook to add cluster.x-k8s.io/deployment-name label, that is

// redundant for managed MachineDeployments given that we already have topology.cluster.x-k8s.io/deployment-name.

desiredMachineDeploymentObj.Spec.Selector.MatchLabels = map[string]string{}

desiredMachineDeploymentObj.Spec.Selector.MatchLabels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name

desiredMachineDeploymentObj.Spec.Selector.MatchLabels[clusterv1.ClusterTopologyOwnedLabel] = ""

desiredMachineDeploymentObj.Spec.Selector.MatchLabels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name

// Also set the labels in .spec.template.labels so that they are propagated to

// MachineSet.labels and MachineSet.spec.template.labels and thus to Machine.labels.

// Note: the labels in MachineSet are used to properly cleanup templates when the MachineSet is deleted.

if desiredMachineDeploymentObj.Spec.Template.Labels == nil {

desiredMachineDeploymentObj.Spec.Template.Labels = map[string]string{}

}

desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name

desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterTopologyOwnedLabel] = ""

desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name

fabriziopandini · 2022-09-08T11:39:45Z

internal/controllers/topology/cluster/desired_state.go

@@ -724,14 +737,17 @@ func templateToObject(in templateToInput) (*unstructured.Unstructured, error) {
 	labels[clusterv1.ClusterLabelName] = in.cluster.Name
 	labels[clusterv1.ClusterTopologyOwnedLabel] = ""

+	templateLabels := mergeMap(labels, in.labels)


Given that we are accepting labels and annotations in input, I think we should drop adding labels in this func so there will be a unique point where we define all the labels for the new objects

k8s-ci-robot · 2022-09-09T17:19:20Z

@MaxFedotov: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-test-main	`8de4049`	link	true	`/test pull-cluster-api-test-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot · 2022-09-29T02:28:00Z

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

mnaser · 2022-11-16T14:08:16Z

@MaxFedotov do you have sometime to push this through? this is pretty important in our case for autoscaling :)

sbueringer · 2022-11-16T14:11:06Z

We are currently waiting on the corresponding proposal: #7331

I would also like to see this merged, but we have to merge / get consensus on the proposal first.

ykakarap

@MaxFedotov the proposal is now merged. Proposal here.
The proposal contains all the details for the questions on this PR.

Do you have bandwidth to continue working on this?
I am happy to pick up the pending work here, in case you do not have bandwidth 🙂

ykakarap · 2022-12-08T01:20:29Z

internal/controllers/topology/cluster/desired_state.go

-	labels := map[string]string{}
-	labels[clusterv1.ClusterLabelName] = in.cluster.Name
-	labels[clusterv1.ClusterTopologyOwnedLabel] = ""
-


How about keeping this and doing a mergeMap with in.Labels and using that as in the GenerateTemplate function. This way we wont have to duplicate this everywhere templateToObjects is used?

@MaxFedotov the proposal is now merged. Proposal here. The proposal contains all the details for the questions on this PR.

Do you have bandwidth to continue working on this? I am happy to pick up the pending work here, in case you do not have bandwidth 🙂

Hi @ykakarap! I would be grateful if you take this on. Unfortunately i don't have free time right now to continue with it :(

ykakarap · 2022-12-09T01:23:37Z

Hi @ykakarap! I would be grateful if you take this on. Unfortunately i don't have free time right now to continue with it :(

No problem.Thanks for letting me know. I will take it up from here. :)

ykakarap · 2023-01-13T05:14:20Z

As per #7088 (comment) opened a PR #7917 to continue this work.

sbueringer · 2023-01-19T14:38:32Z

/close
in favor of #7917

Thx @ykakarap for picking this up!

k8s-ci-robot · 2023-01-19T14:38:38Z

@sbueringer: Closed this PR.

In response to this:

/close
in favor of #7917

Thx @ykakarap for picking this up!

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-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 18, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2022

k8s-ci-robot requested review from JoelSpeed and sbueringer August 18, 2022 14:26

killianmuldoon reviewed Aug 18, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 18, 2022

propagate labels and annotations from ClusterClass and Cluster topolo…

66aacfb

…gy to KCP and MD

MaxFedotov force-pushed the md-and-kcp-labels-and-annotations branch from e1370a1 to 66aacfb Compare August 18, 2022 15:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 18, 2022

fabriziopandini reviewed Sep 8, 2022

View reviewed changes

apply review notes

8de4049

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 9, 2022

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

ykakarap reviewed Dec 8, 2022

View reviewed changes

ykakarap mentioned this pull request Dec 12, 2022

Umbrella issue for In place propagation of changes affecting Kubernetes objects only proposal implementation #7731

Closed

19 tasks

k8s-ci-robot closed this Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Propagate labels and annotations from ClusterClass and Cluster topology to KCP and MD #7088

✨ Propagate labels and annotations from ClusterClass and Cluster topology to KCP and MD #7088

MaxFedotov commented Aug 18, 2022

linux-foundation-easycla bot commented Aug 18, 2022 •

edited

Loading

k8s-ci-robot commented Aug 18, 2022

k8s-ci-robot commented Aug 18, 2022

k8s-ci-robot commented Aug 18, 2022

k8s-ci-robot commented Aug 18, 2022

killianmuldoon left a comment

MaxFedotov commented Aug 25, 2022

killianmuldoon commented Aug 29, 2022

sbueringer commented Aug 31, 2022

fabriziopandini left a comment

fabriziopandini Sep 8, 2022

MaxFedotov Sep 9, 2022 •

edited

Loading

fabriziopandini Sep 8, 2022

MaxFedotov Sep 9, 2022

fabriziopandini Sep 8, 2022

MaxFedotov Sep 9, 2022

fabriziopandini Sep 8, 2022

k8s-ci-robot commented Sep 9, 2022

k8s-ci-robot commented Sep 29, 2022

mnaser commented Nov 16, 2022

sbueringer commented Nov 16, 2022 •

edited

Loading

ykakarap left a comment •

edited

Loading

ykakarap Dec 8, 2022

MaxFedotov Dec 8, 2022

ykakarap commented Dec 9, 2022

ykakarap commented Jan 13, 2023

sbueringer commented Jan 19, 2023

k8s-ci-robot commented Jan 19, 2023

		machineLabels := mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels)
		machineAnnotations := mergeMap(topologyMetadata.Annotations, clusterClassMetadata.Annotations)

	func templateToObject(in templateToInput) (*unstructured.Unstructured, error) {
	// NOTE: The cluster label is added at creation time so this object could be read by the ClusterTopology
	// controller immediately after creation, even before other controllers are going to add the label (if missing).
	labels := map[string]string{}
	labels[clusterv1.ClusterLabelName] = in.cluster.Name
	labels[clusterv1.ClusterTopologyOwnedLabel] = ""

	templateLabels := mergeMap(labels, in.labels)

		machineLabels := mergeMap(machineDeploymentTopology.Metadata.Labels, machineDeploymentBlueprint.Metadata.Labels)
		machineAnnotations := mergeMap(machineDeploymentTopology.Metadata.Annotations, machineDeploymentBlueprint.Metadata.Annotations)

	// Apply Labels
	// NOTE: On top of all the labels applied to managed objects we are applying the ClusterTopologyMachineDeploymentLabel
	// keeping track of the MachineDeployment name from the Topology; this will be used to identify the object in next reconcile loops.
	labels := map[string]string{}
	labels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name
	labels[clusterv1.ClusterTopologyOwnedLabel] = ""
	labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name
	desiredMachineDeploymentObj.SetLabels(mergeMap(labels, machineLabels))

	// Apply Annotations
	desiredMachineDeploymentObj.SetAnnotations(machineAnnotations)

	// Set the selector with the subset of labels identifying controlled machines.
	// NOTE: this prevents the web hook to add cluster.x-k8s.io/deployment-name label, that is
	// redundant for managed MachineDeployments given that we already have topology.cluster.x-k8s.io/deployment-name.
	desiredMachineDeploymentObj.Spec.Selector.MatchLabels = map[string]string{}
	desiredMachineDeploymentObj.Spec.Selector.MatchLabels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name
	desiredMachineDeploymentObj.Spec.Selector.MatchLabels[clusterv1.ClusterTopologyOwnedLabel] = ""
	desiredMachineDeploymentObj.Spec.Selector.MatchLabels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name

	// Also set the labels in .spec.template.labels so that they are propagated to
	// MachineSet.labels and MachineSet.spec.template.labels and thus to Machine.labels.
	// Note: the labels in MachineSet are used to properly cleanup templates when the MachineSet is deleted.
	if desiredMachineDeploymentObj.Spec.Template.Labels == nil {
	desiredMachineDeploymentObj.Spec.Template.Labels = map[string]string{}
	}
	desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name
	desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterTopologyOwnedLabel] = ""
	desiredMachineDeploymentObj.Spec.Template.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name

✨ Propagate labels and annotations from ClusterClass and Cluster topology to KCP and MD #7088

✨ Propagate labels and annotations from ClusterClass and Cluster topology to KCP and MD #7088

Conversation

MaxFedotov commented Aug 18, 2022

linux-foundation-easycla bot commented Aug 18, 2022 • edited Loading

k8s-ci-robot commented Aug 18, 2022

k8s-ci-robot commented Aug 18, 2022

k8s-ci-robot commented Aug 18, 2022

k8s-ci-robot commented Aug 18, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

MaxFedotov commented Aug 25, 2022

killianmuldoon commented Aug 29, 2022

sbueringer commented Aug 31, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxFedotov Sep 9, 2022 • 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

k8s-ci-robot commented Sep 9, 2022

k8s-ci-robot commented Sep 29, 2022

mnaser commented Nov 16, 2022

sbueringer commented Nov 16, 2022 • edited Loading

ykakarap left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykakarap commented Dec 9, 2022

ykakarap commented Jan 13, 2023

sbueringer commented Jan 19, 2023

k8s-ci-robot commented Jan 19, 2023

linux-foundation-easycla bot commented Aug 18, 2022 •

edited

Loading

MaxFedotov Sep 9, 2022 •

edited

Loading

sbueringer commented Nov 16, 2022 •

edited

Loading

ykakarap left a comment •

edited

Loading