From a97475601a5754c395fb3a2ebcacc667a9bc7888 Mon Sep 17 00:00:00 2001 From: Benamar Mekhissi Date: Mon, 21 Oct 2024 08:18:31 -0400 Subject: [PATCH 01/15] Fix VRG resource updates causing RDSpec list alternation Fix an issue where the VRG resource was frequently updated, causing the RDSpec to alternate between an empty and non-empty list. This behavior directly impacted failover and relocation. If the list was empty during these actions, PVC restore was skipped, leading to incomplete recovery. Signed-off-by: Benamar Mekhissi --- internal/controller/drplacementcontrol.go | 5 +- .../controller/drplacementcontrolvolsync.go | 227 +++++------------- 2 files changed, 56 insertions(+), 176 deletions(-) diff --git a/internal/controller/drplacementcontrol.go b/internal/controller/drplacementcontrol.go index 4ce206d5a..b662c3516 100644 --- a/internal/controller/drplacementcontrol.go +++ b/internal/controller/drplacementcontrol.go @@ -935,8 +935,7 @@ func (d *DRPCInstance) ensureCleanupAndVolSyncReplicationSetup(srcCluster string // in the MW, but the VRGs in the vrgs slice are fetched using MCV. vrg, ok := d.vrgs[srcCluster] if !ok || len(vrg.Spec.VolSync.RDSpec) != 0 { - return fmt.Errorf(fmt.Sprintf("Waiting for RDSpec count on cluster %s to go to zero. VRG OK? %v", - srcCluster, ok)) + return fmt.Errorf(fmt.Sprintf("Waiting for RDSpec count on cluster %s to go to zero. VRG OK? %v", srcCluster, ok)) } err = d.EnsureCleanup(srcCluster) @@ -1793,7 +1792,7 @@ func (d *DRPCInstance) cleanupForVolSync(clusterToSkip string) error { // Recreate the VRG ManifestWork for the secondary. This typically happens during Hub Recovery. if errors.IsNotFound(err) { - err := d.createVolSyncDestManifestWork(clusterToSkip) + err := d.ensureVolSyncSetup(clusterToSkip) if err != nil { return err } diff --git a/internal/controller/drplacementcontrolvolsync.go b/internal/controller/drplacementcontrolvolsync.go index 6e5b6624d..52484b212 100644 --- a/internal/controller/drplacementcontrolvolsync.go +++ b/internal/controller/drplacementcontrolvolsync.go @@ -32,20 +32,14 @@ func (d *DRPCInstance) EnsureVolSyncReplicationSetup(homeCluster string) error { return nil } - err = d.ensureVolSyncReplicationCommon(homeCluster) - if err != nil { - return err - } - - return d.ensureVolSyncReplicationDestination(homeCluster) + return d.ensureVolSyncSetup(homeCluster) } -func (d *DRPCInstance) ensureVolSyncReplicationCommon(srcCluster string) error { - // Make sure we have Source and Destination VRGs - Source should already have been created at this point +func (d *DRPCInstance) ensureVolSyncSetup(srcCluster string) error { d.setProgression(rmn.ProgressionEnsuringVolSyncSetup) // Create or update the destination VRG - err := d.createVolSyncDestManifestWork(srcCluster) + err := d.createOrUpdateVolSyncDestManifestWork(srcCluster) if err != nil { return err } @@ -94,93 +88,6 @@ func (d *DRPCInstance) ensureVolSyncReplicationCommon(srcCluster string) error { return nil } -func (d *DRPCInstance) ensureVolSyncReplicationDestination(srcCluster string) error { - d.setProgression(rmn.ProgressionSettingupVolsyncDest) - - srcVRG, found := d.vrgs[srcCluster] - if !found { - return fmt.Errorf("failed to find source VolSync VRG in cluster %s. VRGs %v", srcCluster, d.vrgs) - } - - d.log.Info("Ensuring VolSync replication destination") - - if len(srcVRG.Status.ProtectedPVCs) == 0 { - d.log.Info("ProtectedPVCs on pirmary cluster is empty") - - return WaitForSourceCluster - } - - for dstCluster, dstVRG := range d.vrgs { - if dstCluster == srcCluster { - continue - } - - if dstVRG == nil { - return fmt.Errorf("invalid VolSync VRG entry") - } - - volSyncPVCCount := d.getVolSyncPVCCount(srcCluster) - if len(dstVRG.Spec.VolSync.RDSpec) != volSyncPVCCount || d.containsMismatchVolSyncPVCs(srcVRG, dstVRG) { - err := d.updateDestinationVRG(dstCluster, srcVRG, dstVRG) - if err != nil { - return fmt.Errorf("failed to update dst VRG on cluster %s - %w", dstCluster, err) - } - } - - d.log.Info(fmt.Sprintf("Ensured VolSync replication destination for cluster %s", dstCluster)) - // TODO: Should we handle more than one dstVRG? For now, just settle for one. - break - } - - return nil -} - -// containsMismatchVolSyncPVCs returns true if a VolSync protected pvc in the source VRG is not -// found in the destination VRG RDSpecs. Since we never delete protected PVCS from the source VRG, -// we don't check for other case - a protected PVC in destination not found in the source. -func (d *DRPCInstance) containsMismatchVolSyncPVCs(srcVRG *rmn.VolumeReplicationGroup, - dstVRG *rmn.VolumeReplicationGroup, -) bool { - for _, protectedPVC := range srcVRG.Status.ProtectedPVCs { - if !protectedPVC.ProtectedByVolSync { - continue - } - - for _, rdSpec := range dstVRG.Spec.VolSync.RDSpec { - if protectedPVC.Name == rdSpec.ProtectedPVC.Name && - protectedPVC.Namespace == rdSpec.ProtectedPVC.Namespace { - return false - } - } - - // VolSync PVC not found in destination. - return true - } - - return false -} - -func (d *DRPCInstance) updateDestinationVRG(clusterName string, srcVRG *rmn.VolumeReplicationGroup, - dstVRG *rmn.VolumeReplicationGroup, -) error { - // clear RDSpec - dstVRG.Spec.VolSync.RDSpec = nil - - for _, protectedPVC := range srcVRG.Status.ProtectedPVCs { - if !protectedPVC.ProtectedByVolSync { - continue - } - - rdSpec := rmn.VolSyncReplicationDestinationSpec{ - ProtectedPVC: protectedPVC, - } - - dstVRG.Spec.VolSync.RDSpec = append(dstVRG.Spec.VolSync.RDSpec, rdSpec) - } - - return d.updateVRGSpec(clusterName, dstVRG) -} - func (d *DRPCInstance) IsVolSyncReplicationRequired(homeCluster string) (bool, error) { if d.volSyncDisabled { d.log.Info("VolSync is disabled") @@ -214,85 +121,16 @@ func (d *DRPCInstance) IsVolSyncReplicationRequired(homeCluster string) (bool, e return !required, nil } -func (d *DRPCInstance) getVolSyncPVCCount(homeCluster string) int { - pvcCount := 0 - vrg := d.vrgs[homeCluster] - - if vrg == nil { - d.log.Info(fmt.Sprintf("getVolSyncPVCCount: VRG not available on cluster %s", homeCluster)) - - return pvcCount - } - - for _, protectedPVC := range vrg.Status.ProtectedPVCs { - if protectedPVC.ProtectedByVolSync { - pvcCount++ - } - } - - return pvcCount -} - -func (d *DRPCInstance) updateVRGSpec(clusterName string, tgtVRG *rmn.VolumeReplicationGroup) error { - mw, err := d.mwu.FindManifestWorkByType(rmnutil.MWTypeVRG, clusterName) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - - d.log.Error(err, "failed to update VRG") - - return fmt.Errorf("failed to update VRG MW, in namespace %s (%w)", - clusterName, err) - } - - d.log.Info(fmt.Sprintf("Updating VRG ownedby MW %s for cluster %s", mw.Name, clusterName)) - - vrg, err := rmnutil.ExtractVRGFromManifestWork(mw) - if err != nil { - d.log.Error(err, "failed to update VRG state") - - return err - } - - if vrg.Spec.ReplicationState != rmn.Secondary { - d.log.Info(fmt.Sprintf("VRG %s is not secondary on this cluster %s", vrg.Name, mw.Namespace)) - - return fmt.Errorf("failed to update MW due to wrong VRG state (%v) for the request", - vrg.Spec.ReplicationState) - } - - vrg.Spec.VolSync.RDSpec = tgtVRG.Spec.VolSync.RDSpec - - vrgClientManifest, err := d.mwu.GenerateManifest(vrg) - if err != nil { - d.log.Error(err, "failed to generate manifest") - - return fmt.Errorf("failed to generate VRG manifest (%w)", err) - } - - mw.Spec.Workload.Manifests[0] = *vrgClientManifest - - err = d.reconciler.Update(d.ctx, mw) - if err != nil { - return fmt.Errorf("failed to update MW (%w)", err) - } - - d.log.Info(fmt.Sprintf("Updated VRG running in cluster %s. VRG (%s)", clusterName, vrg.Name)) - - return nil -} - -// createVolSyncDestManifestWork creates volsync Secondaries skipping the cluster referenced in clusterToSkip. -// Typically, clusterToSkip is passed in as the cluster where volsync is the Primary. -func (d *DRPCInstance) createVolSyncDestManifestWork(clusterToSkip string) error { +// createOrUpdateVolSyncDestManifestWork creates or updates volsync Secondaries skipping the cluster srcCluster. +// The srcCluster is primary cluster. +func (d *DRPCInstance) createOrUpdateVolSyncDestManifestWork(srcCluster string) error { // create VRG ManifestWork - d.log.Info("Creating VRG ManifestWork for destination clusters", - "Last State:", d.getLastDRState(), "homeCluster", clusterToSkip) + d.log.Info("Creating or updating VRG ManifestWork for destination clusters", + "Last State:", d.getLastDRState(), "homeCluster", srcCluster) // Create or update ManifestWork for all the peers for _, dstCluster := range rmnutil.DRPolicyClusterNames(d.drPolicy) { - if dstCluster == clusterToSkip { + if dstCluster == srcCluster { // skip source cluster continue } @@ -308,10 +146,14 @@ func (d *DRPCInstance) createVolSyncDestManifestWork(clusterToSkip string) error annotations[DRPCNameAnnotation] = d.instance.Name annotations[DRPCNamespaceAnnotation] = d.instance.Namespace - vrg := d.generateVRG(dstCluster, rmn.Secondary) + vrg, err := d.refreshRDSpec(srcCluster, dstCluster) + if err != nil { + return err + } + if err := d.mwu.CreateOrUpdateVRGManifestWork( d.instance.Name, d.vrgNamespace, - dstCluster, vrg, annotations); err != nil { + dstCluster, *vrg, annotations); err != nil { d.log.Error(err, "failed to create or update VolumeReplicationGroup manifest") return fmt.Errorf("failed to create or update VolumeReplicationGroup manifest in namespace %s (%w)", dstCluster, err) @@ -321,9 +163,48 @@ func (d *DRPCInstance) createVolSyncDestManifestWork(clusterToSkip string) error break } + d.log.Info("Ensured VolSync replication for destination clusters") + return nil } +func (d *DRPCInstance) refreshRDSpec(srcCluster, dstCluster string) (*rmn.VolumeReplicationGroup, error) { + d.setProgression(rmn.ProgressionSettingupVolsyncDest) + + srcVRG, found := d.vrgs[srcCluster] + if !found { + return nil, fmt.Errorf("failed to find source VolSync VRG in cluster %s. VRGs %v", srcCluster, d.vrgs) + } + + if len(srcVRG.Status.ProtectedPVCs) == 0 { + d.log.Info("ProtectedPVCs on pirmary cluster is empty") + + return nil, WaitForSourceCluster + } + + dstVRG := d.generateVRG(dstCluster, rmn.Secondary) + d.resetRDSpec(srcVRG, &dstVRG) + + return &dstVRG, nil +} + +func (d *DRPCInstance) resetRDSpec(srcVRG, dstVRG *rmn.VolumeReplicationGroup, +) { + dstVRG.Spec.VolSync.RDSpec = nil + + for _, protectedPVC := range srcVRG.Status.ProtectedPVCs { + if !protectedPVC.ProtectedByVolSync { + continue + } + + rdSpec := rmn.VolSyncReplicationDestinationSpec{ + ProtectedPVC: protectedPVC, + } + + dstVRG.Spec.VolSync.RDSpec = append(dstVRG.Spec.VolSync.RDSpec, rdSpec) + } +} + func (d *DRPCInstance) ResetVolSyncRDOnPrimary(clusterName string) error { if d.volSyncDisabled { d.log.Info("VolSync is disabled") From c46cc59d5a093cc933c7737137b6c4cb8902d782 Mon Sep 17 00:00:00 2001 From: Benamar Mekhissi Date: Mon, 21 Oct 2024 13:20:53 -0400 Subject: [PATCH 02/15] Update function to return last operation result and error This commit modifies the utility function that creates the ManifestWork to return an additional value indicating the last operation result alongside the error. The result can be one of three values: created, updated, or none. This change is needed to track whether the ManifestWork resource was newly created, updated, or left unchanged. Signed-off-by: Benamar Mekhissi --- internal/controller/drplacementcontrol.go | 2 +- .../drplacementcontrol_controller.go | 4 +- .../drplacementcontrol_controller_test.go | 3 +- .../controller/drplacementcontrolvolsync.go | 33 +++++++------- internal/controller/util/mw_util.go | 43 +++++++++++++------ 5 files changed, 53 insertions(+), 32 deletions(-) diff --git a/internal/controller/drplacementcontrol.go b/internal/controller/drplacementcontrol.go index b662c3516..04aae2cb1 100644 --- a/internal/controller/drplacementcontrol.go +++ b/internal/controller/drplacementcontrol.go @@ -1530,7 +1530,7 @@ func (d *DRPCInstance) createVRGManifestWork(homeCluster string, repState rmn.Re annotations[DRPCNameAnnotation] = d.instance.Name annotations[DRPCNamespaceAnnotation] = d.instance.Namespace - if err := d.mwu.CreateOrUpdateVRGManifestWork( + if _, err := d.mwu.CreateOrUpdateVRGManifestWork( d.instance.Name, d.vrgNamespace, homeCluster, vrg, annotations); err != nil { d.log.Error(err, "failed to create or update VolumeReplicationGroup manifest") diff --git a/internal/controller/drplacementcontrol_controller.go b/internal/controller/drplacementcontrol_controller.go index 227b2af7c..0d73a51bf 100644 --- a/internal/controller/drplacementcontrol_controller.go +++ b/internal/controller/drplacementcontrol_controller.go @@ -2244,7 +2244,7 @@ func adoptExistingVRGManifestWork( annotations[DRPCNameAnnotation] = drpc.Name annotations[DRPCNamespaceAnnotation] = drpc.Namespace - err := mwu.CreateOrUpdateVRGManifestWork(drpc.Name, vrgNamespace, cluster, *vrg, annotations) + _, err := mwu.CreateOrUpdateVRGManifestWork(drpc.Name, vrgNamespace, cluster, *vrg, annotations) if err != nil { log.Info("error updating VRG via ManifestWork during adoption", "error", err, "cluster", cluster) } @@ -2281,7 +2281,7 @@ func adoptOrphanVRG( vrg.Annotations[DRPCUIDAnnotation] = string(drpc.UID) - if err := mwu.CreateOrUpdateVRGManifestWork( + if _, err := mwu.CreateOrUpdateVRGManifestWork( drpc.Name, vrgNamespace, cluster, *vrg, annotations); err != nil { log.Info("error creating VRG via ManifestWork during adoption", "error", err, "cluster", cluster) diff --git a/internal/controller/drplacementcontrol_controller_test.go b/internal/controller/drplacementcontrol_controller_test.go index d65596972..dc51ac818 100644 --- a/internal/controller/drplacementcontrol_controller_test.go +++ b/internal/controller/drplacementcontrol_controller_test.go @@ -1004,7 +1004,8 @@ func createVRGMW(name, namespace, homeCluster string) { TargetNamespace: namespace, } - Expect(mwu.CreateOrUpdateVRGManifestWork(name, namespace, homeCluster, *vrg, nil)).To(Succeed()) + _, err := mwu.CreateOrUpdateVRGManifestWork(name, namespace, homeCluster, *vrg, nil) + Expect(err).To(Succeed()) } func updateManifestWorkStatus(clusterNamespace, vrgNamespace, mwType, workType string) { diff --git a/internal/controller/drplacementcontrolvolsync.go b/internal/controller/drplacementcontrolvolsync.go index 52484b212..36cfe45f7 100644 --- a/internal/controller/drplacementcontrolvolsync.go +++ b/internal/controller/drplacementcontrolvolsync.go @@ -10,6 +10,7 @@ import ( rmnutil "github.com/ramendr/ramen/internal/controller/util" "github.com/ramendr/ramen/internal/controller/volsync" "k8s.io/apimachinery/pkg/api/errors" + ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) func (d *DRPCInstance) EnsureVolSyncReplicationSetup(homeCluster string) error { @@ -39,15 +40,12 @@ func (d *DRPCInstance) ensureVolSyncSetup(srcCluster string) error { d.setProgression(rmn.ProgressionEnsuringVolSyncSetup) // Create or update the destination VRG - err := d.createOrUpdateVolSyncDestManifestWork(srcCluster) + opResult, err := d.createOrUpdateVolSyncDestManifestWork(srcCluster) if err != nil { return err } - vrgMWCount := d.mwu.GetVRGManifestWorkCount(rmnutil.DRPolicyClusterNames(d.drPolicy)) - - const maxNumberOfVRGs = 2 - if len(d.vrgs) != maxNumberOfVRGs || vrgMWCount != maxNumberOfVRGs { + if opResult == ctrlutil.OperationResultCreated { return WaitForVolSyncManifestWorkCreation } @@ -123,7 +121,7 @@ func (d *DRPCInstance) IsVolSyncReplicationRequired(homeCluster string) (bool, e // createOrUpdateVolSyncDestManifestWork creates or updates volsync Secondaries skipping the cluster srcCluster. // The srcCluster is primary cluster. -func (d *DRPCInstance) createOrUpdateVolSyncDestManifestWork(srcCluster string) error { +func (d *DRPCInstance) createOrUpdateVolSyncDestManifestWork(srcCluster string) (ctrlutil.OperationResult, error) { // create VRG ManifestWork d.log.Info("Creating or updating VRG ManifestWork for destination clusters", "Last State:", d.getLastDRState(), "homeCluster", srcCluster) @@ -137,8 +135,9 @@ func (d *DRPCInstance) createOrUpdateVolSyncDestManifestWork(srcCluster string) err := d.ensureNamespaceManifestWork(dstCluster) if err != nil { - return fmt.Errorf("creating ManifestWork couldn't ensure namespace '%s' on cluster %s exists", - d.instance.Namespace, dstCluster) + return ctrlutil.OperationResultNone, + fmt.Errorf("creating ManifestWork couldn't ensure namespace '%s' on cluster %s exists", + d.instance.Namespace, dstCluster) } annotations := make(map[string]string) @@ -148,24 +147,26 @@ func (d *DRPCInstance) createOrUpdateVolSyncDestManifestWork(srcCluster string) vrg, err := d.refreshRDSpec(srcCluster, dstCluster) if err != nil { - return err + return ctrlutil.OperationResultNone, err } - if err := d.mwu.CreateOrUpdateVRGManifestWork( + opResult, err := d.mwu.CreateOrUpdateVRGManifestWork( d.instance.Name, d.vrgNamespace, - dstCluster, *vrg, annotations); err != nil { + dstCluster, *vrg, annotations) + if err != nil { d.log.Error(err, "failed to create or update VolumeReplicationGroup manifest") - return fmt.Errorf("failed to create or update VolumeReplicationGroup manifest in namespace %s (%w)", dstCluster, err) + return ctrlutil.OperationResultNone, + fmt.Errorf("failed to create or update VRG MW in namespace %s (%w)", dstCluster, err) } + d.log.Info(fmt.Sprintf("Ensured VolSync replication for destination cluster %s. op %s", dstCluster, opResult)) + // For now, assume only a pair of clusters in the DRClusterSet - break + return opResult, nil } - d.log.Info("Ensured VolSync replication for destination clusters") - - return nil + return ctrlutil.OperationResultNone, nil } func (d *DRPCInstance) refreshRDSpec(srcCluster, dstCluster string) (*rmn.VolumeReplicationGroup, error) { diff --git a/internal/controller/util/mw_util.go b/internal/controller/util/mw_util.go index 55b7a04d9..44ab2a516 100644 --- a/internal/controller/util/mw_util.go +++ b/internal/controller/util/mw_util.go @@ -26,6 +26,7 @@ import ( csiaddonsv1alpha1 "github.com/csi-addons/kubernetes-csi-addons/api/csiaddons/v1alpha1" rmn "github.com/ramendr/ramen/api/v1alpha1" + ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) const ( @@ -121,10 +122,10 @@ func IsManifestInAppliedState(mw *ocmworkv1.ManifestWork) bool { func (mwu *MWUtil) CreateOrUpdateVRGManifestWork( name, namespace, homeCluster string, vrg rmn.VolumeReplicationGroup, annotations map[string]string, -) error { +) (ctrlutil.OperationResult, error) { manifestWork, err := mwu.generateVRGManifestWork(name, namespace, homeCluster, vrg, annotations) if err != nil { - return err + return ctrlutil.OperationResultNone, err } return mwu.createOrUpdateManifestWork(manifestWork, homeCluster) @@ -163,7 +164,9 @@ func (mwu *MWUtil) CreateOrUpdateMModeManifestWork( return err } - return mwu.createOrUpdateManifestWork(manifestWork, cluster) + _, err = mwu.createOrUpdateManifestWork(manifestWork, cluster) + + return err } func (mwu *MWUtil) generateMModeManifestWork(name, cluster string, @@ -242,7 +245,9 @@ func (mwu *MWUtil) CreateOrUpdateNFManifestWork( return err } - return mwu.createOrUpdateManifestWork(manifestWork, homeCluster) + _, err = mwu.createOrUpdateManifestWork(manifestWork, homeCluster) + + return err } func (mwu *MWUtil) generateNFManifestWork(name, homeCluster string, @@ -280,7 +285,9 @@ func (mwu *MWUtil) CreateOrUpdateDRCConfigManifestWork(cluster string, cConfig r return err } - return mwu.createOrUpdateManifestWork(manifestWork, cluster) + _, err = mwu.createOrUpdateManifestWork(manifestWork, cluster) + + return err } func (mwu *MWUtil) generateDRCConfigManifestWork( @@ -358,7 +365,9 @@ func (mwu *MWUtil) CreateOrUpdateNamespaceManifest( PropagationPolicy: ocmworkv1.DeletePropagationPolicyTypeOrphan, } - return mwu.createOrUpdateManifestWork(manifestWork, managedClusterNamespace) + _, err = mwu.createOrUpdateManifestWork(manifestWork, managedClusterNamespace) + + return err } func Namespace(name string) *corev1.Namespace { @@ -453,7 +462,7 @@ func (mwu *MWUtil) CreateOrUpdateDrClusterManifestWork( manifests[i] = *manifest } - return mwu.createOrUpdateManifestWork( + _, err := mwu.createOrUpdateManifestWork( mwu.newManifestWork( DrClusterManifestWorkName, clusterName, @@ -462,6 +471,8 @@ func (mwu *MWUtil) CreateOrUpdateDrClusterManifestWork( ), clusterName, ) + + return err } var ( @@ -591,25 +602,29 @@ func (mwu *MWUtil) newManifestWork(name string, mcNamespace string, func (mwu *MWUtil) createOrUpdateManifestWork( mw *ocmworkv1.ManifestWork, managedClusternamespace string, -) error { +) (ctrlutil.OperationResult, error) { key := types.NamespacedName{Name: mw.Name, Namespace: managedClusternamespace} foundMW := &ocmworkv1.ManifestWork{} err := mwu.Client.Get(mwu.Ctx, key, foundMW) if err != nil { if !errors.IsNotFound(err) { - return errorswrapper.Wrap(err, fmt.Sprintf("failed to fetch ManifestWork %s", key)) + return ctrlutil.OperationResultNone, errorswrapper.Wrap(err, fmt.Sprintf("failed to fetch ManifestWork %s", key)) } mwu.Log.Info("Creating ManifestWork", "cluster", managedClusternamespace, "MW", mw) - return mwu.Client.Create(mwu.Ctx, mw) + if err := mwu.Create(mwu.Ctx, mw); err != nil { + return ctrlutil.OperationResultNone, err + } + + return ctrlutil.OperationResultCreated, nil } if !reflect.DeepEqual(foundMW.Spec, mw.Spec) { mwu.Log.Info("Updating ManifestWork", "name", mw.Name, "namespace", foundMW.Namespace) - return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { if err := mwu.Client.Get(mwu.Ctx, key, foundMW); err != nil { return err } @@ -618,9 +633,13 @@ func (mwu *MWUtil) createOrUpdateManifestWork( return mwu.Client.Update(mwu.Ctx, foundMW) }) + + if err == nil { + return ctrlutil.OperationResultUpdated, nil + } } - return nil + return ctrlutil.OperationResultNone, nil } func (mwu *MWUtil) GetVRGManifestWorkCount(drClusters []string) int { From fcf6be9c9ebcffb4f227db8e6187e6c7aa404f4e Mon Sep 17 00:00:00 2001 From: Benamar Mekhissi Date: Wed, 23 Oct 2024 12:43:07 -0400 Subject: [PATCH 03/15] Unit test to validate RDSpec changes Signed-off-by: Benamar Mekhissi --- internal/controller/drplacementcontrol.go | 7 +- .../drplacementcontrol_controller_test.go | 207 ++++++++++++++++-- internal/controller/util/mw_util.go | 15 -- 3 files changed, 187 insertions(+), 42 deletions(-) diff --git a/internal/controller/drplacementcontrol.go b/internal/controller/drplacementcontrol.go index 04aae2cb1..0ca21ce3b 100644 --- a/internal/controller/drplacementcontrol.go +++ b/internal/controller/drplacementcontrol.go @@ -1405,6 +1405,8 @@ func (d *DRPCInstance) moveVRGToSecondaryEverywhere() bool { } func (d *DRPCInstance) cleanupSecondaries(skipCluster string) (bool, error) { + d.log.Info("Cleaning up secondaries.") + for _, clusterName := range rmnutil.DRPolicyClusterNames(d.drPolicy) { if skipCluster == clusterName { continue @@ -1759,7 +1761,7 @@ func (d *DRPCInstance) EnsureCleanup(clusterToSkip string) error { } if !clean { - msg := "cleaning secondaries" + msg := "cleaning up secondaries" addOrUpdateCondition(&d.instance.Status.Conditions, rmn.ConditionPeerReady, d.instance.Generation, metav1.ConditionFalse, rmn.ReasonCleaning, msg) @@ -1774,8 +1776,7 @@ func (d *DRPCInstance) EnsureCleanup(clusterToSkip string) error { //nolint:gocognit func (d *DRPCInstance) cleanupForVolSync(clusterToSkip string) error { - d.log.Info("VolSync needs both VRGs. No need to clean up secondary") - d.log.Info("Ensure secondary on peer") + d.log.Info("VolSync needs both VRGs. Ensure secondary setup on peer") peersReady := true diff --git a/internal/controller/drplacementcontrol_controller_test.go b/internal/controller/drplacementcontrol_controller_test.go index dc51ac818..ca44e9918 100644 --- a/internal/controller/drplacementcontrol_controller_test.go +++ b/internal/controller/drplacementcontrol_controller_test.go @@ -56,14 +56,12 @@ const ( SyncDRPolicyName = "my-sync-dr-peers" MModeReplicationID = "storage-replication-id-1" MModeCSIProvisioner = "test.csi.com" - - pvcCount = 2 // Count of fake PVCs reported in the VRG status ) var ( - NumberOfVrgsToReturnWhenRebuildingState = 0 - - UseApplicationSet = false + ProtectedPVCCount = 2 // Count of fake PVCs reported in the VRG status + RunningVolSyncTests = false + UseApplicationSet = false west1Cluster = &spokeClusterV1.ManagedCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -425,7 +423,7 @@ func (f FakeMCVGetter) GetVRGFromManagedCluster(resourceName, resourceNamespace, return nil, fmt.Errorf("%s: Faking cluster down %s", getFunctionNameAtIndex(2), managedCluster) } - vrg, err := getVRGFromManifestWork(managedCluster, resourceNamespace) + vrg, err := GetFakeVRGFromMCVUsingMW(managedCluster, resourceNamespace) if err != nil { if errors.IsNotFound(err) { return fakeVRGConditionally(resourceNamespace, managedCluster, err) @@ -506,7 +504,8 @@ func getVRGNamespace(defaultNamespace string) string { } //nolint:funlen -func getVRGFromManifestWork(managedCluster, resourceNamespace string) (*rmn.VolumeReplicationGroup, error) { +func GetFakeVRGFromMCVUsingMW(managedCluster, resourceNamespace string, +) (*rmn.VolumeReplicationGroup, error) { manifestLookupKey := types.NamespacedName{ Name: rmnutil.ManifestWorkName(DRPCCommonName, getVRGNamespace(resourceNamespace), "vrg"), Namespace: managedCluster, @@ -540,14 +539,10 @@ func getVRGFromManifestWork(managedCluster, resourceNamespace string) (*rmn.Volu vrg.Status.FinalSyncComplete = true vrg.Status.ProtectedPVCs = []rmn.ProtectedPVC{} - for i := 0; i < pvcCount; i++ { - protectedPVC := rmn.ProtectedPVC{} - protectedPVC.Name = fmt.Sprintf("fakePVC%d", i) - protectedPVC.StorageIdentifiers.ReplicationID.ID = MModeReplicationID - protectedPVC.StorageIdentifiers.StorageProvisioner = MModeCSIProvisioner - protectedPVC.StorageIdentifiers.ReplicationID.Modes = []rmn.MMode{rmn.MModeFailover} - - vrg.Status.ProtectedPVCs = append(vrg.Status.ProtectedPVCs, protectedPVC) + if RunningVolSyncTests { + createFakeProtectedPVCsForVolSync(vrg) + } else { + createFakeProtectedPVCsForVolRep(vrg) } // Always report conditions as a success? @@ -598,6 +593,28 @@ func getVRGFromManifestWork(managedCluster, resourceNamespace string) (*rmn.Volu return vrg, nil } +func createFakeProtectedPVCsForVolRep(vrg *rmn.VolumeReplicationGroup) { + for i := 0; i < ProtectedPVCCount; i++ { + protectedPVC := rmn.ProtectedPVC{} + protectedPVC.Name = fmt.Sprintf("fakePVC%d", i) + protectedPVC.StorageIdentifiers.ReplicationID.ID = MModeReplicationID + protectedPVC.StorageIdentifiers.StorageProvisioner = MModeCSIProvisioner + protectedPVC.StorageIdentifiers.ReplicationID.Modes = []rmn.MMode{rmn.MModeFailover} + + vrg.Status.ProtectedPVCs = append(vrg.Status.ProtectedPVCs, protectedPVC) + } +} + +func createFakeProtectedPVCsForVolSync(vrg *rmn.VolumeReplicationGroup) { + for i := 0; i < ProtectedPVCCount; i++ { + protectedPVC := rmn.ProtectedPVC{} + protectedPVC.Name = fmt.Sprintf("fakePVC-%d-for-volsync", i) + protectedPVC.ProtectedByVolSync = true + + vrg.Status.ProtectedPVCs = append(vrg.Status.ProtectedPVCs, protectedPVC) + } +} + func fakeVRGWithMModesProtectedPVC(vrgNamespace string) *rmn.VolumeReplicationGroup { vrg := getDefaultVRG(vrgNamespace).DeepCopy() vrg.Status = rmn.VolumeReplicationGroupStatus{ @@ -1384,7 +1401,7 @@ func verifyDRPCStatusPreferredClusterExpectation(namespace string, drState rmn.D return d.ClusterName == East1ManagedCluster && idx != -1 && condition.Reason == string(drState) && - len(updatedDRPC.Status.ResourceConditions.ResourceMeta.ProtectedPVCs) == pvcCount + len(updatedDRPC.Status.ResourceConditions.ResourceMeta.ProtectedPVCs) == ProtectedPVCCount } return false @@ -1495,7 +1512,7 @@ func runFailoverAction(placementObj client.Object, fromCluster, toCluster string fenceCluster(fromCluster, manualFence) } - recoverToFailoverCluster(placementObj, fromCluster, toCluster) + recoverToFailoverCluster(placementObj, fromCluster, toCluster, false) // TODO: DRCluster as part of Unfence operation, first unfences // the NetworkFence CR and then deletes it. Hence, by the // time this test is made, depending upon whether NetworkFence @@ -1548,7 +1565,7 @@ func runRelocateAction(placementObj client.Object, fromCluster string, isSyncDR resetdrCluster(toCluster1) } - relocateToPreferredCluster(placementObj, fromCluster) + relocateToPreferredCluster(placementObj, fromCluster, false) // TODO: DRCluster as part of Unfence operation, first unfences // the NetworkFence CR and then deletes it. Hence, by the // time this test is made, depending upon whether NetworkFence @@ -1598,7 +1615,7 @@ func clearDRActionAfterRelocate(userPlacementRule *plrv1.PlacementRule, preferre Expect(decision.ClusterName).To(Equal(preferredCluster)) } -func relocateToPreferredCluster(placementObj client.Object, fromCluster string) { +func relocateToPreferredCluster(placementObj client.Object, fromCluster string, skipWaitForWMDeletion bool) { toCluster1 := "east1-cluster" setDRPCSpecExpectationTo(placementObj.GetNamespace(), toCluster1, fromCluster, rmn.ActionRelocate) @@ -1609,12 +1626,14 @@ func relocateToPreferredCluster(placementObj client.Object, fromCluster string) verifyDRPCStatusPreferredClusterExpectation(placementObj.GetNamespace(), rmn.Relocated) verifyVRGManifestWorkCreatedAsPrimary(placementObj.GetNamespace(), toCluster1) - waitForVRGMWDeletion(West1ManagedCluster, placementObj.GetNamespace()) + if !skipWaitForWMDeletion { + waitForVRGMWDeletion(West1ManagedCluster, placementObj.GetNamespace()) + } waitForCompletion(string(rmn.Relocated)) } -func recoverToFailoverCluster(placementObj client.Object, fromCluster, toCluster string) { +func recoverToFailoverCluster(placementObj client.Object, fromCluster, toCluster string, skipWaitForWMDeletion bool) { setDRPCSpecExpectationTo(placementObj.GetNamespace(), fromCluster, toCluster, rmn.ActionFailover) updateManifestWorkStatus(toCluster, placementObj.GetNamespace(), "vrg", ocmworkv1.WorkApplied) @@ -1623,7 +1642,9 @@ func recoverToFailoverCluster(placementObj client.Object, fromCluster, toCluster verifyDRPCStatusPreferredClusterExpectation(placementObj.GetNamespace(), rmn.FailedOver) verifyVRGManifestWorkCreatedAsPrimary(placementObj.GetNamespace(), toCluster) - waitForVRGMWDeletion(fromCluster, placementObj.GetNamespace()) + if !skipWaitForWMDeletion { + waitForVRGMWDeletion(fromCluster, placementObj.GetNamespace()) + } waitForCompletion(string(rmn.FailedOver)) } @@ -1753,7 +1774,7 @@ func verifyInitialDRPCDeployment(userPlacement client.Object, preferredCluster s func verifyFailoverToSecondary(placementObj client.Object, toCluster string, isSyncDR bool, ) { - recoverToFailoverCluster(placementObj, East1ManagedCluster, toCluster) + recoverToFailoverCluster(placementObj, East1ManagedCluster, toCluster, false) // TODO: DRCluster as part of Unfence operation, first unfences // the NetworkFence CR and then deletes it. Hence, by the @@ -1787,7 +1808,7 @@ func verifyActionResultForPlacement(placement *clrapiv1beta1.Placement, homeClus Expect(placementDecision).ShouldNot(BeNil()) Expect(placementDecision.GetLabels()[rmnutil.ExcludeFromVeleroBackup]).Should(Equal("true")) Expect(placementDecision.Status.Decisions[0].ClusterName).Should(Equal(homeCluster)) - vrg, err := getVRGFromManifestWork(homeCluster, placement.GetNamespace()) + vrg, err := GetFakeVRGFromMCVUsingMW(homeCluster, placement.GetNamespace()) Expect(err).NotTo(HaveOccurred()) switch plType { @@ -2653,8 +2674,146 @@ var _ = Describe("DRPlacementControl Reconciler", func() { Expect(getManifestWorkCount(East1ManagedCluster)).Should(Equal(0)) }) }) + + Context("Test DRPlacementControl With VolSync Setup", func() { + var userPlacementRule *plrv1.PlacementRule + var drpc *rmn.DRPlacementControl + + Specify("DRClusters", func() { + RunningVolSyncTests = true + populateDRClusters() + }) + When("The Application is deployed for VolSync", func() { + It("Should deploy to East1ManagedCluster", func() { + var placementObj client.Object + placementObj, drpc = InitialDeploymentAsync( + DefaultDRPCNamespace, UserPlacementRuleName, East1ManagedCluster, UsePlacementRule) + userPlacementRule = placementObj.(*plrv1.PlacementRule) + Expect(userPlacementRule).NotTo(BeNil()) + verifyInitialDRPCDeployment(userPlacementRule, East1ManagedCluster) + verifyDRPCOwnedByPlacement(userPlacementRule, getLatestDRPC(DefaultDRPCNamespace)) + }) + }) + When("DRAction is changed to Failover", func() { + It("Should failover to Secondary (West1ManagedCluster)", func() { + recoverToFailoverCluster(userPlacementRule, East1ManagedCluster, West1ManagedCluster, true) + Expect(getVRGManifestWorkCount()).Should(Equal(2)) + verifyRDSpecAfterActionSwitch(West1ManagedCluster, East1ManagedCluster, 2) + }) + }) + When("DRAction is set to Relocate", func() { + It("Should relocate to Primary (East1ManagedCluster)", func() { + relocateToPreferredCluster(userPlacementRule, West1ManagedCluster, true) + Expect(getVRGManifestWorkCount()).Should(Equal(2)) + verifyRDSpecAfterActionSwitch(East1ManagedCluster, West1ManagedCluster, 2) + }) + }) + When("DRAction is changed back to Failover using only 1 protectedPVC", func() { + It("Should failover to secondary (West1ManagedCluster)", func() { + ProtectedPVCCount = 1 + recoverToFailoverCluster(userPlacementRule, East1ManagedCluster, West1ManagedCluster, true) + Expect(getVRGManifestWorkCount()).Should(Equal(2)) + verifyRDSpecAfterActionSwitch(West1ManagedCluster, East1ManagedCluster, 1) + ProtectedPVCCount = 2 + }) + }) + When("DRAction is set back to Relocate using only 1 protectedPVC", func() { + It("Should relocate to Primary (East1ManagedCluster)", func() { + ProtectedPVCCount = 1 + relocateToPreferredCluster(userPlacementRule, West1ManagedCluster, true) + Expect(getVRGManifestWorkCount()).Should(Equal(2)) + verifyRDSpecAfterActionSwitch(East1ManagedCluster, West1ManagedCluster, 1) + ProtectedPVCCount = 2 + }) + }) + When("DRAction is changed back to Failover using only 10 protectedPVC", func() { + It("Should failover to secondary (West1ManagedCluster)", func() { + ProtectedPVCCount = 10 + recoverToFailoverCluster(userPlacementRule, East1ManagedCluster, West1ManagedCluster, true) + Expect(getVRGManifestWorkCount()).Should(Equal(2)) + verifyRDSpecAfterActionSwitch(West1ManagedCluster, East1ManagedCluster, 10) + ProtectedPVCCount = 2 + }) + }) + When("DRAction is set back to Relocate using only 10 protectedPVC", func() { + It("Should relocate to Primary (East1ManagedCluster)", func() { + ProtectedPVCCount = 10 + relocateToPreferredCluster(userPlacementRule, West1ManagedCluster, true) + Expect(getVRGManifestWorkCount()).Should(Equal(2)) + verifyRDSpecAfterActionSwitch(East1ManagedCluster, West1ManagedCluster, 10) + ProtectedPVCCount = 2 + }) + }) + When("Deleting DRPolicy with DRPC references", func() { + It("Should retain the deleted DRPolicy in the API server", func() { + deleteDRPolicyAsync() + ensureDRPolicyIsNotDeleted(drpc) + }) + }) + When("Deleting user PlacementRule", func() { + It("Should cleanup DRPC", func() { + deleteUserPlacementRule(UserPlacementRuleName, DefaultDRPCNamespace) + }) + }) + + When("Deleting DRPC", func() { + It("Should delete VRG and NS MWs and MCVs from Primary (East1ManagedCluster)", func() { + Expect(getManifestWorkCount(East1ManagedCluster)).Should(BeElementOf(3, 4)) // DRCluster + VRG MW + deleteDRPC() + waitForCompletion("deleted") + Expect(getManifestWorkCount(East1ManagedCluster)).Should(Equal(1)) // DRCluster + Expect(getManagedClusterViewCount(East1ManagedCluster)).Should(Equal(0)) // NS + VRG MCV + ensureNamespaceMWsDeletedFromAllClusters(DefaultDRPCNamespace) + }) + It("should delete the DRPC causing its referenced drpolicy to be deleted"+ + " by drpolicy controller since no DRPCs reference it anymore", func() { + ensureDRPolicyIsDeleted(drpc.Spec.DRPolicyRef.Name) + }) + }) + Specify("delete drclusters", func() { + RunningVolSyncTests = false + deleteDRClustersAsync() + }) + }) }) +func getVRGManifestWorkCount() int { + count := 0 + + for _, drCluster := range drClusters { + mwName := rmnutil.ManifestWorkName(DRPCCommonName, DefaultDRPCNamespace, rmnutil.MWTypeVRG) + mw := &ocmworkv1.ManifestWork{} + + err := k8sClient.Get(context.TODO(), types.NamespacedName{Name: mwName, Namespace: drCluster.Name}, mw) + if err == nil { + count++ + } + } + + return count +} + +func getVRGFromManifestWork(clusterNamespace string) (*rmn.VolumeReplicationGroup, error) { + mwName := rmnutil.ManifestWorkName(DRPCCommonName, DefaultDRPCNamespace, rmnutil.MWTypeVRG) + mw := &ocmworkv1.ManifestWork{} + + err := k8sClient.Get(context.TODO(), types.NamespacedName{Name: mwName, Namespace: clusterNamespace}, mw) + Expect(err).NotTo(HaveOccurred()) + + return rmnutil.ExtractVRGFromManifestWork(mw) +} + +func verifyRDSpecAfterActionSwitch(primaryCluster, secondaryCluster string, numOfRDSpecs int) { + // For Primary Cluster + vrg, err := getVRGFromManifestWork(primaryCluster) + Expect(err).NotTo(HaveOccurred()) + Expect(len(vrg.Spec.VolSync.RDSpec)).Should(Equal(0)) + // For Secondary Cluster + vrg, err = getVRGFromManifestWork(secondaryCluster) + Expect(err).NotTo(HaveOccurred()) + Expect(len(vrg.Spec.VolSync.RDSpec)).Should(Equal(numOfRDSpecs)) +} + func verifyDRPCStateAndProgression(expectedAction rmn.DRAction, expectedPhase rmn.DRState, exptectedPorgression rmn.ProgressionStatus, ) { diff --git a/internal/controller/util/mw_util.go b/internal/controller/util/mw_util.go index 44ab2a516..89f392ebb 100644 --- a/internal/controller/util/mw_util.go +++ b/internal/controller/util/mw_util.go @@ -642,21 +642,6 @@ func (mwu *MWUtil) createOrUpdateManifestWork( return ctrlutil.OperationResultNone, nil } -func (mwu *MWUtil) GetVRGManifestWorkCount(drClusters []string) int { - count := 0 - - for _, clusterName := range drClusters { - _, err := mwu.FindManifestWorkByType(MWTypeVRG, clusterName) - if err != nil { - continue - } - - count++ - } - - return count -} - func (mwu *MWUtil) DeleteNamespaceManifestWork(clusterName string, annotations map[string]string) error { mwName := mwu.BuildManifestWorkName(MWTypeNS) mw := &ocmworkv1.ManifestWork{} From d7f0b8f29be914f5ae78e4c5f41af95fdf5884de Mon Sep 17 00:00:00 2001 From: Benamar Mekhissi Date: Thu, 24 Oct 2024 10:05:27 -0400 Subject: [PATCH 04/15] Fix rare issue with missing PVsRestored condition in ProtectedPVC. In certain edge cases, ProtectedPVCs may fail to add the PVsRestored condition permanently, causing the relocate process to get stuck in the WaitForReadiness progression. Signed-off-by: Benamar Mekhissi --- internal/controller/vrg_volsync.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/controller/vrg_volsync.go b/internal/controller/vrg_volsync.go index 2ef03aecf..d2059262f 100644 --- a/internal/controller/vrg_volsync.go +++ b/internal/controller/vrg_volsync.go @@ -33,6 +33,9 @@ func (v *VRGInstance) restorePVsAndPVCsForVolSync() (int, error) { failoverAction := v.instance.Spec.Action == ramendrv1alpha1.VRGActionFailover var err error + // Source conditions are not needed and should not be added to vrg.status.ProtectedPVCs, + // as this would result in incorrect information. + rdSpec.ProtectedPVC.Conditions = nil cg, ok := rdSpec.ProtectedPVC.Labels[ConsistencyGroupLabel] if ok && util.IsCGEnabled(v.instance.Annotations) { @@ -75,7 +78,8 @@ func (v *VRGInstance) restorePVsAndPVCsForVolSync() (int, error) { } if numPVsRestored != len(v.instance.Spec.VolSync.RDSpec) { - return numPVsRestored, fmt.Errorf("failed to restore all PVCs using RDSpec (%v)", v.instance.Spec.VolSync.RDSpec) + return numPVsRestored, fmt.Errorf("failed to restore all PVCs. Restored %d PVCs out of %d RDSpecs", + numPVsRestored, len(v.instance.Spec.VolSync.RDSpec)) } v.log.Info("Success restoring VolSync PVs", "Total", numPVsRestored) @@ -142,8 +146,8 @@ func (v *VRGInstance) reconcilePVCAsVolSyncPrimary(pvc corev1.PersistentVolumeCl protectedPVC := v.findProtectedPVC(pvc.Namespace, pvc.Name) if protectedPVC == nil { - protectedPVC = newProtectedPVC - v.instance.Status.ProtectedPVCs = append(v.instance.Status.ProtectedPVCs, *protectedPVC) + v.instance.Status.ProtectedPVCs = append(v.instance.Status.ProtectedPVCs, *newProtectedPVC) + protectedPVC = &v.instance.Status.ProtectedPVCs[len(v.instance.Status.ProtectedPVCs)-1] } else if !reflect.DeepEqual(protectedPVC, newProtectedPVC) { newProtectedPVC.Conditions = protectedPVC.Conditions newProtectedPVC.DeepCopyInto(protectedPVC) From aae36952cdaa7329b5615b6aab4d2200e6f2728e Mon Sep 17 00:00:00 2001 From: Benamar Mekhissi Date: Wed, 30 Oct 2024 13:27:54 -0400 Subject: [PATCH 05/15] Rename a function and fix error msgs Signed-off-by: Benamar Mekhissi --- internal/controller/drplacementcontrol.go | 10 +++++----- internal/controller/drplacementcontrol_controller.go | 2 +- internal/controller/drplacementcontrolvolsync.go | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/controller/drplacementcontrol.go b/internal/controller/drplacementcontrol.go index 0ca21ce3b..6d3e1ce7b 100644 --- a/internal/controller/drplacementcontrol.go +++ b/internal/controller/drplacementcontrol.go @@ -874,7 +874,7 @@ func (d *DRPCInstance) RunRelocate() (bool, error) { addOrUpdateCondition(&d.instance.Status.Conditions, rmn.ConditionAvailable, d.instance.Generation, d.getConditionStatusForTypeAvailable(), string(d.instance.Status.Phase), errMsg) - return !done, fmt.Errorf(errMsg) + return !done, fmt.Errorf("%s", errMsg) } if d.getLastDRState() != rmn.Relocating && !d.validatePeerReady() { @@ -935,7 +935,7 @@ func (d *DRPCInstance) ensureCleanupAndVolSyncReplicationSetup(srcCluster string // in the MW, but the VRGs in the vrgs slice are fetched using MCV. vrg, ok := d.vrgs[srcCluster] if !ok || len(vrg.Spec.VolSync.RDSpec) != 0 { - return fmt.Errorf(fmt.Sprintf("Waiting for RDSpec count on cluster %s to go to zero. VRG OK? %v", srcCluster, ok)) + return fmt.Errorf("waiting for RDSpec count on cluster %s to go to zero. VRG OK? %v", srcCluster, ok) } err = d.EnsureCleanup(srcCluster) @@ -1521,10 +1521,10 @@ func (d *DRPCInstance) createVRGManifestWork(homeCluster string, repState rmn.Re } // create VRG ManifestWork - d.log.Info("Creating VRG ManifestWork", + d.log.Info("Creating VRG ManifestWork", "ReplicationState", repState, "Last State:", d.getLastDRState(), "cluster", homeCluster) - vrg := d.generateVRG(homeCluster, repState) + vrg := d.newVRG(homeCluster, repState) vrg.Spec.VolSync.Disabled = d.volSyncDisabled annotations := make(map[string]string) @@ -1589,7 +1589,7 @@ func (d *DRPCInstance) setVRGAction(vrg *rmn.VolumeReplicationGroup) { vrg.Spec.Action = action } -func (d *DRPCInstance) generateVRG(dstCluster string, repState rmn.ReplicationState) rmn.VolumeReplicationGroup { +func (d *DRPCInstance) newVRG(dstCluster string, repState rmn.ReplicationState) rmn.VolumeReplicationGroup { vrg := rmn.VolumeReplicationGroup{ TypeMeta: metav1.TypeMeta{Kind: "VolumeReplicationGroup", APIVersion: "ramendr.openshift.io/v1alpha1"}, ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controller/drplacementcontrol_controller.go b/internal/controller/drplacementcontrol_controller.go index 0d73a51bf..8ffda4f85 100644 --- a/internal/controller/drplacementcontrol_controller.go +++ b/internal/controller/drplacementcontrol_controller.go @@ -1134,7 +1134,7 @@ func getVRGsFromManagedClusters( vrgs[drCluster.Name] = vrg - log.Info("VRG location", "VRG on", drCluster.Name) + log.Info("VRG location", "VRG on", drCluster.Name, "replicationState", vrg.Spec.ReplicationState) } // We are done if we successfully queried all drClusters diff --git a/internal/controller/drplacementcontrolvolsync.go b/internal/controller/drplacementcontrolvolsync.go index 36cfe45f7..56958cfa8 100644 --- a/internal/controller/drplacementcontrolvolsync.go +++ b/internal/controller/drplacementcontrolvolsync.go @@ -183,7 +183,7 @@ func (d *DRPCInstance) refreshRDSpec(srcCluster, dstCluster string) (*rmn.Volume return nil, WaitForSourceCluster } - dstVRG := d.generateVRG(dstCluster, rmn.Secondary) + dstVRG := d.newVRG(dstCluster, rmn.Secondary) d.resetRDSpec(srcVRG, &dstVRG) return &dstVRG, nil @@ -237,7 +237,7 @@ func (d *DRPCInstance) ResetVolSyncRDOnPrimary(clusterName string) error { if vrg.Spec.ReplicationState != rmn.Primary { d.log.Info(fmt.Sprintf("VRG %s not primary on this cluster %s", vrg.Name, mw.Namespace)) - return fmt.Errorf(fmt.Sprintf("VRG %s not primary on this cluster %s", vrg.Name, mw.Namespace)) + return fmt.Errorf("vrg %s is not set as primary on this cluster, %s", vrg.Name, mw.Namespace) } if len(vrg.Spec.VolSync.RDSpec) == 0 { From b05e435e45f704ad78f4d34d867301085206c5f3 Mon Sep 17 00:00:00 2001 From: Benamar Mekhissi Date: Wed, 30 Oct 2024 14:14:54 -0400 Subject: [PATCH 06/15] Optimize VRG ManifestWork Handling to Prevent Redundant Updates When ensuring the VRG ManifestWork, the process now begins by retrieving the VRG from an existing ManifestWork, if available, and updating it as needed. If the ManifestWork does not exist, it will be created. This update-instead-of-create approach avoids overwriting other fields unintentionally and ensures consistency by always starting from a base VRG state. Signed-off-by: Benamar Mekhissi --- internal/controller/drplacementcontrol.go | 71 ++++++++++++++----- .../controller/drplacementcontrolvolsync.go | 18 +---- internal/controller/util/mw_util.go | 20 ++++++ 3 files changed, 73 insertions(+), 36 deletions(-) diff --git a/internal/controller/drplacementcontrol.go b/internal/controller/drplacementcontrol.go index 6d3e1ce7b..95af0b612 100644 --- a/internal/controller/drplacementcontrol.go +++ b/internal/controller/drplacementcontrol.go @@ -1525,7 +1525,6 @@ func (d *DRPCInstance) createVRGManifestWork(homeCluster string, repState rmn.Re "Last State:", d.getLastDRState(), "cluster", homeCluster) vrg := d.newVRG(homeCluster, repState) - vrg.Spec.VolSync.Disabled = d.volSyncDisabled annotations := make(map[string]string) @@ -1549,12 +1548,57 @@ func (d *DRPCInstance) ensureVRGManifestWork(homeCluster string) error { d.log.Info("Ensure VRG ManifestWork", "Last State:", d.getLastDRState(), "cluster", homeCluster) - cachedVrg := d.vrgs[homeCluster] - if cachedVrg == nil { - return fmt.Errorf("failed to get vrg from cluster %s", homeCluster) + mw, mwErr := d.mwu.FindManifestWorkByType(rmnutil.MWTypeVRG, homeCluster) + if mwErr != nil { + if errors.IsNotFound(mwErr) { + cachedVrg := d.vrgs[homeCluster] + if cachedVrg == nil { + return fmt.Errorf("failed to get vrg from cluster %s", homeCluster) + } + + return d.createVRGManifestWork(homeCluster, cachedVrg.Spec.ReplicationState) + } + + return fmt.Errorf("ensure VRG ManifestWork failed (%w)", mwErr) } - return d.createVRGManifestWork(homeCluster, cachedVrg.Spec.ReplicationState) + vrg, err := rmnutil.ExtractVRGFromManifestWork(mw) + if err != nil { + return fmt.Errorf("error extracting VRG from ManifestWork for cluster %s. Error: %w", homeCluster, err) + } + + d.updateVRGOptionalFields(vrg, homeCluster) + + return d.mwu.UpdateVRGManifestWork(vrg, mw) +} + +// updateVRGOptionalFields ensures that the optional fields in the VRG object are up to date. +// This function does not modify the following fields: +// - ObjectMeta.Name +// - ObjectMeta.Namespace +// - Spec.PVCSelector +// - Spec.ReplicationState +// - Spec.PrepareForFinalSync +// - Spec.RunFinalSync +// - Spec.VolSync.RDSpec +// +// These fields are either set during the initial creation of the VRG (e.g., name and namespace) +// or updated as needed, such as the PrepareForFinalSync and RunFinalSync fields. +func (d *DRPCInstance) updateVRGOptionalFields(vrg *rmn.VolumeReplicationGroup, homeCluster string) { + vrg.ObjectMeta.Annotations = map[string]string{ + DestinationClusterAnnotationKey: homeCluster, + DoNotDeletePVCAnnotation: d.instance.GetAnnotations()[DoNotDeletePVCAnnotation], + DRPCUIDAnnotation: string(d.instance.UID), + rmnutil.IsCGEnabledAnnotation: d.instance.GetAnnotations()[rmnutil.IsCGEnabledAnnotation], + } + + vrg.Spec.ProtectedNamespaces = d.instance.Spec.ProtectedNamespaces + vrg.Spec.S3Profiles = AvailableS3Profiles(d.drClusters) + vrg.Spec.KubeObjectProtection = d.instance.Spec.KubeObjectProtection + vrg.Spec.Async = d.generateVRGSpecAsync() + vrg.Spec.Sync = d.generateVRGSpecSync() + vrg.Spec.VolSync.Disabled = d.volSyncDisabled + d.setVRGAction(vrg) } func (d *DRPCInstance) ensurePlacement(homeCluster string) error { @@ -1595,25 +1639,14 @@ func (d *DRPCInstance) newVRG(dstCluster string, repState rmn.ReplicationState) ObjectMeta: metav1.ObjectMeta{ Name: d.instance.Name, Namespace: d.vrgNamespace, - Annotations: map[string]string{ - DestinationClusterAnnotationKey: dstCluster, - DoNotDeletePVCAnnotation: d.instance.GetAnnotations()[DoNotDeletePVCAnnotation], - DRPCUIDAnnotation: string(d.instance.UID), - rmnutil.IsCGEnabledAnnotation: d.instance.GetAnnotations()[rmnutil.IsCGEnabledAnnotation], - }, }, Spec: rmn.VolumeReplicationGroupSpec{ - PVCSelector: d.instance.Spec.PVCSelector, - ProtectedNamespaces: d.instance.Spec.ProtectedNamespaces, - ReplicationState: repState, - S3Profiles: AvailableS3Profiles(d.drClusters), - KubeObjectProtection: d.instance.Spec.KubeObjectProtection, + PVCSelector: d.instance.Spec.PVCSelector, + ReplicationState: repState, }, } - d.setVRGAction(&vrg) - vrg.Spec.Async = d.generateVRGSpecAsync() - vrg.Spec.Sync = d.generateVRGSpecSync() + d.updateVRGOptionalFields(&vrg, dstCluster) return vrg } diff --git a/internal/controller/drplacementcontrolvolsync.go b/internal/controller/drplacementcontrolvolsync.go index 56958cfa8..7bd8a4581 100644 --- a/internal/controller/drplacementcontrolvolsync.go +++ b/internal/controller/drplacementcontrolvolsync.go @@ -248,21 +248,5 @@ func (d *DRPCInstance) ResetVolSyncRDOnPrimary(clusterName string) error { vrg.Spec.VolSync.RDSpec = nil - vrgClientManifest, err := d.mwu.GenerateManifest(vrg) - if err != nil { - d.log.Error(err, "failed to generate manifest") - - return fmt.Errorf("failed to generate VRG manifest (%w)", err) - } - - mw.Spec.Workload.Manifests[0] = *vrgClientManifest - - err = d.reconciler.Update(d.ctx, mw) - if err != nil { - return fmt.Errorf("failed to update MW (%w)", err) - } - - d.log.Info(fmt.Sprintf("Updated VRG running in cluster %s to secondary. VRG (%v)", clusterName, vrg)) - - return nil + return d.mwu.UpdateVRGManifestWork(vrg, mw) } diff --git a/internal/controller/util/mw_util.go b/internal/controller/util/mw_util.go index 89f392ebb..d10d007db 100644 --- a/internal/controller/util/mw_util.go +++ b/internal/controller/util/mw_util.go @@ -704,6 +704,26 @@ func (mwu *MWUtil) DeleteManifestWork(mwName, mwNamespace string) error { return nil } +func (mwu *MWUtil) UpdateVRGManifestWork(vrg *rmn.VolumeReplicationGroup, mw *ocmworkv1.ManifestWork) error { + vrgClientManifest, err := mwu.GenerateManifest(vrg) + if err != nil { + mwu.Log.Error(err, "failed to generate manifest") + + return fmt.Errorf("failed to generate VRG manifest (%w)", err) + } + + mw.Spec.Workload.Manifests[0] = *vrgClientManifest + + err = mwu.Client.Update(mwu.Ctx, mw) + if err != nil { + return fmt.Errorf("failed to update MW (%w)", err) + } + + mwu.Log.Info(fmt.Sprintf("Added VRG %s to MW %s for cluster %s", vrg.GetName(), mw.GetName(), mw.GetNamespace())) + + return nil +} + func ExtractVRGFromManifestWork(mw *ocmworkv1.ManifestWork) (*rmn.VolumeReplicationGroup, error) { if len(mw.Spec.Workload.Manifests) == 0 { return nil, fmt.Errorf("invalid VRG ManifestWork for type: %s", mw.Name) From e561914590a786e8e90481adce9a44e85ea63198 Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Tue, 22 Oct 2024 15:52:57 -0400 Subject: [PATCH 07/15] Define a constant for VRClass param schedulingInterval Signed-off-by: Shyamsundar Ranganathan --- internal/controller/volumereplicationgroup_controller.go | 3 +++ internal/controller/vrg_volrep.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/controller/volumereplicationgroup_controller.go b/internal/controller/volumereplicationgroup_controller.go index 5c1e2c66a..fc48ce2a5 100644 --- a/internal/controller/volumereplicationgroup_controller.go +++ b/internal/controller/volumereplicationgroup_controller.go @@ -512,6 +512,9 @@ const ( // Maintenance mode label MModesLabel = "ramendr.openshift.io/maintenancemodes" + + // VolumeReplicationClass schedule parameter key + VRClassScheduleKey = "schedulingInterval" ) func (v *VRGInstance) requeue() { diff --git a/internal/controller/vrg_volrep.go b/internal/controller/vrg_volrep.go index 067abccc5..e7c823f78 100644 --- a/internal/controller/vrg_volrep.go +++ b/internal/controller/vrg_volrep.go @@ -1266,7 +1266,7 @@ func (v *VRGInstance) selectVolumeReplicationClass( for index := range v.replClassList.Items { replicationClass := &v.replClassList.Items[index] - schedulingInterval, found := replicationClass.Spec.Parameters["schedulingInterval"] + schedulingInterval, found := replicationClass.Spec.Parameters[VRClassScheduleKey] if storageClass.Provisioner != replicationClass.Spec.Provisioner || !found { // skip this replication class if provisioner does not match or if schedule not found From a01306b69423965787ce3d14a801c19a9f98a0ab Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Tue, 22 Oct 2024 15:54:09 -0400 Subject: [PATCH 08/15] Implement methods to findAllPeers to update DRPolicy peer status Changes in this commit include the implementation to generate peer information per pair of clusters, given the various classes across the clusters in a DRPolicy. The methods to fetch the required classes from the clusters or to update the DRPolicy status is yet to be implemented. Signed-off-by: Shyamsundar Ranganathan --- internal/controller/drpolicy_controller.go | 14 +- internal/controller/drpolicy_peerclass.go | 313 +++++++++++++++++++++ 2 files changed, 323 insertions(+), 4 deletions(-) create mode 100644 internal/controller/drpolicy_peerclass.go diff --git a/internal/controller/drpolicy_controller.go b/internal/controller/drpolicy_controller.go index a51fb896c..362e53dd1 100644 --- a/internal/controller/drpolicy_controller.go +++ b/internal/controller/drpolicy_controller.go @@ -61,6 +61,8 @@ const ReasonDRClustersUnavailable = "DRClustersUnavailable" // +kubebuilder:rbac:groups="",namespace=system,resources=secrets,verbs=get;update // +kubebuilder:rbac:groups="policy.open-cluster-management.io",namespace=system,resources=placementbindings,verbs=get;create;update;delete // +kubebuilder:rbac:groups="policy.open-cluster-management.io",namespace=system,resources=policies,verbs=get;create;update;delete +// +kubebuilder:rbac:groups=cluster.open-cluster-management.io,resources=managedclusters,verbs=get;list;watch +// +kubebuilder:rbac:groups=view.open-cluster-management.io,resources=managedclusterviews,verbs=get;list;watch;create;update;patch;delete // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -136,19 +138,23 @@ func (r *DRPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{}, fmt.Errorf("error in intiating policy metrics: %w", err) } - return r.reconcile(drpolicy, drclusters, secretsUtil, ramenConfig, log) + return r.reconcile(u, drclusters, secretsUtil, ramenConfig) } -func (r *DRPolicyReconciler) reconcile(drpolicy *ramen.DRPolicy, +func (r *DRPolicyReconciler) reconcile( + u *drpolicyUpdater, drclusters *ramen.DRClusterList, secretsUtil *util.SecretsUtil, ramenConfig *ramen.RamenConfig, - log logr.Logger, ) (ctrl.Result, error) { - if err := propagateS3Secret(drpolicy, drclusters, secretsUtil, ramenConfig, log); err != nil { + if err := propagateS3Secret(u.object, drclusters, secretsUtil, ramenConfig, u.log); err != nil { return ctrl.Result{}, fmt.Errorf("drpolicy deploy: %w", err) } + if err := updatePeerClasses(u); err != nil { + return ctrl.Result{}, fmt.Errorf("drpolicy peerClass update: %w", err) + } + return ctrl.Result{}, nil } diff --git a/internal/controller/drpolicy_peerclass.go b/internal/controller/drpolicy_peerclass.go new file mode 100644 index 000000000..0c0b27a07 --- /dev/null +++ b/internal/controller/drpolicy_peerclass.go @@ -0,0 +1,313 @@ +// SPDX-FileCopyrightText: The RamenDR authors +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +import ( + "context" + "fmt" + "slices" + + volrep "github.com/csi-addons/kubernetes-csi-addons/api/replication.storage/v1alpha1" + "github.com/go-logr/logr" + snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + "github.com/ramendr/ramen/internal/controller/util" + storagev1 "k8s.io/api/storage/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// classLists contains [storage|snapshot|replication]classes from ManagedClusters with the required ramen storageID or, +// replicationID labels +type classLists struct { + clusterID string + sClasses []*storagev1.StorageClass + vsClasses []*snapv1.VolumeSnapshotClass + vrClasses []*volrep.VolumeReplicationClass +} + +// peerList contains a single peer relationship between a PAIR of clusters for a common storageClassName across +// these peers. This should directly translate to DRPolicy.Status.[Async|Sync] updates. +// NOTE: storageID discussed in comments relates to the value of the label "ramendr.openshift.io/storageid" for the +// respective class. replicationID in comments relates to the value of the label "ramendr.openshift.io/replicationid" +// for the respective class +type peerList struct { + // replicationID is an empty string (indicating no common VolumeReplicationClass) or the common replicationID value + // for the corresponding VRClass on each peer + replicationID string + + // storageIDs is a list containing, + // - A single storageID if the storageClassName across the peers have the same storageID, denoting a synchronous + // pairing for the storageClassName + // - It is a pair of storageIDs, if there exists an asynchronous pairing between the clusters, either due to + // a common replicationID or due to required VolumeSnapshotClasses on each cluster + storageIDs []string + + // storageClassName is the name of a StorageClass that is common across the peers + storageClassName string + + // clusterIDs is a list of 2 IDs that denote the IDs for the clusters in this peer relationship + clusterIDs []string +} + +// isAsyncVSClassPeer inspects provided pair of classLists for a matching VolumeSnapshotClass, that is linked to the +// StorageClass whose storageID is respectively sIDA or sIDB +func isAsyncVSClassPeer(clA, clB classLists, sIDA, sIDB string) bool { + sidA := "" + sidB := "" + + // No provisioner match as we can do cross provisioner VSC based protection + for vscAidx := range clA.vsClasses { + sidA = clA.vsClasses[vscAidx].GetLabels()[StorageIDLabel] + if sidA == "" || sidA != sIDA { + // reset on mismatch, to exit the loop with an empty string if there is no match + sidA = "" + + continue + } + + break + } + + if sidA == "" { + return false + } + + for vscBidx := range clB.vsClasses { + sidB = clB.vsClasses[vscBidx].GetLabels()[StorageIDLabel] + if sidB == "" || sidB != sIDB { + continue + } + + return true + } + + return false +} + +// getVRID inspects VolumeReplicationClass in the passed in classLists at the specified index, and returns, +// - an empty string if the VRClass fails to match the passed in storageID or schedule, or +// - the value of replicationID on the VRClass +func getVRID(cl classLists, vrcIdx int, inSID string, schedule string) string { + sID := cl.vrClasses[vrcIdx].GetLabels()[StorageIDLabel] + if sID == "" || inSID != sID { + return "" + } + + if cl.vrClasses[vrcIdx].Spec.Parameters[VRClassScheduleKey] != schedule { + return "" + } + + rID := cl.vrClasses[vrcIdx].GetLabels()[VolumeReplicationIDLabel] + + return rID +} + +// getAsyncVRClassPeer inspects if there is a common replicationID among the vrClasses in the passed in classLists, +// that relate to the corresponding storageIDs and schedule, and returns the replicationID or "" if there was no match +func getAsyncVRClassPeer(clA, clB classLists, sIDA, sIDB string, schedule string) string { + for vrcAidx := range clA.vrClasses { + ridA := getVRID(clA, vrcAidx, sIDA, schedule) + if ridA == "" { + continue + } + + for vrcBidx := range clB.vrClasses { + ridB := getVRID(clB, vrcBidx, sIDB, schedule) + if ridB == "" { + continue + } + + if ridA != ridB { // TODO: Check provisioner match? + continue + } + + return ridA + } + } + + return "" +} + +// getAsyncPeers determines if scName in the first classList has asynchronous peers in the remaining classLists. +// The clusterID and sID are the corresponding IDs for the first cluster in the classList, and the schedule is +// the desired asynchronous schedule that requires to be matched +// nolint:gocognit +func getAsyncPeers(scName string, clusterID string, sID string, cls []classLists, schedule string) []peerList { + peers := []peerList{} + + for _, cl := range cls[1:] { + for scIdx := range cl.sClasses { + if cl.sClasses[scIdx].GetName() != scName { + continue + } + + sIDcl := cl.sClasses[scIdx].GetLabels()[StorageIDLabel] + if sID == sIDcl { + break + } + + rID := getAsyncVRClassPeer(cls[0], cl, sID, sIDcl, schedule) + if rID == "" { + if !isAsyncVSClassPeer(cls[0], cl, sID, sIDcl) { + continue + } + } + + peers = append(peers, peerList{ + storageClassName: scName, + storageIDs: []string{sID, sIDcl}, + clusterIDs: []string{clusterID, cl.clusterID}, + replicationID: rID, + }) + + break + } + } + + return peers +} + +// getSyncPeers determines if scName passed has asynchronous peers in the passed in classLists. +// The clusterID and sID are the corresponding IDs for the passed in scName to find a match +func getSyncPeers(scName string, clusterID string, sID string, cls []classLists) []peerList { + peers := []peerList{} + + for _, cl := range cls { + for idx := range cl.sClasses { + if cl.sClasses[idx].GetName() != scName { + continue + } + + if sID != cl.sClasses[idx].GetLabels()[StorageIDLabel] { + break + } + + // TODO: Check provisioner match? + + peers = append(peers, peerList{ + storageClassName: scName, + storageIDs: []string{sID}, + clusterIDs: []string{clusterID, cl.clusterID}, + }) + + break + } + } + + return peers +} + +// findPeers finds all sync and async peers for the scName and cluster at the index startClsIdx of classLists, +// across other remaining elements post the startClsIdx in the classLists +func findPeers(cls []classLists, scName string, startClsIdx int, schedule string) ([]peerList, []peerList) { + scIdx := 0 + for scIdx = range cls[startClsIdx].sClasses { + if cls[startClsIdx].sClasses[scIdx].Name == scName { + break + } + } + + if !util.HasLabel(cls[startClsIdx].sClasses[scIdx], StorageIDLabel) { + return nil, nil + } + + sID := cls[startClsIdx].sClasses[scIdx].Labels[StorageIDLabel] + // TODO: Check if Sync is non-nil? + syncPeers := getSyncPeers(scName, cls[startClsIdx].clusterID, sID, cls[startClsIdx+1:]) + + asyncPeers := []peerList{} + if schedule != "" { + asyncPeers = getAsyncPeers(scName, cls[startClsIdx].clusterID, sID, cls[startClsIdx:], schedule) + } + + return syncPeers, asyncPeers +} + +// unionStorageClasses returns a union of all StorageClass names found in all clusters in the passed in classLists +func unionStorageClasses(cls []classLists) []string { + allSCs := []string{} + + for clsIdx := range cls { + for scIdx := range cls[clsIdx].sClasses { + if slices.Contains(allSCs, cls[clsIdx].sClasses[scIdx].Name) { + continue + } + + allSCs = append(allSCs, cls[clsIdx].sClasses[scIdx].Name) + } + } + + return allSCs +} + +// findAllPeers finds all PAIRs of peers in the passed in classLists. It does an exhaustive search for each scName in +// the prior index of classLists (starting at index 0) with all clusters from that index forward +func findAllPeers(cls []classLists, schedule string) ([]peerList, []peerList) { + syncPeers := []peerList{} + asyncPeers := []peerList{} + + if len(cls) <= 1 { + return syncPeers, asyncPeers + } + + sClassNames := unionStorageClasses(cls) + + for clsIdx := range cls[:len(cls)-1] { + for scIdx := range cls[clsIdx].sClasses { + if !slices.Contains(sClassNames, cls[clsIdx].sClasses[scIdx].Name) { + continue + } + + sPeers, aPeers := findPeers(cls, cls[clsIdx].sClasses[scIdx].Name, clsIdx, schedule) + if len(sPeers) != 0 { + syncPeers = append(syncPeers, sPeers...) + } + + if len(aPeers) != 0 { + asyncPeers = append(asyncPeers, aPeers...) + } + } + } + + return syncPeers, asyncPeers +} + +// updatePeerClassStatus updates the DRPolicy.Status.[Async|Sync] peer lists based on passed in peerList values +func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerList) { +} + +// getClusterClasses inspects, using ManagedClusterView, the ManagedCluster claims for all storage related classes, +// and returns the set of classLists for the passed in clusters +func getClusterClasses( + ctx context.Context, + log logr.Logger, + client client.Client, + cluster string, +) (classLists, error) { + return classLists{}, nil +} + +// updatePeerClasses inspects required classes from the clusters that are part of the DRPolicy and updates DRPolicy +// status with the peer information across these clusters +func updatePeerClasses(u *drpolicyUpdater) error { + cls := []classLists{} + + if len(u.object.Spec.DRClusters) <= 1 { + return fmt.Errorf("cannot form peerClasses, insufficient clusters (%d) in policy", len(u.object.Spec.DRClusters)) + } + + for idx := range u.object.Spec.DRClusters { + clusterClasses, err := getClusterClasses(u.ctx, u.log, u.client, u.object.Spec.DRClusters[idx]) + if err != nil { + return err + } + + cls = append(cls, clusterClasses) + } + + syncPeers, asyncPeers := findAllPeers(cls, u.object.Spec.SchedulingInterval) + + updatePeerClassStatus(u, syncPeers, asyncPeers) + + return nil +} From f47cf35d48f3f90dbc4148fb58196fd54baedf7c Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Tue, 22 Oct 2024 15:56:51 -0400 Subject: [PATCH 09/15] Add unit test cases for findAllPeers implementation Signed-off-by: Shyamsundar Ranganathan --- .../drpolicy_peerclass_internal_test.go | 1105 +++++++++++++++++ 1 file changed, 1105 insertions(+) create mode 100644 internal/controller/drpolicy_peerclass_internal_test.go diff --git a/internal/controller/drpolicy_peerclass_internal_test.go b/internal/controller/drpolicy_peerclass_internal_test.go new file mode 100644 index 000000000..59c1cda71 --- /dev/null +++ b/internal/controller/drpolicy_peerclass_internal_test.go @@ -0,0 +1,1105 @@ +// SPDX-FileCopyrightText: The RamenDR authors +// SPDX-License-Identifier: Apache-2.0 + +package controllers + +import ( + volrep "github.com/csi-addons/kubernetes-csi-addons/api/replication.storage/v1alpha1" + snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + storagev1 "k8s.io/api/storage/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// nolint:dupl +var _ = Describe("updatePeerClassesInternal", func() { + DescribeTable("findAllPeers", + func( + cls []classLists, + schedule string, + syncPeers []peerList, + asyncPeers []peerList, + ) { + sPeers, aPeers := findAllPeers(cls, schedule) + Expect(sPeers).Should(HaveExactElements(syncPeers)) + Expect(aPeers).Should(HaveExactElements(asyncPeers)) + }, + Entry("Empty classLists", []classLists{}, "1m", []peerList{}, []peerList{}), + Entry("Not enough clusters", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "identical", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single sync peer", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "identical", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "identical", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{ + { + replicationID: "", + storageIDs: []string{"identical"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + }, + []peerList{}, + ), + Entry("Multiple sync peer", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "identical1", + }, + }, + Provisioner: "sample.csi.com", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc2", + Labels: map[string]string{ + StorageIDLabel: "identical2", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "identical1", + }, + }, + Provisioner: "sample.csi.com", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc2", + Labels: map[string]string{ + StorageIDLabel: "identical2", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{ + { + replicationID: "", + storageIDs: []string{"identical1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + { + replicationID: "", + storageIDs: []string{"identical2"}, + storageClassName: "sc2", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + }, + []peerList{}, + ), + Entry("Single async peer, missing related [VR|VS]Classes", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, having unrelated VRClasses", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID-mismatch", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID-mismatch", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, having unrelated VSClasses", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID-mismatch", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID-mismatch", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, having unrelated [VR|VS]Classes", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID-mismatch", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID-mismatch", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID-mismatch", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID-mismatch", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, having related VRClasses", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{ + { + replicationID: "cl-1-2-rID", + storageIDs: []string{"cl-1-sID", "cl-2-sID"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + }, + ), + Entry("Single async peer, having related VSClasses", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{ + { + replicationID: "", + storageIDs: []string{"cl-1-sID", "cl-2-sID"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + }, + ), + Entry("Single async peer, having related VSClasses on one but not the other cluster", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID-mismatch", + }, + }, + Driver: "sample.csi.com", + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, with VRClasses missing rID", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, with VRClasses mismatching schedule", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "2m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Single async peer, having related VRClasses with mismatching rIDs", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID", + VolumeReplicationIDLabel: "cl-1-2-rID-mismatch", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{}, + ), + Entry("Multiple async peer, having related [VR|VS]Classes", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID1", + }, + }, + Provisioner: "sample.csi.com", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc2", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID2", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID1", + }, + }, + Driver: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-1-sID2", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID1", + }, + }, + Provisioner: "sample.csi.com", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc2", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID2", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vsClasses: []*snapv1.VolumeSnapshotClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vsc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID1", + }, + }, + Driver: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-2-sID2", + VolumeReplicationIDLabel: "cl-1-2-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{}, + []peerList{ + { + replicationID: "", + storageIDs: []string{"cl-1-sID1", "cl-2-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + { + replicationID: "cl-1-2-rID", + storageIDs: []string{"cl-1-sID2", "cl-2-sID2"}, + storageClassName: "sc2", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + }, + ), + Entry("Multiple sync and async peer, having Classes", + []classLists{ + { + clusterID: "cl-1", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[1|2]-sID1", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[1|2]-sID1", + VolumeReplicationIDLabel: "cl-[1|2]-[3|4]-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-2", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[1|2]-sID1", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[1|2]-sID1", + VolumeReplicationIDLabel: "cl-[1|2]-[3|4]-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-3", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[3|4]-sID1", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[3|4]-sID1", + VolumeReplicationIDLabel: "cl-[1|2]-[3|4]-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + { + clusterID: "cl-4", + sClasses: []*storagev1.StorageClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[3|4]-sID1", + }, + }, + Provisioner: "sample.csi.com", + }, + }, + vrClasses: []*volrep.VolumeReplicationClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "vrc1", + Labels: map[string]string{ + StorageIDLabel: "cl-[3|4]-sID1", + VolumeReplicationIDLabel: "cl-[1|2]-[3|4]-rID", + }, + }, + Spec: volrep.VolumeReplicationClassSpec{ + Provisioner: "sample.csi.com", + Parameters: map[string]string{ + VRClassScheduleKey: "1m", + }, + }, + }, + }, + }, + }, + "1m", + []peerList{ + { + replicationID: "", + storageIDs: []string{"cl-[1|2]-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-2"}, + }, + { + replicationID: "", + storageIDs: []string{"cl-[3|4]-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-3", "cl-4"}, + }, + }, + []peerList{ + { + replicationID: "cl-[1|2]-[3|4]-rID", + storageIDs: []string{"cl-[1|2]-sID1", "cl-[3|4]-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-3"}, + }, + { + replicationID: "cl-[1|2]-[3|4]-rID", + storageIDs: []string{"cl-[1|2]-sID1", "cl-[3|4]-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-1", "cl-4"}, + }, + { + replicationID: "cl-[1|2]-[3|4]-rID", + storageIDs: []string{"cl-[1|2]-sID1", "cl-[3|4]-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-2", "cl-3"}, + }, + { + replicationID: "cl-[1|2]-[3|4]-rID", + storageIDs: []string{"cl-[1|2]-sID1", "cl-[3|4]-sID1"}, + storageClassName: "sc1", + clusterIDs: []string{"cl-2", "cl-4"}, + }, + }, + ), + ) +}) From 86bb3d8ca101e300e14fdc10173b69368bb473df Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Wed, 23 Oct 2024 17:31:30 -0400 Subject: [PATCH 10/15] Add functionality to getClusterClasses in peerClass update feature This commit adds: - Inspecting each DRCluster in the DRPolicy for related ManagedCluster claims for various storage related classes - Creating an MCV for such claims to fetch the corresponding class - Updating classLists with the information fetched per cluster - Pruning MCVs for classes that are no longer claimed - Deleting MCVs when DRCluster is deleted - Adding a watch for ManagedCluster and MCV in the DRPolicy reconciler Signed-off-by: Shyamsundar Ranganathan --- cmd/main.go | 12 +- .../controller/drclusterconfig_controller.go | 17 +- internal/controller/drclusters.go | 4 + .../drplacementcontrol_controller_test.go | 45 ++- internal/controller/drpolicy_controller.go | 44 ++- internal/controller/drpolicy_peerclass.go | 211 +++++++++++++- internal/controller/util/labels.go | 3 + internal/controller/util/managedcluster.go | 42 +++ internal/controller/util/mcv_util.go | 275 ++++++++++++------ internal/controller/util/mw_util.go | 3 + 10 files changed, 518 insertions(+), 138 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 406cb3dc4..4aaa754ed 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -206,10 +206,14 @@ func setupReconcilersCluster(mgr ctrl.Manager, ramenConfig *ramendrv1alpha1.Rame func setupReconcilersHub(mgr ctrl.Manager) { if err := (&controllers.DRPolicyReconciler{ - Client: mgr.GetClient(), - APIReader: mgr.GetAPIReader(), - Log: ctrl.Log.WithName("controllers").WithName("DRPolicy"), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Log: ctrl.Log.WithName("controllers").WithName("DRPolicy"), + Scheme: mgr.GetScheme(), + MCVGetter: rmnutil.ManagedClusterViewGetterImpl{ + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + }, ObjectStoreGetter: controllers.S3ObjectStoreGetter(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "DRPolicy") diff --git a/internal/controller/drclusterconfig_controller.go b/internal/controller/drclusterconfig_controller.go index 2cb7ea7ce..abed094c6 100644 --- a/internal/controller/drclusterconfig_controller.go +++ b/internal/controller/drclusterconfig_controller.go @@ -38,11 +38,6 @@ const ( drCConfigOwnerName = "ramen" maxReconcileBackoff = 5 * time.Minute - - // Prefixes for various ClusterClaims - ccSCPrefix = "storage.class" - ccVSCPrefix = "snapshot.class" - ccVRCPrefix = "replication.class" ) // DRClusterConfigReconciler reconciles a DRClusterConfig object @@ -237,11 +232,11 @@ func (r *DRClusterConfigReconciler) createSCClusterClaims( continue } - if err := r.ensureClusterClaim(ctx, log, ccSCPrefix, sClasses.Items[i].GetName()); err != nil { + if err := r.ensureClusterClaim(ctx, log, util.CCSCPrefix, sClasses.Items[i].GetName()); err != nil { return nil, err } - claims = append(claims, claimName(ccSCPrefix, sClasses.Items[i].GetName())) + claims = append(claims, claimName(util.CCSCPrefix, sClasses.Items[i].GetName())) } return claims, nil @@ -263,11 +258,11 @@ func (r *DRClusterConfigReconciler) createVSCClusterClaims( continue } - if err := r.ensureClusterClaim(ctx, log, ccVSCPrefix, vsClasses.Items[i].GetName()); err != nil { + if err := r.ensureClusterClaim(ctx, log, util.CCVSCPrefix, vsClasses.Items[i].GetName()); err != nil { return nil, err } - claims = append(claims, claimName(ccVSCPrefix, vsClasses.Items[i].GetName())) + claims = append(claims, claimName(util.CCVSCPrefix, vsClasses.Items[i].GetName())) } return claims, nil @@ -289,11 +284,11 @@ func (r *DRClusterConfigReconciler) createVRCClusterClaims( continue } - if err := r.ensureClusterClaim(ctx, log, ccVRCPrefix, vrClasses.Items[i].GetName()); err != nil { + if err := r.ensureClusterClaim(ctx, log, util.CCVRCPrefix, vrClasses.Items[i].GetName()); err != nil { return nil, err } - claims = append(claims, claimName(ccVRCPrefix, vrClasses.Items[i].GetName())) + claims = append(claims, claimName(util.CCVRCPrefix, vrClasses.Items[i].GetName())) } return claims, nil diff --git a/internal/controller/drclusters.go b/internal/controller/drclusters.go index fa3bbb3ad..9f9799b2c 100644 --- a/internal/controller/drclusters.go +++ b/internal/controller/drclusters.go @@ -245,6 +245,10 @@ func drClusterUndeploy( return fmt.Errorf("drcluster '%v' referenced in one or more existing drPolicy resources", drcluster.Name) } + if err := deleteViewsForClasses(mcv, log, drcluster.GetName()); err != nil { + return err + } + if err := mwu.DeleteManifestWork(mwu.BuildManifestWorkName(util.MWTypeDRCConfig), drcluster.GetName()); err != nil { return err } diff --git a/internal/controller/drplacementcontrol_controller_test.go b/internal/controller/drplacementcontrol_controller_test.go index ca44e9918..823cb9f37 100644 --- a/internal/controller/drplacementcontrol_controller_test.go +++ b/internal/controller/drplacementcontrol_controller_test.go @@ -11,7 +11,9 @@ import ( "strings" "time" + volrep "github.com/csi-addons/kubernetes-csi-addons/api/replication.storage/v1alpha1" "github.com/go-logr/logr" + snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/format" @@ -37,6 +39,7 @@ import ( plrv1 "github.com/stolostron/multicloud-operators-placementrule/pkg/apis/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + storagev1 "k8s.io/api/storage/v1" clrapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1" gppv1 "open-cluster-management.io/governance-policy-propagator/api/v1" ) @@ -222,6 +225,33 @@ func getFunctionNameAtIndex(idx int) string { return result[len(result)-1] } +func (f FakeMCVGetter) GetSClassFromManagedCluster(resourceName, managedCluster string, annotations map[string]string, +) (*storagev1.StorageClass, error) { + return nil, nil +} + +func (f FakeMCVGetter) ListSClassMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return &viewv1beta1.ManagedClusterViewList{}, nil +} + +func (f FakeMCVGetter) GetVSClassFromManagedCluster(resourceName, managedCluster string, annotations map[string]string, +) (*snapv1.VolumeSnapshotClass, error) { + return nil, nil +} + +func (f FakeMCVGetter) ListVSClassMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return &viewv1beta1.ManagedClusterViewList{}, nil +} + +func (f FakeMCVGetter) GetVRClassFromManagedCluster(resourceName, managedCluster string, annotations map[string]string, +) (*volrep.VolumeReplicationClass, error) { + return nil, nil +} + +func (f FakeMCVGetter) ListVRClassMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return &viewv1beta1.ManagedClusterViewList{}, nil +} + // GetMModeFromManagedCluster: MMode code uses GetMModeFromManagedCluster to create a MCV and not fetch it, that // is done using ListMCV. As a result this fake function creates an MCV for record keeping purposes and returns // a nil mcv back in case of success @@ -343,21 +373,6 @@ func (f FakeMCVGetter) DeleteManagedClusterView(clusterName, mcvName string, log return f.Delete(context.TODO(), mcv) } -func (f FakeMCVGetter) GetNamespaceFromManagedCluster( - resourceName, managedCluster, namespaceString string, annotations map[string]string, -) (*corev1.Namespace, error) { - appNamespaceObj := &corev1.Namespace{} - - // err := k8sClient.Get(context.TODO(), appNamespaceLookupKey, appNamespaceObj) - foundMW := &ocmworkv1.ManifestWork{} - mwName := fmt.Sprintf(rmnutil.ManifestWorkNameFormat, resourceName, namespaceString, rmnutil.MWTypeNS) - err := k8sClient.Get(context.TODO(), - types.NamespacedName{Name: mwName, Namespace: managedCluster}, - foundMW) - - return appNamespaceObj, err -} - func getDefaultVRG(namespace string) *rmn.VolumeReplicationGroup { return &rmn.VolumeReplicationGroup{ TypeMeta: metav1.TypeMeta{Kind: "VolumeReplicationGroup", APIVersion: "ramendr.openshift.io/v1alpha1"}, diff --git a/internal/controller/drpolicy_controller.go b/internal/controller/drpolicy_controller.go index 362e53dd1..6e2a39036 100644 --- a/internal/controller/drpolicy_controller.go +++ b/internal/controller/drpolicy_controller.go @@ -10,12 +10,14 @@ import ( "github.com/go-logr/logr" "github.com/google/uuid" + viewv1beta1 "github.com/stolostron/multicloud-operators-foundation/pkg/apis/view/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/workqueue" + ocmv1 "open-cluster-management.io/api/cluster/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -35,6 +37,7 @@ type DRPolicyReconciler struct { APIReader client.Reader Log logr.Logger Scheme *runtime.Scheme + MCVGetter util.ManagedClusterViewGetter ObjectStoreGetter ObjectStoreGetter RateLimiter *workqueue.TypedRateLimiter[reconcile.Request] } @@ -48,6 +51,9 @@ const ReasonDRClusterNotFound = "DRClusterNotFound" // ReasonDRClustersUnavailable is set when the DRPolicy has none of the referenced DRCluster(s) are in a validated state const ReasonDRClustersUnavailable = "DRClustersUnavailable" +// AllDRPolicyAnnotation is added to related resources that can be watched to reconcile all related DRPolicy resources +const AllDRPolicyAnnotation = "drpolicy.ramendr.openshift.io" + //nolint:lll //+kubebuilder:rbac:groups=ramendr.openshift.io,resources=drpolicies,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=ramendr.openshift.io,resources=drpolicies/status,verbs=get;update;patch @@ -151,7 +157,7 @@ func (r *DRPolicyReconciler) reconcile( return ctrl.Result{}, fmt.Errorf("drpolicy deploy: %w", err) } - if err := updatePeerClasses(u); err != nil { + if err := updatePeerClasses(u, r.MCVGetter); err != nil { return ctrl.Result{}, fmt.Errorf("drpolicy peerClass update: %w", err) } @@ -435,9 +441,18 @@ func (r *DRPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { ). Watches( &ramen.DRCluster{}, - handler.EnqueueRequestsFromMapFunc(r.drClusterMapFunc), + handler.EnqueueRequestsFromMapFunc(r.objectNameAsClusterMapFunc), builder.WithPredicates(util.CreateOrDeleteOrResourceVersionUpdatePredicate{}), ). + Watches( + &ocmv1.ManagedCluster{}, + handler.EnqueueRequestsFromMapFunc(r.objectNameAsClusterMapFunc), + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), + ). + Watches( + &viewv1beta1.ManagedClusterView{}, + handler.EnqueueRequestsFromMapFunc(r.mcvMapFun), + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})). Complete(r) } @@ -488,7 +503,28 @@ func (r *DRPolicyReconciler) secretMapFunc(ctx context.Context, secret client.Ob return requests } -func (r *DRPolicyReconciler) drClusterMapFunc(ctx context.Context, drcluster client.Object) []reconcile.Request { +// objectNameAsClusterMapFunc returns a list of DRPolicies that contain the object.Name. A DRCluster or a +// ManagedCluster object can be passed in as the cluster to find the list of policies to reconcile +func (r *DRPolicyReconciler) objectNameAsClusterMapFunc( + ctx context.Context, cluster client.Object, +) []reconcile.Request { + return r.getDRPoliciesForCluster(cluster.GetName()) +} + +func (r *DRPolicyReconciler) mcvMapFun(ctx context.Context, obj client.Object) []reconcile.Request { + mcv, ok := obj.(*viewv1beta1.ManagedClusterView) + if !ok { + return []reconcile.Request{} + } + + if _, ok := mcv.Annotations[AllDRPolicyAnnotation]; !ok { + return []ctrl.Request{} + } + + return r.getDRPoliciesForCluster(obj.GetNamespace()) +} + +func (r *DRPolicyReconciler) getDRPoliciesForCluster(clusterName string) []reconcile.Request { drpolicies := &ramen.DRPolicyList{} if err := r.Client.List(context.TODO(), drpolicies); err != nil { return []reconcile.Request{} @@ -498,7 +534,7 @@ func (r *DRPolicyReconciler) drClusterMapFunc(ctx context.Context, drcluster cli for idx := range drpolicies.Items { drpolicy := &drpolicies.Items[idx] - if util.DrpolicyContainsDrcluster(drpolicy, drcluster.GetName()) { + if util.DrpolicyContainsDrcluster(drpolicy, clusterName) { add := reconcile.Request{ NamespacedName: types.NamespacedName{ Name: drpolicy.GetName(), diff --git a/internal/controller/drpolicy_peerclass.go b/internal/controller/drpolicy_peerclass.go index 0c0b27a07..a6ee233e9 100644 --- a/internal/controller/drpolicy_peerclass.go +++ b/internal/controller/drpolicy_peerclass.go @@ -4,7 +4,6 @@ package controllers import ( - "context" "fmt" "slices" @@ -12,8 +11,8 @@ import ( "github.com/go-logr/logr" snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" "github.com/ramendr/ramen/internal/controller/util" + "github.com/stolostron/multicloud-operators-foundation/pkg/apis/view/v1beta1" storagev1 "k8s.io/api/storage/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) // classLists contains [storage|snapshot|replication]classes from ManagedClusters with the required ramen storageID or, @@ -276,20 +275,212 @@ func findAllPeers(cls []classLists, schedule string) ([]peerList, []peerList) { func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerList) { } +// pruneClassViews prunes existing views in mcvs, for classes that are not found in survivorClassNames +func pruneClassViews( + m util.ManagedClusterViewGetter, + log logr.Logger, + clusterName string, + survivorClassNames []string, + mcvs *v1beta1.ManagedClusterViewList, +) error { + for mcvIdx := range mcvs.Items { + if slices.Contains(survivorClassNames, mcvs.Items[mcvIdx].Spec.Scope.Name) { + continue + } + + if err := m.DeleteManagedClusterView(clusterName, mcvs.Items[mcvIdx].Name, log); err != nil { + return err + } + } + + return nil +} + +func pruneVRClassViews( + m util.ManagedClusterViewGetter, + log logr.Logger, + clusterName string, + survivorClassNames []string, +) error { + mcvList, err := m.ListVRClassMCVs(clusterName) + if err != nil { + return err + } + + return pruneClassViews(m, log, clusterName, survivorClassNames, mcvList) +} + +// getVRClassesFromCluster gets VolumeReplicationClasses that are claimed in the ManagedClusterInstance +func getVRClassesFromCluster( + u *drpolicyUpdater, + m util.ManagedClusterViewGetter, + mc *util.ManagedClusterInstance, + clusterName string, +) ([]*volrep.VolumeReplicationClass, error) { + vrClasses := []*volrep.VolumeReplicationClass{} + + vrClassNames := mc.VolumeReplicationClassClaims() + if len(vrClassNames) == 0 { + return vrClasses, nil + } + + annotations := make(map[string]string) + annotations[AllDRPolicyAnnotation] = clusterName + + for _, vrcName := range vrClassNames { + sClass, err := m.GetVRClassFromManagedCluster(vrcName, clusterName, annotations) + if err != nil { + return []*volrep.VolumeReplicationClass{}, err + } + + vrClasses = append(vrClasses, sClass) + } + + return vrClasses, pruneVRClassViews(m, u.log, clusterName, vrClassNames) +} + +func pruneVSClassViews( + m util.ManagedClusterViewGetter, + log logr.Logger, + clusterName string, + survivorClassNames []string, +) error { + mcvList, err := m.ListVSClassMCVs(clusterName) + if err != nil { + return err + } + + return pruneClassViews(m, log, clusterName, survivorClassNames, mcvList) +} + +// getVSClassesFromCluster gets VolumeSnapshotClasses that are claimed in the ManagedClusterInstance +func getVSClassesFromCluster( + u *drpolicyUpdater, + m util.ManagedClusterViewGetter, + mc *util.ManagedClusterInstance, + clusterName string, +) ([]*snapv1.VolumeSnapshotClass, error) { + vsClasses := []*snapv1.VolumeSnapshotClass{} + + vsClassNames := mc.VolumeSnapshotClassClaims() + if len(vsClassNames) == 0 { + return vsClasses, nil + } + + annotations := make(map[string]string) + annotations[AllDRPolicyAnnotation] = clusterName + + for _, vscName := range vsClassNames { + sClass, err := m.GetVSClassFromManagedCluster(vscName, clusterName, annotations) + if err != nil { + return []*snapv1.VolumeSnapshotClass{}, err + } + + vsClasses = append(vsClasses, sClass) + } + + return vsClasses, pruneVSClassViews(m, u.log, clusterName, vsClassNames) +} + +func pruneSClassViews( + m util.ManagedClusterViewGetter, + log logr.Logger, + clusterName string, + survivorClassNames []string, +) error { + mcvList, err := m.ListSClassMCVs(clusterName) + if err != nil { + return err + } + + return pruneClassViews(m, log, clusterName, survivorClassNames, mcvList) +} + +// getSClassesFromCluster gets StorageClasses that are claimed in the ManagedClusterInstance +func getSClassesFromCluster( + u *drpolicyUpdater, + m util.ManagedClusterViewGetter, + mc *util.ManagedClusterInstance, + clusterName string, +) ([]*storagev1.StorageClass, error) { + sClasses := []*storagev1.StorageClass{} + + sClassNames := mc.StorageClassClaims() + if len(sClassNames) == 0 { + return sClasses, nil + } + + annotations := make(map[string]string) + annotations[AllDRPolicyAnnotation] = clusterName + + for _, scName := range sClassNames { + sClass, err := m.GetSClassFromManagedCluster(scName, clusterName, annotations) + if err != nil { + return []*storagev1.StorageClass{}, err + } + + sClasses = append(sClasses, sClass) + } + + return sClasses, pruneSClassViews(m, u.log, clusterName, sClassNames) +} + // getClusterClasses inspects, using ManagedClusterView, the ManagedCluster claims for all storage related classes, // and returns the set of classLists for the passed in clusters func getClusterClasses( - ctx context.Context, - log logr.Logger, - client client.Client, + u *drpolicyUpdater, + m util.ManagedClusterViewGetter, cluster string, ) (classLists, error) { - return classLists{}, nil + mc, err := util.NewManagedClusterInstance(u.ctx, u.client, cluster) + if err != nil { + return classLists{}, err + } + + clID, err := mc.ClusterID() + if err != nil { + return classLists{}, err + } + + sClasses, err := getSClassesFromCluster(u, m, mc, cluster) + if err != nil || len(sClasses) == 0 { + return classLists{}, err + } + + vsClasses, err := getVSClassesFromCluster(u, m, mc, cluster) + if err != nil { + return classLists{}, err + } + + vrClasses, err := getVRClassesFromCluster(u, m, mc, cluster) + if err != nil { + return classLists{}, err + } + + return classLists{ + clusterID: clID, + sClasses: sClasses, + vrClasses: vrClasses, + vsClasses: vsClasses, + }, nil +} + +// deleteViewsForClasses deletes all views created for classes for the passed in clusterName +func deleteViewsForClasses(m util.ManagedClusterViewGetter, log logr.Logger, clusterName string) error { + if err := pruneSClassViews(m, log, clusterName, []string{}); err != nil { + return err + } + + if err := pruneVSClassViews(m, log, clusterName, []string{}); err != nil { + return err + } + + return pruneVRClassViews(m, log, clusterName, []string{}) } // updatePeerClasses inspects required classes from the clusters that are part of the DRPolicy and updates DRPolicy // status with the peer information across these clusters -func updatePeerClasses(u *drpolicyUpdater) error { +func updatePeerClasses(u *drpolicyUpdater, m util.ManagedClusterViewGetter) error { cls := []classLists{} if len(u.object.Spec.DRClusters) <= 1 { @@ -297,11 +488,15 @@ func updatePeerClasses(u *drpolicyUpdater) error { } for idx := range u.object.Spec.DRClusters { - clusterClasses, err := getClusterClasses(u.ctx, u.log, u.client, u.object.Spec.DRClusters[idx]) + clusterClasses, err := getClusterClasses(u, m, u.object.Spec.DRClusters[idx]) if err != nil { return err } + if len(clusterClasses.sClasses) == 0 { + continue + } + cls = append(cls, clusterClasses) } diff --git a/internal/controller/util/labels.go b/internal/controller/util/labels.go index 3409b45c7..bdc3a318c 100644 --- a/internal/controller/util/labels.go +++ b/internal/controller/util/labels.go @@ -13,6 +13,9 @@ const ( labelOwnerName = "ramendr.openshift.io/owner-name" MModesLabel = "ramendr.openshift.io/maintenancemodes" + SClassLabel = "ramendr.openshift.io/storageclass" + VSClassLabel = "ramendr.openshift.io/volumesnapshotclass" + VRClassLabel = "ramendr.openshift.io/volumereplicationclass" ExcludeFromVeleroBackup = "velero.io/exclude-from-backup" ) diff --git a/internal/controller/util/managedcluster.go b/internal/controller/util/managedcluster.go index 0a13c214c..605c2f27c 100644 --- a/internal/controller/util/managedcluster.go +++ b/internal/controller/util/managedcluster.go @@ -6,6 +6,7 @@ package util import ( "context" "fmt" + "strings" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -13,10 +14,18 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + // Prefixes for various ClusterClaims + CCSCPrefix = "storage.class" + CCVSCPrefix = "snapshot.class" + CCVRCPrefix = "replication.class" +) + type ManagedClusterInstance struct { object *ocmv1.ManagedCluster } +// NewManagedClusterInstance creates an ManagedClusterInstance instance, reading the ManagedCluster resource for cluster func NewManagedClusterInstance( ctx context.Context, client client.Client, @@ -54,6 +63,7 @@ func NewManagedClusterInstance( }, nil } +// ClusterID returns the clusterID claimed by the ManagedCluster, or error if it is empty or not found func (mci *ManagedClusterInstance) ClusterID() (string, error) { id := "" @@ -73,3 +83,35 @@ func (mci *ManagedClusterInstance) ClusterID() (string, error) { return id, nil } + +// classClaims returns a list of class claims with the passed in prefix from the ManagedCluster +func (mci *ManagedClusterInstance) classClaims(prefix string) []string { + classNames := []string{} + + for idx := range mci.object.Status.ClusterClaims { + if !strings.HasPrefix(mci.object.Status.ClusterClaims[idx].Name, prefix+".") { + continue + } + + className := strings.TrimPrefix(mci.object.Status.ClusterClaims[idx].Name, prefix+".") + if className == "" { + continue + } + + classNames = append(classNames, className) + } + + return classNames +} + +func (mci *ManagedClusterInstance) StorageClassClaims() []string { + return mci.classClaims(CCSCPrefix) +} + +func (mci *ManagedClusterInstance) VolumeSnapshotClassClaims() []string { + return mci.classClaims(CCVSCPrefix) +} + +func (mci *ManagedClusterInstance) VolumeReplicationClassClaims() []string { + return mci.classClaims(CCVRCPrefix) +} diff --git a/internal/controller/util/mcv_util.go b/internal/controller/util/mcv_util.go index 71d0a5cf5..bdb0fd365 100644 --- a/internal/controller/util/mcv_util.go +++ b/internal/controller/util/mcv_util.go @@ -9,9 +9,11 @@ import ( "fmt" "strings" + volrep "github.com/csi-addons/kubernetes-csi-addons/api/replication.storage/v1alpha1" "github.com/go-logr/logr" + snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" errorswrapper "github.com/pkg/errors" - corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -24,7 +26,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" ) -// begin MCV code +// nolint: interfacebloat type ManagedClusterViewGetter interface { GetVRGFromManagedCluster( resourceName, resourceNamespace, managedCluster string, @@ -40,13 +42,28 @@ type ManagedClusterViewGetter interface { ListMModesMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) + GetSClassFromManagedCluster( + resourceName, managedCluster string, + annotations map[string]string) (*storagev1.StorageClass, error) + + ListSClassMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) + + GetVSClassFromManagedCluster( + resourceName, managedCluster string, + annotations map[string]string) (*snapv1.VolumeSnapshotClass, error) + + ListVSClassMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) + + GetVRClassFromManagedCluster( + resourceName, managedCluster string, + annotations map[string]string) (*volrep.VolumeReplicationClass, error) + + ListVRClassMCVs(managedCluster string) (*viewv1beta1.ManagedClusterViewList, error) + GetResource(mcv *viewv1beta1.ManagedClusterView, resource interface{}) error DeleteManagedClusterView(clusterName, mcvName string, logger logr.Logger) error - GetNamespaceFromManagedCluster(resourceName, resourceNamespace, managedCluster string, - annotations map[string]string) (*corev1.Namespace, error) - DeleteVRGManagedClusterView(resourceName, resourceNamespace, clusterName, resourceType string) error DeleteNamespaceManagedClusterView(resourceName, resourceNamespace, clusterName, resourceType string) error @@ -59,28 +76,62 @@ type ManagedClusterViewGetterImpl struct { APIReader client.Reader } -func (m ManagedClusterViewGetterImpl) GetVRGFromManagedCluster(resourceName, resourceNamespace, managedCluster string, +// getResourceFromManagedCluster gets the resource named resourceName in the resourceNamespace (empty if cluster scoped) +// with the passed in group, version, and kind on the managedCluster. The created ManagedClusterView(MCV) has the +// passed in annotations and mcvLabel added to it. The MCV is named mcvname, and fetched into the passed in "resource" +// interface +func (m ManagedClusterViewGetterImpl) getResourceFromManagedCluster( + resourceName, resourceNamespace, managedCluster string, annotations map[string]string, -) (*rmn.VolumeReplicationGroup, error) { - logger := ctrl.Log.WithName("MCV").WithValues("resourceName", resourceName, "cluster", managedCluster) - // get VRG and verify status through ManagedClusterView + mcvName, mcvLabel, kind, group, version string, + resource interface{}, +) error { + logger := ctrl.Log.WithName("MCV").WithValues("resouceName", resourceName, "cluster", managedCluster) + mcvMeta := metav1.ObjectMeta{ - Name: BuildManagedClusterViewName(resourceName, resourceNamespace, "vrg"), - Namespace: managedCluster, - Annotations: annotations, + Name: mcvName, + Namespace: managedCluster, + } + + if mcvLabel != "" { + mcvMeta.Labels = map[string]string{mcvLabel: ""} + } + + if annotations != nil { + mcvMeta.Annotations = annotations } mcvViewscope := viewv1beta1.ViewScope{ - Kind: "VolumeReplicationGroup", - Group: rmn.GroupVersion.Group, - Version: rmn.GroupVersion.Version, - Name: resourceName, - Namespace: resourceNamespace, + Kind: kind, + Group: group, + Version: version, + Name: resourceName, + } + + if resourceNamespace != "" { + mcvViewscope.Namespace = resourceNamespace } + return m.getManagedClusterResource(mcvMeta, mcvViewscope, resource, logger) +} + +func (m ManagedClusterViewGetterImpl) GetVRGFromManagedCluster(resourceName, resourceNamespace, managedCluster string, + annotations map[string]string, +) (*rmn.VolumeReplicationGroup, error) { vrg := &rmn.VolumeReplicationGroup{} - err := m.getManagedClusterResource(mcvMeta, mcvViewscope, vrg, logger) + err := m.getResourceFromManagedCluster( + resourceName, + resourceNamespace, + managedCluster, + annotations, + BuildManagedClusterViewName(resourceName, resourceNamespace, "vrg"), + "", + "VolumeReplicationGroup", + rmn.GroupVersion.Group, + rmn.GroupVersion.Version, + vrg, + ) return vrg, err } @@ -88,28 +139,20 @@ func (m ManagedClusterViewGetterImpl) GetVRGFromManagedCluster(resourceName, res func (m ManagedClusterViewGetterImpl) GetNFFromManagedCluster(resourceName, resourceNamespace, managedCluster string, annotations map[string]string, ) (*csiaddonsv1alpha1.NetworkFence, error) { - logger := ctrl.Log.WithName("MCV").WithValues("resouceName", resourceName) - // get NetworkFence and verify status through ManagedClusterView - mcvMeta := metav1.ObjectMeta{ - Name: BuildManagedClusterViewName(resourceName, resourceNamespace, "nf"), - Namespace: managedCluster, - } - - if annotations != nil { - mcvMeta.Annotations = annotations - } - - mcvViewscope := viewv1beta1.ViewScope{ - Kind: "NetworkFence", - Group: csiaddonsv1alpha1.GroupVersion.Group, - Version: csiaddonsv1alpha1.GroupVersion.Version, - Name: "network-fence-" + resourceName, - Namespace: resourceNamespace, - } - nf := &csiaddonsv1alpha1.NetworkFence{} - err := m.getManagedClusterResource(mcvMeta, mcvViewscope, nf, logger) + err := m.getResourceFromManagedCluster( + resourceName, + resourceNamespace, + managedCluster, + annotations, + BuildManagedClusterViewName(resourceName, resourceNamespace, "nf"), + "", + "NetworkFence", + csiaddonsv1alpha1.GroupVersion.Group, + csiaddonsv1alpha1.GroupVersion.Version, + nf, + ) return nf, err } @@ -117,49 +160,118 @@ func (m ManagedClusterViewGetterImpl) GetNFFromManagedCluster(resourceName, reso func (m ManagedClusterViewGetterImpl) GetMModeFromManagedCluster(resourceName, managedCluster string, annotations map[string]string, ) (*rmn.MaintenanceMode, error) { - logger := ctrl.Log.WithName("MCV").WithValues("resouceName", resourceName) - // get MaintenanceMode and verify status through ManagedClusterView - mcvMeta := metav1.ObjectMeta{ - Name: BuildManagedClusterViewName(resourceName, "", MWTypeMMode), - Namespace: managedCluster, - Labels: map[string]string{ - MModesLabel: "", - }, - } - - if annotations != nil { - mcvMeta.Annotations = annotations - } - - mcvViewscope := viewv1beta1.ViewScope{ - Kind: "MaintenanceMode", - Group: rmn.GroupVersion.Group, - Version: rmn.GroupVersion.Version, - Name: resourceName, - } - mMode := &rmn.MaintenanceMode{} - err := m.getManagedClusterResource(mcvMeta, mcvViewscope, mMode, logger) + err := m.getResourceFromManagedCluster( + resourceName, + "", + managedCluster, + annotations, + BuildManagedClusterViewName(resourceName, "", MWTypeMMode), + MModesLabel, + "MaintenanceMode", + rmn.GroupVersion.Group, + rmn.GroupVersion.Version, + mMode, + ) return mMode, err } -func (m ManagedClusterViewGetterImpl) ListMModesMCVs(cluster string) (*viewv1beta1.ManagedClusterViewList, error) { - matchLabels := map[string]string{ - MModesLabel: "", - } +func (m ManagedClusterViewGetterImpl) listMCVsWithLabel(cluster string, matchLabels map[string]string) ( + *viewv1beta1.ManagedClusterViewList, + error, +) { listOptions := []client.ListOption{ client.InNamespace(cluster), client.MatchingLabels(matchLabels), } - mModeMCVs := &viewv1beta1.ManagedClusterViewList{} - if err := m.APIReader.List(context.TODO(), mModeMCVs, listOptions...); err != nil { + mcvs := &viewv1beta1.ManagedClusterViewList{} + if err := m.APIReader.List(context.TODO(), mcvs, listOptions...); err != nil { return nil, err } - return mModeMCVs, nil + return mcvs, nil +} + +func (m ManagedClusterViewGetterImpl) ListMModesMCVs(cluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return m.listMCVsWithLabel(cluster, map[string]string{MModesLabel: ""}) +} + +func (m ManagedClusterViewGetterImpl) GetSClassFromManagedCluster(resourceName, managedCluster string, + annotations map[string]string, +) (*storagev1.StorageClass, error) { + sc := &storagev1.StorageClass{} + + err := m.getResourceFromManagedCluster( + resourceName, + "", + managedCluster, + annotations, + BuildManagedClusterViewName(resourceName, "", MWTypeSClass), + SClassLabel, + "StorageClass", + storagev1.SchemeGroupVersion.Group, + storagev1.SchemeGroupVersion.Version, + sc, + ) + + return sc, err +} + +func (m ManagedClusterViewGetterImpl) ListSClassMCVs(cluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return m.listMCVsWithLabel(cluster, map[string]string{SClassLabel: ""}) +} + +func (m ManagedClusterViewGetterImpl) GetVSClassFromManagedCluster(resourceName, managedCluster string, + annotations map[string]string, +) (*snapv1.VolumeSnapshotClass, error) { + vsc := &snapv1.VolumeSnapshotClass{} + + err := m.getResourceFromManagedCluster( + resourceName, + "", + managedCluster, + annotations, + BuildManagedClusterViewName(resourceName, "", MWTypeVSClass), + VSClassLabel, + "VolumeSnapshotClass", + snapv1.SchemeGroupVersion.Group, + snapv1.SchemeGroupVersion.Version, + vsc, + ) + + return vsc, err +} + +func (m ManagedClusterViewGetterImpl) ListVSClassMCVs(cluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return m.listMCVsWithLabel(cluster, map[string]string{VSClassLabel: ""}) +} + +func (m ManagedClusterViewGetterImpl) GetVRClassFromManagedCluster(resourceName, managedCluster string, + annotations map[string]string, +) (*volrep.VolumeReplicationClass, error) { + vrc := &volrep.VolumeReplicationClass{} + + err := m.getResourceFromManagedCluster( + resourceName, + "", + managedCluster, + annotations, + BuildManagedClusterViewName(resourceName, "", MWTypeVRClass), + VRClassLabel, + "VolumeReplicationClass", + volrep.GroupVersion.Group, + volrep.GroupVersion.Version, + vrc, + ) + + return vrc, err +} + +func (m ManagedClusterViewGetterImpl) ListVRClassMCVs(cluster string) (*viewv1beta1.ManagedClusterViewList, error) { + return m.listMCVsWithLabel(cluster, map[string]string{VRClassLabel: ""}) } // outputs a string for use in creating a ManagedClusterView name @@ -181,35 +293,6 @@ func ClusterScopedResourceNameFromMCVName(mcvName string) string { return strings.Join(splitName[:len(splitName)-2], "-") } -func (m ManagedClusterViewGetterImpl) GetNamespaceFromManagedCluster( - resourceName, managedCluster, namespaceString string, annotations map[string]string, -) (*corev1.Namespace, error) { - logger := ctrl.Log.WithName("MCV").WithValues("resouceName", resourceName) - - // get Namespace and verify status through ManagedClusterView - mcvMeta := metav1.ObjectMeta{ - Name: BuildManagedClusterViewName(resourceName, namespaceString, MWTypeNS), - Namespace: managedCluster, - } - - if annotations != nil { - mcvMeta.Annotations = annotations - } - - mcvViewscope := viewv1beta1.ViewScope{ - Kind: "Namespace", - Group: corev1.SchemeGroupVersion.Group, - Version: corev1.SchemeGroupVersion.Version, - Name: namespaceString, - } - - namespace := &corev1.Namespace{} - - err := m.getManagedClusterResource(mcvMeta, mcvViewscope, namespace, logger) - - return namespace, err -} - /* Description: queries a managed cluster for a resource type, and populates a variable with the results. Requires: diff --git a/internal/controller/util/mw_util.go b/internal/controller/util/mw_util.go index d10d007db..6dd0cb6d3 100644 --- a/internal/controller/util/mw_util.go +++ b/internal/controller/util/mw_util.go @@ -46,6 +46,9 @@ const ( MWTypeNS string = "ns" MWTypeNF string = "nf" MWTypeMMode string = "mmode" + MWTypeSClass string = "sc" + MWTypeVSClass string = "vsc" + MWTypeVRClass string = "vrc" MWTypeDRCConfig string = "drcconfig" ) From 573935163e5e4949c28234d88c1c80894a191cf1 Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Fri, 25 Oct 2024 10:29:38 -0400 Subject: [PATCH 11/15] Add clusterIDs to peerClass values Each entry in peerClasses denotes a relationship for a StorageClass that has the same name across EXACTLY 2 clusters. This allows to create multiple peer pairs to represent a cross sync or async relationship. It also enables to represent more than 2 clusters in a DRPolicy (in the future). It is the value of the metadata.uid of the kube-system namespace, and hence serves to isolate cluster identity to this ID rather than its name on the hub cluster. Signed-off-by: Shyamsundar Ranganathan --- api/v1alpha1/drpolicy_types.go | 4 ++++ api/v1alpha1/zz_generated.deepcopy.go | 5 +++++ .../crd/bases/ramendr.openshift.io_drpolicies.yaml | 14 ++++++++++++++ ...ft.io_protectedvolumereplicationgrouplists.yaml | 14 ++++++++++++++ ...mendr.openshift.io_volumereplicationgroups.yaml | 14 ++++++++++++++ 5 files changed, 51 insertions(+) diff --git a/api/v1alpha1/drpolicy_types.go b/api/v1alpha1/drpolicy_types.go index 5eee53189..6ecac12ef 100644 --- a/api/v1alpha1/drpolicy_types.go +++ b/api/v1alpha1/drpolicy_types.go @@ -97,6 +97,10 @@ type PeerClass struct { // StorageClassName is the name of a StorageClass that is available across the peers //+optional StorageClassName string `json:"storageClassName,omitempty"` + + // ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + // The IDs are based on the value of the metadata.uid of the kube-system namespace + ClusterIDs []string `json:"clusterIDs,omitempty"` } const ( diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c39af6791..687a5ccb8 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -754,6 +754,11 @@ func (in *PeerClass) DeepCopyInto(out *PeerClass) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.ClusterIDs != nil { + in, out := &in.ClusterIDs, &out.ClusterIDs + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PeerClass. diff --git a/config/crd/bases/ramendr.openshift.io_drpolicies.yaml b/config/crd/bases/ramendr.openshift.io_drpolicies.yaml index e6891873a..100109f21 100644 --- a/config/crd/bases/ramendr.openshift.io_drpolicies.yaml +++ b/config/crd/bases/ramendr.openshift.io_drpolicies.yaml @@ -233,6 +233,13 @@ spec: that have related async relationships. (one per pair of peers in the policy) items: properties: + clusterIDs: + description: |- + ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + The IDs are based on the value of the metadata.uid of the kube-system namespace + items: + type: string + type: array replicationID: description: |- ReplicationID is the common value for the label "ramendr.openshift.io/replicationID" on the corresponding @@ -333,6 +340,13 @@ spec: that have related sync relationships. (one per pair of peers in the policy) items: properties: + clusterIDs: + description: |- + ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + The IDs are based on the value of the metadata.uid of the kube-system namespace + items: + type: string + type: array replicationID: description: |- ReplicationID is the common value for the label "ramendr.openshift.io/replicationID" on the corresponding diff --git a/config/crd/bases/ramendr.openshift.io_protectedvolumereplicationgrouplists.yaml b/config/crd/bases/ramendr.openshift.io_protectedvolumereplicationgrouplists.yaml index 806def40b..e092eabad 100644 --- a/config/crd/bases/ramendr.openshift.io_protectedvolumereplicationgrouplists.yaml +++ b/config/crd/bases/ramendr.openshift.io_protectedvolumereplicationgrouplists.yaml @@ -127,6 +127,13 @@ spec: creates a PVC using a newer StorageClass that is determined to be common across the peers. items: properties: + clusterIDs: + description: |- + ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + The IDs are based on the value of the metadata.uid of the kube-system namespace + items: + type: string + type: array replicationID: description: |- ReplicationID is the common value for the label "ramendr.openshift.io/replicationID" on the corresponding @@ -457,6 +464,13 @@ spec: creates a PVC using a newer StorageClass that is determined to be common across the peers. items: properties: + clusterIDs: + description: |- + ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + The IDs are based on the value of the metadata.uid of the kube-system namespace + items: + type: string + type: array replicationID: description: |- ReplicationID is the common value for the label "ramendr.openshift.io/replicationID" on the corresponding diff --git a/config/crd/bases/ramendr.openshift.io_volumereplicationgroups.yaml b/config/crd/bases/ramendr.openshift.io_volumereplicationgroups.yaml index c56788a88..897d2e33f 100644 --- a/config/crd/bases/ramendr.openshift.io_volumereplicationgroups.yaml +++ b/config/crd/bases/ramendr.openshift.io_volumereplicationgroups.yaml @@ -77,6 +77,13 @@ spec: creates a PVC using a newer StorageClass that is determined to be common across the peers. items: properties: + clusterIDs: + description: |- + ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + The IDs are based on the value of the metadata.uid of the kube-system namespace + items: + type: string + type: array replicationID: description: |- ReplicationID is the common value for the label "ramendr.openshift.io/replicationID" on the corresponding @@ -406,6 +413,13 @@ spec: creates a PVC using a newer StorageClass that is determined to be common across the peers. items: properties: + clusterIDs: + description: |- + ClusterIDs is a list of two clusterIDs that represent this peer relationship for a common StorageClassName + The IDs are based on the value of the metadata.uid of the kube-system namespace + items: + type: string + type: array replicationID: description: |- ReplicationID is the common value for the label "ramendr.openshift.io/replicationID" on the corresponding From 7ed13ca7a88fdeee951d50af2396aa65e48803db Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Sat, 26 Oct 2024 12:11:03 -0400 Subject: [PATCH 12/15] Update PeerClass in DRPolicy status for Sync and Async peers Updation also prunes no longer existing peers from the policy status, and adds new peers to the policy as required. Signed-off-by: Shyamsundar Ranganathan --- internal/controller/drpolicy_peerclass.go | 98 +++++++++++++++++++++-- 1 file changed, 90 insertions(+), 8 deletions(-) diff --git a/internal/controller/drpolicy_peerclass.go b/internal/controller/drpolicy_peerclass.go index a6ee233e9..93fe57d1e 100644 --- a/internal/controller/drpolicy_peerclass.go +++ b/internal/controller/drpolicy_peerclass.go @@ -10,9 +10,11 @@ import ( volrep "github.com/csi-addons/kubernetes-csi-addons/api/replication.storage/v1alpha1" "github.com/go-logr/logr" snapv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" - "github.com/ramendr/ramen/internal/controller/util" "github.com/stolostron/multicloud-operators-foundation/pkg/apis/view/v1beta1" storagev1 "k8s.io/api/storage/v1" + + ramen "github.com/ramendr/ramen/api/v1alpha1" + "github.com/ramendr/ramen/internal/controller/util" ) // classLists contains [storage|snapshot|replication]classes from ManagedClusters with the required ramen storageID or, @@ -48,6 +50,92 @@ type peerList struct { clusterIDs []string } +// peerClassMatchesPeer compares the storage class name across the PeerClass and passed in peer for a match, and if +// matched compares the clusterIDs that this peer represents. No further matching is required to determine a unique +// PeerClass matching a peer +func peerClassMatchesPeer(pc ramen.PeerClass, peer peerList) bool { + if pc.StorageClassName != peer.storageClassName { + return false + } + + if !slices.Equal(pc.ClusterIDs, peer.clusterIDs) { + return false + } + + return true +} + +// findStatusPeerInPeers finds PeerClass in peers, and returns true and the peer if founds +func findStatusPeerInPeers(pc ramen.PeerClass, peers []peerList) (bool, peerList) { + for _, peer := range peers { + if !peerClassMatchesPeer(pc, peer) { + continue + } + + return true, peer + } + + return false, peerList{} +} + +// findPeerInStatusPeer finds passed in peer in passed in list of PeerClass, and returns true if found +func findPeerInStatusPeer(peer peerList, pcs []ramen.PeerClass) bool { + for _, pc := range pcs { + if !peerClassMatchesPeer(pc, peer) { + continue + } + + return true + } + + return false +} + +func peerClassFromPeer(p peerList) ramen.PeerClass { + return ramen.PeerClass{ + ClusterIDs: p.clusterIDs, + StorageClassName: p.storageClassName, + StorageID: p.storageIDs, + ReplicationID: p.replicationID, + } +} + +// pruneAndUpdateStatusPeers prunes the peer classes in status based on current peers that are passed in, and also adds +// new peers to status. +func pruneAndUpdateStatusPeers(statusPeers []ramen.PeerClass, peers []peerList) []ramen.PeerClass { + outStatusPeers := []ramen.PeerClass{} + + // Prune and update existing + for _, peerClass := range statusPeers { + found, peer := findStatusPeerInPeers(peerClass, peers) + if !found { + continue + } + + outStatusPeers = append(outStatusPeers, peerClassFromPeer(peer)) + } + + // Add new peers + for _, peer := range peers { + found := findPeerInStatusPeer(peer, statusPeers) + if found { + continue + } + + outStatusPeers = append(outStatusPeers, peerClassFromPeer(peer)) + } + + return outStatusPeers +} + +// updatePeerClassStatus updates the DRPolicy.Status.[Async|Sync] peer lists based on passed in peerList values +func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerList) error { + u.object.Status.Async.PeerClasses = pruneAndUpdateStatusPeers(u.object.Status.Async.PeerClasses, asyncPeers) + u.object.Status.Sync.PeerClasses = pruneAndUpdateStatusPeers(u.object.Status.Sync.PeerClasses, syncPeers) + + return u.statusUpdate() +} + // isAsyncVSClassPeer inspects provided pair of classLists for a matching VolumeSnapshotClass, that is linked to the // StorageClass whose storageID is respectively sIDA or sIDB func isAsyncVSClassPeer(clA, clB classLists, sIDA, sIDB string) bool { @@ -271,10 +359,6 @@ func findAllPeers(cls []classLists, schedule string) ([]peerList, []peerList) { return syncPeers, asyncPeers } -// updatePeerClassStatus updates the DRPolicy.Status.[Async|Sync] peer lists based on passed in peerList values -func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerList) { -} - // pruneClassViews prunes existing views in mcvs, for classes that are not found in survivorClassNames func pruneClassViews( m util.ManagedClusterViewGetter, @@ -502,7 +586,5 @@ func updatePeerClasses(u *drpolicyUpdater, m util.ManagedClusterViewGetter) erro syncPeers, asyncPeers := findAllPeers(cls, u.object.Spec.SchedulingInterval) - updatePeerClassStatus(u, syncPeers, asyncPeers) - - return nil + return updatePeerClassStatus(u, syncPeers, asyncPeers) } From 904f49288c919b5a8591d7622232f5aef28b7c3b Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Mon, 28 Oct 2024 12:41:49 -0400 Subject: [PATCH 13/15] Rename peerList structure to peerInfo instead Signed-off-by: Shyamsundar Ranganathan --- internal/controller/drpolicy_peerclass.go | 52 +++++++-------- .../drpolicy_peerclass_internal_test.go | 66 +++++++++---------- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/internal/controller/drpolicy_peerclass.go b/internal/controller/drpolicy_peerclass.go index 93fe57d1e..dc0d6e824 100644 --- a/internal/controller/drpolicy_peerclass.go +++ b/internal/controller/drpolicy_peerclass.go @@ -26,12 +26,12 @@ type classLists struct { vrClasses []*volrep.VolumeReplicationClass } -// peerList contains a single peer relationship between a PAIR of clusters for a common storageClassName across +// peerInfo contains a single peer relationship between a PAIR of clusters for a common storageClassName across // these peers. This should directly translate to DRPolicy.Status.[Async|Sync] updates. // NOTE: storageID discussed in comments relates to the value of the label "ramendr.openshift.io/storageid" for the // respective class. replicationID in comments relates to the value of the label "ramendr.openshift.io/replicationid" // for the respective class -type peerList struct { +type peerInfo struct { // replicationID is an empty string (indicating no common VolumeReplicationClass) or the common replicationID value // for the corresponding VRClass on each peer replicationID string @@ -53,7 +53,7 @@ type peerList struct { // peerClassMatchesPeer compares the storage class name across the PeerClass and passed in peer for a match, and if // matched compares the clusterIDs that this peer represents. No further matching is required to determine a unique // PeerClass matching a peer -func peerClassMatchesPeer(pc ramen.PeerClass, peer peerList) bool { +func peerClassMatchesPeer(pc ramen.PeerClass, peer peerInfo) bool { if pc.StorageClassName != peer.storageClassName { return false } @@ -65,8 +65,8 @@ func peerClassMatchesPeer(pc ramen.PeerClass, peer peerList) bool { return true } -// findStatusPeerInPeers finds PeerClass in peers, and returns true and the peer if founds -func findStatusPeerInPeers(pc ramen.PeerClass, peers []peerList) (bool, peerList) { +// findStatusPeerInPeers finds PeerClass in passed in peers, and returns true and the peer if founds +func findStatusPeerInPeers(pc ramen.PeerClass, peers []peerInfo) (bool, peerInfo) { for _, peer := range peers { if !peerClassMatchesPeer(pc, peer) { continue @@ -75,11 +75,11 @@ func findStatusPeerInPeers(pc ramen.PeerClass, peers []peerList) (bool, peerList return true, peer } - return false, peerList{} + return false, peerInfo{} } // findPeerInStatusPeer finds passed in peer in passed in list of PeerClass, and returns true if found -func findPeerInStatusPeer(peer peerList, pcs []ramen.PeerClass) bool { +func findPeerInStatusPeer(peer peerInfo, pcs []ramen.PeerClass) bool { for _, pc := range pcs { if !peerClassMatchesPeer(pc, peer) { continue @@ -91,18 +91,18 @@ func findPeerInStatusPeer(peer peerList, pcs []ramen.PeerClass) bool { return false } -func peerClassFromPeer(p peerList) ramen.PeerClass { +func peerClassFromPeer(peer peerInfo) ramen.PeerClass { return ramen.PeerClass{ - ClusterIDs: p.clusterIDs, - StorageClassName: p.storageClassName, - StorageID: p.storageIDs, - ReplicationID: p.replicationID, + ClusterIDs: peer.clusterIDs, + StorageClassName: peer.storageClassName, + StorageID: peer.storageIDs, + ReplicationID: peer.replicationID, } } // pruneAndUpdateStatusPeers prunes the peer classes in status based on current peers that are passed in, and also adds // new peers to status. -func pruneAndUpdateStatusPeers(statusPeers []ramen.PeerClass, peers []peerList) []ramen.PeerClass { +func pruneAndUpdateStatusPeers(statusPeers []ramen.PeerClass, peers []peerInfo) []ramen.PeerClass { outStatusPeers := []ramen.PeerClass{} // Prune and update existing @@ -128,8 +128,8 @@ func pruneAndUpdateStatusPeers(statusPeers []ramen.PeerClass, peers []peerList) return outStatusPeers } -// updatePeerClassStatus updates the DRPolicy.Status.[Async|Sync] peer lists based on passed in peerList values -func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerList) error { +// updatePeerClassStatus updates the DRPolicy.Status.[Async|Sync] peer lists based on passed in peerInfo values +func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerInfo) error { u.object.Status.Async.PeerClasses = pruneAndUpdateStatusPeers(u.object.Status.Async.PeerClasses, asyncPeers) u.object.Status.Sync.PeerClasses = pruneAndUpdateStatusPeers(u.object.Status.Sync.PeerClasses, syncPeers) @@ -219,8 +219,8 @@ func getAsyncVRClassPeer(clA, clB classLists, sIDA, sIDB string, schedule string // The clusterID and sID are the corresponding IDs for the first cluster in the classList, and the schedule is // the desired asynchronous schedule that requires to be matched // nolint:gocognit -func getAsyncPeers(scName string, clusterID string, sID string, cls []classLists, schedule string) []peerList { - peers := []peerList{} +func getAsyncPeers(scName string, clusterID string, sID string, cls []classLists, schedule string) []peerInfo { + peers := []peerInfo{} for _, cl := range cls[1:] { for scIdx := range cl.sClasses { @@ -240,7 +240,7 @@ func getAsyncPeers(scName string, clusterID string, sID string, cls []classLists } } - peers = append(peers, peerList{ + peers = append(peers, peerInfo{ storageClassName: scName, storageIDs: []string{sID, sIDcl}, clusterIDs: []string{clusterID, cl.clusterID}, @@ -256,8 +256,8 @@ func getAsyncPeers(scName string, clusterID string, sID string, cls []classLists // getSyncPeers determines if scName passed has asynchronous peers in the passed in classLists. // The clusterID and sID are the corresponding IDs for the passed in scName to find a match -func getSyncPeers(scName string, clusterID string, sID string, cls []classLists) []peerList { - peers := []peerList{} +func getSyncPeers(scName string, clusterID string, sID string, cls []classLists) []peerInfo { + peers := []peerInfo{} for _, cl := range cls { for idx := range cl.sClasses { @@ -271,7 +271,7 @@ func getSyncPeers(scName string, clusterID string, sID string, cls []classLists) // TODO: Check provisioner match? - peers = append(peers, peerList{ + peers = append(peers, peerInfo{ storageClassName: scName, storageIDs: []string{sID}, clusterIDs: []string{clusterID, cl.clusterID}, @@ -286,7 +286,7 @@ func getSyncPeers(scName string, clusterID string, sID string, cls []classLists) // findPeers finds all sync and async peers for the scName and cluster at the index startClsIdx of classLists, // across other remaining elements post the startClsIdx in the classLists -func findPeers(cls []classLists, scName string, startClsIdx int, schedule string) ([]peerList, []peerList) { +func findPeers(cls []classLists, scName string, startClsIdx int, schedule string) ([]peerInfo, []peerInfo) { scIdx := 0 for scIdx = range cls[startClsIdx].sClasses { if cls[startClsIdx].sClasses[scIdx].Name == scName { @@ -302,7 +302,7 @@ func findPeers(cls []classLists, scName string, startClsIdx int, schedule string // TODO: Check if Sync is non-nil? syncPeers := getSyncPeers(scName, cls[startClsIdx].clusterID, sID, cls[startClsIdx+1:]) - asyncPeers := []peerList{} + asyncPeers := []peerInfo{} if schedule != "" { asyncPeers = getAsyncPeers(scName, cls[startClsIdx].clusterID, sID, cls[startClsIdx:], schedule) } @@ -329,9 +329,9 @@ func unionStorageClasses(cls []classLists) []string { // findAllPeers finds all PAIRs of peers in the passed in classLists. It does an exhaustive search for each scName in // the prior index of classLists (starting at index 0) with all clusters from that index forward -func findAllPeers(cls []classLists, schedule string) ([]peerList, []peerList) { - syncPeers := []peerList{} - asyncPeers := []peerList{} +func findAllPeers(cls []classLists, schedule string) ([]peerInfo, []peerInfo) { + syncPeers := []peerInfo{} + asyncPeers := []peerInfo{} if len(cls) <= 1 { return syncPeers, asyncPeers diff --git a/internal/controller/drpolicy_peerclass_internal_test.go b/internal/controller/drpolicy_peerclass_internal_test.go index 59c1cda71..77426f069 100644 --- a/internal/controller/drpolicy_peerclass_internal_test.go +++ b/internal/controller/drpolicy_peerclass_internal_test.go @@ -18,14 +18,14 @@ var _ = Describe("updatePeerClassesInternal", func() { func( cls []classLists, schedule string, - syncPeers []peerList, - asyncPeers []peerList, + syncPeers []peerInfo, + asyncPeers []peerInfo, ) { sPeers, aPeers := findAllPeers(cls, schedule) Expect(sPeers).Should(HaveExactElements(syncPeers)) Expect(aPeers).Should(HaveExactElements(asyncPeers)) }, - Entry("Empty classLists", []classLists{}, "1m", []peerList{}, []peerList{}), + Entry("Empty classLists", []classLists{}, "1m", []peerInfo{}, []peerInfo{}), Entry("Not enough clusters", []classLists{ { @@ -44,8 +44,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single sync peer", []classLists{ @@ -79,7 +79,7 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{ + []peerInfo{ { replicationID: "", storageIDs: []string{"identical"}, @@ -87,7 +87,7 @@ var _ = Describe("updatePeerClassesInternal", func() { clusterIDs: []string{"cl-1", "cl-2"}, }, }, - []peerList{}, + []peerInfo{}, ), Entry("Multiple sync peer", []classLists{ @@ -139,7 +139,7 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{ + []peerInfo{ { replicationID: "", storageIDs: []string{"identical1"}, @@ -153,7 +153,7 @@ var _ = Describe("updatePeerClassesInternal", func() { clusterIDs: []string{"cl-1", "cl-2"}, }, }, - []peerList{}, + []peerInfo{}, ), Entry("Single async peer, missing related [VR|VS]Classes", []classLists{ @@ -187,8 +187,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, having unrelated VRClasses", []classLists{ @@ -256,8 +256,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, having unrelated VSClasses", []classLists{ @@ -313,8 +313,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, having unrelated [VR|VS]Classes", []classLists{ @@ -404,8 +404,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, having related VRClasses", []classLists{ @@ -473,8 +473,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{ + []peerInfo{}, + []peerInfo{ { replicationID: "cl-1-2-rID", storageIDs: []string{"cl-1-sID", "cl-2-sID"}, @@ -537,8 +537,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{ + []peerInfo{}, + []peerInfo{ { replicationID: "", storageIDs: []string{"cl-1-sID", "cl-2-sID"}, @@ -601,8 +601,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, with VRClasses missing rID", []classLists{ @@ -669,8 +669,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, with VRClasses mismatching schedule", []classLists{ @@ -738,8 +738,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Single async peer, having related VRClasses with mismatching rIDs", []classLists{ @@ -807,8 +807,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{}, + []peerInfo{}, + []peerInfo{}, ), Entry("Multiple async peer, having related [VR|VS]Classes", []classLists{ @@ -916,8 +916,8 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{}, - []peerList{ + []peerInfo{}, + []peerInfo{ { replicationID: "", storageIDs: []string{"cl-1-sID1", "cl-2-sID1"}, @@ -1060,7 +1060,7 @@ var _ = Describe("updatePeerClassesInternal", func() { }, }, "1m", - []peerList{ + []peerInfo{ { replicationID: "", storageIDs: []string{"cl-[1|2]-sID1"}, @@ -1074,7 +1074,7 @@ var _ = Describe("updatePeerClassesInternal", func() { clusterIDs: []string{"cl-3", "cl-4"}, }, }, - []peerList{ + []peerInfo{ { replicationID: "cl-[1|2]-[3|4]-rID", storageIDs: []string{"cl-[1|2]-sID1", "cl-[3|4]-sID1"}, From c93fd211293a351cac5c55890989798f950e08c5 Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Wed, 30 Oct 2024 10:57:55 -0400 Subject: [PATCH 14/15] Simplify labels and annotation addtion in getResourceFromManagedCluster Signed-off-by: Shyamsundar Ranganathan --- internal/controller/util/mcv_util.go | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/internal/controller/util/mcv_util.go b/internal/controller/util/mcv_util.go index bdb0fd365..0e7a9e692 100644 --- a/internal/controller/util/mcv_util.go +++ b/internal/controller/util/mcv_util.go @@ -78,12 +78,12 @@ type ManagedClusterViewGetterImpl struct { // getResourceFromManagedCluster gets the resource named resourceName in the resourceNamespace (empty if cluster scoped) // with the passed in group, version, and kind on the managedCluster. The created ManagedClusterView(MCV) has the -// passed in annotations and mcvLabel added to it. The MCV is named mcvname, and fetched into the passed in "resource" +// passed in annotations and labels added to it. The MCV is named mcvname, and fetched into the passed in "resource" // interface func (m ManagedClusterViewGetterImpl) getResourceFromManagedCluster( resourceName, resourceNamespace, managedCluster string, - annotations map[string]string, - mcvName, mcvLabel, kind, group, version string, + annotations map[string]string, labels map[string]string, + mcvName, kind, group, version string, resource interface{}, ) error { logger := ctrl.Log.WithName("MCV").WithValues("resouceName", resourceName, "cluster", managedCluster) @@ -93,13 +93,8 @@ func (m ManagedClusterViewGetterImpl) getResourceFromManagedCluster( Namespace: managedCluster, } - if mcvLabel != "" { - mcvMeta.Labels = map[string]string{mcvLabel: ""} - } - - if annotations != nil { - mcvMeta.Annotations = annotations - } + mcvMeta.Labels = labels + mcvMeta.Annotations = annotations mcvViewscope := viewv1beta1.ViewScope{ Kind: kind, @@ -125,8 +120,8 @@ func (m ManagedClusterViewGetterImpl) GetVRGFromManagedCluster(resourceName, res resourceNamespace, managedCluster, annotations, + nil, BuildManagedClusterViewName(resourceName, resourceNamespace, "vrg"), - "", "VolumeReplicationGroup", rmn.GroupVersion.Group, rmn.GroupVersion.Version, @@ -146,8 +141,8 @@ func (m ManagedClusterViewGetterImpl) GetNFFromManagedCluster(resourceName, reso resourceNamespace, managedCluster, annotations, + nil, BuildManagedClusterViewName(resourceName, resourceNamespace, "nf"), - "", "NetworkFence", csiaddonsv1alpha1.GroupVersion.Group, csiaddonsv1alpha1.GroupVersion.Version, @@ -167,8 +162,8 @@ func (m ManagedClusterViewGetterImpl) GetMModeFromManagedCluster(resourceName, m "", managedCluster, annotations, + map[string]string{MModesLabel: ""}, BuildManagedClusterViewName(resourceName, "", MWTypeMMode), - MModesLabel, "MaintenanceMode", rmn.GroupVersion.Group, rmn.GroupVersion.Version, @@ -209,8 +204,8 @@ func (m ManagedClusterViewGetterImpl) GetSClassFromManagedCluster(resourceName, "", managedCluster, annotations, + map[string]string{SClassLabel: ""}, BuildManagedClusterViewName(resourceName, "", MWTypeSClass), - SClassLabel, "StorageClass", storagev1.SchemeGroupVersion.Group, storagev1.SchemeGroupVersion.Version, @@ -234,8 +229,8 @@ func (m ManagedClusterViewGetterImpl) GetVSClassFromManagedCluster(resourceName, "", managedCluster, annotations, + map[string]string{VSClassLabel: ""}, BuildManagedClusterViewName(resourceName, "", MWTypeVSClass), - VSClassLabel, "VolumeSnapshotClass", snapv1.SchemeGroupVersion.Group, snapv1.SchemeGroupVersion.Version, @@ -259,8 +254,8 @@ func (m ManagedClusterViewGetterImpl) GetVRClassFromManagedCluster(resourceName, "", managedCluster, annotations, + map[string]string{VRClassLabel: ""}, BuildManagedClusterViewName(resourceName, "", MWTypeVRClass), - VRClassLabel, "VolumeReplicationClass", volrep.GroupVersion.Group, volrep.GroupVersion.Version, From 618ab187beb76ced2ee89de8b27de75699ab7a11 Mon Sep 17 00:00:00 2001 From: Shyamsundar Ranganathan Date: Wed, 30 Oct 2024 12:27:42 -0400 Subject: [PATCH 15/15] Simplify isAsyncVSClassPeer using a subroutine for repeted loops Signed-off-by: Shyamsundar Ranganathan --- internal/controller/drpolicy_peerclass.go | 38 +++++++---------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/internal/controller/drpolicy_peerclass.go b/internal/controller/drpolicy_peerclass.go index dc0d6e824..de430d1f0 100644 --- a/internal/controller/drpolicy_peerclass.go +++ b/internal/controller/drpolicy_peerclass.go @@ -136,32 +136,11 @@ func updatePeerClassStatus(u *drpolicyUpdater, syncPeers, asyncPeers []peerInfo) return u.statusUpdate() } -// isAsyncVSClassPeer inspects provided pair of classLists for a matching VolumeSnapshotClass, that is linked to the -// StorageClass whose storageID is respectively sIDA or sIDB -func isAsyncVSClassPeer(clA, clB classLists, sIDA, sIDB string) bool { - sidA := "" - sidB := "" - - // No provisioner match as we can do cross provisioner VSC based protection - for vscAidx := range clA.vsClasses { - sidA = clA.vsClasses[vscAidx].GetLabels()[StorageIDLabel] - if sidA == "" || sidA != sIDA { - // reset on mismatch, to exit the loop with an empty string if there is no match - sidA = "" - - continue - } - - break - } - - if sidA == "" { - return false - } - - for vscBidx := range clB.vsClasses { - sidB = clB.vsClasses[vscBidx].GetLabels()[StorageIDLabel] - if sidB == "" || sidB != sIDB { +// hasVSClassMatchingSID returns if classLists has a VolumeSnapshotClass matching the passed in storageID +func hasVSClassMatchingSID(cl classLists, sID string) bool { + for idx := range cl.vsClasses { + sid := cl.vsClasses[idx].GetLabels()[StorageIDLabel] + if sid == "" || sid != sID { continue } @@ -171,6 +150,13 @@ func isAsyncVSClassPeer(clA, clB classLists, sIDA, sIDB string) bool { return false } +// isAsyncVSClassPeer inspects provided pair of classLists for a matching VolumeSnapshotClass, that is linked to the +// StorageClass whose storageID is respectively sIDA or sIDB +func isAsyncVSClassPeer(clA, clB classLists, sIDA, sIDB string) bool { + // No provisioner match as we can do cross provisioner VSC based protection + return hasVSClassMatchingSID(clA, sIDA) && hasVSClassMatchingSID(clB, sIDB) +} + // getVRID inspects VolumeReplicationClass in the passed in classLists at the specified index, and returns, // - an empty string if the VRClass fails to match the passed in storageID or schedule, or // - the value of replicationID on the VRClass