Consistent metadata propagation behaviour #5240

sbueringer · 2021-09-15T14:19:58Z

I'm not entirely sure how it should be, but let's discuss in this issue how we want metadata to propagate and then adjust the implementation accordingly.

Right now the metadata is propagated as follows: ( from => to)

KubeadmControlPlane

.labels =>
.annotations =>
.spec.machineTemplate.metadata.labels => Machine.labels, InfraMachine.labels, KubeadmConfig.labels
.spec.machineTemplate.metadata.annotations => Machine.annotations, InfraMachine.annotations, KubeadmConfig.annotations

MachineDeployment

.labels =>
.annotations => MS.annotations
.spec.template.metadata.labels => MS.labels, MS.spec.template.metadata.labels
.spec.template.metadata.annotations => MS.spec.template.metadata.annotations

MachineSet

.labels =>
.annotations =>
.spec.template.metadata.labels => Machine.labels, InfraMachine.labels, KubeadmConfig.labels
.spec.template.metadata.annotations => Machine.annotations, InfraMachine.annotations

/kind bug

vincepri · 2021-09-30T14:47:41Z

/milestone Next

Is this a bug?

vincepri · 2021-09-30T14:48:22Z

/priority important-soon

sbueringer · 2021-09-30T15:22:14Z

/milestone Next

Is this a bug?

@vincepri Depends on what we want to behavior to be. I'm not sure what we want but the current behaviour seems to be a bit inconsistent. I'm not sure if the expected behavior is specified somewhere.

schrej · 2021-09-30T15:51:10Z

We're relying on the .spec.template.metadata for MachineDeployment -> MachineSet -> Machine/InfraMachine and KubeadmControlPlane -> Machine/InfraMachine, so we would definitely like to keep that behavior.

In my opinion it would also be good to extend MachineDeployment -> MachineSet to also copy over labels. Otherwise there is no way to set labels on those MachineSets when using Deployments.

cc @MaxRink

@sbueringer I think DockerMachine should be InfraMachine for the KubeadmControlPlane, shouldn't it?

sbueringer · 2021-09-30T16:30:41Z

@sbueringer I think DockerMachine should be InfraMachine for the KubeadmControlPlane, shouldn't it?
Yes, thx, fixed.

My opinion on the situation:

KCP => lgtm

MD.annotations => MS.annotations

I think it's strange to propagate top-level MD.annotations to MS.annotations

MD.spec.template.metadata.labels => MS.labels, MS.spec.template.metadata.labels
MD.spec.template.metadata.annotations => MS.spec.template.metadata.annotations

It think we should also propagate MD.spec.template.metadata.annotations to MS.annotations

(I would rather propagate MD.spec.template.metadata.{labels,annotations} then top-level MD.{labels.annotations}. I don't think top-level metadata should be propagated at all)

MS.spec.template.metadata.labels => Machine.labels, InfraMachine.labels, KubeadmConfig.labels
MS.spec.template.metadata.annotations => Machine.annotations, InfraMachine.annotations

Seems more consistent to me to also propagate MS.spec.template.metadata.annotations to KubeadmConfig.annotations

schrej · 2021-09-30T16:38:00Z

It think we should also propagate MD.spec.template.metadata.annotations to MS.annotations
Seems more consistent to me to also propagate MS.spec.template.metadata.annotations to KubeadmConfig.annotations

I think we should always either propagate both, annotations and labels, or neither. It's not very intuitive (nor logical) if only one is propagated.

sbueringer · 2021-09-30T16:40:51Z

It think we should also propagate MD.spec.template.metadata.annotations to MS.annotations
Seems more consistent to me to also propagate MS.spec.template.metadata.annotations to KubeadmConfig.annotations

I think we should always either propagate both, annotations and labels, or neither. It's not very intuitive (nor logical) if only one is propagated.

100% agree. Basically my comment comes down to: "let's always propagate both and let's stop propagating top-level MD annotations"

randomvariable · 2021-09-30T16:47:51Z

I agree with sorting this out. Me and @sedefsavas were looking at this the other day and were pretty confused about what gets propagated and doesn't.

fabriziopandini · 2021-09-30T17:21:14Z

+1 for consistency
however, for MachineDeployments IMO top level label/annotations should go to MS, template label/annotations should go to machines.

randomvariable · 2021-09-30T17:52:09Z

Discussing with @schrej , @vincepri and @sbueringer in Slack, the conclusions we've reached is that:

MDs should behave like appsv1/Deployment
ReplicaSets and KCP should behave like ReplicaSets (or appsv1/StatefulSet in the case of KCP).

In which case, the behaviour is, where d = appsv1/deployment and r = replicaset

d.annotations => r.annotations
d.labels => nil
d.spec.template.annotations => d.spec.template.annotations
d.spec.template.labels => r.labels & r.spec.template.labels

This is in https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/deployment/sync.go#L146-L150 and https://github.com/kubernetes/kubernetes/blob/38746752af6d0ec955f4511a727da37347da3b92/pkg/controller/deployment/util/deployment_util.go#L232 with a comment here: kubernetes/kubernetes#92896 (comment)

From this, it would appear the outstanding task is to propagate MS.spec.template.metadata.annotations to KubeadmConfig.meta.annotations if we want to be consistent.

vincepri · 2021-09-30T17:53:06Z

/milestone v1.0

randomvariable · 2021-09-30T17:54:30Z

There is an outstanding question about InfraMachineTemplate.Spec.Template.Meta and KubeadmConfigTemplate.Spec.Template.Meta . Should all providers implement this, what is the merge behaviour, and who should when there's conflict?

sbueringer · 2021-09-30T18:03:42Z

/reopen

The current implementation just takes everything in .spec.template of InfraMachineTemplate.Spec.Template.Meta and KubeadmConfigTemplate.Spec.Template.Meta and overwrites it with propagated metadata:

cluster-api/controllers/external/util.go

Line 144 in 9fdf087

    
           func GenerateTemplate(in *GenerateTemplateInput) (*unstructured.Unstructured, error) {

Neither KubeadmConfigTemplate nor DockerMachineTemplate have .Spec.Template.Meta fields

k8s-ci-robot · 2021-09-30T18:03:51Z

@sbueringer: Reopened this issue.

In response to this:

/reopen

The current implementation just takes everything in .spec.template of InfraMachineTemplate.Spec.Template.Meta and KubeadmConfigTemplate.Spec.Template.Meta and overwrites it with propagated metadata:

cluster-api/controllers/external/util.go

Line 144 in 9fdf087

func GenerateTemplate(in *GenerateTemplateInput) (*unstructured.Unstructured, error) {

Neither KubeadmConfigTemplate nor DockerMachineTemplate has .Spec.Template.Meta fields

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.

randomvariable · 2021-09-30T18:32:50Z

The same is true for the other infra providers, so we should add something to the v1beta1 docs?

sbueringer · 2021-10-01T09:01:29Z

The same is true for the other infra providers, so we should add something to the v1beta1 docs?

In wonder if/where we have guidance how the templates should be implemented.

randomvariable · 2021-10-01T13:06:51Z

In wonder if/where we have guidance how the templates should be implemented.

I'll have a look and knock something up.

killianmuldoon · 2022-03-07T10:33:37Z

/assign

I'll take a look at closing out the last couple of issues related to this.

v1alpha4 <-> v1beta1 conversions were failing and never tested. As part of the PR - Add a missing gcpclustertemplate_conversion.go file - Add conversion test to make sure conversions work - Add ObjectMeta to GCPClusterTemplateResource and GCPMachineTemplateResource. See kubernetes-sigs/cluster-api#5240

k8s-triage-robot · 2022-04-06T10:49:53Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-04-06T10:50:04Z

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

sbueringer · 2022-04-06T11:31:48Z

/reopen
/remove-lifecycle rotten

k8s-ci-robot · 2022-04-06T11:31:58Z

@sbueringer: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

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-triage-robot · 2022-07-05T12:09:48Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

killianmuldoon · 2022-07-15T15:38:41Z

/remove-lifecycle-state

schrej · 2022-07-20T13:49:48Z

/reopen
/remove-lifecycle stale

k8s-ci-robot · 2022-07-20T13:50:03Z

@schrej: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle stale

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.

killianmuldoon · 2022-07-20T15:33:08Z

@schrej Is there something specific outstanding on this issue we should tackle?

schrej · 2022-07-20T15:44:33Z

Oh sorry, just saw your misspelled lifecycle command and missed the PR.

/close

k8s-ci-robot · 2022-07-20T15:44:46Z

@schrej: Closing this issue.

In response to this:

Oh sorry, just saw your misspelled lifecycle command and missed the PR.

/close

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.

killianmuldoon · 2022-07-20T15:47:18Z

I didn't even notice that until now 😆

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 15, 2021

sbueringer mentioned this issue Sep 15, 2021

ClusterClass template name *only* update triggers reconcilliation #5227

Closed

k8s-ci-robot added this to the Next milestone Sep 30, 2021

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 30, 2021

sbueringer mentioned this issue Sep 30, 2021

🐛 Propagate MS.spec.template.annotations to bootstrap config #5365

Merged

k8s-ci-robot modified the milestones: Next, v1.0 Sep 30, 2021

k8s-ci-robot closed this as completed in #5365 Sep 30, 2021

k8s-ci-robot reopened this Sep 30, 2021

randomvariable mentioned this issue Oct 1, 2021

📖 Document the requirements for template and list resources for infrastructure providers #5371

Merged

This was referenced Oct 6, 2021

Missing documentation: metadata propagation #5397

Closed

Topology controller should avoid template rotation in case of changes to template metadata only #5413

Closed

dharmjit mentioned this issue Oct 19, 2021

🌱 Added ObjectMeta to DockerMachineTemplateResource, DockerClusterTemplateResource #5441

Merged

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 5, 2022

k8s-ci-robot assigned killianmuldoon Mar 7, 2022

pydctw mentioned this issue Mar 21, 2022

Fix v1alpha4 <-> v1beta1 API conversions kubernetes-sigs/cluster-api-provider-gcp#559

Merged

3 tasks

k8s-ci-robot closed this as completed Apr 6, 2022

k8s-ci-robot reopened this Apr 6, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 6, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 5, 2022

killianmuldoon mentioned this issue Jul 15, 2022

📖 Update docs for metadata propagation #6935

Merged

k8s-ci-robot closed this as completed in #6935 Jul 20, 2022

k8s-ci-robot reopened this Jul 20, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 20, 2022

k8s-ci-robot closed this as completed Jul 20, 2022

killianmuldoon mentioned this issue Jul 25, 2022

Added ObjectMeta to KubeadmConfigTemplateResource, KubeadmControlPlaneTemplateResource #6976

Closed

sbueringer mentioned this issue Oct 6, 2022

📖 In place propagation of changes affecting Kubernetes objects only #7331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent metadata propagation behaviour #5240

Consistent metadata propagation behaviour #5240

sbueringer commented Sep 15, 2021 •

edited

Loading

vincepri commented Sep 30, 2021

vincepri commented Sep 30, 2021

sbueringer commented Sep 30, 2021 •

edited

Loading

schrej commented Sep 30, 2021 •

edited

Loading

sbueringer commented Sep 30, 2021 •

edited

Loading

schrej commented Sep 30, 2021 •

edited

Loading

sbueringer commented Sep 30, 2021 •

edited

Loading

randomvariable commented Sep 30, 2021 •

edited

Loading

fabriziopandini commented Sep 30, 2021

randomvariable commented Sep 30, 2021

vincepri commented Sep 30, 2021

randomvariable commented Sep 30, 2021

sbueringer commented Sep 30, 2021 •

edited

Loading

k8s-ci-robot commented Sep 30, 2021

randomvariable commented Sep 30, 2021

sbueringer commented Oct 1, 2021

randomvariable commented Oct 1, 2021

killianmuldoon commented Mar 7, 2022

k8s-triage-robot commented Apr 6, 2022

k8s-ci-robot commented Apr 6, 2022

sbueringer commented Apr 6, 2022

k8s-ci-robot commented Apr 6, 2022

k8s-triage-robot commented Jul 5, 2022

killianmuldoon commented Jul 15, 2022

schrej commented Jul 20, 2022

k8s-ci-robot commented Jul 20, 2022

killianmuldoon commented Jul 20, 2022

schrej commented Jul 20, 2022

k8s-ci-robot commented Jul 20, 2022

killianmuldoon commented Jul 20, 2022

Consistent metadata propagation behaviour #5240

Consistent metadata propagation behaviour #5240

Comments

sbueringer commented Sep 15, 2021 • edited Loading

KubeadmControlPlane

MachineDeployment

MachineSet

vincepri commented Sep 30, 2021

vincepri commented Sep 30, 2021

sbueringer commented Sep 30, 2021 • edited Loading

schrej commented Sep 30, 2021 • edited Loading

sbueringer commented Sep 30, 2021 • edited Loading

schrej commented Sep 30, 2021 • edited Loading

sbueringer commented Sep 30, 2021 • edited Loading

randomvariable commented Sep 30, 2021 • edited Loading

fabriziopandini commented Sep 30, 2021

randomvariable commented Sep 30, 2021

vincepri commented Sep 30, 2021

randomvariable commented Sep 30, 2021

sbueringer commented Sep 30, 2021 • edited Loading

k8s-ci-robot commented Sep 30, 2021

randomvariable commented Sep 30, 2021

sbueringer commented Oct 1, 2021

randomvariable commented Oct 1, 2021

killianmuldoon commented Mar 7, 2022

k8s-triage-robot commented Apr 6, 2022

k8s-ci-robot commented Apr 6, 2022

sbueringer commented Apr 6, 2022

k8s-ci-robot commented Apr 6, 2022

k8s-triage-robot commented Jul 5, 2022

killianmuldoon commented Jul 15, 2022

schrej commented Jul 20, 2022

k8s-ci-robot commented Jul 20, 2022

killianmuldoon commented Jul 20, 2022

schrej commented Jul 20, 2022

k8s-ci-robot commented Jul 20, 2022

killianmuldoon commented Jul 20, 2022

sbueringer commented Sep 15, 2021 •

edited

Loading

sbueringer commented Sep 30, 2021 •

edited

Loading

schrej commented Sep 30, 2021 •

edited

Loading

sbueringer commented Sep 30, 2021 •

edited

Loading

schrej commented Sep 30, 2021 •

edited

Loading

sbueringer commented Sep 30, 2021 •

edited

Loading

randomvariable commented Sep 30, 2021 •

edited

Loading

sbueringer commented Sep 30, 2021 •

edited

Loading