🌱 MachineSet controller: delete Bootstrap object when creating InfraMachine object failed #11211
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @goushicui. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
needs-ok-to-test
k8s-ci-robot
added
ok-to-test
sbueringer
changed the title
sbueringer
changed the title
| log.Error(err, "Failed to cleanup bootstrap configuration object after Machine creation error", bootstrapRef.Kind, klog.KRef(bootstrapRef.Namespace, bootstrapRef.Name)) | ||
| } | ||
| } | ||
| conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.InfrastructureTemplateCloningFailedReason, clusterv1.ConditionSeverityError, err.Error()) | ||
| return ctrl.Result{}, errors.Wrapf(err, "failed to clone infrastructure machine from %s %s while creating a machine", |
There was a problem hiding this comment.
We might want to add the error to the previous one, instead of just logging it out
Co-authored-by: Vince Prignano <[email protected]>
Co-authored-by: Vince Prignano <[email protected]>
| @@ -541,6 +541,12 @@ func (r *Reconciler) syncReplicas(ctx context.Context, cluster *clusterv1.Cluste | |||
| }, | |||
| }) | |||
| if err != nil { | |||
| // Cleanup the bootstrap resource if we can't create the InfraMachine; or we might risk to leak it. | |||
There was a problem hiding this comment.
@goushicui I think what Vince was suggesting is something like this:
if err != nil {
var deleteErr error
if bootstrapRef != nil {
// Cleanup the bootstrap resource if we can't create the InfraMachine; or we might risk to leak it.
if err := r.Client.Delete(ctx, util.ObjectReferenceToUnstructured(*bootstrapRef)); err != nil && !apierrors.IsNotFound(err) {
deleteErr = errors.Wrapf(err, "failed to cleanup %s %s after %s creation failed", bootstrapRef.Kind, klog.KRef(bootstrapRef.Namespace, bootstrapRef.Name), (&ms.Spec.Template.Spec.InfrastructureRef).Kind)
}
}
conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.InfrastructureTemplateCloningFailedReason, clusterv1.ConditionSeverityError, err.Error())
return ctrl.Result{}, kerrors.NewAggregate([]error{errors.Wrapf(err, "failed to clone infrastructure machine from %s %s while creating a machine",
ms.Spec.Template.Spec.InfrastructureRef.Kind,
klog.KRef(ms.Spec.Template.Spec.InfrastructureRef.Namespace, ms.Spec.Template.Spec.InfrastructureRef.Name)), deleteErr})
}| @@ -541,6 +541,12 @@ func (r *Reconciler) syncReplicas(ctx context.Context, cluster *clusterv1.Cluste | |||
| }, | |||
| }) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Let's please add unit test coverage
What this PR does / why we need it:
If you do not delete the bootstrapRef after failing to create the machineRef, it will cause the bootstrapRef to leak.
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 #