Add VolumeGroupReplication support #1472

ELENAGER · 2024-06-24T20:37:28Z

This version for VolumeGroupReplication support introduces code for separation of PVCs, created as part of consistency group. On every VRG reconcile we are iterating over all PVCs, protected by volume replication, and marking those, who are part of consistency group. Afterwards, during VR reconciliation, we will create VR for every PVC, that is not part of the consistency group, and VGR for all PVCs, that are using the same storage id.

ShyamsundarR

I also created this for some common scheme to process CGs across volsync and volrep, that relates to some comments here: #1483

controllers/volumereplicationgroup_controller.go

ShyamsundarR · 2024-07-05T15:27:40Z

controllers/vrg_volrep.go

@@ -62,7 +64,7 @@ func (v *VRGInstance) reconcileVolRepsAsPrimary() {
 		}

 		// If VR did not reach primary state, it is fine to still upload the PV and continue processing
-		requeueResult, _, err := v.processVRAsPrimary(pvcNamespacedName, log)
+		requeueResult, _, err := v.processVRAsPrimary(pvcNamespacedName, pvc, log)


At some point I am thinking we should process a group here, and if the group is processed then check the PVCs reported by the group as protected and check it against the PVCs that we decide should belong to this group.

Assume we have a list of groups, with PVCs that belong to the group. We need to process the group only once as Primary, and future PVCs can be validated as part of the group status (i.e already protected by the group) or not.

ShyamsundarR

Still need to review vrg_volrep.go file. Other changes are reviewed, nothing major, mostly integration points that may need some additional handling.

internal/controller/volumereplicationgroup_controller.go

ShyamsundarR · 2024-07-23T13:30:52Z

internal/controller/volumereplicationgroup_controller.go

@@ -548,6 +556,10 @@ func (v *VRGInstance) processVRG() ctrl.Result {
 		return v.invalid(err, "Failed to process list of PVCs to protect", true)
 	}

+	if err := v.updatePVCListForCG(); err != nil {


(future) At some point for clarity, we potentially should remove the optimizations for PVC grouping into either lists and let the separate handle it. As currently we look at various conditions and optimize out calling the separate function. This is to help code redability and not functionality as such.

ShyamsundarR · 2024-07-23T13:31:57Z

internal/controller/volumereplicationgroup_controller.go

+		return nil
+	}
+
+	for idx := range v.volRepPVCs {


This becomes common for both Volsync and Volrep, i.e PVC labeling. @BenamarMk @youhangwang (FYI)

That's correct. Currently, VolSync is not using this function yet (addConsistencyGroupLabel). The temporary solution in the VolSync-related CG code involves the user labeling the PVCs. We liked the idea of having users label the PVCs as it allows for multiple CGs per storageId. This is still missing from @youhangwang's PR.
I think we should implement the following for both VolSync and VolRep:

Provide a common label CG name, such as ramendr.openshift.io/cg-label for a lack of a better name.

If the user labels the PVC with that label, then the PVC label value for CG should be the combination of ramendr.openshift.io/consistency-group: storageId + cg-label.

yes, I'm waiting for this pr merged, then I can rebase my pr to adopt this func. or maybe I can copy this func into my pr to handle volcyncPVCs.

1. Provide a common label CG name, such as `ramendr.openshift.io/cg-label` for a lack of a better name. 2. If the user labels the PVC with that label, then the PVC label value for CG should be the combination of `ramendr.openshift.io/consistency-group: storageId + cg-label`.

Ack! if it is required that PVCs be separated into CGs of their own, even as they are selected by a common pvcSelector provided to Ramen, this label can help create sub-cg groups as desired.

Would it be safe to assume that this would be in the future, IOW not part of this PR or the initial CG PRs. The initial set would just group all PVCs to a cg, when possible, giving no further granularity to the app owner.

ShyamsundarR · 2024-07-23T13:38:31Z

internal/controller/volumereplicationgroup_controller.go

@@ -668,10 +680,18 @@ func (v *VRGInstance) updatePVCList() error {
 		return nil
 	}

-	if err := v.updateReplicationClassList(); err != nil {
-		v.log.Error(err, "Failed to get VolumeReplicationClass list")
+	if rmnutil.IsCGEnabled(v.instance.GetAnnotations()) {


@BenamarMk if the CG annotation is added to an already created and reconciled DRPC/VRG, will it cause issues here. I.e now the VRG reconciliation would shift to CG based processing and potentially leave stale non-CG resources (at least in the Volrep case).

IOW, CG enabling is during initial reconcile from DRPC and should not be enabled later, is that somehow prevented overall from DRPC creating the VRG?

@ShyamsundarR Yes, that would violate the CG requirement. Existing workloads are not permitted to be converted to use CG retroactively. If a user inadvertently adds the CG annotation to an already created and reconciled DRPC/VRG, it could lead to inconsistent states, given that PVCs has to be labeled to use CG as well.

CG enabling should occur only during the initial reconciliation of DRPC and should not be enabled afterward. This is not currently prevented by the DRPC when creating the VRG, so if a user adds the CG annotation later, the user would eventually encounter CG errors.

ShyamsundarR · 2024-07-23T13:40:39Z

internal/controller/volumereplicationgroup_controller.go

@@ -763,6 +858,17 @@ func (v *VRGInstance) separatePVCsUsingStorageClassProvisioner(pvcList *corev1.P
 			}
 		}

+		if !replicationClassMatchFound {


If CG is enabled, and SC has the replicationID label, then we should error out if we do not find a matching VGRClass, and not default the PVC to Volsync.

This either gets added here or with the PR #1487

ShyamsundarR · 2024-07-23T13:44:57Z

test/addons/rook-pool/storage-class.yaml

@@ -6,6 +6,8 @@ apiVersion: storage.k8s.io/v1
 kind: StorageClass
 metadata:
    name: rook-ceph-block
+    labels:


A similar change would be needed to the CephFS storageClass from file test/addons/rook-cephfs/kustomization.yaml ?

ShyamsundarR

validateVRStatus should ensure the desired PVC is part of the VGR group status, so that we know it is protected. Currently if a new PVC is added to the group we will report that as protected based on the existing VGR output.

ShyamsundarR · 2024-07-23T19:49:17Z

internal/controller/vrg_volrep.go

@@ -67,7 +68,7 @@ func (v *VRGInstance) reconcileVolRepsAsPrimary() {
 		}

 		// If VR did not reach primary state, it is fine to still upload the PV and continue processing
-		requeueResult, _, err := v.processVRAsPrimary(pvcNamespacedName, log)
+		requeueResult, _, err := v.processVRAsPrimary(pvcNamespacedName, pvc, log)


A group request will protect all PVCs in the group, and so when we invoke protection for a single PVC here, without ensuring we are done with prior steps like pvcUnprotectVolRepIfDeleted, preparePVCForVRProtection we may introduce races that may get in the way of protection.

While the code to create/update VR or VGR is common, I think we may need to do certain operations to the group here before we proceed with a VGR for the group.

I am in parallel thinking how best to achieve this with the existing code as well.

ShyamsundarR · 2024-07-23T19:54:14Z

internal/controller/vrg_volrep.go

@@ -67,7 +68,7 @@ func (v *VRGInstance) reconcileVolRepsAsPrimary() {
 		}

 		// If VR did not reach primary state, it is fine to still upload the PV and continue processing
-		requeueResult, _, err := v.processVRAsPrimary(pvcNamespacedName, log)
+		requeueResult, _, err := v.processVRAsPrimary(pvcNamespacedName, pvc, log)


pvcUnprotectVolRepIfDeleted should remove the CG label from the PVC, such that this PVC is no longer protected as part of the group.

Currently if the function above is traced (and required flags enabled) it would delete the entire VGR and not remove the PVC from the VGR.

While the feature of PVC deletion is still not the default (as there are other DRPC changes that are required), we should be aware of this problem if this is enabled.

ShyamsundarR · 2024-07-23T21:50:17Z

internal/controller/vrg_volrep.go

+		return "", fmt.Errorf("missing storageID for PVC %s/%s", pvc.GetNamespace(), pvc.GetName())
+	}
+
+	vgrName := storageID + "-vgr"


There can be multiple workloads independently DR protected in the same namespace. These can have PVCs from the same SC with the same StorageID. The name will then conflict.

I would suggest, munging this with the VRG name as well. We cannot have 2 VRGs with the same name in the same namespace, so using that as part of the name maybe an option. It does create a possibility that we will have a longer name, so any shortening here may assist in that.

I changed VGR name to {vrg}-{storageID}, @ShyamsundarR what is we will remove -drpc from vrg name? It will shorten the whole name a little

Signed-off-by: Elena Gershkovich <[email protected]>

ELENAGER requested review from ShyamsundarR and BenamarMk June 24, 2024 20:37

ELENAGER force-pushed the elena_tests branch 5 times, most recently from edae774 to ee1b472 Compare July 1, 2024 09:10

ShyamsundarR reviewed Jul 5, 2024

View reviewed changes

ELENAGER force-pushed the elena_tests branch 5 times, most recently from ff8fcf1 to 45dd82a Compare July 10, 2024 16:23

ELENAGER force-pushed the elena_tests branch 10 times, most recently from 2f4eee7 to 2854dc7 Compare July 22, 2024 14:36

ShyamsundarR reviewed Jul 23, 2024

View reviewed changes

ELENAGER force-pushed the elena_tests branch 3 times, most recently from fc759d3 to bebc6ae Compare July 25, 2024 10:21

ELENAGER force-pushed the elena_tests branch 3 times, most recently from 54c0651 to 989c437 Compare August 27, 2024 10:33

ELENAGER force-pushed the elena_tests branch 2 times, most recently from fe41328 to 5b446a3 Compare September 3, 2024 13:09

ELENAGER force-pushed the elena_tests branch 3 times, most recently from 2b9ce80 to d02aad6 Compare September 17, 2024 11:57

ELENAGER force-pushed the elena_tests branch from 830172b to ffb9e5a Compare September 19, 2024 10:17

ELENAGER force-pushed the elena_tests branch 4 times, most recently from ca3299c to ceee8b6 Compare October 1, 2024 14:41

ELENAGER force-pushed the elena_tests branch 6 times, most recently from 6311db7 to c704dc2 Compare October 9, 2024 14:24

ELENAGER force-pushed the elena_tests branch from c704dc2 to ce04011 Compare October 15, 2024 09:32

ELENAGER force-pushed the elena_tests branch 6 times, most recently from 5897f1e to 94c9841 Compare November 4, 2024 10:12

ELENAGER force-pushed the elena_tests branch from 62afd4e to 3cc7b69 Compare November 10, 2024 09:30

ELENAGER added 2 commits November 10, 2024 12:06

Add VolumeGroupReplication support.

9c92c5b

Signed-off-by: Elena Gershkovich <[email protected]>

Add CG support to VolumeReplicationGroupStatus and VRGResourceMeta

ead5e73

Signed-off-by: Elena Gershkovich <[email protected]>

ELENAGER force-pushed the elena_tests branch from 3cc7b69 to ead5e73 Compare November 10, 2024 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VolumeGroupReplication support #1472

Add VolumeGroupReplication support #1472

ELENAGER commented Jun 24, 2024

ShyamsundarR left a comment

ShyamsundarR Jul 5, 2024

ShyamsundarR left a comment

ShyamsundarR Jul 23, 2024

ShyamsundarR Jul 23, 2024

BenamarMk Jul 24, 2024 •

edited

Loading

youhangwang Jul 25, 2024 •

edited

Loading

ShyamsundarR Jul 25, 2024

BenamarMk Jul 25, 2024

ShyamsundarR Jul 23, 2024

BenamarMk Jul 24, 2024

ShyamsundarR Jul 23, 2024

ShyamsundarR Jul 23, 2024

ShyamsundarR left a comment

ShyamsundarR Jul 23, 2024

ShyamsundarR Jul 23, 2024

ShyamsundarR Jul 23, 2024

ELENAGER Jul 24, 2024

Add VolumeGroupReplication support #1472

Are you sure you want to change the base?

Add VolumeGroupReplication support #1472

Conversation

ELENAGER commented Jun 24, 2024

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenamarMk Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

youhangwang Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenamarMk Jul 24, 2024 •

edited

Loading

youhangwang Jul 25, 2024 •

edited

Loading