Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 22 additions & 20 deletions internal/controllers/machine/machine_controller_phases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,29 +132,26 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, s *scope) (ctrl.Res
cluster := s.cluster
m := s.machine

// If the Bootstrap ref is nil (and so the machine should use user generated data secret), return.
if m.Spec.Bootstrap.ConfigRef == nil {
return ctrl.Result{}, nil
}

// Call generic external reconciler if we have an external reference.
obj, err := r.reconcileExternal(ctx, cluster, m, m.Spec.Bootstrap.ConfigRef)
if err != nil {
if apierrors.IsNotFound(err) {
s.bootstrapConfigIsNotFound = true

if !s.machine.DeletionTimestamp.IsZero() {
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Call generic external reconciler if we have an external reference.
obj, err := r.reconcileExternal(ctx, cluster, m, m.Spec.Bootstrap.ConfigRef)
if err != nil {
if apierrors.IsNotFound(err) {
s.bootstrapConfigIsNotFound = true

if !s.machine.DeletionTimestamp.IsZero() {
// 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
}
log.Info("Could not find bootstrap config object, requeuing", m.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(m.Spec.Bootstrap.ConfigRef.Namespace, m.Spec.Bootstrap.ConfigRef.Name))
// TODO: we can make this smarter and requeue only if we are before node ref is set
return ctrl.Result{RequeueAfter: externalReadyWait}, nil
}
log.Info("Could not find bootstrap config object, requeuing", m.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(m.Spec.Bootstrap.ConfigRef.Namespace, m.Spec.Bootstrap.ConfigRef.Name))
// TODO: we can make this smarter and requeue only if we are before node ref is set
return ctrl.Result{RequeueAfter: externalReadyWait}, nil
return ctrl.Result{}, err
}
return ctrl.Result{}, err
s.bootstrapConfig = obj
}
s.bootstrapConfig = obj

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -163,6 +160,11 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, s *scope) (ctrl.Res
return ctrl.Result{}, nil
}

// If the Bootstrap ref is nil (and so the machine should use user generated data secret), return.
if m.Spec.Bootstrap.ConfigRef == nil {
return ctrl.Result{}, nil
}

// Determine if the bootstrap provider is ready.
ready, err := external.IsReady(s.bootstrapConfig)
if err != nil {
Expand Down