🐛 Set Machine's BootstrapReady when there is no ConfigRef #11459

zaneb · 2024-11-21T08:18:25Z

If there is no ConfigRef but the bootstrap data secret is set by the user (instead of a bootstrap provider), then BootstrapReady should be true. This is the case for MachineSet, and was originally the case for Machine since 5113f80. However, in d93eadc this changed as a side effect of ensuring that bootstrap config object can continue to be reconciled after the bootstrap provider has produced the bootstrap data secret.

This change ensures that, once a bootstrap data secret exists, in the case of a ConfigRef it can still be reconciled, while in the case there is no ConfigRef, BootstrapReady is set.

/area machine

k8s-ci-robot · 2024-11-21T08:18:33Z

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

k8s-ci-robot · 2024-11-21T08:18:34Z

Welcome @zaneb!

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 · 2024-11-21T08:18:35Z

Hi @zaneb. 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-sigs/prow repository.

JoelSpeed · 2024-11-21T09:40:57Z

/ok-to-test

JoelSpeed · 2024-11-21T10:41:03Z

internal/controllers/machine/machine_controller_phases.go

 	}
-	s.bootstrapConfig = obj

 	// If the bootstrap data is populated, set ready and return.
 	if m.Spec.Bootstrap.DataSecretName != nil {


Why does this come after reconciling the bootstrap config? The data secret name is either set by the user, in which case bootstrap config is not needed, or, it is set by the bootstrap provider, in which case the bootstrap config has already been reconciled. Wondering if we just need to move this block above?

I believe #7394 has the answer to that:

to ensure the ownerReference, and other changes, are set before the dataSecretName is checked. This is designed to help in backup and restore use cases, or any other case where the ownerReference is removed.

fabriziopandini · 2024-11-21T22:26:56Z

internal/controllers/machine/machine_controller_phases.go

-				// Tolerate bootstrap object not found when the machine is being deleted.
-				// TODO: we can also relax this and tolerate the absence of the bootstrap ref way before, e.g. after node ref is set
-				return ctrl.Result{}, nil
+	if m.Spec.Bootstrap.ConfigRef != nil {


What about

Suggested change

if m.Spec.Bootstrap.ConfigRef != nil {

// If the bootstrap data secret is set by the user, set ready and return.

if m.Spec.Bootstrap.ConfigRef == nil {

if m.Spec.Bootstrap.DataSecretName == nil {

return ctrl.Result{}, errors.New("either spec.bootstrap.dataSecretName or spec.bootstrap.configRef must be populated")

}

m.Status.BootstrapReady = true

conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition)

return ctrl.Result{}, nil

}

// Call generic external reconciler if we have an external reference.

...

// Drop >> If the bootstrap data is populated, set ready and return.

...

So we take care of user provided data secret first, and then we handle when data secret is generated by the bootstrap provider (without mixing the two use cases)

That looks like it would work (though people thought the same about this). I guess it comes down to whether it's worse to duplicate the code that marks the bootstrap ready, or to duplicate an if statement.

Feel free to add more tests to ensure the solution works and we don't have regressions (and that the fact that test did not catch the issue with the previous PR, is a clear signal that we need more tests).

WRT to the implementation options, I would prefer to not mix up the two use cases in the code because the resulting code is simpler to read and to reason about and this usually pays back on the long run.

Note for self: Separation between use cases doesn't seems to be addressed in latest commit, also unit test are missing

There's obviously no unit tests but the latest commit is exactly what you asked for in terms of separation of the use cases I thought?

Sorry but I don't think the last commits address the separation between use cases as the snipped above will do

I'm afraid I'm going to need you to explain in more detail, because the control flow in the updated patch is identical to the snippet you gave above.
It checks for a missing bootstrap ref and handles that case and returns early, leaving the rest of the function to handle the case where there is a bootstrap ref. That's what I understood from when you said:

take care of user provided data secret first, and then we handle when data secret is generated by the bootstrap provider (without mixing the two use cases)

If there is no ConfigRef but the bootstrap data secret is set by the user (instead of a bootstrap provider), then BootstrapReady should be true. This is the case for MachineSet, and was originally the case for Machine since 5113f80. However, in d93eadc this changed as a side effect of ensuring that bootstrap config object can continue to be reconciled after the bootstrap provider has produced the bootstrap data secret. This change ensures that, once a bootstrap data secret exists, in the case of a ConfigRef it can still be reconciled, while in the case there is no ConfigRef, BootstrapReady is set. Signed-off-by: Zane Bitter <[email protected]>

chrischdi

Could we also add some test coverage for the resulting machine to ensure we don't have regressions in future for this?

k8s-ci-robot added area/machine Issues or PRs related to machine lifecycle management cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 21, 2024

k8s-ci-robot requested review from fabriziopandini and richardcase November 21, 2024 08:18

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 Nov 21, 2024

zaneb mentioned this pull request Nov 21, 2024

🐛 bootstrap: add support for data secret metal3-io/cluster-api-provider-metal3#2086

Merged

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 Nov 21, 2024

JoelSpeed reviewed Nov 21, 2024

View reviewed changes

fabriziopandini reviewed Nov 21, 2024

View reviewed changes

zaneb force-pushed the machine-bootstrap-ready branch from 8ebd235 to 834bead Compare November 28, 2024 02:43

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 28, 2024

zaneb force-pushed the machine-bootstrap-ready branch from 834bead to f74c2e3 Compare November 28, 2024 02:44

chrischdi reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Set Machine's BootstrapReady when there is no ConfigRef #11459

🐛 Set Machine's BootstrapReady when there is no ConfigRef #11459

zaneb commented Nov 21, 2024

k8s-ci-robot commented Nov 21, 2024

k8s-ci-robot commented Nov 21, 2024

k8s-ci-robot commented Nov 21, 2024

JoelSpeed commented Nov 21, 2024

JoelSpeed Nov 21, 2024

zaneb Nov 22, 2024

fabriziopandini Nov 21, 2024

zaneb Nov 22, 2024

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Dec 5, 2024

zaneb Dec 17, 2024

fabriziopandini Dec 20, 2024 •

edited

Loading

zaneb Dec 23, 2024

chrischdi left a comment

-	if m.Spec.Bootstrap.ConfigRef != nil {
+	// If the bootstrap data secret is set by the user, set ready and return.
+	if m.Spec.Bootstrap.ConfigRef == nil {
+		if  m.Spec.Bootstrap.DataSecretName == nil {
+			return ctrl.Result{}, errors.New("either spec.bootstrap.dataSecretName or spec.bootstrap.configRef must be populated")
+		}
+		m.Status.BootstrapReady = true
+		conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition)
+		return ctrl.Result{}, nil
+	}
+	// Call generic external reconciler if we have an external reference.
+	...
+	// Drop >> If the bootstrap data is populated, set ready and return.
+	...

🐛 Set Machine's BootstrapReady when there is no ConfigRef #11459

Are you sure you want to change the base?

🐛 Set Machine's BootstrapReady when there is no ConfigRef #11459

Conversation

zaneb commented Nov 21, 2024

k8s-ci-robot commented Nov 21, 2024

k8s-ci-robot commented Nov 21, 2024

k8s-ci-robot commented Nov 21, 2024

JoelSpeed commented Nov 21, 2024

JoelSpeed Nov 21, 2024

Choose a reason for hiding this comment

zaneb Nov 22, 2024

Choose a reason for hiding this comment

fabriziopandini Nov 21, 2024

Choose a reason for hiding this comment

zaneb Nov 22, 2024

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Dec 5, 2024

Choose a reason for hiding this comment

zaneb Dec 17, 2024

Choose a reason for hiding this comment

fabriziopandini Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

zaneb Dec 23, 2024

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

fabriziopandini Nov 25, 2024 •

edited

Loading

fabriziopandini Dec 20, 2024 •

edited

Loading