🌱 Flag for old infra machine naming #10576

lentzi90 · 2024-05-08T06:46:31Z

What this PR does / why we need it:

There is a breaking change to the way InfraMachines are named in v1.7.0. This commit adds a flag to the KCP and core controller for getting the old behavior back. It defaults to disabled so the new behavior stays. It is also marked as deprecated and to be removed in v1.9.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Stop-gap solution for #10463

/area provider/control-plane-kubeadm
/area machineset

mdbooth

I think this looks good.

Another observation. In both of the places where we're modifying the object's name we already have a MachineTemplateSpec:

cluster-api/api/v1beta1/machineset_types.go

Lines 91 to 102 in c6783a2

    
           type MachineTemplateSpec struct { 
        
           	// Standard object's metadata. 
        
           	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata 
        
           	// +optional 
        
           	ObjectMeta `json:"metadata,omitempty"` 
        
           	// Specification of the desired behavior of the machine. 
        
           	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status 
        
           	// +optional 
        
           	Spec MachineSpec `json:"spec,omitempty"` 
        
           }

I think it would be quite a natural extension to the API to add, e.g. a GenerateName field to MachineTemplateSpec. We could then restore the Machine.Name==InfraMachine.Name relationship, and allow the user to control the naming of their Machines (and ultimately their Nodes) arbitrarily if required.

I think this would be pretty simple to implement, backwards compatible, and would solve the node naming problem long term. I could probably make time to implement it.

mdbooth · 2024-05-08T12:42:57Z

controlplane/kubeadm/internal/controllers/helpers.go

@@ -180,12 +180,16 @@ func (r *KubeadmControlPlaneReconciler) cloneConfigsAndGenerateMachine(ctx conte
 		UID:        kcp.UID,
 	}

+	infraMachineName := machine.Name
+	if r.DeprecatedInfraMachineNaming {
+		infraMachineName = names.SimpleNameGenerator.GenerateName(kcp.Spec.MachineTemplate.InfrastructureRef.Name + "-")


Just an observation: this remains the default behaviour of GenerateTemplate() if Name is not given:

cluster-api/controllers/external/util.go

Lines 169 to 172 in c6783a2

if to.GetName() == "" {

to.SetName(names.SimpleNameGenerator.GenerateName(in.Template.GetName() + "-"))

}

to.SetNamespace(in.Namespace)

You could omit this and simply not set Name at all on line 192 below if DeprecatedInfraMachineNaming is set.

The disadvantage of that is that the behaviour would be implicit and not local to use, and therefore easy to regress again.

If keeping this code, I wonder if it's worth removing the default from GenerateTemplate(). When I looked before I don't think there were any callers which weren't setting Name. And if there were CI would find them pretty quickly.

@sbueringer ?

Current implementation seems like the most explicit way to implement it. So it's fine for me

There are no writes of the Name field in core CAPI, but these are exported functions. So I wouldn't change it

controlplane/kubeadm/main.go

mdbooth · 2024-05-08T13:01:56Z

internal/controllers/machineset/machineset_controller.go

+			if r.DeprecatedInfraMachineNaming {
+				infraMachineName = names.SimpleNameGenerator.GenerateName(ms.Spec.Template.Spec.InfrastructureRef.Name + "-")
+			}


Same observation as for KCP.

config/manager/manager.yaml

feature/feature.go

sbueringer · 2024-05-08T16:00:49Z

controlplane/kubeadm/internal/controllers/helpers.go

@@ -180,12 +180,16 @@ func (r *KubeadmControlPlaneReconciler) cloneConfigsAndGenerateMachine(ctx conte
 		UID:        kcp.UID,
 	}

+	infraMachineName := machine.Name
+	if r.DeprecatedInfraMachineNaming {
+		infraMachineName = names.SimpleNameGenerator.GenerateName(kcp.Spec.MachineTemplate.InfrastructureRef.Name + "-")


Current implementation seems like the most explicit way to implement it. So it's fine for me

There are no writes of the Name field in core CAPI, but these are exported functions. So I wouldn't change it

sbueringer · 2024-05-08T16:11:23Z

Just some minor findings. Let's try to get this merged until Monday the latest (I know a few folks including me have a public holidy tomorrow :))

There is a breaking change to the way InfraMachines are named in v1.7.0. This commit adds a flag to the KCP and core controller for getting the old behavior back. It defaults to disabled so the new behavior stays. It is also marked as deprecated and to be removed in v1.9. Signed-off-by: Lennart Jern <lennart.jern@est.tech>

mdbooth

/lgtm

k8s-ci-robot · 2024-05-10T09:05:51Z

LGTM label has been added.

Git tree hash: 3cc7b0e56c76b05235d3095a4ec7369f074e60a0

sbueringer · 2024-05-10T16:07:43Z

Thx!

/lgtm
/approve

sbueringer · 2024-05-10T16:07:51Z

/cherry-pick release-1.7

k8s-ci-robot · 2024-05-10T16:07:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

k8s-infra-cherrypick-robot · 2024-05-10T16:07:53Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.7

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-infra-cherrypick-robot · 2024-05-10T16:42:02Z

@sbueringer: new pull request created: #10587

In response to this:

/cherry-pick release-1.7

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 added area/provider/control-plane-kubeadm Issues or PRs related to KCP area/machineset Issues or PRs related to machinesets cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 8, 2024

k8s-ci-robot requested review from jackfrancis and stmcginnis May 8, 2024 06:46

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2024

lentzi90 force-pushed the lentzi90/feature-gate-infra-machine-naming branch from 74cdc9a to 6b027f4 Compare May 8, 2024 09:14

mdbooth reviewed May 8, 2024

View reviewed changes

sbueringer reviewed May 8, 2024

View reviewed changes

lentzi90 force-pushed the lentzi90/feature-gate-infra-machine-naming branch from 6b027f4 to 43f13bc Compare May 10, 2024 07:24

mdbooth reviewed May 10, 2024

View reviewed changes

k8s-ci-robot assigned mdbooth May 10, 2024

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

k8s-ci-robot assigned sbueringer May 10, 2024

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

k8s-ci-robot merged commit 180b50f into kubernetes-sigs:main May 10, 2024
20 checks passed

k8s-ci-robot added this to the v1.8 milestone May 10, 2024

k8s-infra-cherrypick-robot mentioned this pull request May 10, 2024

[release-1.7] 🌱 Flag for old infra machine naming #10587

Merged

lentzi90 deleted the lentzi90/feature-gate-infra-machine-naming branch May 10, 2024 18:58

lentzi90 mentioned this pull request May 16, 2024

Make it possible to change infra machine naming scheme #10463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Flag for old infra machine naming #10576

🌱 Flag for old infra machine naming #10576

lentzi90 commented May 8, 2024

mdbooth left a comment

mdbooth May 8, 2024

sbueringer May 8, 2024

mdbooth May 8, 2024

sbueringer May 8, 2024

sbueringer commented May 8, 2024

mdbooth left a comment

k8s-ci-robot commented May 10, 2024

sbueringer commented May 10, 2024

sbueringer commented May 10, 2024

k8s-ci-robot commented May 10, 2024

k8s-infra-cherrypick-robot commented May 10, 2024

k8s-infra-cherrypick-robot commented May 10, 2024

	type MachineTemplateSpec struct {
	// Standard object's metadata.
	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
	// +optional
	ObjectMeta `json:"metadata,omitempty"`

	// Specification of the desired behavior of the machine.
	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status
	// +optional
	Spec MachineSpec `json:"spec,omitempty"`
	}

	if to.GetName() == "" {
	to.SetName(names.SimpleNameGenerator.GenerateName(in.Template.GetName() + "-"))
	}
	to.SetNamespace(in.Namespace)

🌱 Flag for old infra machine naming #10576

🌱 Flag for old infra machine naming #10576

Conversation

lentzi90 commented May 8, 2024

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth May 8, 2024

Choose a reason for hiding this comment

sbueringer May 8, 2024

Choose a reason for hiding this comment

mdbooth May 8, 2024

Choose a reason for hiding this comment

sbueringer May 8, 2024

Choose a reason for hiding this comment

sbueringer commented May 8, 2024

mdbooth left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 10, 2024

sbueringer commented May 10, 2024

sbueringer commented May 10, 2024

k8s-ci-robot commented May 10, 2024

k8s-infra-cherrypick-robot commented May 10, 2024

k8s-infra-cherrypick-robot commented May 10, 2024