From 0fb2fc8389de34908d82995268ce9c4d32538a93 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 18 Mar 2024 17:16:01 +0100 Subject: [PATCH 1/2] Avoid leaving orphaned InfrastructureCluster when create control plane fails --- .../topology/cluster/reconcile_state.go | 77 ++++--- .../topology/cluster/reconcile_state_test.go | 216 ++++++++++++++++-- 2 files changed, 238 insertions(+), 55 deletions(-) diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index a375f58bf9b6..583d37c6bec4 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -27,6 +27,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/wait" @@ -75,17 +76,30 @@ func (r *Reconciler) reconcileState(ctx context.Context, s *scope.Scope) error { } // Reconcile desired state of the InfrastructureCluster object. - if err := r.reconcileInfrastructureCluster(ctx, s); err != nil { - return err + createdInfraCluster, errInfraCluster := r.reconcileInfrastructureCluster(ctx, s) + if errInfraCluster != nil { + return errInfraCluster } // Reconcile desired state of the ControlPlane object. - if err := r.reconcileControlPlane(ctx, s); err != nil { - return err + createdControlPlane, errControlPlane := r.reconcileControlPlane(ctx, s) + if errControlPlane != nil { + // NOTE: report control plane error immediately only if we did not just create the infrastructure cluster; otherwise attempt reconcile cluster before returning. + if !createdInfraCluster { + return errControlPlane + } + + // In this case (reconcileInfrastructureCluster passed reporting creation of the infrastructure cluster object, reconcileControlPlane - which is expected to create the control plane object - failed), + // if the creation of the control plane actually did not happen, blank out ControlPlaneRef from desired cluster. + if s.Current.Cluster.Spec.ControlPlaneRef == nil && !createdControlPlane { + s.Desired.Cluster.Spec.ControlPlaneRef = nil + } } // Reconcile desired state of the Cluster object. - if err := r.reconcileCluster(ctx, s); err != nil { + errCluster := r.reconcileCluster(ctx, s) + err := kerrors.NewAggregate([]error{errControlPlane, errCluster}) + if err != nil { return err } @@ -283,12 +297,12 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope } // reconcileInfrastructureCluster reconciles the desired state of the InfrastructureCluster object. -func (r *Reconciler) reconcileInfrastructureCluster(ctx context.Context, s *scope.Scope) error { +func (r *Reconciler) reconcileInfrastructureCluster(ctx context.Context, s *scope.Scope) (bool, error) { ctx, _ = tlog.LoggerFrom(ctx).WithObject(s.Desired.InfrastructureCluster).Into(ctx) ignorePaths, err := contract.InfrastructureCluster().IgnorePaths(s.Desired.InfrastructureCluster) if err != nil { - return errors.Wrap(err, "failed to calculate ignore paths") + return false, errors.Wrap(err, "failed to calculate ignore paths") } return r.reconcileReferencedObject(ctx, reconcileReferencedObjectInput{ @@ -301,14 +315,14 @@ func (r *Reconciler) reconcileInfrastructureCluster(ctx context.Context, s *scop // reconcileControlPlane works to bring the current state of a managed topology in line with the desired state. This involves // updating the cluster where needed. -func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) error { +func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) (bool, error) { // If the ControlPlane has defined a current or desired MachineHealthCheck attempt to reconcile it. // MHC changes are not Kubernetes version dependent, therefore proceed with MHC reconciliation // even if the Control Plane is pending an upgrade. if s.Desired.ControlPlane.MachineHealthCheck != nil || s.Current.ControlPlane.MachineHealthCheck != nil { // Reconcile the current and desired state of the MachineHealthCheck. if err := r.reconcileMachineHealthCheck(ctx, s.Current.ControlPlane.MachineHealthCheck, s.Desired.ControlPlane.MachineHealthCheck); err != nil { - return err + return false, err } } @@ -316,7 +330,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) // Do not reconcile the control plane yet to avoid updating the control plane while it is still pending a // version upgrade. This will prevent the control plane from performing a double rollout. if s.UpgradeTracker.ControlPlane.IsPendingUpgrade { - return nil + return false, nil } // If the clusterClass mandates the controlPlane has infrastructureMachines, reconcile it. if s.Blueprint.HasControlPlaneInfrastructureMachine() { @@ -324,7 +338,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) cpInfraRef, err := contract.ControlPlane().MachineTemplate().InfrastructureRef().Get(s.Desired.ControlPlane.Object) if err != nil { - return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate}) + return false, errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate}) } // Create or update the MachineInfrastructureTemplate of the control plane. @@ -337,25 +351,26 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) templateNamePrefix: controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name), }, ); err != nil { - return err + return false, err } // The controlPlaneObject.Spec.machineTemplate.infrastructureRef has to be updated in the desired object err = contract.ControlPlane().MachineTemplate().InfrastructureRef().Set(s.Desired.ControlPlane.Object, refToUnstructured(cpInfraRef)) if err != nil { - return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object}) + return false, errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object}) } } // Create or update the ControlPlaneObject for the ControlPlaneState. ctx, _ = tlog.LoggerFrom(ctx).WithObject(s.Desired.ControlPlane.Object).Into(ctx) - if err := r.reconcileReferencedObject(ctx, reconcileReferencedObjectInput{ + created, err := r.reconcileReferencedObject(ctx, reconcileReferencedObjectInput{ cluster: s.Current.Cluster, current: s.Current.ControlPlane.Object, desired: s.Desired.ControlPlane.Object, versionGetter: contract.ControlPlane().Version().Get, - }); err != nil { - return err + }) + if err != nil { + return created, err } // If the controlPlane has infrastructureMachines and the InfrastructureMachineTemplate has changed on this reconcile @@ -364,7 +379,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) if s.Blueprint.HasControlPlaneInfrastructureMachine() && s.Current.ControlPlane.InfrastructureMachineTemplate != nil { if s.Current.ControlPlane.InfrastructureMachineTemplate.GetName() != s.Desired.ControlPlane.InfrastructureMachineTemplate.GetName() { if err := r.Client.Delete(ctx, s.Current.ControlPlane.InfrastructureMachineTemplate); err != nil { - return errors.Wrapf(err, "failed to delete oldinfrastructure machine template %s of control plane %s", + return created, errors.Wrapf(err, "failed to delete oldinfrastructure machine template %s of control plane %s", tlog.KObj{Obj: s.Current.ControlPlane.InfrastructureMachineTemplate}, tlog.KObj{Obj: s.Current.ControlPlane.Object}, ) @@ -372,7 +387,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) } } - return nil + return created, nil } // reconcileMachineHealthCheck creates, updates, deletes or leaves untouched a MachineHealthCheck depending on the difference between the @@ -824,7 +839,7 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp * log := tlog.LoggerFrom(ctx).WithMachinePool(mp.Object) cluster := s.Current.Cluster infraCtx, _ := log.WithObject(mp.InfrastructureMachinePoolObject).Into(ctx) - if err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ + if _, err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ cluster: cluster, desired: mp.InfrastructureMachinePoolObject, }); err != nil { @@ -832,7 +847,7 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp * } bootstrapCtx, _ := log.WithObject(mp.BootstrapObject).Into(ctx) - if err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{ + if _, err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{ cluster: cluster, desired: mp.BootstrapObject, }); err != nil { @@ -883,7 +898,7 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, curr cluster := s.Current.Cluster infraCtx, _ := log.WithObject(desiredMP.InfrastructureMachinePoolObject).Into(ctx) - if err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ + if _, err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ cluster: cluster, current: currentMP.InfrastructureMachinePoolObject, desired: desiredMP.InfrastructureMachinePoolObject, @@ -892,7 +907,7 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, curr } bootstrapCtx, _ := log.WithObject(desiredMP.BootstrapObject).Into(ctx) - if err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{ + if _, err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{ cluster: cluster, current: currentMP.BootstrapObject, desired: desiredMP.BootstrapObject, @@ -1022,7 +1037,7 @@ type reconcileReferencedObjectInput struct { // reconcileReferencedObject reconciles the desired state of the referenced object. // NOTE: After a referenced object is created it is assumed that the reference should // never change (only the content of the object can eventually change). Thus, we are checking for strict compatibility. -func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcileReferencedObjectInput) error { +func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcileReferencedObjectInput) (bool, error) { log := tlog.LoggerFrom(ctx) // If there is no current object, create it. @@ -1030,36 +1045,36 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile log.Infof("Creating %s", tlog.KObj{Obj: in.desired}) helper, err := r.patchHelperFactory(ctx, nil, in.desired, structuredmerge.IgnorePaths(in.ignorePaths)) if err != nil { - return errors.Wrap(createErrorWithoutObjectName(ctx, err, in.desired), "failed to create patch helper") + return false, errors.Wrap(createErrorWithoutObjectName(ctx, err, in.desired), "failed to create patch helper") } if err := helper.Patch(ctx); err != nil { - return createErrorWithoutObjectName(ctx, err, in.desired) + return false, createErrorWithoutObjectName(ctx, err, in.desired) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired}) - return nil + return true, nil } // Check if the current and desired referenced object are compatible. if allErrs := check.ObjectsAreStrictlyCompatible(in.current, in.desired); len(allErrs) > 0 { - return allErrs.ToAggregate() + return false, allErrs.ToAggregate() } // Check differences between current and desired state, and eventually patch the current object. patchHelper, err := r.patchHelperFactory(ctx, in.current, in.desired, structuredmerge.IgnorePaths(in.ignorePaths)) if err != nil { - return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) + return false, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) } if !patchHelper.HasChanges() { log.V(3).Infof("No changes for %s", tlog.KObj{Obj: in.desired}) - return nil + return false, nil } log.Infof("Patching %s", tlog.KObj{Obj: in.desired}) if err := patchHelper.Patch(ctx); err != nil { - return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.current}) + return false, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.current}) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, updateEventReason, "Updated %q%s", tlog.KObj{Obj: in.desired}, logUnstructuredVersionChange(in.current, in.desired, in.versionGetter)) - return nil + return false, nil } func logUnstructuredVersionChange(current, desired *unstructured.Unstructured, versionGetter unstructuredVersionGetter) string { diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 7dffa9f535be..e437a38803cc 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -1202,14 +1202,16 @@ func TestReconcileInfrastructureCluster(t *testing.T) { externalChanges string desired *unstructured.Unstructured want *unstructured.Unstructured + wantCreated bool wantErr bool }{ { - name: "Should create desired InfrastructureCluster if the current does not exists yet", - original: nil, - desired: clusterInfrastructure1, - want: clusterInfrastructure1, - wantErr: false, + name: "Should create desired InfrastructureCluster if the current does not exists yet", + original: nil, + desired: clusterInfrastructure1, + want: clusterInfrastructure1, + wantCreated: true, + wantErr: false, }, { name: "No-op if current InfrastructureCluster is equal to desired", @@ -1280,12 +1282,13 @@ func TestReconcileInfrastructureCluster(t *testing.T) { patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), recorder: env.GetEventRecorderFor("test"), } - err = r.reconcileInfrastructureCluster(ctx, s) + created, err := r.reconcileInfrastructureCluster(ctx, s) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return } g.Expect(err).ToNot(HaveOccurred()) + g.Expect(created).To(Equal(tt.wantCreated)) got := tt.want.DeepCopy() // this is required otherwise Get will modify tt.want err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(tt.want), got) @@ -1383,17 +1386,19 @@ func TestReconcileControlPlane(t *testing.T) { upgradeTracker *scope.UpgradeTracker desired *scope.ControlPlaneState want *scope.ControlPlaneState + wantCreated bool wantRotation bool wantErr bool }{ // Testing reconciliation of a control plane without machines. { - name: "Should create desired ControlPlane without machine infrastructure if the current does not exist", - class: ccWithoutControlPlaneInfrastructure, - original: nil, - desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, - wantErr: false, + name: "Should create desired ControlPlane without machine infrastructure if the current does not exist", + class: ccWithoutControlPlaneInfrastructure, + original: nil, + desired: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithoutInfrastructure.DeepCopy()}, + wantCreated: true, + wantErr: false, }, { name: "Should update the ControlPlane without machine infrastructure", @@ -1440,12 +1445,13 @@ func TestReconcileControlPlane(t *testing.T) { // Testing reconciliation of a control plane with machines. { - name: "Should create desired ControlPlane with machine infrastructure if the current does not exist", - class: ccWithControlPlaneInfrastructure, - original: nil, - desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - wantErr: false, + name: "Should create desired ControlPlane with machine infrastructure if the current does not exist", + class: ccWithControlPlaneInfrastructure, + original: nil, + desired: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithInfrastructure.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + wantCreated: true, + wantErr: false, }, { name: "Should rotate machine infrastructure in case of changes to the desired template", @@ -1559,12 +1565,13 @@ func TestReconcileControlPlane(t *testing.T) { } // Run reconcileControlPlane with the states created in the initial section of the test. - err = r.reconcileControlPlane(ctx, s) + created, err := r.reconcileControlPlane(ctx, s) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return } g.Expect(err).ToNot(HaveOccurred()) + g.Expect(created).To(Equal(tt.wantCreated)) // Create ControlPlane object for fetching data into gotControlPlaneObject := builder.TestControlPlane("", "").Build() @@ -1814,7 +1821,7 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { } // Run reconcileControlPlane with the states created in the initial section of the test. - err = r.reconcileControlPlane(ctx, s) + _, err = r.reconcileControlPlane(ctx, s) g.Expect(err).ToNot(HaveOccurred()) gotCP := s.Desired.ControlPlane.Object.DeepCopy() @@ -2554,7 +2561,8 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { // desired is the desired control-plane object handed over to reconcileReferencedObject. desired object // want is the expected control-plane object after calling reconcileReferencedObject. - want object + want object + wantCreated bool } tests := []struct { @@ -2595,6 +2603,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { }, }, }, + wantCreated: true, }, reconcileStep{ name: "Drop enable-hostpath-provisioner", @@ -2639,6 +2648,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { }, }, }, + wantCreated: true, }, reconcileStep{ name: "Drop the label with dots", @@ -2686,6 +2696,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { "foo": "ccValue", }, }, + wantCreated: true, }, externalStep{ name: "User changes value", @@ -2745,6 +2756,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { }, }, }, + wantCreated: true, }, externalStep{ name: "User adds an additional extraArg", @@ -2807,6 +2819,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { "machineTemplate": map[string]interface{}{}, }, }, + wantCreated: true, }, externalStep{ name: "User adds an additional object", @@ -2989,12 +3002,14 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { s.Desired.ControlPlane.Object.Object["spec"] = step.desired.spec } - // Execute a reconcile.0 - g.Expect(r.reconcileReferencedObject(ctx, reconcileReferencedObjectInput{ + // Execute a reconcile + created, err := r.reconcileReferencedObject(ctx, reconcileReferencedObjectInput{ cluster: s.Current.Cluster, current: s.Current.ControlPlane.Object, desired: s.Desired.ControlPlane.Object, - })).To(Succeed()) + }) + g.Expect(err).To(Succeed()) + g.Expect(created).To(Equal(step.wantCreated)) // Build the object for comparison. want := &unstructured.Unstructured{ @@ -3221,6 +3236,159 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { } } +func TestReconcileState(t *testing.T) { + t.Run("Cluster get reconciled with infrastructure Ref only when reconcileInfrastructureCluster pass and reconcileControlPlane fails ", func(t *testing.T) { + g := NewWithT(t) + + currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() + + infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() + controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() + desiredCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithInfrastructureCluster(infrastructureCluster). + WithControlPlane(controlPlane). + Build() + + // cluster requires a UID because reconcileClusterShim will create a cluster shim + // which has the cluster set as Owner in an OwnerReference. + // A valid OwnerReferences requires a uid. + currentdCluster.SetUID("foo") + + // NOTE: it is ok to use create given that the Cluster are created by user. + g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed()) + + s := scope.New(currentdCluster) + s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}} + s.Current.ControlPlane = &scope.ControlPlaneState{} + s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}} + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-cluster") + g.Expect(err).ToNot(HaveOccurred()) + prepareControlPlaneState(g, s.Desired.ControlPlane, namespace.GetName()) + + // Force reconcile control plane to fail + controlPlane.SetNamespace("do-not-exist") + + r := Reconciler{ + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + err = r.reconcileState(ctx, s) + g.Expect(err).To(HaveOccurred()) + + got := currentdCluster.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil()) + g.Expect(got.Spec.ControlPlaneRef).To(BeNil()) + + g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, currentdCluster)).To(Succeed()) + }) + t.Run("Cluster get reconciled with both infrastructure Ref and control plane ref when both reconcileInfrastructureCluster and reconcileControlPlane pass", func(t *testing.T) { + g := NewWithT(t) + + currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() + + infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() + controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() + desiredCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithInfrastructureCluster(infrastructureCluster). + WithControlPlane(controlPlane). + Build() + + // cluster requires a UID because reconcileClusterShim will create a cluster shim + // which has the cluster set as Owner in an OwnerReference. + // A valid OwnerReferences requires a uid. + currentdCluster.SetUID("foo") + + // NOTE: it is ok to use create given that the Cluster are created by user. + g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed()) + + s := scope.New(currentdCluster) + s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}} + s.Current.ControlPlane = &scope.ControlPlaneState{} + s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}} + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-cluster") + g.Expect(err).ToNot(HaveOccurred()) + prepareControlPlaneState(g, s.Desired.ControlPlane, namespace.GetName()) + + r := Reconciler{ + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + err = r.reconcileState(ctx, s) + g.Expect(err).ToNot(HaveOccurred()) + + got := currentdCluster.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil()) + g.Expect(got.Spec.ControlPlaneRef).ToNot(BeNil()) + + g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentdCluster)).To(Succeed()) + }) + t.Run("Cluster does not get reconciled when reconcileControlPlane fails and infrastructure Ref is set", func(t *testing.T) { + g := NewWithT(t) + + infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() + controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() + + currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithInfrastructureCluster(infrastructureCluster). + Build() + + desiredCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithInfrastructureCluster(infrastructureCluster). + WithControlPlane(controlPlane). + Build() + + // cluster requires a UID because reconcileClusterShim will create a cluster shim + // which has the cluster set as Owner in an OwnerReference. + // A valid OwnerReferences requires a uid. + currentdCluster.SetUID("foo") + + // NOTE: it is ok to use create given that the Cluster are created by user. + g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed()) + + s := scope.New(currentdCluster) + s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}} + s.Current.ControlPlane = &scope.ControlPlaneState{} + s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}} + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-cluster") + g.Expect(err).ToNot(HaveOccurred()) + prepareControlPlaneState(g, s.Desired.ControlPlane, namespace.GetName()) + + // Force reconcile control plane to fail + controlPlane.SetNamespace("do-not-exist") + + r := Reconciler{ + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + err = r.reconcileState(ctx, s) + g.Expect(err).To(HaveOccurred()) + + got := currentdCluster.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil()) + g.Expect(got.Spec.ControlPlaneRef).To(BeNil()) + + g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentdCluster)).To(Succeed()) + }) +} + func newFakeMachineDeploymentTopologyState(name string, infrastructureMachineTemplate, bootstrapTemplate *unstructured.Unstructured, machineHealthCheck *clusterv1.MachineHealthCheck) *scope.MachineDeploymentState { mdState := &scope.MachineDeploymentState{ Object: builder.MachineDeployment(metav1.NamespaceDefault, name). From 5f141e815f8603323448f9bdaca6a12deb36b012 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 18 Mar 2024 21:49:26 +0100 Subject: [PATCH 2/2] Best effort cleanup of referenced templates/objects --- .../topology/cluster/desired_state.go | 13 +- .../topology/cluster/desired_state_test.go | 9 +- .../topology/cluster/reconcile_state.go | 209 ++++++++++--- .../topology/cluster/reconcile_state_test.go | 285 ++++++++++++++++-- 4 files changed, 452 insertions(+), 64 deletions(-) diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index 3e6542d6d062..0060d0849f95 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -107,7 +107,7 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (* ctx, desiredState.ControlPlane.Object, selectorForControlPlaneMHC(), - s.Current.Cluster.Name, + s.Current.Cluster, s.Blueprint.ControlPlaneMachineHealthCheckClass()) } @@ -796,7 +796,7 @@ func computeMachineDeployment(ctx context.Context, s *scope.Scope, machineDeploy ctx, desiredMachineDeploymentObj, selectorForMachineDeploymentMHC(desiredMachineDeploymentObj), - s.Current.Cluster.Name, + s.Current.Cluster, s.Blueprint.MachineDeploymentMachineHealthCheckClass(&machineDeploymentTopology)) } return desiredMachineDeployment, nil @@ -1357,7 +1357,7 @@ func ownerReferenceTo(obj client.Object, gvk schema.GroupVersionKind) *metav1.Ow } } -func computeMachineHealthCheck(ctx context.Context, healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass) *clusterv1.MachineHealthCheck { +func computeMachineHealthCheck(ctx context.Context, healthCheckTarget client.Object, selector *metav1.LabelSelector, cluster *clusterv1.Cluster, check *clusterv1.MachineHealthCheckClass) *clusterv1.MachineHealthCheck { // Create a MachineHealthCheck with the spec given in the ClusterClass. mhc := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ @@ -1370,9 +1370,14 @@ func computeMachineHealthCheck(ctx context.Context, healthCheckTarget client.Obj Labels: map[string]string{ clusterv1.ClusterTopologyOwnedLabel: "", }, + // Note: we are adding an ownerRef to Cluster so the MHC will be automatically garbage collected + // in case deletion is triggered before an object reconcile happens. + OwnerReferences: []metav1.OwnerReference{ + *ownerReferenceTo(cluster, clusterv1.GroupVersion.WithKind("Cluster")), + }, }, Spec: clusterv1.MachineHealthCheckSpec{ - ClusterName: clusterName, + ClusterName: cluster.Name, Selector: *selector, UnhealthyConditions: check.UnhealthyConditions, MaxUnhealthy: check.MaxUnhealthy, diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index a4d1e3d5e4bc..c9d5179ea466 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -2874,7 +2874,7 @@ func Test_computeMachineHealthCheck(t *testing.T) { "foo": "bar", }} healthCheckTarget := builder.MachineDeployment("ns1", "md1").Build() - clusterName := "cluster1" + cluster := builder.Cluster("ns1", "cluster1").Build() want := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ APIVersion: clusterv1.GroupVersion.String(), @@ -2888,9 +2888,12 @@ func Test_computeMachineHealthCheck(t *testing.T) { "cluster.x-k8s.io/cluster-name": "cluster1", clusterv1.ClusterTopologyOwnedLabel: "", }, + OwnerReferences: []metav1.OwnerReference{ + *ownerReferenceTo(cluster, clusterv1.GroupVersion.WithKind("Cluster")), + }, }, Spec: clusterv1.MachineHealthCheckSpec{ - ClusterName: "cluster1", + ClusterName: cluster.Name, Selector: metav1.LabelSelector{MatchLabels: map[string]string{ "foo": "bar", }}, @@ -2916,7 +2919,7 @@ func Test_computeMachineHealthCheck(t *testing.T) { t.Run("set all fields correctly", func(t *testing.T) { g := NewWithT(t) - got := computeMachineHealthCheck(ctx, healthCheckTarget, selector, clusterName, mhcSpec) + got := computeMachineHealthCheck(ctx, healthCheckTarget, selector, cluster, mhcSpec) g.Expect(got).To(BeComparableTo(want), cmp.Diff(got, want)) }) diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 583d37c6bec4..510bd1a65b65 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -46,6 +46,7 @@ import ( "sigs.k8s.io/cluster-api/internal/hooks" tlog "sigs.k8s.io/cluster-api/internal/log" "sigs.k8s.io/cluster-api/internal/topology/check" + "sigs.k8s.io/cluster-api/util" ) const ( @@ -89,7 +90,7 @@ func (r *Reconciler) reconcileState(ctx context.Context, s *scope.Scope) error { return errControlPlane } - // In this case (reconcileInfrastructureCluster passed reporting creation of the infrastructure cluster object, reconcileControlPlane - which is expected to create the control plane object - failed), + // In this case (reconcileInfrastructureCluster reported creation of the infrastructure cluster object, reconcileControlPlane - which is expected to create the control plane object - failed), // if the creation of the control plane actually did not happen, blank out ControlPlaneRef from desired cluster. if s.Current.Cluster.Spec.ControlPlaneRef == nil && !createdControlPlane { s.Desired.Cluster.Spec.ControlPlaneRef = nil @@ -139,14 +140,18 @@ func (r *Reconciler) reconcileClusterShim(ctx context.Context, s *scope.Scope) e shim.APIVersion = corev1.SchemeGroupVersion.String() // Add the shim as a temporary owner for the InfrastructureCluster. - ownerRefs := s.Desired.InfrastructureCluster.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret"))) - s.Desired.InfrastructureCluster.SetOwnerReferences(ownerRefs) + s.Desired.InfrastructureCluster.SetOwnerReferences( + util.EnsureOwnerRef(s.Desired.InfrastructureCluster.GetOwnerReferences(), + *ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret")), + ), + ) // Add the shim as a temporary owner for the ControlPlane. - ownerRefs = s.Desired.ControlPlane.Object.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret"))) - s.Desired.ControlPlane.Object.SetOwnerReferences(ownerRefs) + s.Desired.ControlPlane.Object.SetOwnerReferences( + util.EnsureOwnerRef(s.Desired.ControlPlane.Object.GetOwnerReferences(), + *ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret")), + ), + ) } // If the InfrastructureCluster and the ControlPlane objects have been already created @@ -333,6 +338,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) return false, nil } // If the clusterClass mandates the controlPlane has infrastructureMachines, reconcile it. + infrastructureMachineCleanupFunc := func() {} if s.Blueprint.HasControlPlaneInfrastructureMachine() { ctx, _ := tlog.LoggerFrom(ctx).WithObject(s.Desired.ControlPlane.InfrastructureMachineTemplate).Into(ctx) @@ -342,21 +348,36 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) } // Create or update the MachineInfrastructureTemplate of the control plane. - if err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ + createdInfrastructureTemplate, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ cluster: s.Current.Cluster, ref: cpInfraRef, current: s.Current.ControlPlane.InfrastructureMachineTemplate, desired: s.Desired.ControlPlane.InfrastructureMachineTemplate, compatibilityChecker: check.ObjectsAreCompatible, templateNamePrefix: controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name), - }, - ); err != nil { + }) + if err != nil { return false, err } + if createdInfrastructureTemplate { + infrastructureMachineCleanupFunc = func() { + // Best effort cleanup of the InfrastructureMachineTemplate; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, s.Desired.ControlPlane.InfrastructureMachineTemplate); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(s.Desired.ControlPlane.InfrastructureMachineTemplate.GetObjectKind().GroupVersionKind().Kind, s.Desired.ControlPlane.InfrastructureMachineTemplate.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup InfrastructureMachineTemplate for control plane while handling creation or update error. The object will be garbage collected when the cluster is deleted.") + } + } + } + // The controlPlaneObject.Spec.machineTemplate.infrastructureRef has to be updated in the desired object err = contract.ControlPlane().MachineTemplate().InfrastructureRef().Set(s.Desired.ControlPlane.Object, refToUnstructured(cpInfraRef)) if err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate (only on creation). + infrastructureMachineCleanupFunc() return false, errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object}) } } @@ -370,6 +391,8 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) versionGetter: contract.ControlPlane().Version().Get, }) if err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate (only on creation). + infrastructureMachineCleanupFunc() return created, err } @@ -581,28 +604,66 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, s *scope.Scope log := tlog.LoggerFrom(ctx).WithMachineDeployment(md.Object) cluster := s.Current.Cluster infraCtx, _ := log.WithObject(md.InfrastructureMachineTemplate).Into(ctx) - if err := r.reconcileReferencedTemplate(infraCtx, reconcileReferencedTemplateInput{ + infrastructureMachineCleanupFunc := func() {} + createdInfra, err := r.reconcileReferencedTemplate(infraCtx, reconcileReferencedTemplateInput{ cluster: cluster, desired: md.InfrastructureMachineTemplate, - }); err != nil { + }) + if err != nil { return errors.Wrapf(err, "failed to create %s", md.Object.Kind) } + if createdInfra { + infrastructureMachineCleanupFunc = func() { + // Best effort cleanup of the InfrastructureMachineTemplate; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, md.InfrastructureMachineTemplate); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(md.InfrastructureMachineTemplate.GetObjectKind().GroupVersionKind().Kind, md.InfrastructureMachineTemplate.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup InfrastructureMachineTemplate for MachineDeployment while handling creation error. The object will be garbage collected when the cluster is deleted.") + } + } + } + bootstrapCtx, _ := log.WithObject(md.BootstrapTemplate).Into(ctx) - if err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{ + bootstrapCleanupFunc := func() {} + createdBootstrap, err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{ cluster: cluster, desired: md.BootstrapTemplate, - }); err != nil { + }) + if err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate (only on creation). + infrastructureMachineCleanupFunc() return errors.Wrapf(err, "failed to create %s", md.Object.Kind) } + if createdBootstrap { + bootstrapCleanupFunc = func() { + // Best effort cleanup of the BootstrapTemplate; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, md.BootstrapTemplate); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(md.BootstrapTemplate.GetObjectKind().GroupVersionKind().Kind, md.BootstrapTemplate.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup BootstrapTemplate for MachineDeployment while handling creation error. The object will be garbage collected when the cluster is deleted.") + } + } + } + log = log.WithObject(md.Object) log.Infof(fmt.Sprintf("Creating %s", tlog.KObj{Obj: md.Object})) helper, err := r.patchHelperFactory(ctx, nil, md.Object) if err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate & BootstrapTemplate (only on creation). + infrastructureMachineCleanupFunc() + bootstrapCleanupFunc() return createErrorWithoutObjectName(ctx, err, md.Object) } if err := helper.Patch(ctx); err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate & BootstrapTemplate (only on creation). + infrastructureMachineCleanupFunc() + bootstrapCleanupFunc() return createErrorWithoutObjectName(ctx, err, md.Object) } r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: md.Object}) @@ -655,33 +716,68 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, s *scope.Scope cluster := s.Current.Cluster infraCtx, _ := log.WithObject(desiredMD.InfrastructureMachineTemplate).Into(ctx) - if err := r.reconcileReferencedTemplate(infraCtx, reconcileReferencedTemplateInput{ + infrastructureMachineCleanupFunc := func() {} + createdInfra, err := r.reconcileReferencedTemplate(infraCtx, reconcileReferencedTemplateInput{ cluster: cluster, ref: &desiredMD.Object.Spec.Template.Spec.InfrastructureRef, current: currentMD.InfrastructureMachineTemplate, desired: desiredMD.InfrastructureMachineTemplate, templateNamePrefix: infrastructureMachineTemplateNamePrefix(cluster.Name, mdTopologyName), compatibilityChecker: check.ObjectsAreCompatible, - }); err != nil { + }) + if err != nil { return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMD.Object}) } + if createdInfra { + infrastructureMachineCleanupFunc = func() { + // Best effort cleanup of the InfrastructureMachineTemplate; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, desiredMD.InfrastructureMachineTemplate); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(desiredMD.InfrastructureMachineTemplate.GetObjectKind().GroupVersionKind().Kind, desiredMD.InfrastructureMachineTemplate.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup InfrastructureMachineTemplate for MachineDeployment while handling update error. The object will be garbage collected when the cluster is deleted.") + } + } + } + bootstrapCtx, _ := log.WithObject(desiredMD.BootstrapTemplate).Into(ctx) - if err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{ + bootstrapCleanupFunc := func() {} + createdBootstrap, err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{ cluster: cluster, ref: desiredMD.Object.Spec.Template.Spec.Bootstrap.ConfigRef, current: currentMD.BootstrapTemplate, desired: desiredMD.BootstrapTemplate, templateNamePrefix: bootstrapTemplateNamePrefix(cluster.Name, mdTopologyName), compatibilityChecker: check.ObjectsAreInTheSameNamespace, - }); err != nil { + }) + if err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate (only on template rotation). + infrastructureMachineCleanupFunc() return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMD.Object}) } + if createdBootstrap { + bootstrapCleanupFunc = func() { + // Best effort cleanup of the BootstrapTemplate; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, desiredMD.BootstrapTemplate); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(desiredMD.BootstrapTemplate.GetObjectKind().GroupVersionKind().Kind, desiredMD.BootstrapTemplate.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup BootstrapTemplate for MachineDeployment while handling update error. The object will be garbage collected when the cluster is deleted.") + } + } + } + // Check differences between current and desired MachineDeployment, and eventually patch the current object. log = log.WithObject(desiredMD.Object) patchHelper, err := r.patchHelperFactory(ctx, currentMD.Object, desiredMD.Object) if err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate & BootstrapTemplate (only on template rotation). + infrastructureMachineCleanupFunc() + bootstrapCleanupFunc() return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: currentMD.Object}) } if !patchHelper.HasChanges() { @@ -691,6 +787,9 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, s *scope.Scope log.Infof("Patching %s", tlog.KObj{Obj: currentMD.Object}) if err := patchHelper.Patch(ctx); err != nil { + // Best effort cleanup of the InfrastructureMachineTemplate & BootstrapTemplate (only on template rotation). + infrastructureMachineCleanupFunc() + bootstrapCleanupFunc() return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: currentMD.Object}) } r.recorder.Eventf(cluster, corev1.EventTypeNormal, updateEventReason, "Updated %q%s", tlog.KObj{Obj: currentMD.Object}, logMachineDeploymentVersionChange(currentMD.Object, desiredMD.Object)) @@ -839,28 +938,66 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp * log := tlog.LoggerFrom(ctx).WithMachinePool(mp.Object) cluster := s.Current.Cluster infraCtx, _ := log.WithObject(mp.InfrastructureMachinePoolObject).Into(ctx) - if _, err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ + infrastructureMachineMachinePoolCleanupFunc := func() {} + createdInfrastructureMachinePool, err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ cluster: cluster, desired: mp.InfrastructureMachinePoolObject, - }); err != nil { + }) + if err != nil { return errors.Wrapf(err, "failed to create %s", mp.Object.Kind) } + if createdInfrastructureMachinePool { + infrastructureMachineMachinePoolCleanupFunc = func() { + // Best effort cleanup of the InfrastructureMachinePool; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, mp.InfrastructureMachinePoolObject); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(mp.InfrastructureMachinePoolObject.GetObjectKind().GroupVersionKind().Kind, mp.InfrastructureMachinePoolObject.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup InfrastructureMachinePoolObject for MachinePool while handling creation error. The object will be garbage collected when the cluster is deleted.") + } + } + } + bootstrapCtx, _ := log.WithObject(mp.BootstrapObject).Into(ctx) - if _, err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{ + bootstrapCleanupFunc := func() {} + createdBootstrap, err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{ cluster: cluster, desired: mp.BootstrapObject, - }); err != nil { + }) + if err != nil { + // Best effort cleanup of the InfrastructureMachinePool (only on creation). + infrastructureMachineMachinePoolCleanupFunc() return errors.Wrapf(err, "failed to create %s", mp.Object.Kind) } + if createdBootstrap { + bootstrapCleanupFunc = func() { + // Best effort cleanup of the BootstrapConfig; + // If this fails, the object will be garbage collected when the cluster is deleted. + if err := r.Client.Delete(ctx, mp.BootstrapObject); err != nil { + log := tlog.LoggerFrom(ctx). + WithValues(mp.BootstrapObject.GetObjectKind().GroupVersionKind().Kind, mp.BootstrapObject.GetName()). + WithValues("err", err.Error()) + log.Infof("WARNING! Failed to cleanup BootstrapObject for MachinePool while handling creation error. The object will be garbage collected when the cluster is deleted.") + } + } + } + log = log.WithObject(mp.Object) log.Infof(fmt.Sprintf("Creating %s", tlog.KObj{Obj: mp.Object})) helper, err := r.patchHelperFactory(ctx, nil, mp.Object) if err != nil { + // Best effort cleanup of the InfrastructureMachinePool & BootstrapConfig (only on creation). + infrastructureMachineMachinePoolCleanupFunc() + bootstrapCleanupFunc() return createErrorWithoutObjectName(ctx, err, mp.Object) } if err := helper.Patch(ctx); err != nil { + // Best effort cleanup of the InfrastructureMachinePool & BootstrapConfig (only on creation). + infrastructureMachineMachinePoolCleanupFunc() + bootstrapCleanupFunc() return createErrorWithoutObjectName(ctx, err, mp.Object) } r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: mp.Object}) @@ -1035,6 +1172,7 @@ type reconcileReferencedObjectInput struct { } // reconcileReferencedObject reconciles the desired state of the referenced object. +// Returns true if the referencedObject is created. // NOTE: After a referenced object is created it is assumed that the reference should // never change (only the content of the object can eventually change). Thus, we are checking for strict compatibility. func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcileReferencedObjectInput) (bool, error) { @@ -1107,6 +1245,7 @@ type reconcileReferencedTemplateInput struct { } // reconcileReferencedTemplate reconciles the desired state of a referenced Template. +// Returns true if the referencedTemplate is created. // NOTE: According to Cluster API operational practices, when a referenced Template changes a template rotation is required: // 1. create a new Template // 2. update the reference @@ -1114,7 +1253,7 @@ type reconcileReferencedTemplateInput struct { // This function specifically takes care of the first step and updates the reference locally. So the remaining steps // can be executed afterwards. // NOTE: This func has a side effect in case of template rotation, changing both the desired object and the object reference. -func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconcileReferencedTemplateInput) error { +func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconcileReferencedTemplateInput) (bool, error) { log := tlog.LoggerFrom(ctx) // If there is no current object, create the desired object. @@ -1122,34 +1261,34 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci log.Infof("Creating %s", tlog.KObj{Obj: in.desired}) helper, err := r.patchHelperFactory(ctx, nil, in.desired) if err != nil { - return errors.Wrap(createErrorWithoutObjectName(ctx, err, in.desired), "failed to create patch helper") + return false, errors.Wrap(createErrorWithoutObjectName(ctx, err, in.desired), "failed to create patch helper") } if err := helper.Patch(ctx); err != nil { - return createErrorWithoutObjectName(ctx, err, in.desired) + return false, createErrorWithoutObjectName(ctx, err, in.desired) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired}) - return nil + return true, nil } if in.ref == nil { - return errors.Errorf("failed to rotate %s: ref should not be nil", in.desired.GroupVersionKind()) + return false, errors.Errorf("failed to rotate %s: ref should not be nil", in.desired.GroupVersionKind()) } // Check if the current and desired referenced object are compatible. if allErrs := in.compatibilityChecker(in.current, in.desired); len(allErrs) > 0 { - return allErrs.ToAggregate() + return false, allErrs.ToAggregate() } // Check differences between current and desired objects, and if there are changes eventually start the template rotation. patchHelper, err := r.patchHelperFactory(ctx, in.current, in.desired) if err != nil { - return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) + return false, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current}) } // Return if no changes are detected. if !patchHelper.HasChanges() { log.V(3).Infof("No changes for %s", tlog.KObj{Obj: in.desired}) - return nil + return false, nil } // If there are no changes in the spec, and thus only changes in metadata, instead of doing a full template @@ -1157,10 +1296,10 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci if !patchHelper.HasSpecChanges() { log.Infof("Patching %s", tlog.KObj{Obj: in.desired}) if err := patchHelper.Patch(ctx); err != nil { - return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired}) + return false, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired}) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, updateEventReason, "Updated %q (metadata changes)", tlog.KObj{Obj: in.desired}) - return nil + return false, nil } // Create the new template. @@ -1174,10 +1313,10 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci log.Infof("Creating %s", tlog.KObj{Obj: in.desired}) helper, err := r.patchHelperFactory(ctx, nil, in.desired) if err != nil { - return errors.Wrap(createErrorWithoutObjectName(ctx, err, in.desired), "failed to create patch helper") + return false, errors.Wrap(createErrorWithoutObjectName(ctx, err, in.desired), "failed to create patch helper") } if err := helper.Patch(ctx); err != nil { - return createErrorWithoutObjectName(ctx, err, in.desired) + return false, createErrorWithoutObjectName(ctx, err, in.desired) } r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q as a replacement for %q (template rotation)", tlog.KObj{Obj: in.desired}, in.ref.Name) @@ -1186,7 +1325,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci // TODO: find a way to make side effect more explicit in.ref.Name = newName - return nil + return true, nil } // createErrorWithoutObjectName removes the name of the object from the error message. As each new Create call involves an diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index e437a38803cc..89dcd98d97be 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -1665,6 +1665,64 @@ func TestReconcileControlPlane(t *testing.T) { } } +func TestReconcileControlPlaneCleanup(t *testing.T) { + infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1-cluster-class"). + WithSpecFields(map[string]interface{}{"spec.template.spec.foo": "foo"}). + Build() + ccWithControlPlaneInfrastructure := &scope.ControlPlaneBlueprint{InfrastructureMachineTemplate: infrastructureMachineTemplate} + + infrastructureMachineTemplateCopy := infrastructureMachineTemplate.DeepCopy() + infrastructureMachineTemplateCopy.SetName("infrav1-cluster") + controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "cp1"). + WithInfrastructureMachineTemplate(infrastructureMachineTemplateCopy). + WithSpecFields(map[string]interface{}{"spec.foo": "foo"}). + Build() + + t.Run("cleanup InfrastructureMachineTemplate in case of errors", func(t *testing.T) { + g := NewWithT(t) + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-control-plane") + g.Expect(err).ToNot(HaveOccurred()) + ccWithControlPlaneInfrastructure = prepareControlPlaneBluePrint(ccWithControlPlaneInfrastructure, namespace.GetName()) + + s := scope.New(builder.Cluster(namespace.GetName(), "cluster1").Build()) + s.Blueprint = &scope.ClusterBlueprint{ + ClusterClass: &clusterv1.ClusterClass{ + Spec: clusterv1.ClusterClassSpec{ + ControlPlane: clusterv1.ControlPlaneClass{ + MachineInfrastructure: &clusterv1.LocalObjectTemplate{ + Ref: contract.ObjToRef(infrastructureMachineTemplate), + }, + }, + }, + }, + } + s.Current.ControlPlane = &scope.ControlPlaneState{} + s.Desired = &scope.ClusterState{ + ControlPlane: &scope.ControlPlaneState{Object: controlPlane, InfrastructureMachineTemplate: infrastructureMachineTemplateCopy}, + } + s.Desired.ControlPlane = prepareControlPlaneState(g, s.Desired.ControlPlane, namespace.GetName()) + + // Force control plane creation to fail + s.Desired.ControlPlane.Object.SetNamespace("do-not-exist") + + r := Reconciler{ + Client: env, + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + created, err := r.reconcileControlPlane(ctx, s) + g.Expect(err).To(HaveOccurred()) + g.Expect(created).To(BeFalse()) + + gotInfrastructureMachineTemplate := infrastructureMachineTemplateCopy.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(infrastructureMachineTemplateCopy), gotInfrastructureMachineTemplate) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) +} + func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) { // Create InfrastructureMachineTemplates for test cases infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build() @@ -2202,6 +2260,130 @@ func TestReconcileMachineDeployments(t *testing.T) { } } +func TestReconcileMachineDeploymentsCleanup(t *testing.T) { + t.Run("cleanup InfrastructureMachineTemplate and BootstrapTemplate in case of errors on creation", func(t *testing.T) { + g := NewWithT(t) + + infrastructureMachineTemplate1 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-1").Build() + bootstrapTemplate1 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-1").Build() + md1 := newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate1, bootstrapTemplate1, nil) + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-machine-deployments") + g.Expect(err).ToNot(HaveOccurred()) + md1 = prepareMachineDeploymentState(md1, namespace.GetName()) + + s := scope.New(builder.Cluster(namespace.GetName(), "cluster-1").Build()) + s.Current.MachineDeployments = map[string]*scope.MachineDeploymentState{} + s.Desired = &scope.ClusterState{ + MachineDeployments: map[string]*scope.MachineDeploymentState{ + md1.Object.Name: md1, + }, + } + + // Force md creation to fail + s.Desired.MachineDeployments[md1.Object.Name].Object.Namespace = "do-not-exist" + + r := Reconciler{ + Client: env.GetClient(), + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + err = r.reconcileMachineDeployments(ctx, s) + g.Expect(err).To(HaveOccurred()) + + gotBootstrapTemplateRef := md1.Object.Spec.Template.Spec.Bootstrap.ConfigRef + gotBootstrapTemplate := unstructured.Unstructured{} + gotBootstrapTemplate.SetKind(gotBootstrapTemplateRef.Kind) + gotBootstrapTemplate.SetAPIVersion(gotBootstrapTemplateRef.APIVersion) + + err = env.GetAPIReader().Get(ctx, client.ObjectKey{ + Namespace: gotBootstrapTemplateRef.Namespace, + Name: gotBootstrapTemplateRef.Name, + }, &gotBootstrapTemplate) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + gotInfrastructureMachineTemplateRef := md1.Object.Spec.Template.Spec.InfrastructureRef + gotInfrastructureMachineTemplate := unstructured.Unstructured{} + gotInfrastructureMachineTemplate.SetKind(gotInfrastructureMachineTemplateRef.Kind) + gotInfrastructureMachineTemplate.SetAPIVersion(gotInfrastructureMachineTemplateRef.APIVersion) + + err = env.GetAPIReader().Get(ctx, client.ObjectKey{ + Namespace: gotInfrastructureMachineTemplateRef.Namespace, + Name: gotInfrastructureMachineTemplateRef.Name, + }, &gotInfrastructureMachineTemplate) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) + t.Run("cleanup InfrastructureMachineTemplate and BootstrapTemplate in case of errors on upgrade", func(t *testing.T) { + g := NewWithT(t) + + infrastructureMachineTemplate2 := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-2").Build() + bootstrapTemplate2 := builder.TestBootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-2").Build() + md2 := newFakeMachineDeploymentTopologyState("md-2", infrastructureMachineTemplate2, bootstrapTemplate2, nil) + + bootstrapTemplate2WithChanges := bootstrapTemplate2.DeepCopy() + g.Expect(unstructured.SetNestedField(bootstrapTemplate2WithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) + infrastructureMachineTemplate2WithChanges := infrastructureMachineTemplate2.DeepCopy() + g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate2WithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed()) + md2WithTemplateChanges := newFakeMachineDeploymentTopologyState(md2.Object.Name, infrastructureMachineTemplate2WithChanges, bootstrapTemplate2WithChanges, nil) + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-machine-deployments") + g.Expect(err).ToNot(HaveOccurred()) + md2 = prepareMachineDeploymentState(md2, namespace.GetName()) + md2WithTemplateChanges = prepareMachineDeploymentState(md2WithTemplateChanges, namespace.GetName()) + + s := scope.New(builder.Cluster(namespace.GetName(), "cluster-1").Build()) + s.Current.MachineDeployments = map[string]*scope.MachineDeploymentState{ + md2.Object.Name: md2, + } + s.Desired = &scope.ClusterState{ + MachineDeployments: map[string]*scope.MachineDeploymentState{ + md2WithTemplateChanges.Object.Name: md2WithTemplateChanges, + }, + } + + // Force md upgrade to fail + s.Desired.MachineDeployments[md2WithTemplateChanges.Object.Name].Object.Namespace = "do-not-exist" + + r := Reconciler{ + Client: env.GetClient(), + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + err = r.reconcileMachineDeployments(ctx, s) + g.Expect(err).To(HaveOccurred()) + + newBootstrapTemplateRef := md2WithTemplateChanges.Object.Spec.Template.Spec.Bootstrap.ConfigRef + newBootstrapTemplate := unstructured.Unstructured{} + newBootstrapTemplate.SetKind(newBootstrapTemplateRef.Kind) + newBootstrapTemplate.SetAPIVersion(newBootstrapTemplateRef.APIVersion) + + err = env.GetAPIReader().Get(ctx, client.ObjectKey{ + Namespace: newBootstrapTemplateRef.Namespace, + Name: newBootstrapTemplateRef.Name, + }, &newBootstrapTemplate) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + newInfrastructureMachineTemplateRef := md2WithTemplateChanges.Object.Spec.Template.Spec.InfrastructureRef + newInfrastructureMachineTemplate := unstructured.Unstructured{} + newInfrastructureMachineTemplate.SetKind(newInfrastructureMachineTemplateRef.Kind) + newInfrastructureMachineTemplate.SetAPIVersion(newInfrastructureMachineTemplateRef.APIVersion) + + err = env.GetAPIReader().Get(ctx, client.ObjectKey{ + Namespace: newInfrastructureMachineTemplateRef.Namespace, + Name: newInfrastructureMachineTemplateRef.Name, + }, &newInfrastructureMachineTemplate) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) +} + func TestReconcileMachinePools(t *testing.T) { defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)() @@ -2536,6 +2718,65 @@ func TestReconcileMachinePools(t *testing.T) { } } +func TestReconcileMachinePoolsCleanup(t *testing.T) { + infrastructureMachinePool1 := builder.TestInfrastructureMachinePool(metav1.NamespaceDefault, "infrastructure-machinepool-1").Build() + bootstrapConfig1 := builder.TestBootstrapConfig(metav1.NamespaceDefault, "bootstrap-config-1").Build() + mp1 := newFakeMachinePoolTopologyState("mp-1", infrastructureMachinePool1, bootstrapConfig1) + + t.Run("cleanup InfrastructureMachinePool and BootstrapConfig in case of errors", func(t *testing.T) { + g := NewWithT(t) + + // Create namespace and modify input to have correct namespace set + namespace, err := env.CreateNamespace(ctx, "reconcile-machine-pools") + g.Expect(err).ToNot(HaveOccurred()) + mp1 = prepareMachinePoolState(mp1, namespace.GetName()) + + s := scope.New(builder.Cluster(namespace.GetName(), "cluster-1").Build()) + s.Current.MachinePools = map[string]*scope.MachinePoolState{} + s.Desired = &scope.ClusterState{ + MachinePools: map[string]*scope.MachinePoolState{ + mp1.Object.Name: mp1, + }, + } + + // Force mp creation to fail + s.Desired.MachinePools[mp1.Object.Name].Object.Namespace = "do-not-exist" + + r := Reconciler{ + Client: env.GetClient(), + APIReader: env.GetAPIReader(), + patchHelperFactory: serverSideApplyPatchHelperFactory(env, ssa.NewCache()), + recorder: env.GetEventRecorderFor("test"), + } + err = r.reconcileMachinePools(ctx, s) + g.Expect(err).To(HaveOccurred()) + + gotBootstrapObjectRef := mp1.Object.Spec.Template.Spec.Bootstrap.ConfigRef + gotBootstrapObject := unstructured.Unstructured{} + gotBootstrapObject.SetKind(gotBootstrapObjectRef.Kind) + gotBootstrapObject.SetAPIVersion(gotBootstrapObjectRef.APIVersion) + + err = env.GetAPIReader().Get(ctx, client.ObjectKey{ + Namespace: gotBootstrapObjectRef.Namespace, + Name: gotBootstrapObjectRef.Name, + }, &gotBootstrapObject) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + gotInfrastructureMachinePoolObjectRef := mp1.Object.Spec.Template.Spec.InfrastructureRef + gotInfrastructureMachinePoolObject := unstructured.Unstructured{} + gotInfrastructureMachinePoolObject.SetKind(gotInfrastructureMachinePoolObjectRef.Kind) + gotInfrastructureMachinePoolObject.SetAPIVersion(gotInfrastructureMachinePoolObjectRef.APIVersion) + + err = env.GetAPIReader().Get(ctx, client.ObjectKey{ + Namespace: gotInfrastructureMachinePoolObjectRef.Namespace, + Name: gotInfrastructureMachinePoolObjectRef.Name, + }, &gotInfrastructureMachinePoolObject) + + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) +} + // TestReconcileReferencedObjectSequences tests multiple subsequent calls to reconcileReferencedObject // for a control-plane object to verify that the objects are reconciled as expected by tracking managed fields correctly. // NOTE: by Extension this tests validates managed field handling in mergePatches, and thus its usage in other parts of the @@ -3008,7 +3249,7 @@ func TestReconcileReferencedObjectSequences(t *testing.T) { current: s.Current.ControlPlane.Object, desired: s.Desired.ControlPlane.Object, }) - g.Expect(err).To(Succeed()) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(created).To(Equal(step.wantCreated)) // Build the object for comparison. @@ -3240,7 +3481,7 @@ func TestReconcileState(t *testing.T) { t.Run("Cluster get reconciled with infrastructure Ref only when reconcileInfrastructureCluster pass and reconcileControlPlane fails ", func(t *testing.T) { g := NewWithT(t) - currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() + currentCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() @@ -3252,12 +3493,12 @@ func TestReconcileState(t *testing.T) { // cluster requires a UID because reconcileClusterShim will create a cluster shim // which has the cluster set as Owner in an OwnerReference. // A valid OwnerReferences requires a uid. - currentdCluster.SetUID("foo") + currentCluster.SetUID("foo") // NOTE: it is ok to use create given that the Cluster are created by user. - g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed()) + g.Expect(env.CreateAndWait(ctx, currentCluster)).To(Succeed()) - s := scope.New(currentdCluster) + s := scope.New(currentCluster) s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}} s.Current.ControlPlane = &scope.ControlPlaneState{} s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}} @@ -3278,19 +3519,19 @@ func TestReconcileState(t *testing.T) { err = r.reconcileState(ctx, s) g.Expect(err).To(HaveOccurred()) - got := currentdCluster.DeepCopy() - err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got) + got := currentCluster.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentCluster), got) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil()) g.Expect(got.Spec.ControlPlaneRef).To(BeNil()) - g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, currentdCluster)).To(Succeed()) + g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, currentCluster)).To(Succeed()) }) t.Run("Cluster get reconciled with both infrastructure Ref and control plane ref when both reconcileInfrastructureCluster and reconcileControlPlane pass", func(t *testing.T) { g := NewWithT(t) - currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() + currentCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").Build() infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() @@ -3302,12 +3543,12 @@ func TestReconcileState(t *testing.T) { // cluster requires a UID because reconcileClusterShim will create a cluster shim // which has the cluster set as Owner in an OwnerReference. // A valid OwnerReferences requires a uid. - currentdCluster.SetUID("foo") + currentCluster.SetUID("foo") // NOTE: it is ok to use create given that the Cluster are created by user. - g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed()) + g.Expect(env.CreateAndWait(ctx, currentCluster)).To(Succeed()) - s := scope.New(currentdCluster) + s := scope.New(currentCluster) s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}} s.Current.ControlPlane = &scope.ControlPlaneState{} s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}} @@ -3325,14 +3566,14 @@ func TestReconcileState(t *testing.T) { err = r.reconcileState(ctx, s) g.Expect(err).ToNot(HaveOccurred()) - got := currentdCluster.DeepCopy() - err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got) + got := currentCluster.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentCluster), got) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil()) g.Expect(got.Spec.ControlPlaneRef).ToNot(BeNil()) - g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentdCluster)).To(Succeed()) + g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentCluster)).To(Succeed()) }) t.Run("Cluster does not get reconciled when reconcileControlPlane fails and infrastructure Ref is set", func(t *testing.T) { g := NewWithT(t) @@ -3340,7 +3581,7 @@ func TestReconcileState(t *testing.T) { infrastructureCluster := builder.TestInfrastructureCluster(metav1.NamespaceDefault, "infrastructure-cluster1").Build() controlPlane := builder.TestControlPlane(metav1.NamespaceDefault, "controlplane-cluster1").Build() - currentdCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + currentCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). WithInfrastructureCluster(infrastructureCluster). Build() @@ -3352,12 +3593,12 @@ func TestReconcileState(t *testing.T) { // cluster requires a UID because reconcileClusterShim will create a cluster shim // which has the cluster set as Owner in an OwnerReference. // A valid OwnerReferences requires a uid. - currentdCluster.SetUID("foo") + currentCluster.SetUID("foo") // NOTE: it is ok to use create given that the Cluster are created by user. - g.Expect(env.CreateAndWait(ctx, currentdCluster)).To(Succeed()) + g.Expect(env.CreateAndWait(ctx, currentCluster)).To(Succeed()) - s := scope.New(currentdCluster) + s := scope.New(currentCluster) s.Blueprint = &scope.ClusterBlueprint{ClusterClass: &clusterv1.ClusterClass{}} s.Current.ControlPlane = &scope.ControlPlaneState{} s.Desired = &scope.ClusterState{Cluster: desiredCluster, InfrastructureCluster: infrastructureCluster, ControlPlane: &scope.ControlPlaneState{Object: controlPlane}} @@ -3378,14 +3619,14 @@ func TestReconcileState(t *testing.T) { err = r.reconcileState(ctx, s) g.Expect(err).To(HaveOccurred()) - got := currentdCluster.DeepCopy() - err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentdCluster), got) + got := currentCluster.DeepCopy() + err = env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(currentCluster), got) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got.Spec.InfrastructureRef).ToNot(BeNil()) g.Expect(got.Spec.ControlPlaneRef).To(BeNil()) - g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentdCluster)).To(Succeed()) + g.Expect(env.CleanupAndWait(ctx, infrastructureCluster, controlPlane, currentCluster)).To(Succeed()) }) }