From 8ddc847391337b15cf128189a537e7290309ef8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Wed, 30 Aug 2023 11:01:22 +0200 Subject: [PATCH 1/2] chore: replace own multierror with errors.Join --- controllers/internal/multierror.go | 27 --------------------------- controllers/lvmcluster_controller.go | 8 ++++---- 2 files changed, 4 insertions(+), 31 deletions(-) delete mode 100644 controllers/internal/multierror.go diff --git a/controllers/internal/multierror.go b/controllers/internal/multierror.go deleted file mode 100644 index 8380cbabe..000000000 --- a/controllers/internal/multierror.go +++ /dev/null @@ -1,27 +0,0 @@ -package internal - -import ( - "strings" -) - -const DefaultMultiErrorSeparator = ";" - -// NewMultiError creates a MultiError that uses the default separator for each error. -func NewMultiError(errs []error) error { - return &MultiError{Errors: errs, Separator: DefaultMultiErrorSeparator} -} - -// MultiError is an error that aggregates multiple errors together and uses -// a separator to aggregate them when called with Error. -type MultiError struct { - Errors []error - Separator string -} - -func (m *MultiError) Error() string { - errs := make([]string, len(m.Errors)) - for i, err := range m.Errors { - errs[i] = err.Error() - } - return strings.Join(errs, m.Separator) -} diff --git a/controllers/lvmcluster_controller.go b/controllers/lvmcluster_controller.go index 156b62ec1..ad9a6e5aa 100644 --- a/controllers/lvmcluster_controller.go +++ b/controllers/lvmcluster_controller.go @@ -18,19 +18,19 @@ package controllers import ( "context" + "errors" "fmt" "os" "time" configv1 "github.com/openshift/api/config/v1" lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" - "github.com/openshift/lvm-operator/controllers/internal" "github.com/openshift/lvm-operator/pkg/cluster" topolvmv1 "github.com/topolvm/topolvm/api/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" corev1helper "k8s.io/component-helpers/scheduling/corev1" @@ -126,7 +126,7 @@ func (r *LVMClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // Reconcile errors have higher priority than status update errors if reconcileError != nil { return result, reconcileError - } else if statusError != nil && !errors.IsNotFound(statusError) { + } else if statusError != nil && !k8serrors.IsNotFound(statusError) { return result, fmt.Errorf("failed to update LVMCluster status: %w", statusError) } else { return result, nil @@ -200,7 +200,7 @@ func (r *LVMClusterReconciler) reconcile(ctx context.Context, instance *lvmv1alp resourceSyncElapsedTime := time.Since(resourceSyncStart) if len(errs) > 0 { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources managed by LVMCluster within %v: %w", - resourceSyncElapsedTime, internal.NewMultiError(errs)) + resourceSyncElapsedTime, errors.Join(errs...)) } logger.Info("successfully reconciled LVMCluster", "resourceSyncElapsedTime", resourceSyncElapsedTime) From 733a4b0aca529c8ba6d778f9b83e3b36660132bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Wed, 30 Aug 2023 14:09:46 +0200 Subject: [PATCH 2/2] fix: unify deletion logic and fix controller refs --- controllers/lvm_volumegroup.go | 52 ++--- controllers/lvmcluster_controller.go | 2 +- .../lvmcluster_controller_integration_test.go | 178 +++++++++--------- controllers/scc.go | 17 +- controllers/topolvm_controller.go | 69 ++----- controllers/topolvm_csi_driver.go | 10 +- controllers/topolvm_node.go | 34 +--- controllers/topolvm_snapshotclass.go | 18 +- controllers/topolvm_storageclass.go | 10 +- controllers/vgmanager.go | 24 +-- 10 files changed, 176 insertions(+), 238 deletions(-) diff --git a/controllers/lvm_volumegroup.go b/controllers/lvm_volumegroup.go index 4023c2ba4..5b3f2b4dc 100644 --- a/controllers/lvm_volumegroup.go +++ b/controllers/lvm_volumegroup.go @@ -18,7 +18,6 @@ package controllers import ( "context" - "errors" "fmt" lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" @@ -72,40 +71,48 @@ func (c lvmVG) ensureCreated(r *LVMClusterReconciler, ctx context.Context, lvmCl } func (c lvmVG) ensureDeleted(r *LVMClusterReconciler, ctx context.Context, lvmCluster *lvmv1alpha1.LVMCluster) error { - logger := log.FromContext(ctx).WithValues("topolvmNode", c.getName()) + logger := log.FromContext(ctx).WithValues("resourceManager", c.getName()) vgcrs := lvmVolumeGroups(r.Namespace, lvmCluster.Spec.Storage.DeviceClasses) - allVGsDeleted := true + + var volumeGroupsPendingInStatus []string for _, volumeGroup := range vgcrs { - if err := r.Client.Get(ctx, client.ObjectKeyFromObject(volumeGroup), volumeGroup); err != nil { + vgName := client.ObjectKeyFromObject(volumeGroup) + logger := logger.WithValues("LVMVolumeGroup", volumeGroup.GetName()) + + if err := r.Client.Get(ctx, vgName, volumeGroup); err != nil { return client.IgnoreNotFound(err) } - // if not deleted, initiate deletion + // if not marked for deletion, mark now. + // The controller reference will usually propagate all deletion timestamps so this will + // only occur if the propagation from the API server was delayed. if volumeGroup.GetDeletionTimestamp().IsZero() { if err := r.Client.Delete(ctx, volumeGroup); err != nil { return fmt.Errorf("failed to delete LVMVolumeGroup %s: %w", volumeGroup.GetName(), err) } - logger.Info("initiated LVMVolumeGroup deletion", "name", volumeGroup.Name) - allVGsDeleted = false - } else { - // Has the VG been cleaned up on all hosts? - exists := doesVGExistOnHosts(volumeGroup.Name, lvmCluster) - if !exists { - // Remove finalizer - if update := cutil.RemoveFinalizer(volumeGroup, lvmvgFinalizer); update { - if err := r.Client.Update(ctx, volumeGroup); err != nil { - return fmt.Errorf("failed to remove finalizer from LVMVolumeGroup") - } - } - } else { - allVGsDeleted = false + } + + // Has the VG been cleaned up on all hosts? + if doesVGExistInDeviceClassStatus(volumeGroup.Name, lvmCluster) { + volumeGroupsPendingInStatus = append(volumeGroupsPendingInStatus, vgName.String()) + continue + } + + // Remove finalizer + if update := cutil.RemoveFinalizer(volumeGroup, lvmvgFinalizer); update { + if err := r.Client.Update(ctx, volumeGroup); err != nil { + return fmt.Errorf("failed to remove finalizer from LVMVolumeGroup %s: %w", volumeGroup.GetName(), err) } } + + logger.Info("initiated LVMVolumeGroup deletion") + volumeGroupsPendingInStatus = append(volumeGroupsPendingInStatus, vgName.String()) } - if !allVGsDeleted { - return errors.New("waiting for all VGs to be deleted") + if len(volumeGroupsPendingInStatus) > 0 { + return fmt.Errorf("waiting for LVMVolumeGroup's to be removed from nodestatus of %s: %v", + client.ObjectKeyFromObject(lvmCluster), volumeGroupsPendingInStatus) } return nil @@ -136,8 +143,7 @@ func lvmVolumeGroups(namespace string, deviceClasses []lvmv1alpha1.DeviceClass) return lvmVolumeGroups } -func doesVGExistOnHosts(volumeGroup string, instance *lvmv1alpha1.LVMCluster) bool { - +func doesVGExistInDeviceClassStatus(volumeGroup string, instance *lvmv1alpha1.LVMCluster) bool { dcStatuses := instance.Status.DeviceClassStatuses for _, dc := range dcStatuses { if dc.Name == volumeGroup { diff --git a/controllers/lvmcluster_controller.go b/controllers/lvmcluster_controller.go index ad9a6e5aa..0211aacdd 100644 --- a/controllers/lvmcluster_controller.go +++ b/controllers/lvmcluster_controller.go @@ -168,7 +168,7 @@ func (r *LVMClusterReconciler) reconcile(ctx context.Context, instance *lvmv1alp resources := []resourceManager{ &csiDriver{}, - &topolvmController{r.TopoLVMLeaderElectionPassthrough}, + &topolvmController{}, &topolvmNode{}, &vgManager{}, &lvmVG{}, diff --git a/controllers/lvmcluster_controller_integration_test.go b/controllers/lvmcluster_controller_integration_test.go index 1872eca8d..5c7251946 100644 --- a/controllers/lvmcluster_controller_integration_test.go +++ b/controllers/lvmcluster_controller_integration_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -39,8 +40,6 @@ var _ = Describe("LVMCluster controller", func() { interval = time.Millisecond * 250 ) - ctx := context.Background() - // LVMCluster CR details lvmClusterName := types.NamespacedName{Name: testLvmClusterName, Namespace: testLvmClusterNamespace} lvmClusterOut := &lvmv1alpha1.LVMCluster{} @@ -64,6 +63,13 @@ var _ = Describe("LVMCluster controller", func() { }, } + // this is a custom matcher that verifies for a correct owner-ref set with LVMCluster + containLVMClusterOwnerRefField := ContainElement(SatisfyAll( + HaveField("Name", lvmClusterIn.Name), + HaveField("BlockOwnerDeletion", pointer.Bool(true)), + HaveField("Controller", pointer.Bool(true)), + )) + nodeIn := &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: testNodeName, @@ -114,7 +120,10 @@ var _ = Describe("LVMCluster controller", func() { scOut := &storagev1.StorageClass{} Context("Reconciliation on creating an LVMCluster CR", func() { - It("should reconcile LVMCluster CR creation, ", func() { + SetDefaultEventuallyTimeout(timeout) + SetDefaultEventuallyPollingInterval(interval) + + It("should reconcile LVMCluster CR creation, ", func(ctx context.Context) { By("verifying CR status on reconciliation") // create node as it should be present Expect(k8sClient.Create(ctx, nodeIn)).Should(Succeed()) @@ -129,135 +138,128 @@ var _ = Describe("LVMCluster controller", func() { // lvmcluster controller expecting this to be present to set the status properly Expect(k8sClient.Create(ctx, lvmVolumeGroupNodeStatusIn)).Should(Succeed()) - // placeholder to check CR status.Ready field to be true + By("verifying LVMCluster .Status.Ready is true") Eventually(func() bool { - err := k8sClient.Get(ctx, lvmClusterName, lvmClusterOut) - if err != nil { + if err := k8sClient.Get(ctx, lvmClusterName, lvmClusterOut); err != nil { return false } return lvmClusterOut.Status.Ready - }, timeout, interval).Should(Equal(true)) + }).Should(BeTrue()) - // placeholder to check CR status.State field to be Ready + By("verifying LVMCluster .Status.State is Ready") Eventually(func() bool { - err := k8sClient.Get(ctx, lvmClusterName, lvmClusterOut) - if err != nil { + if err := k8sClient.Get(ctx, lvmClusterName, lvmClusterOut); err != nil { return false } return lvmClusterOut.Status.State == lvmv1alpha1.LVMStatusReady - }, timeout, interval).Should(BeTrue()) + }).Should(BeTrue()) - By("confirming presence of CSIDriver resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, csiDriverName, csiDriverOut) - return err == nil - }, timeout, interval).Should(BeTrue()) + By("confirming presence of CSIDriver") + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, csiDriverName, csiDriverOut) + }).WithContext(ctx).Should(Succeed()) - By("confirming presence of VgManager resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, vgManagerNamespacedName, vgManagerDaemonset) - return err == nil - }, timeout, interval).Should(BeTrue()) + By("confirming presence of vg-manager") + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, vgManagerNamespacedName, vgManagerDaemonset) + }).WithContext(ctx).Should(Succeed()) - By("confirming presence of Topolvm Controller deployment") - Eventually(func() bool { - err := k8sClient.Get(ctx, controllerName, controllerOut) - return err == nil - }, timeout, interval).Should(BeTrue()) + By("confirming presence of TopoLVM Controller Deployment") + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, controllerName, controllerOut) + }).WithContext(ctx).Should(Succeed()) By("confirming the existence of CSI Node resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, csiNodeName, csiNodeOut) - return err == nil - }, timeout, interval).Should(BeTrue()) + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, csiNodeName, csiNodeOut) + }).WithContext(ctx).Should(Succeed()) By("confirming the existence of LVMVolumeGroup resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, lvmVolumeGroupName, lvmVolumeGroupOut) - return err == nil - }, timeout, interval).Should(BeTrue()) + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, lvmVolumeGroupName, lvmVolumeGroupOut) + }).WithContext(ctx).Should(Succeed()) By("confirming creation of TopolvmStorageClasses") for _, scName := range scNames { - Eventually(func() bool { - err := k8sClient.Get(ctx, scName, scOut) - return err == nil - }, timeout, interval).Should(BeTrue()) + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, scName, scOut) + }).WithContext(ctx).Should(Succeed()) scOut = &storagev1.StorageClass{} } }) }) Context("Reconciliation on deleting the LVMCluster CR", func() { - It("should reconcile LVMCluster CR deletion ", func() { - By("confirming absence of lvm cluster CR and deletion of operator created resources") + It("should reconcile LVMCluster CR deletion ", func(ctx context.Context) { // delete lvmVolumeGroupNodeStatus as it should be deleted by vgmanager // and if it is present lvmcluster reconciler takes it as vg is present on node - // we will now remove the node which will cause the LVM cluster status to also lose that vg + By("confirming absence of lvm cluster CR and deletion of operator created resources") Expect(k8sClient.Delete(ctx, nodeIn)).Should(Succeed()) // deletion of LVMCluster CR and thus also the NodeStatus through the removal controller - Eventually(func() bool { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(lvmVolumeGroupNodeStatusIn), - &lvmv1alpha1.LVMVolumeGroupNodeStatus{}) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, client.ObjectKeyFromObject(lvmVolumeGroupNodeStatusIn), lvmVolumeGroupNodeStatusIn) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) // deletion of LVMCluster CR - Eventually(func() bool { - err := k8sClient.Delete(ctx, lvmClusterOut) - return err != nil - }, timeout, interval).Should(BeTrue()) + By("deleting the LVMClusterCR") + Expect(k8sClient.Delete(ctx, lvmClusterOut)).Should(Succeed()) // auto deletion of CSI Driver resource based on CR deletion By("confirming absence of CSI Driver Resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, csiDriverName, csiDriverOut) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) - - // ensure that VgManager has owner reference of LVMCluster. (envTest does not support garbage collection) - By("confirming VgManager resource has owner reference of LVMCluster resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, vgManagerNamespacedName, vgManagerDaemonset) - return err == nil && vgManagerDaemonset.OwnerReferences[0].Name == lvmClusterIn.Name - }, timeout, interval).Should(BeTrue()) - - // auto deletion of Topolvm Controller deployment based on CR deletion - By("confirming absence of Topolvm Controller Deployment") - Eventually(func() bool { - err := k8sClient.Get(ctx, controllerName, controllerOut) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) - - By("confirming absence of CSI Node Resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, csiNodeName, csiNodeOut) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, csiDriverName, csiDriverOut) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) + + // envtest does not support garbage collection, so we simulate the deletion + // see https://book.kubebuilder.io/reference/envtest.html?highlight=considerations#testing-considerations + By("confirming vg-manager has owner reference of LVMCluster") + Expect(k8sClient.Get(ctx, vgManagerNamespacedName, vgManagerDaemonset)).Should(Succeed()) + Expect(vgManagerDaemonset.OwnerReferences).To(containLVMClusterOwnerRefField) + Expect(k8sClient.Delete(ctx, vgManagerDaemonset)).To(Succeed(), "simulated ownerref cleanup should succeed") + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, vgManagerNamespacedName, vgManagerDaemonset) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) + + // envtest does not support garbage collection, so we simulate the deletion + // see https://book.kubebuilder.io/reference/envtest.html?highlight=considerations#testing-considerations + By("confirming TopoLVM controller resource has owner reference of LVMCluster") + Expect(k8sClient.Get(ctx, controllerName, controllerOut)).Should(Succeed()) + Expect(controllerOut.OwnerReferences).To(containLVMClusterOwnerRefField) + Expect(k8sClient.Delete(ctx, controllerOut)).To(Succeed(), "simulated ownerref cleanup should succeed") + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, controllerName, controllerOut) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) + + // envtest does not support garbage collection, so we simulate the deletion + // see https://book.kubebuilder.io/reference/envtest.html?highlight=considerations#testing-considerations + By("confirming TopoLVM Node DaemonSet has owner reference of LVMCluster") + Expect(k8sClient.Get(ctx, csiNodeName, csiNodeOut)).Should(Succeed()) + Expect(csiNodeOut.OwnerReferences).To(containLVMClusterOwnerRefField) + Expect(k8sClient.Delete(ctx, csiNodeOut)).To(Succeed(), "simulated ownerref cleanup should succeed") + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, csiNodeName, csiNodeOut) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) By("confirming absence of LVMVolumeGroup Resource") - Eventually(func() bool { - err := k8sClient.Get(ctx, lvmVolumeGroupName, lvmVolumeGroupOut) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) + // technically we also set ownerrefs on volume groups so we would also need to check, + // but our controller still deletes them (in addition to the ownerrefs) + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, lvmVolumeGroupName, lvmVolumeGroupOut) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) By("confirming absence of TopolvmStorageClasses") for _, scName := range scNames { - Eventually(func() bool { - err := k8sClient.Get(ctx, scName, scOut) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) - scOut = &storagev1.StorageClass{} + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, scName, scOut) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) } By("confirming absence of LVMCluster CR") - Eventually(func() bool { - err := k8sClient.Get(ctx, lvmClusterName, lvmClusterOut) - return errors.IsNotFound(err) - }, timeout, interval).Should(BeTrue()) - + Eventually(func(ctx context.Context) error { + return k8sClient.Get(ctx, lvmClusterName, lvmClusterOut) + }).WithContext(ctx).Should(Satisfy(errors.IsNotFound)) }) }) diff --git a/controllers/scc.go b/controllers/scc.go index 403aa5482..9b923ad27 100644 --- a/controllers/scc.go +++ b/controllers/scc.go @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -71,12 +72,20 @@ func (c openshiftSccs) ensureDeleted(r *LVMClusterReconciler, ctx context.Contex logger := log.FromContext(ctx).WithValues("resourceManager", c.getName()) sccs := getAllSCCs(r.Namespace) for _, scc := range sccs { + name := types.NamespacedName{Name: scName} + logger := logger.WithValues("SecurityContextConstraint", scName) + if err := r.Client.Get(ctx, name, scc); err != nil { + return client.IgnoreNotFound(err) + } + + if !scc.GetDeletionTimestamp().IsZero() { + return fmt.Errorf("the SecurityContextConstraint %s is still present, waiting for deletion", scName) + } + if err := r.Delete(ctx, scc); err != nil { - if !errors.IsNotFound(err) { - return fmt.Errorf("failed to delete SecurityContextConstraint %s: %w", scc.GetName(), err) - } - logger.Info("SecurityContextConstraint is already deleted", "SecurityContextConstraint", scc.Name) + return fmt.Errorf("failed to delete SecurityContextConstraint %s: %w", scc.GetName(), err) } + logger.Info("initiated SecurityContextConstraint deletion") } return nil } diff --git a/controllers/topolvm_controller.go b/controllers/topolvm_controller.go index 0c2f9ed68..515d6923e 100644 --- a/controllers/topolvm_controller.go +++ b/controllers/topolvm_controller.go @@ -25,10 +25,8 @@ import ( lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - k8serror "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" cutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -39,7 +37,6 @@ const ( ) type topolvmController struct { - topoLVMLeaderElectionPassthrough v1.LeaderElection } // topolvmController unit satisfies resourceManager interface @@ -55,7 +52,7 @@ func (c topolvmController) ensureCreated(r *LVMClusterReconciler, ctx context.Co logger := log.FromContext(ctx).WithValues("resourceManager", c.getName()) // get the desired state of topolvm controller deployment - desiredDeployment := getControllerDeployment(lvmCluster, r.Namespace, r.ImageName, c.topoLVMLeaderElectionPassthrough) + desiredDeployment := getControllerDeployment(r.Namespace, r.ImageName, r.TopoLVMLeaderElectionPassthrough) existingDeployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: desiredDeployment.Name, @@ -63,12 +60,22 @@ func (c topolvmController) ensureCreated(r *LVMClusterReconciler, ctx context.Co }, } - if err := cutil.SetControllerReference(lvmCluster, existingDeployment, r.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference for csi controller: %w", err) - } - result, err := cutil.CreateOrUpdate(ctx, r.Client, existingDeployment, func() error { - return c.setTopolvmControllerDesiredState(existingDeployment, desiredDeployment) + if err := cutil.SetControllerReference(lvmCluster, existingDeployment, r.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference for csi controller: %w", err) + } + // at creation, deep copy desired deployment into existing + if existingDeployment.CreationTimestamp.IsZero() { + desiredDeployment.DeepCopyInto(existingDeployment) + return nil + } + + // for update, topolvm controller is interested in only updating container images + // labels, volumes, service account etc can remain unchanged + existingDeployment.Spec.Template.Spec.Containers = desiredDeployment.Spec.Template.Spec.Containers + existingDeployment.Spec.Template.Spec.InitContainers = desiredDeployment.Spec.Template.Spec.InitContainers + + return nil }) if err != nil { @@ -84,52 +91,12 @@ func (c topolvmController) ensureCreated(r *LVMClusterReconciler, ctx context.Co return nil } +// ensureDeleted is a noop. Deletion will be handled by ownerref func (c topolvmController) ensureDeleted(r *LVMClusterReconciler, ctx context.Context, _ *lvmv1alpha1.LVMCluster) error { - logger := log.FromContext(ctx).WithValues("resourceManager", c.getName()) - existingDeployment := &appsv1.Deployment{} - - err := r.Client.Get(ctx, types.NamespacedName{Name: TopolvmControllerDeploymentName, Namespace: r.Namespace}, existingDeployment) - // already deleted in previous reconcile - if k8serror.IsNotFound(err) { - logger.Info("csi controller deleted", "TopolvmController", existingDeployment.Name) - return nil - } - - if err != nil { - return fmt.Errorf("failed to retrieve csi controller deployment %s: %w", existingDeployment.GetName(), err) - } - - // if not deleted, initiate deletion - if !existingDeployment.GetDeletionTimestamp().IsZero() { - // set deletion in-progress for next reconcile to confirm deletion - return fmt.Errorf("topolvm controller deployment %s is already marked for deletion", existingDeployment.Name) - } - - if err = r.Client.Delete(ctx, existingDeployment); err != nil { - return fmt.Errorf("failed to delete topolvm controller deployment %s: %w", existingDeployment.GetName(), err) - } - - logger.Info("initiated topolvm controller deployment deletion", "TopolvmController", existingDeployment.Name) - return nil -} - -func (c topolvmController) setTopolvmControllerDesiredState(existing, desired *appsv1.Deployment) error { - - // at creation, deep copy desired deployment into existing - if existing.CreationTimestamp.IsZero() { - desired.DeepCopyInto(existing) - return nil - } - - // for update, topolvm controller is interested in only updating container images - // labels, volumes, service account etc can remain unchanged - existing.Spec.Template.Spec.Containers = desired.Spec.Template.Spec.Containers - existing.Spec.Template.Spec.InitContainers = desired.Spec.Template.Spec.InitContainers - return nil } -func getControllerDeployment(lvmCluster *lvmv1alpha1.LVMCluster, namespace string, initImage string, topoLVMLeaderElectionPassthrough v1.LeaderElection) *appsv1.Deployment { +func getControllerDeployment(namespace string, initImage string, topoLVMLeaderElectionPassthrough v1.LeaderElection) *appsv1.Deployment { // Topolvm CSI Controller Deployment var replicas int32 = 1 volumes := []corev1.Volume{ diff --git a/controllers/topolvm_csi_driver.go b/controllers/topolvm_csi_driver.go index f8cc27317..59ba40029 100644 --- a/controllers/topolvm_csi_driver.go +++ b/controllers/topolvm_csi_driver.go @@ -21,7 +21,6 @@ import ( "fmt" lvmv1alpha1 "github.com/openshift/lvm-operator/api/v1alpha1" - "github.com/pkg/errors" storagev1 "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -62,16 +61,15 @@ func (c csiDriver) ensureCreated(r *LVMClusterReconciler, ctx context.Context, _ } func (c csiDriver) ensureDeleted(r *LVMClusterReconciler, ctx context.Context, _ *lvmv1alpha1.LVMCluster) error { - logger := log.FromContext(ctx).WithValues("resourceManager", c.getName()) + name := types.NamespacedName{Name: TopolvmCSIDriverName} + logger := log.FromContext(ctx).WithValues("resourceManager", c.getName(), "CSIDriver", TopolvmCSIDriverName) csiDriverResource := &storagev1.CSIDriver{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: TopolvmCSIDriverName}, csiDriverResource); err != nil { + if err := r.Client.Get(ctx, name, csiDriverResource); err != nil { return client.IgnoreNotFound(err) } - // if not deleted, initiate deletion if !csiDriverResource.GetDeletionTimestamp().IsZero() { - // set deletion in-progress for next reconcile to confirm deletion - return errors.Errorf("topolvm csi driver %s is already marked for deletion", csiDriverResource.Name) + return fmt.Errorf("the CSIDriver %s is still present, waiting for deletion", TopolvmCSIDriverName) } if err := r.Client.Delete(ctx, csiDriverResource); err != nil { diff --git a/controllers/topolvm_node.go b/controllers/topolvm_node.go index 6eb80cab1..639427980 100644 --- a/controllers/topolvm_node.go +++ b/controllers/topolvm_node.go @@ -27,9 +27,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" - "sigs.k8s.io/controller-runtime/pkg/client" cutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -60,13 +58,11 @@ func (n topolvmNode) ensureCreated(r *LVMClusterReconciler, ctx context.Context, }, } - err := cutil.SetControllerReference(lvmCluster, ds, r.Scheme) - if err != nil { - return fmt.Errorf("failed to set controller reference to topolvm node daemonset: %w", err) - } - logger.Info("running CreateOrUpdate") result, err := cutil.CreateOrUpdate(ctx, r.Client, ds, func() error { + if err := cutil.SetControllerReference(lvmCluster, ds, r.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference to topolvm node daemonset: %w", err) + } // at creation, deep copy the whole daemonSet if ds.CreationTimestamp.IsZero() { dsTemplate.DeepCopyInto(ds) @@ -106,28 +102,8 @@ func (n topolvmNode) ensureCreated(r *LVMClusterReconciler, ctx context.Context, return nil } -// ensureDeleted should wait for the resources to be cleaned up -func (n topolvmNode) ensureDeleted(r *LVMClusterReconciler, ctx context.Context, _ *lvmv1alpha1.LVMCluster) error { - logger := log.FromContext(ctx).WithValues("topolvmNode", n.getName()) - NodeDaemonSet := &appsv1.DaemonSet{} - - if err := r.Client.Get(ctx, - types.NamespacedName{Name: TopolvmNodeDaemonsetName, Namespace: r.Namespace}, - NodeDaemonSet); err != nil { - return client.IgnoreNotFound(err) - } - - // if not deleted, initiate deletion - if !NodeDaemonSet.GetDeletionTimestamp().IsZero() { - return fmt.Errorf("topolvm csi node daemonset %s is already marked for deletion", TopolvmNodeDaemonsetName) - } - - if err := r.Client.Delete(ctx, NodeDaemonSet); err != nil { - return fmt.Errorf("failed to delete topolvm node daemonset %s: %w", TopolvmNodeDaemonsetName, err) - } - - logger.Info("initiated topolvm node DaemonSet deletion", "name", TopolvmNodeDaemonsetName) - +// ensureDeleted is a noop. Deletion will be handled by ownerref +func (n topolvmNode) ensureDeleted(_ *LVMClusterReconciler, _ context.Context, _ *lvmv1alpha1.LVMCluster) error { return nil } diff --git a/controllers/topolvm_snapshotclass.go b/controllers/topolvm_snapshotclass.go index 995d1ed23..3e7d11a8b 100644 --- a/controllers/topolvm_snapshotclass.go +++ b/controllers/topolvm_snapshotclass.go @@ -69,32 +69,30 @@ func (s topolvmVolumeSnapshotClass) ensureCreated(r *LVMClusterReconciler, ctx c func (s topolvmVolumeSnapshotClass) ensureDeleted(r *LVMClusterReconciler, ctx context.Context, lvmCluster *lvmv1alpha1.LVMCluster) error { logger := log.FromContext(ctx).WithValues("resourceManager", s.getName()) - // construct name of volume snapshot class based on CR spec deviceClass field and - // delete the corresponding volume snapshot class for _, deviceClass := range lvmCluster.Spec.Storage.DeviceClasses { - vsc := &snapapi.VolumeSnapshotClass{} + // construct name of volume snapshot class based on CR spec deviceClass field and + // delete the corresponding volume snapshot class vscName := getVolumeSnapshotClassName(deviceClass.Name) - err := r.Client.Get(ctx, types.NamespacedName{Name: vscName}, vsc) + logger := logger.WithValues("VolumeSnapshotClass", vscName) + vsc := &snapapi.VolumeSnapshotClass{} if err := r.Client.Get(ctx, types.NamespacedName{Name: vscName}, vsc); err != nil { // this is necessary in case the VolumeSnapshotClass CRDs are not registered in the Distro, e.g. for OpenShift Local if discovery.IsGroupDiscoveryFailedError(errors.Unwrap(err)) { - logger.Info("topolvm volume snapshot classes do not exist on the cluster, ignoring", "VolumeSnapshotClass", vscName) + logger.Info("VolumeSnapshotClasses do not exist on the cluster, ignoring") return nil } return client.IgnoreNotFound(err) } - // VolumeSnapshotClass exists, initiate deletion if !vsc.GetDeletionTimestamp().IsZero() { - // return error for next reconcile to confirm deletion - return fmt.Errorf("topolvm volume snapshot class %s is already marked for deletion", vscName) + return fmt.Errorf("the VolumeSnapshotClass %s is still present, waiting for deletion", vscName) } - if err = r.Client.Delete(ctx, vsc); err != nil { + if err := r.Client.Delete(ctx, vsc); err != nil { return fmt.Errorf("failed to delete topolvm VolumeSnapshotClass %s: %w", vscName, err) } - logger.Info("initiated topolvm volume snapshot class deletion", "VolumeSnapshotClass", vscName) + logger.Info("initiated VolumeSnapshotClass deletion") } return nil } diff --git a/controllers/topolvm_storageclass.go b/controllers/topolvm_storageclass.go index a3f374bb0..9605e021b 100644 --- a/controllers/topolvm_storageclass.go +++ b/controllers/topolvm_storageclass.go @@ -66,22 +66,22 @@ func (s topolvmStorageClass) ensureDeleted(r *LVMClusterReconciler, ctx context. // construct name of storage class based on CR spec deviceClass field and // delete the corresponding storage class for _, deviceClass := range lvmCluster.Spec.Storage.DeviceClasses { - sc := &storagev1.StorageClass{} scName := getStorageClassName(deviceClass.Name) + logger := logger.WithValues("StorageClass", scName) + sc := &storagev1.StorageClass{} if err := r.Client.Get(ctx, types.NamespacedName{Name: scName}, sc); err != nil { return client.IgnoreNotFound(err) } if !sc.GetDeletionTimestamp().IsZero() { - // return error for next reconcile to confirm deletion - return fmt.Errorf("topolvm storage class %s is already marked for deletion", scName) + return fmt.Errorf("the StorageClass %s is still present, waiting for deletion", scName) } if err := r.Client.Delete(ctx, sc); err != nil { - return fmt.Errorf("failed to delete topolvm storage class %s: %w", scName, err) + return fmt.Errorf("failed to delete StorageClass %s: %w", scName, err) } - logger.Info("initiated topolvm storage class deletion", "StorageClass", scName) + logger.Info("initiated StorageClass deletion") } return nil } diff --git a/controllers/vgmanager.go b/controllers/vgmanager.go index 0994971a6..714b723e7 100644 --- a/controllers/vgmanager.go +++ b/controllers/vgmanager.go @@ -45,12 +45,6 @@ func (v vgManager) ensureCreated(r *LVMClusterReconciler, ctx context.Context, l // get desired daemonset spec dsTemplate := newVGManagerDaemonset(lvmCluster, r.Namespace, r.ImageName) - // controller reference - err := ctrl.SetControllerReference(lvmCluster, &dsTemplate, r.Scheme) - if err != nil { - return fmt.Errorf("failed to set controller reference on vgManager daemonset %q. %v", dsTemplate.Name, err) - } - // create desired daemonset or update mutable fields on existing one ds := &appsv1.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ @@ -62,14 +56,15 @@ func (v vgManager) ensureCreated(r *LVMClusterReconciler, ctx context.Context, l // the anonymous mutate function modifies the daemonset object after fetching it. // if the daemonset does not already exist, it creates it, otherwise, it updates it result, err := ctrl.CreateOrUpdate(ctx, r.Client, ds, func() error { + if err := ctrl.SetControllerReference(lvmCluster, ds, r.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference on vgManager daemonset %q. %v", dsTemplate.Name, err) + } // at creation, deep copy the whole daemonset if ds.CreationTimestamp.IsZero() { dsTemplate.DeepCopyInto(ds) return nil } // if update, update only mutable fields - - // copy selector labels to daemonset and template initMapIfNil(&ds.ObjectMeta.Labels) initMapIfNil(&ds.Spec.Template.ObjectMeta.Labels) for key, value := range dsTemplate.Labels { @@ -77,24 +72,11 @@ func (v vgManager) ensureCreated(r *LVMClusterReconciler, ctx context.Context, l ds.Spec.Template.ObjectMeta.Labels[key] = value } - // containers ds.Spec.Template.Spec.Containers = dsTemplate.Spec.Template.Spec.Containers - - // volumes ds.Spec.Template.Spec.Volumes = dsTemplate.Spec.Template.Spec.Volumes - - // service account ds.Spec.Template.Spec.ServiceAccountName = dsTemplate.Spec.Template.Spec.ServiceAccountName - - // controller reference - err := ctrl.SetControllerReference(lvmCluster, ds, r.Scheme) - if err != nil { - return fmt.Errorf("failed to update controller reference on vgManager daemonset %q. %v", ds.Name, err) - } - // tolerations ds.Spec.Template.Spec.Tolerations = dsTemplate.Spec.Template.Spec.Tolerations - // nodeSelector if non-nil if dsTemplate.Spec.Template.Spec.Affinity != nil { setDaemonsetNodeSelector(dsTemplate.Spec.Template.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution, ds) }