🌱 Add NamingStrategy to MachineDeployment

[adilGhaffarDev]

What this PR does / why we need it:

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 part of #10577

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[k8s-ci-robot]

[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 enxebre 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

[enxebre]

/hold
on #11123 so we make sure feedback there is incorporated here

[adilGhaffarDev]

/area machineset
/area machinedeployment

[adilGhaffarDev]

cc @sbueringer @fabriziopandini @chrischdi
This one is ready for review, please check.

[lentzi90]

I have verified that it is working as expected with a real end-to-end test. Thanks for working on this @adilGhaffarDev !

[adilGhaffarDev]

/unhold
Unholding because kcp pr is merged.

[sbueringer]

I don't know when I'll get around to it. Not much time but a lot of work left until code freeze / PTO.

In general I'm fine with keeping the flag for another minor release

[enxebre]

This should be checked on admission and not be a possible path in practice. I think It's worth a // comment clarifying that.

[adilGhaffarDev]

we are checking it in webhook: https://github.com/kubernetes-sigs/cluster-api/pull/11172/files#diff-b6bae0ed28a4f55a318d296bc63afcff0c7751968346a551ef23f7360a89fd74R313
Where should I add the // comment ?

[enxebre]

I think It'd be nice to have a comment on the piece of code that will never be exercised in practice
e.g.

// This should never happen as this is validated on admission.
if !strings.Contains(nameTemplate, "{{ .random }}") {
...

[enxebre]

This changes current behaviour as today we are using machineName := names.SimpleNameGenerator.GenerateName(fmt.Sprintf("%s-", machineSet.Name)) for this code path, right? is that intended? if so why?

[adilGhaffarDev]

Yes, I agree let's keep the default behavior the same as before, I will add another variable .machineSet.name in MachineNamingStrategy.Template in addition to .cluster.name , .machineDeployment.name and .random. End users can use it or skip it however they want.
But default will always be "{{ .machineSet.name }}-{{ .random }}"

[sbueringer]

TBD in my opinion if the MS controller should read this from the MD or if the MD controller should propagate the field down to the MS object and then the MS controller should read it from there

(I'm not sure myself at the moment :))

[adilGhaffarDev]

Adding MachineNamingStrategy to MachineSet with values populated by the MachineDeployment controller would enable users to customize the MachineNamingStrategy for MachineSets that are not managed by a MachineDeployment. If we want to support this flexibility, we could consider adding MachineNamingStrategy to MachineSet too. Open to suggestions on this approach.

[enxebre]

I'm leaning toward propagating via machineSet support, which I think is actually part of the original driving use case #10577.
That said, the current approach doesn't preclude us from implementing the latter in the future.

[adilGhaffarDev]

I was adding MachineNamingStrategy to MachineSet and encountered an issue. Currently, MachineNamingStrategy.Template allows users to include the variable machineDeployment.name. However, if a MachineSet is not owned by a MachineDeployment, attempting to use the machineDeployment.name variable in the MachineNamingStrategy of a MachineSet will result in a failure.
To simplify this, I propose we remove support for machineDeployment.name and only retain machineSet.name. This change would streamline the logic, reducing potential confusion. If we were to support both variables, we would need to introduce a separate MachineNamingStrategy type specifically for MachineSet that only includes the machineSet.name variable, which would add complexity.
I’m leaning towards the simpler approach of removing machineDeployment.name and solely using machineSet.name. What are your thoughts on this?

[sbueringer]

Sounds good to me

[k8s-ci-robot]

@adilGhaffarDev: 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	b409bfa	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-sigs/prow repository. I understand the commands that are listed here.