From 882a0e09bfc88e3d0449d79586a816b0b83bcb4d Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Tue, 12 Sep 2023 13:13:01 +0100 Subject: [PATCH 1/2] Reorder VSphereDeploymentZone reconcile to fix deletion Signed-off-by: killianmuldoon --- .../vspheredeploymentzone_controller.go | 44 +++++---- .../vspheredeploymentzone_controller_test.go | 94 ++++++++++++++++--- 2 files changed, 107 insertions(+), 31 deletions(-) diff --git a/controllers/vspheredeploymentzone_controller.go b/controllers/vspheredeploymentzone_controller.go index 5f75bd2aba..dd1ee22803 100644 --- a/controllers/vspheredeploymentzone_controller.go +++ b/controllers/vspheredeploymentzone_controller.go @@ -112,16 +112,6 @@ func (r vsphereDeploymentZoneReconciler) Reconcile(ctx context.Context, request return reconcile.Result{}, err } - failureDomain := &infrav1.VSphereFailureDomain{} - failureDomainKey := client.ObjectKey{Name: vsphereDeploymentZone.Spec.FailureDomain} - if err := r.Client.Get(ctx, failureDomainKey, failureDomain); err != nil { - if apierrors.IsNotFound(err) { - log.V(4).Info("Failure Domain not found, won't reconcile", "key", failureDomainKey) - return reconcile.Result{}, nil - } - return reconcile.Result{}, err - } - patchHelper, err := patch.NewHelper(vsphereDeploymentZone, r.Client) if err != nil { return reconcile.Result{}, errors.Wrapf( @@ -135,7 +125,6 @@ func (r vsphereDeploymentZoneReconciler) Reconcile(ctx context.Context, request vsphereDeploymentZoneContext := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: r.ControllerContext, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: failureDomain, Logger: log, PatchHelper: patchHelper, } @@ -160,6 +149,13 @@ func (r vsphereDeploymentZoneReconciler) Reconcile(ctx context.Context, request } func (r vsphereDeploymentZoneReconciler) reconcileNormal(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext) error { + failureDomain := &infrav1.VSphereFailureDomain{} + failureDomainKey := client.ObjectKey{Name: deploymentZoneCtx.VSphereDeploymentZone.Spec.FailureDomain} + // Return an error if the FailureDomain can not be retrieved. + if err := r.Client.Get(ctx, failureDomainKey, failureDomain); err != nil { + return errors.Wrap(err, "VSphereFailureDomain could not be retrieved") + } + deploymentZoneCtx.VSphereFailureDomain = failureDomain authSession, err := r.getVCenterSession(ctx, deploymentZoneCtx) if err != nil { deploymentZoneCtx.Logger.V(4).Error(err, "unable to create session") @@ -252,7 +248,6 @@ func (r vsphereDeploymentZoneReconciler) reconcileDelete(ctx context.Context, de machines := &clusterv1.MachineList{} if err := r.Client.List(ctx, machines); err != nil { - r.Logger.Error(err, "unable to list machines") return errors.Wrapf(err, "unable to list machines") } @@ -264,12 +259,26 @@ func (r vsphereDeploymentZoneReconciler) reconcileDelete(ctx context.Context, de }) if len(machinesUsingDeploymentZone) > 0 { machineNamesStr := util.MachinesAsString(machinesUsingDeploymentZone.SortedByCreationTimestamp()) - err := errors.Errorf("%s is currently in use by machines: %s", deploymentZoneCtx.VSphereDeploymentZone.Name, machineNamesStr) - r.Logger.Error(err, "Error deleting VSphereDeploymentZone", "name", deploymentZoneCtx.VSphereDeploymentZone.Name) + err := errors.Errorf("error deleting VSphereDeploymentZone: %s is currently in use by machines: %s", deploymentZoneCtx.VSphereDeploymentZone.Name, machineNamesStr) return err } - if err := updateOwnerReferences(ctx, deploymentZoneCtx.VSphereFailureDomain, r.Client, func() []metav1.OwnerReference { + failureDomain := &infrav1.VSphereFailureDomain{} + failureDomainKey := client.ObjectKey{Name: deploymentZoneCtx.VSphereDeploymentZone.Spec.FailureDomain} + // Return an error if the FailureDomain can not be retrieved. + if err := r.Client.Get(ctx, failureDomainKey, failureDomain); err != nil { + // If the VSphereFailureDomain is not found return early and remove the finalizer. + // This prevents early deletion of the VSphereFailureDomain from blocking VSphereDeploymentZone deletion. + if apierrors.IsNotFound(err) { + ctrlutil.RemoveFinalizer(deploymentZoneCtx.VSphereDeploymentZone, infrav1.DeploymentZoneFinalizer) + return nil + } + return err + } + deploymentZoneCtx.VSphereFailureDomain = failureDomain + + // Reconcile the deletion of the VSphereFailureDomain by removing ownerReferences and deleting if necessary. + if err := updateOwnerReferences(deploymentZoneCtx, deploymentZoneCtx.VSphereFailureDomain, r.Client, func() []metav1.OwnerReference { return clusterutilv1.RemoveOwnerRef(deploymentZoneCtx.VSphereFailureDomain.OwnerReferences, metav1.OwnerReference{ APIVersion: infrav1.GroupVersion.String(), Kind: deploymentZoneCtx.VSphereDeploymentZone.Kind, @@ -280,14 +289,13 @@ func (r vsphereDeploymentZoneReconciler) reconcileDelete(ctx context.Context, de } if len(deploymentZoneCtx.VSphereFailureDomain.OwnerReferences) == 0 { - deploymentZoneCtx.Logger.Info("deleting vsphereFailureDomain", "name", deploymentZoneCtx.VSphereFailureDomain.Name) + deploymentZoneCtx.Logger.Info("deleting VSphereFailureDomain", "name", deploymentZoneCtx.VSphereFailureDomain.Name) if err := r.Client.Delete(ctx, deploymentZoneCtx.VSphereFailureDomain); err != nil && !apierrors.IsNotFound(err) { - deploymentZoneCtx.Logger.Error(err, "failed to delete related %s %s", deploymentZoneCtx.VSphereFailureDomain.GroupVersionKind(), deploymentZoneCtx.VSphereFailureDomain.Name) + return errors.Wrapf(err, "failed to delete VSphereFailureDomain %s", deploymentZoneCtx.VSphereFailureDomain.Name) } } ctrlutil.RemoveFinalizer(deploymentZoneCtx.VSphereDeploymentZone, infrav1.DeploymentZoneFinalizer) - return nil } diff --git a/controllers/vspheredeploymentzone_controller_test.go b/controllers/vspheredeploymentzone_controller_test.go index a61422cd14..5c69af5e48 100644 --- a/controllers/vspheredeploymentzone_controller_test.go +++ b/controllers/vspheredeploymentzone_controller_test.go @@ -144,7 +144,6 @@ var _ = Describe("VSphereDeploymentZoneReconciler", func() { conditions.IsTrue(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) }, timeout).Should(BeTrue()) - By("sets the owner ref on the vsphereFailureDomain object") Expect(testEnv.Get(ctx, failureDomainKey, vsphereFailureDomain)).To(Succeed()) ownerRefs := vsphereFailureDomain.GetOwnerReferences() Expect(ownerRefs).To(HaveLen(1)) @@ -228,7 +227,6 @@ var _ = Describe("VSphereDeploymentZoneReconciler", func() { }) It("should delete the associated failure domain", func() { - By("deleting the vsphere deployment zone") Expect(testEnv.Delete(ctx, vsphereDeploymentZone)).To(Succeed()) Eventually(func() bool { @@ -257,7 +255,6 @@ var _ = Describe("VSphereDeploymentZoneReconciler", func() { machineUsingDeplZone.Spec.FailureDomain = pointer.String(vsphereDeploymentZone.Name) Expect(testEnv.Create(ctx, machineUsingDeplZone)).To(Succeed()) - By("deleting the vsphere deployment zone") Expect(testEnv.Delete(ctx, vsphereDeploymentZone)).To(Succeed()) Eventually(func() bool { @@ -275,10 +272,8 @@ var _ = Describe("VSphereDeploymentZoneReconciler", func() { machineBeingDeleted.Finalizers = []string{clusterv1.MachineFinalizer} Expect(testEnv.Create(ctx, machineBeingDeleted)).To(Succeed()) - By("Deleting the machine") Expect(testEnv.Delete(ctx, machineBeingDeleted)).To(Succeed()) - By("deleting the vsphere deployment zone") Expect(testEnv.Delete(ctx, vsphereDeploymentZone)).To(Succeed()) Eventually(func() bool { @@ -295,7 +290,6 @@ var _ = Describe("VSphereDeploymentZoneReconciler", func() { machineNotUsingDeplZone := createMachine("machine-without-zone", "cluster-without-zone", machineNamespace.Name, true) Expect(testEnv.Create(ctx, machineNotUsingDeplZone)).To(Succeed()) - By("deleting the vsphere deployment zone") Expect(testEnv.Delete(ctx, vsphereDeploymentZone)).To(Succeed()) Eventually(func() bool { @@ -398,7 +392,6 @@ func TestVSphereDeploymentZone_Reconcile(t *testing.T) { conditions.IsTrue(vsphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition) }, timeout).Should(BeTrue()) - By("sets the owner ref on the vsphereFailureDomain object") g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereFailureDomain), vsphereFailureDomain)).To(Succeed()) ownerRefs := vsphereFailureDomain.GetOwnerReferences() g.Expect(ownerRefs).To(HaveLen(1)) @@ -406,7 +399,6 @@ func TestVSphereDeploymentZone_Reconcile(t *testing.T) { g.Expect(ownerRefs[0].Kind).To(Equal("VSphereDeploymentZone")) }) - By("deleting deployment zone") t.Run("it should delete associated failure domain", func(t *testing.T) { g := NewWithT(t) @@ -466,8 +458,82 @@ func TestVSphereDeploymentZone_Reconcile(t *testing.T) { g.Expect(testEnv.Delete(ctx, vsphereDeploymentZone)).To(Succeed()) g.Eventually(func() bool { - err := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereFailureDomain), vsphereFailureDomain) - return apierrors.IsNotFound(err) + failureDomainErr := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereFailureDomain), vsphereFailureDomain) + deploymentZoneErr := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone) + return apierrors.IsNotFound(failureDomainErr) && apierrors.IsNotFound(deploymentZoneErr) + }, timeout).Should(BeTrue()) + }) + + t.Run("VSphereDeploymentZone should never become ready if VSphereFailureDomain does not exist", func(t *testing.T) { + g := NewWithT(t) + + vsphereDeploymentZone := &infrav1.VSphereDeploymentZone{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "blah-", + }, + Spec: infrav1.VSphereDeploymentZoneSpec{ + Server: simr.ServerURL().Host, + FailureDomain: "fd1", + ControlPlane: pointer.Bool(true), + PlacementConstraint: infrav1.PlacementConstraint{ + ResourcePool: "DC0_C0_RP1", + Folder: "/", + }, + }} + g.Expect(testEnv.Create(ctx, vsphereDeploymentZone)).To(Succeed()) + + defer func(do ...client.Object) { + g.Expect(testEnv.Cleanup(ctx, do...)).To(Succeed()) + }(vsphereDeploymentZone) + + g.Eventually(func() bool { + if err := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone); err != nil { + return false + } + return len(vsphereDeploymentZone.Finalizers) > 0 + }, timeout).Should(BeTrue()) + + g.Consistently(func(g Gomega) bool { + g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone)).To(Succeed()) + + return vsphereDeploymentZone.Status.Ready != nil && !*vsphereDeploymentZone.Status.Ready + }, timeout).Should(BeFalse()) + }) + + t.Run("it should delete the VSphereDeploymentZone when the associated VSphereFailureDomain does not exist", func(t *testing.T) { + g := NewWithT(t) + + vsphereDeploymentZone := &infrav1.VSphereDeploymentZone{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "blah-", + }, + Spec: infrav1.VSphereDeploymentZoneSpec{ + Server: simr.ServerURL().Host, + FailureDomain: "fd1", + ControlPlane: pointer.Bool(true), + PlacementConstraint: infrav1.PlacementConstraint{ + ResourcePool: "DC0_C0_RP1", + Folder: "/", + }, + }} + g.Expect(testEnv.Create(ctx, vsphereDeploymentZone)).To(Succeed()) + + defer func(do ...client.Object) { + g.Expect(testEnv.Cleanup(ctx, do...)).To(Succeed()) + }(vsphereDeploymentZone) + + g.Eventually(func() bool { + if err := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone); err != nil { + return false + } + return len(vsphereDeploymentZone.Finalizers) > 0 + }, timeout).Should(BeTrue()) + + g.Expect(testEnv.Delete(ctx, vsphereDeploymentZone)).To(Succeed()) + + g.Eventually(func() bool { + deploymentZoneErr := testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone) + return apierrors.IsNotFound(deploymentZoneErr) }, timeout).Should(BeTrue()) }) } @@ -592,6 +658,9 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { Name: "blah", Finalizers: []string{infrav1.DeploymentZoneFinalizer}, }, + Spec: infrav1.VSphereDeploymentZoneSpec{ + FailureDomain: "blah-fd", + }, } vsphereFailureDomain := &infrav1.VSphereFailureDomain{ @@ -692,7 +761,7 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { Name: vsphereDeploymentZone.Name, }} - t.Run("not used by other deployment zones", func(t *testing.T) { + t.Run("when not used by other deployment zones", func(t *testing.T) { mgmtContext := fake.NewControllerManagerContext(vsphereFailureDomain) controllerCtx := fake.NewControllerContext(mgmtContext) deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ @@ -708,7 +777,7 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) }) - t.Run("used by other deployment zones", func(t *testing.T) { + t.Run("when used by other deployment zones", func(t *testing.T) { vsphereFailureDomain.OwnerReferences = append(vsphereFailureDomain.OwnerReferences, metav1.OwnerReference{ APIVersion: infrav1.GroupVersion.String(), Kind: vsphereDeploymentZone.Kind, @@ -720,7 +789,6 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: vsphereFailureDomain, Logger: logr.Discard(), } From 448c4c4b0c8f6a2ab86cc13951f936e4099c79ce Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Wed, 20 Sep 2023 16:56:46 +0100 Subject: [PATCH 2/2] Remove VsphereFailureDomain from context Signed-off-by: killianmuldoon --- .../vspheredeploymentzone_controller.go | 25 ++++---- ...vspheredeploymentzone_controller_domain.go | 40 ++++++------- ...redeploymentzone_controller_domain_test.go | 57 +++++++++---------- .../vspheredeploymentzone_controller_test.go | 30 ++++------ pkg/context/vspheredeploymentzone_context.go | 6 -- pkg/taggable/get.go | 5 +- 6 files changed, 71 insertions(+), 92 deletions(-) diff --git a/controllers/vspheredeploymentzone_controller.go b/controllers/vspheredeploymentzone_controller.go index dd1ee22803..703752b3e9 100644 --- a/controllers/vspheredeploymentzone_controller.go +++ b/controllers/vspheredeploymentzone_controller.go @@ -155,8 +155,7 @@ func (r vsphereDeploymentZoneReconciler) reconcileNormal(ctx context.Context, de if err := r.Client.Get(ctx, failureDomainKey, failureDomain); err != nil { return errors.Wrap(err, "VSphereFailureDomain could not be retrieved") } - deploymentZoneCtx.VSphereFailureDomain = failureDomain - authSession, err := r.getVCenterSession(ctx, deploymentZoneCtx) + authSession, err := r.getVCenterSession(ctx, deploymentZoneCtx, failureDomain.Spec.Topology.Datacenter) if err != nil { deploymentZoneCtx.Logger.V(4).Error(err, "unable to create session") conditions.MarkFalse(deploymentZoneCtx.VSphereDeploymentZone, infrav1.VCenterAvailableCondition, infrav1.VCenterUnreachableReason, clusterv1.ConditionSeverityError, err.Error()) @@ -173,7 +172,7 @@ func (r vsphereDeploymentZoneReconciler) reconcileNormal(ctx context.Context, de conditions.MarkTrue(deploymentZoneCtx.VSphereDeploymentZone, infrav1.PlacementConstraintMetCondition) // reconcile the failure domain - if err := r.reconcileFailureDomain(ctx, deploymentZoneCtx); err != nil { + if err := r.reconcileFailureDomain(ctx, deploymentZoneCtx, failureDomain); err != nil { deploymentZoneCtx.Logger.V(4).Error(err, "failed to reconcile failure domain", "failureDomain", deploymentZoneCtx.VSphereDeploymentZone.Spec.FailureDomain) deploymentZoneCtx.VSphereDeploymentZone.Status.Ready = pointer.Bool(false) return errors.Wrapf(err, "failed to reconcile failure domain") @@ -205,10 +204,10 @@ func (r vsphereDeploymentZoneReconciler) reconcilePlacementConstraint(ctx contex return nil } -func (r vsphereDeploymentZoneReconciler) getVCenterSession(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext) (*session.Session, error) { +func (r vsphereDeploymentZoneReconciler) getVCenterSession(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, datacenter string) (*session.Session, error) { params := session.NewParams(). WithServer(deploymentZoneCtx.VSphereDeploymentZone.Spec.Server). - WithDatacenter(deploymentZoneCtx.VSphereFailureDomain.Spec.Topology.Datacenter). + WithDatacenter(datacenter). WithUserInfo(r.ControllerContext.Username, r.ControllerContext.Password). WithFeatures(session.Feature{ EnableKeepAlive: r.EnableKeepAlive, @@ -259,8 +258,7 @@ func (r vsphereDeploymentZoneReconciler) reconcileDelete(ctx context.Context, de }) if len(machinesUsingDeploymentZone) > 0 { machineNamesStr := util.MachinesAsString(machinesUsingDeploymentZone.SortedByCreationTimestamp()) - err := errors.Errorf("error deleting VSphereDeploymentZone: %s is currently in use by machines: %s", deploymentZoneCtx.VSphereDeploymentZone.Name, machineNamesStr) - return err + return errors.Errorf("error deleting VSphereDeploymentZone: %s is currently in use by machines: %s", deploymentZoneCtx.VSphereDeploymentZone.Name, machineNamesStr) } failureDomain := &infrav1.VSphereFailureDomain{} @@ -275,11 +273,10 @@ func (r vsphereDeploymentZoneReconciler) reconcileDelete(ctx context.Context, de } return err } - deploymentZoneCtx.VSphereFailureDomain = failureDomain // Reconcile the deletion of the VSphereFailureDomain by removing ownerReferences and deleting if necessary. - if err := updateOwnerReferences(deploymentZoneCtx, deploymentZoneCtx.VSphereFailureDomain, r.Client, func() []metav1.OwnerReference { - return clusterutilv1.RemoveOwnerRef(deploymentZoneCtx.VSphereFailureDomain.OwnerReferences, metav1.OwnerReference{ + if err := updateOwnerReferences(ctx, failureDomain, r.Client, func() []metav1.OwnerReference { + return clusterutilv1.RemoveOwnerRef(failureDomain.OwnerReferences, metav1.OwnerReference{ APIVersion: infrav1.GroupVersion.String(), Kind: deploymentZoneCtx.VSphereDeploymentZone.Kind, Name: deploymentZoneCtx.VSphereDeploymentZone.Name, @@ -288,10 +285,10 @@ func (r vsphereDeploymentZoneReconciler) reconcileDelete(ctx context.Context, de return err } - if len(deploymentZoneCtx.VSphereFailureDomain.OwnerReferences) == 0 { - deploymentZoneCtx.Logger.Info("deleting VSphereFailureDomain", "name", deploymentZoneCtx.VSphereFailureDomain.Name) - if err := r.Client.Delete(ctx, deploymentZoneCtx.VSphereFailureDomain); err != nil && !apierrors.IsNotFound(err) { - return errors.Wrapf(err, "failed to delete VSphereFailureDomain %s", deploymentZoneCtx.VSphereFailureDomain.Name) + if len(failureDomain.OwnerReferences) == 0 { + deploymentZoneCtx.Logger.Info("deleting VSphereFailureDomain", "name", failureDomain.Name) + if err := r.Client.Delete(ctx, failureDomain); err != nil && !apierrors.IsNotFound(err) { + return errors.Wrapf(err, "failed to delete VSphereFailureDomain %s", failureDomain.Name) } } diff --git a/controllers/vspheredeploymentzone_controller_domain.go b/controllers/vspheredeploymentzone_controller_domain.go index 15da22a6b9..3d088475f5 100644 --- a/controllers/vspheredeploymentzone_controller_domain.go +++ b/controllers/vspheredeploymentzone_controller_domain.go @@ -35,40 +35,40 @@ import ( "sigs.k8s.io/cluster-api-provider-vsphere/pkg/taggable" ) -func (r vsphereDeploymentZoneReconciler) reconcileFailureDomain(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext) error { - logger := ctrl.LoggerFrom(ctx).WithValues("VSphereFailureDomain", klog.KObj(deploymentZoneCtx.VSphereFailureDomain)) +func (r vsphereDeploymentZoneReconciler) reconcileFailureDomain(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, vsphereFailureDomain *infrav1.VSphereFailureDomain) error { + logger := ctrl.LoggerFrom(ctx).WithValues("VSphereFailureDomain", klog.KObj(vsphereFailureDomain)) // verify the failure domain for the region - if err := r.reconcileInfraFailureDomain(ctx, deploymentZoneCtx, deploymentZoneCtx.VSphereFailureDomain.Spec.Region); err != nil { + if err := r.reconcileInfraFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Region); err != nil { conditions.MarkFalse(deploymentZoneCtx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition, infrav1.RegionMisconfiguredReason, clusterv1.ConditionSeverityError, err.Error()) logger.Error(err, "region is not configured correctly") return errors.Wrapf(err, "region is not configured correctly") } // verify the failure domain for the zone - if err := r.reconcileInfraFailureDomain(ctx, deploymentZoneCtx, deploymentZoneCtx.VSphereFailureDomain.Spec.Zone); err != nil { + if err := r.reconcileInfraFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone); err != nil { conditions.MarkFalse(deploymentZoneCtx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition, infrav1.ZoneMisconfiguredReason, clusterv1.ConditionSeverityError, err.Error()) logger.Error(err, "zone is not configured correctly") return errors.Wrapf(err, "zone is not configured correctly") } - if computeCluster := deploymentZoneCtx.VSphereFailureDomain.Spec.Topology.ComputeCluster; computeCluster != nil { - if err := r.reconcileComputeCluster(ctx, deploymentZoneCtx); err != nil { + if computeCluster := vsphereFailureDomain.Spec.Topology.ComputeCluster; computeCluster != nil { + if err := r.reconcileComputeCluster(ctx, deploymentZoneCtx, vsphereFailureDomain); err != nil { logger.Error(err, "compute cluster is not configured correctly", "name", *computeCluster) return errors.Wrap(err, "compute cluster is not configured correctly") } } - if err := r.reconcileTopology(ctx, deploymentZoneCtx); err != nil { + if err := r.reconcileTopology(ctx, deploymentZoneCtx, vsphereFailureDomain); err != nil { logger.Error(err, "topology is not configured correctly") return errors.Wrap(err, "topology is not configured correctly") } // Ensure the VSphereDeploymentZone is marked as an owner of the VSphereFailureDomain. - if err := updateOwnerReferences(ctx, deploymentZoneCtx.VSphereFailureDomain, r.Client, + if err := updateOwnerReferences(ctx, vsphereFailureDomain, r.Client, func() []metav1.OwnerReference { return clusterutilv1.EnsureOwnerRef( - deploymentZoneCtx.VSphereFailureDomain.OwnerReferences, + vsphereFailureDomain.OwnerReferences, metav1.OwnerReference{ APIVersion: infrav1.GroupVersion.String(), Kind: deploymentZoneCtx.VSphereDeploymentZone.Kind, @@ -84,15 +84,15 @@ func (r vsphereDeploymentZoneReconciler) reconcileFailureDomain(ctx context.Cont return nil } -func (r vsphereDeploymentZoneReconciler) reconcileInfraFailureDomain(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, failureDomain infrav1.FailureDomain) error { +func (r vsphereDeploymentZoneReconciler) reconcileInfraFailureDomain(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, vsphereFailureDomain *infrav1.VSphereFailureDomain, failureDomain infrav1.FailureDomain) error { if *failureDomain.AutoConfigure { //nolint:staticcheck - return r.createAndAttachMetadata(ctx, deploymentZoneCtx, failureDomain) + return r.createAndAttachMetadata(ctx, deploymentZoneCtx, vsphereFailureDomain, failureDomain) } - return r.verifyFailureDomain(ctx, deploymentZoneCtx, failureDomain) + return r.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, failureDomain) } -func (r vsphereDeploymentZoneReconciler) reconcileTopology(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext) error { - topology := deploymentZoneCtx.VSphereFailureDomain.Spec.Topology +func (r vsphereDeploymentZoneReconciler) reconcileTopology(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, vsphereFailureDomain *infrav1.VSphereFailureDomain) error { + topology := vsphereFailureDomain.Spec.Topology if datastore := topology.Datastore; datastore != "" { if _, err := deploymentZoneCtx.AuthSession.Finder.Datastore(ctx, datastore); err != nil { conditions.MarkFalse(deploymentZoneCtx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition, infrav1.DatastoreNotFoundReason, clusterv1.ConditionSeverityError, "datastore %s is misconfigured", datastore) @@ -123,8 +123,8 @@ func (r vsphereDeploymentZoneReconciler) reconcileTopology(ctx context.Context, return nil } -func (r vsphereDeploymentZoneReconciler) reconcileComputeCluster(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext) error { - computeCluster := deploymentZoneCtx.VSphereFailureDomain.Spec.Topology.ComputeCluster +func (r vsphereDeploymentZoneReconciler) reconcileComputeCluster(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, vsphereFailureDomain *infrav1.VSphereFailureDomain) error { + computeCluster := vsphereFailureDomain.Spec.Topology.ComputeCluster if computeCluster == nil { return nil } @@ -156,12 +156,12 @@ func (r vsphereDeploymentZoneReconciler) reconcileComputeCluster(ctx context.Con // verifyFailureDomain verifies the Failure Domain. It verifies the existence of tag and category specified and // checks whether the specified tags exist on the DataCenter or Compute Cluster or Hosts (in a HostGroup). -func (r vsphereDeploymentZoneReconciler) verifyFailureDomain(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, failureDomain infrav1.FailureDomain) error { +func (r vsphereDeploymentZoneReconciler) verifyFailureDomain(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, vsphereFailureDomain *infrav1.VSphereFailureDomain, failureDomain infrav1.FailureDomain) error { if _, err := deploymentZoneCtx.AuthSession.TagManager.GetTagForCategory(ctx, failureDomain.Name, failureDomain.TagCategory); err != nil { return errors.Wrapf(err, "failed to verify tag %s and category %s", failureDomain.Name, failureDomain.TagCategory) } - objects, err := taggable.GetObjects(ctx, deploymentZoneCtx, failureDomain.Type) + objects, err := taggable.GetObjects(ctx, deploymentZoneCtx, vsphereFailureDomain, failureDomain.Type) if err != nil { return errors.Wrapf(err, "failed to find object") } @@ -179,7 +179,7 @@ func (r vsphereDeploymentZoneReconciler) verifyFailureDomain(ctx context.Context return nil } -func (r vsphereDeploymentZoneReconciler) createAndAttachMetadata(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, failureDomain infrav1.FailureDomain) error { +func (r vsphereDeploymentZoneReconciler) createAndAttachMetadata(ctx context.Context, deploymentZoneCtx *capvcontext.VSphereDeploymentZoneContext, vsphereFailureDomain *infrav1.VSphereFailureDomain, failureDomain infrav1.FailureDomain) error { logger := ctrl.LoggerFrom(ctx, "tag", failureDomain.Name, "category", failureDomain.TagCategory) categoryID, err := metadata.CreateCategory(ctx, deploymentZoneCtx, failureDomain.TagCategory, failureDomain.Type) if err != nil { @@ -193,7 +193,7 @@ func (r vsphereDeploymentZoneReconciler) createAndAttachMetadata(ctx context.Con } logger = logger.WithValues("type", failureDomain.Type) - objects, err := taggable.GetObjects(ctx, deploymentZoneCtx, failureDomain.Type) + objects, err := taggable.GetObjects(ctx, deploymentZoneCtx, vsphereFailureDomain, failureDomain.Type) if err != nil { logger.V(4).Error(err, "failed to find object") return err diff --git a/controllers/vspheredeploymentzone_controller_domain_test.go b/controllers/vspheredeploymentzone_controller_domain_test.go index 8521062980..6fc93dffa2 100644 --- a/controllers/vspheredeploymentzone_controller_domain_test.go +++ b/controllers/vspheredeploymentzone_controller_domain_test.go @@ -87,31 +87,30 @@ func TestVsphereDeploymentZoneReconciler_Reconcile_VerifyFailureDomain_ComputeCl } deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ - ControllerContext: controllerCtx, - VSphereFailureDomain: vsphereFailureDomain, - Logger: logr.Discard(), - AuthSession: authSession, + ControllerContext: controllerCtx, + Logger: logr.Discard(), + AuthSession: authSession, } reconciler := vsphereDeploymentZoneReconciler{controllerCtx} - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Region)).To(Succeed()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Region)).To(Succeed()) stdout := gbytes.NewBuffer() g.Expect(simr.Run("tags.attached.ls k8s-region-west", stdout)).To(Succeed()) g.Expect(stdout).Should(gbytes.Say("Datacenter")) - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(Succeed()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(Succeed()) stdout = gbytes.NewBuffer() g.Expect(simr.Run("tags.attached.ls k8s-region-west-2", stdout)).To(Succeed()) g.Expect(stdout).Should(gbytes.Say("ClusterComputeResource")) vsphereFailureDomain.Spec.Topology.ComputeCluster = pointer.String("DC0_C1") // Since association is verified, the method errors since the tag is not associated to the object. - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) // Since the tag does not belong to the category vsphereFailureDomain.Spec.Zone.TagCategory = "diff-k8s-region" - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) } func TestVsphereDeploymentZoneReconciler_Reconcile_VerifyFailureDomain_HostGroupZone(t *testing.T) { @@ -171,29 +170,28 @@ func TestVsphereDeploymentZoneReconciler_Reconcile_VerifyFailureDomain_HostGroup } deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ - ControllerContext: controllerCtx, - VSphereFailureDomain: vsphereFailureDomain, - Logger: logr.Discard(), - AuthSession: authSession, + ControllerContext: controllerCtx, + Logger: logr.Discard(), + AuthSession: authSession, } reconciler := vsphereDeploymentZoneReconciler{controllerCtx} // Fails since no hosts are tagged - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) stdout := gbytes.NewBuffer() g.Expect(simr.Run("tags.attach k8s-region-west-2 /DC0/host/DC0_C0/DC0_C0_H0", stdout)).To(Succeed()) // Fails as not all hosts are tagged - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) g.Expect(simr.Run("tags.attach k8s-region-west-2 /DC0/host/DC0_C0/DC0_C0_H1", stdout)).To(Succeed()) // Succeeds as all hosts are tagged - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(Succeed()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(Succeed()) // Since the tag does not belong to the category vsphereFailureDomain.Spec.Zone.TagCategory = "diff-k8s-region" - g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) + g.Expect(reconciler.verifyFailureDomain(ctx, deploymentZoneCtx, vsphereFailureDomain, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred()) } func TestVsphereDeploymentZoneReconciler_Reconcile_CreateAndAttachMetadata(t *testing.T) { @@ -282,13 +280,12 @@ func TestVsphereDeploymentZoneReconciler_Reconcile_CreateAndAttachMetadata(t *te } deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ - ControllerContext: controllerCtx, - VSphereFailureDomain: vsphereFailureDomain, - Logger: logr.Discard(), - AuthSession: authSession, + ControllerContext: controllerCtx, + Logger: logr.Discard(), + AuthSession: authSession, } - g.Expect(reconciler.createAndAttachMetadata(ctx, deploymentZoneCtx, tests[0].vsphereFailureDomainSpec.Region)).NotTo(HaveOccurred()) + g.Expect(reconciler.createAndAttachMetadata(ctx, deploymentZoneCtx, vsphereFailureDomain, tests[0].vsphereFailureDomainSpec.Region)).NotTo(HaveOccurred()) stdout := gbytes.NewBuffer() g.Expect(simr.Run("tags.category.info k8s-region", stdout)).To(Succeed()) g.Expect(stdout).To(gbytes.Say("k8s-region")) @@ -303,13 +300,12 @@ func TestVsphereDeploymentZoneReconciler_Reconcile_CreateAndAttachMetadata(t *te } deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ - ControllerContext: controllerCtx, - VSphereFailureDomain: vsphereFailureDomain, - Logger: logr.Discard(), - AuthSession: authSession, + ControllerContext: controllerCtx, + Logger: logr.Discard(), + AuthSession: authSession, } - g.Expect(reconciler.createAndAttachMetadata(ctx, deploymentZoneCtx, tests[1].vsphereFailureDomainSpec.Zone)).NotTo(HaveOccurred()) + g.Expect(reconciler.createAndAttachMetadata(ctx, deploymentZoneCtx, vsphereFailureDomain, tests[1].vsphereFailureDomainSpec.Zone)).NotTo(HaveOccurred()) stdout := gbytes.NewBuffer() g.Expect(simr.Run("tags.category.info k8s-zone", stdout)).To(Succeed()) g.Expect(stdout).To(gbytes.Say("k8s-zone")) @@ -324,13 +320,12 @@ func TestVsphereDeploymentZoneReconciler_Reconcile_CreateAndAttachMetadata(t *te } deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ - ControllerContext: controllerCtx, - VSphereFailureDomain: vsphereFailureDomain, - Logger: logr.Discard(), - AuthSession: authSession, + ControllerContext: controllerCtx, + Logger: logr.Discard(), + AuthSession: authSession, } - g.Expect(reconciler.createAndAttachMetadata(ctx, deploymentZoneCtx, tests[2].vsphereFailureDomainSpec.Zone)).NotTo(HaveOccurred()) + g.Expect(reconciler.createAndAttachMetadata(ctx, deploymentZoneCtx, vsphereFailureDomain, tests[2].vsphereFailureDomainSpec.Zone)).NotTo(HaveOccurred()) stdout := gbytes.NewBuffer() g.Expect(simr.Run("tags.category.info foo", stdout)).To(Succeed()) g.Expect(stdout).To(gbytes.Say("foo")) diff --git a/controllers/vspheredeploymentzone_controller_test.go b/controllers/vspheredeploymentzone_controller_test.go index 5c69af5e48..5a74a72c1b 100644 --- a/controllers/vspheredeploymentzone_controller_test.go +++ b/controllers/vspheredeploymentzone_controller_test.go @@ -495,12 +495,11 @@ func TestVSphereDeploymentZone_Reconcile(t *testing.T) { g.Consistently(func(g Gomega) bool { g.Expect(testEnv.Get(ctx, client.ObjectKeyFromObject(vsphereDeploymentZone), vsphereDeploymentZone)).To(Succeed()) - return vsphereDeploymentZone.Status.Ready != nil && !*vsphereDeploymentZone.Status.Ready }, timeout).Should(BeFalse()) }) - t.Run("it should delete the VSphereDeploymentZone when the associated VSphereFailureDomain does not exist", func(t *testing.T) { + t.Run("Delete the VSphereDeploymentZone when the associated VSphereFailureDomain does not exist", func(t *testing.T) { g := NewWithT(t) vsphereDeploymentZone := &infrav1.VSphereDeploymentZone{ @@ -618,6 +617,17 @@ func TestVsphereDeploymentZone_Failed_ReconcilePlacementConstraint(t *testing.T) mgmtContext.Password = pass controllerCtx := fake.NewControllerContext(mgmtContext) + Expect(controllerCtx.Client.Create(controllerCtx, &infrav1.VSphereFailureDomain{ + ObjectMeta: metav1.ObjectMeta{ + Name: "blah", + }, + Spec: infrav1.VSphereFailureDomainSpec{ + Topology: infrav1.Topology{ + Datacenter: "DC0", + ComputeCluster: pointer.String("DC0_C0"), + }, + }, + })).To(Succeed()) deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, @@ -627,17 +637,6 @@ func TestVsphereDeploymentZone_Failed_ReconcilePlacementConstraint(t *testing.T) ControlPlane: pointer.Bool(true), PlacementConstraint: tt.placementConstraint, }}, - VSphereFailureDomain: &infrav1.VSphereFailureDomain{ - ObjectMeta: metav1.ObjectMeta{ - Name: "blah", - }, - Spec: infrav1.VSphereFailureDomainSpec{ - Topology: infrav1.Topology{ - Datacenter: "DC0", - ComputeCluster: pointer.String("DC0_C0"), - }, - }, - }, Logger: logr.Discard(), } @@ -685,7 +684,6 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: vsphereFailureDomain, Logger: logr.Discard(), } @@ -707,7 +705,6 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: vsphereFailureDomain, Logger: logr.Discard(), } @@ -726,7 +723,6 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: vsphereFailureDomain, Logger: logr.Discard(), } @@ -743,7 +739,6 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: vsphereFailureDomain, Logger: logr.Discard(), } @@ -767,7 +762,6 @@ func TestVSphereDeploymentZoneReconciler_ReconcileDelete(t *testing.T) { deploymentZoneCtx := &capvcontext.VSphereDeploymentZoneContext{ ControllerContext: controllerCtx, VSphereDeploymentZone: vsphereDeploymentZone, - VSphereFailureDomain: vsphereFailureDomain, Logger: logr.Discard(), } diff --git a/pkg/context/vspheredeploymentzone_context.go b/pkg/context/vspheredeploymentzone_context.go index c1a62e51d3..ca3ec273c9 100644 --- a/pkg/context/vspheredeploymentzone_context.go +++ b/pkg/context/vspheredeploymentzone_context.go @@ -31,7 +31,6 @@ import ( type VSphereDeploymentZoneContext struct { *ControllerContext VSphereDeploymentZone *infrav1.VSphereDeploymentZone - VSphereFailureDomain *infrav1.VSphereFailureDomain Logger logr.Logger PatchHelper *patch.Helper AuthSession *session.Session @@ -58,8 +57,3 @@ func (c *VSphereDeploymentZoneContext) String() string { func (c *VSphereDeploymentZoneContext) GetSession() *session.Session { return c.AuthSession } - -// GetVsphereFailureDomain returns the VSphereFailureDomain for the VSphereDeploymentZoneContext. -func (c *VSphereDeploymentZoneContext) GetVsphereFailureDomain() infrav1.VSphereFailureDomain { - return *c.VSphereFailureDomain -} diff --git a/pkg/taggable/get.go b/pkg/taggable/get.go index d5e727f271..933c61b051 100644 --- a/pkg/taggable/get.go +++ b/pkg/taggable/get.go @@ -29,12 +29,11 @@ import ( type taggableContext interface { GetSession() *session.Session - GetVsphereFailureDomain() infrav1.VSphereFailureDomain } // GetObjects returns the objects for a given failure domain. -func GetObjects(ctx context.Context, taggableCtx taggableContext, fdType infrav1.FailureDomainType) (Objects, error) { - finderFunc := find.ObjectFunc(fdType, taggableCtx.GetVsphereFailureDomain().Spec.Topology, taggableCtx.GetSession().Finder) +func GetObjects(ctx context.Context, taggableCtx taggableContext, failureDomain *infrav1.VSphereFailureDomain, fdType infrav1.FailureDomainType) (Objects, error) { + finderFunc := find.ObjectFunc(fdType, failureDomain.Spec.Topology, taggableCtx.GetSession().Finder) objRefs, err := finderFunc(ctx) if err != nil { return nil, err