delete bootstrapRef when create infraRef failed

Author: goushicui

internal/controllers/machineset/machineset_controller.go

@@ -541,10 +541,17 @@ func (r *Reconciler) syncReplicas(ctx context.Context, cluster *clusterv1.Cluste
 				},
 			})
 			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{}, errors.Wrapf(err, "failed to clone infrastructure machine from %s %s while creating a machine",
+				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))
+					klog.KRef(ms.Spec.Template.Spec.InfrastructureRef.Namespace, ms.Spec.Template.Spec.InfrastructureRef.Name)), deleteErr})
 			}
 			log = log.WithValues(infraRef.Kind, klog.KRef(infraRef.Namespace, infraRef.Name))
 			machine.Spec.InfrastructureRef = *infraRef

internal/controllers/machineset/machineset_controller_test.go

@@ -17,7 +17,11 @@ limitations under the License.
 package machineset
 
 import (
+	"context"
 	"fmt"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+	"strings"
 	"testing"
 	"time"
 
@@ -1969,6 +1973,149 @@ func TestMachineSetReconciler_syncReplicas(t *testing.T) {
 	})
 }
 
+func TestMachineSetReconciler_syncReplicas_WithErrors(t *testing.T) {
+	t.Run("should hold off on sync replicas when create Infrastructure of machine failed ", func(t *testing.T) {
+		g := NewWithT(t)
+		fakeClient := fake.NewClientBuilder().WithObjects().WithInterceptorFuncs(interceptor.Funcs{
+			Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
+				// simulate scenarios where infra object creation fails
+				if obj.GetObjectKind().GroupVersionKind().Kind == "GenericInfrastructureMachine" {
+					return fmt.Errorf("inject error for create machineInfrastructure")
+				}
+				return client.Create(ctx, obj, opts...)
+			},
+		}).Build()
+
+		r := &Reconciler{
+			Client: fakeClient,
+		}
+		testCluster := &clusterv1.Cluster{}
+		testCluster.Namespace = "default"
+		testCluster.Name = "test-cluster"
+		version := "v1.14.2"
+		duration10m := &metav1.Duration{Duration: 10 * time.Minute}
+		machineSet := &clusterv1.MachineSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:       "machineset1",
+				Namespace:  metav1.NamespaceDefault,
+				Finalizers: []string{"block-deletion"},
+			},
+			Spec: clusterv1.MachineSetSpec{
+				Replicas:    ptr.To[int32](1),
+				ClusterName: "test-cluster",
+				Template: clusterv1.MachineTemplateSpec{
+					Spec: clusterv1.MachineSpec{
+						ClusterName: testCluster.Name,
+						Version:     &version,
+						Bootstrap: clusterv1.Bootstrap{
+							ConfigRef: &corev1.ObjectReference{
+								APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1",
+								Kind:       "GenericBootstrapConfigTemplate",
+								Name:       "ms-template",
+							},
+						},
+						InfrastructureRef: corev1.ObjectReference{
+							APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
+							Kind:       "GenericInfrastructureMachineTemplate",
+							Name:       "ms-template",
+						},
+						NodeDrainTimeout:        duration10m,
+						NodeDeletionTimeout:     duration10m,
+						NodeVolumeDetachTimeout: duration10m,
+					},
+				},
+			},
+		}
+
+		// Create bootstrap template resource.
+		bootstrapResource := map[string]interface{}{
+			"kind":       "GenericBootstrapConfig",
+			"apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1",
+			"metadata": map[string]interface{}{
+				"annotations": map[string]interface{}{
+					"precedence": "GenericBootstrapConfig",
+				},
+			},
+		}
+		bootstrapTmpl := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"template": bootstrapResource,
+				},
+			},
+		}
+		bootstrapTmpl.SetKind("GenericBootstrapConfigTemplate")
+		bootstrapTmpl.SetAPIVersion("bootstrap.cluster.x-k8s.io/v1beta1")
+		bootstrapTmpl.SetName("ms-template")
+		bootstrapTmpl.SetNamespace(metav1.NamespaceDefault)
+		g.Expect(r.Client.Create(context.TODO(), bootstrapTmpl)).To(Succeed())
+
+		// Create infrastructure template resource.
+		infraResource := map[string]interface{}{
+			"kind":       "GenericInfrastructureMachine",
+			"apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1",
+			"metadata": map[string]interface{}{
+				"annotations": map[string]interface{}{
+					"precedence": "GenericInfrastructureMachineTemplate",
+				},
+			},
+			"spec": map[string]interface{}{
+				"size": "3xlarge",
+			},
+		}
+		infraTmpl := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"template": infraResource,
+				},
+			},
+		}
+		infraTmpl.SetKind("GenericInfrastructureMachineTemplate")
+		infraTmpl.SetAPIVersion("infrastructure.cluster.x-k8s.io/v1beta1")
+		infraTmpl.SetName("ms-template")
+		infraTmpl.SetNamespace(metav1.NamespaceDefault)
+		g.Expect(r.Client.Create(context.TODO(), infraTmpl)).To(Succeed())
+
+		_, err := r.syncReplicas(ctx, testCluster, machineSet, nil)
+		g.Expect(err).To(HaveOccurred())
+
+		// Verify the proper condition is set on the MachineSet.
+		condition := clusterv1.MachinesCreatedCondition
+		g.Expect(conditions.Has(machineSet, condition)).To(BeTrue(), "MachineSet should have the %s condition set", condition)
+
+		machinesCreatedCondition := conditions.Get(machineSet, condition)
+		g.Expect(machinesCreatedCondition.Status).
+			To(Equal(corev1.ConditionFalse), "%s condition status should be %s", condition, corev1.ConditionFalse)
+		g.Expect(machinesCreatedCondition.Reason).
+			To(Equal(clusterv1.InfrastructureTemplateCloningFailedReason), "%s condition reason should be %s", condition, clusterv1.InfrastructureTemplateCloningFailedReason)
+
+		// Verify no new Machines are created.
+		machineList := &clusterv1.MachineList{}
+		g.Expect(r.Client.List(ctx, machineList)).To(Succeed())
+		g.Expect(machineList.Items).To(BeEmpty(), "There should not be any machines")
+
+		// Verify no boostrap object created
+		bootstrapList := &unstructured.UnstructuredList{}
+		bootstrapList.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   bootstrapTmpl.GetObjectKind().GroupVersionKind().Group,
+			Version: bootstrapTmpl.GetObjectKind().GroupVersionKind().Version,
+			Kind:    strings.TrimSuffix(bootstrapTmpl.GetObjectKind().GroupVersionKind().Kind, clusterv1.TemplateSuffix),
+		})
+		g.Expect(r.Client.List(ctx, bootstrapList)).To(Succeed())
+		g.Expect(len(bootstrapList.Items)).To(BeEmpty(), "There should not be any bootstrap object")
+
+		// Verify no infra object created
+		infraList := &unstructured.UnstructuredList{}
+		infraList.SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   infraTmpl.GetObjectKind().GroupVersionKind().Group,
+			Version: infraTmpl.GetObjectKind().GroupVersionKind().Version,
+			Kind:    strings.TrimSuffix(infraTmpl.GetObjectKind().GroupVersionKind().Kind, clusterv1.TemplateSuffix),
+		})
+		g.Expect(r.Client.List(ctx, infraList)).To(Succeed())
+		g.Expect(len(infraList.Items)).To(BeEmpty(), "There should not be any infra object")
+	})
+}
+
 func TestComputeDesiredMachine(t *testing.T) {
 	duration5s := &metav1.Duration{Duration: 5 * time.Second}
 	duration10s := &metav1.Duration{Duration: 10 * time.Second}