Add VolSync Support for PVCs with Block VolumeMode #1582

BenamarMk · 2024-10-02T15:59:56Z

This PR adds support for snapshotting and replicating RBD PVCs with Block VolumeMode using VolSync. Key changes include:

Added the annotation UseVolSyncForRBDAnnotation to the DRPC resource to enable VolSync snapshotting and replication of RBD PVCs with Block VolumeMode on a per-instance basis.
Implemented necessary changes to support Block VolumeMode across all relevant components.

nirs · 2024-10-07T09:20:12Z

internal/controller/util/misc.go

@@ -22,6 +22,8 @@ const (
 	OCMBackupLabelValue string = "ramen"

 	IsCGEnabledAnnotation = "drplacementcontrol.ramendr.openshift.io/is-cg-enabled"
+
+	UseVolSyncForRBDAnnotation = "drplacementcontrol.ramendr.openshift.io/is-rbd-enabled-for-volsync-replication"


The name of the annotation is little bit too specific, ramen should not care about the storage (rbd). The value of the annotation does not match the annotation name and also too specific - about storage. There is no concept of RBD enabled or not for volsync replication.

How about:

PreferVolSyncAnnontation = "drplacementcontrol.ramendr.openshift.io/is-volsync-preferred"

Which means, when we have a VRC, prefer volsync. Current code will use the volrep in this case, with this flag will use volsync.

I expected some feedback on the annotation name, which is a good thing. However, the change in this PR is focused on individual workloads, even when we have VRCs. Essentially, we want to use VolSync for a specific RBD workload. Your suggestion implies that for this workload, VolSync should be preferred, and if it’s not enabled, it should fall back to VolRep. Is that what your comment was getting at?

Ye, isn't this the behavior we want to add?

For CG, when a user chooses to use VolSync, there’s no fallback. This is because if CG is supported for RBD with both VolSync and VolRep, we’ll always default to VolRep. If not, the user can opt for VolSync, but Ramen won’t make that choice automatically.
As for individual PVC protection, why would we need a fallback? I’m not sure I see the need for one.

That said, I’m still exploring better options for naming the annotation.

I think this will more clear if we start with design page or documentation showing the various options and when ramen use which replication mode. Starting with documentation helps to get good APIs and names.

So for now I use the same annotation name as the variable.

UseVolSyncForPVCProtection = "drplacementcontrol.ramendr.openshift.io/use-volsync-for-pvc-protection"

nirs · 2024-10-07T09:22:25Z

internal/controller/util/misc.go

@@ -251,3 +253,11 @@ func TrimToK8sResourceNameLength(name string) string {

 	return name
 }
+
+func IsRBDEnabledForVolSyncReplication(annotations map[string]string) bool {


Same issue with the name as the annotation.

We'll address this when the previous comment is addressed.

nirs · 2024-10-07T09:27:35Z

internal/controller/util/misc.go

+func IsRBDEnabledForVolSyncReplication(annotations map[string]string) bool {
+	if annotations == nil {
+		return false
+	}


No need to check for nil, accessing nil map works (returns nil for everything).

https://go.dev/play/p/kDPVCGVM64f

Got it. Point taken. Thanks!

internal/controller/volsync/vshandler_test.go

nirs · 2024-10-07T09:47:40Z

internal/controller/volsync/vshandler_test.go

+					Expect(pvc.GetName()).To(Equal(rdSpec.ProtectedPVC.Name))
+					Expect(pvc.GetOwnerReferences()[0].Kind).To(Equal("ConfigMap"))
+					Expect(*pvc.Spec.VolumeMode).To(Equal(corev1.PersistentVolumeFilesystem))
+					Expect(k8sClient.Delete(ctx, pvc)).ToNot(HaveOccurred())


If the tests fail before this line, do we run the delete line?

No need. If the test fails, the unit test fails and that's what we want.

If the created pvc has the wrong mode - we still need to delete it when the test ends. Othewise the leftover pvc can cause another test to fail or worse, succeed when the code is broken

I am not sure I understand what you are asking. I don't want to clean up. If the test fails in line 697, I want that line to fail for vshandler_test and no more. When I am debugging the unit test failure, I won't start from the last failure. I start from the first failure and I ignore everything else.

This looks like cleanup after test, not part of the test. Cleanup code should always run regardless of the outcome of the test. We probably have some hook to run cleanup after a test like JustBeforeEach.

So I am not sure about the best practice here. AfterEach should be what you want, but not when the test fails.

Cleanup must run in all cases. If AfterEach does this we should use it. If not we can do cleanup using defer.

nirs · 2024-10-07T10:00:21Z

internal/controller/volsync/vshandler_test.go

+					Expect(pvc.GetName()).To(Equal(rdSpecForBlockPVC.ProtectedPVC.Name))
+					Expect(pvc.GetOwnerReferences()[0].Kind).To(Equal("ConfigMap"))
+					Expect(*pvc.Spec.VolumeMode).To(Equal(corev1.PersistentVolumeBlock))
+					Expect(k8sClient.Delete(ctx, pvc)).ToNot(HaveOccurred())


This is a duplicate of the previous test - we have different setup and one different assertion. Can extract the repeated test code to a helper?

Different setup:

util.AddAnnotation(owner, util.UseVolSyncForRBDAnnotation, "true") Expect(k8sClient.Update(ctx, owner)).To(Succeed()) rdSpecForBlockPVC := rdSpec.DeepCopy() block := corev1.PersistentVolumeBlock rdSpecForBlockPVC.ProtectedPVC.VolumeMode = &block

Different check:

Expect(*pvc.Spec.VolumeMode).To(Equal(corev1.PersistentVolumeBlock))

Did you miss this comment?

internal/controller/volsync/vshandler.go

nirs · 2024-10-10T13:01:54Z

internal/controller/util/misc.go

@@ -22,6 +22,9 @@ const (
 	OCMBackupLabelValue string = "ramen"

 	IsCGEnabledAnnotation = "drplacementcontrol.ramendr.openshift.io/is-cg-enabled"
+
+	// Annotation


The comment is not useful - maybe describe here how this annotation change the system behavior?

nirs · 2024-10-10T13:02:39Z

internal/controller/util/misc.go

 	return annotations[IsCGEnabledAnnotation] == "true"
 }

+func IsRBDEnabledForVolSyncReplication(annotations map[string]string) bool {


Maybe rename to UseVolsyncForPVCProtection()?

Changed to IsPVCMarkedForVolSync

nirs · 2024-10-10T13:03:38Z

internal/controller/drplacementcontrol.go

+				DoNotDeletePVCAnnotation:           d.instance.GetAnnotations()[DoNotDeletePVCAnnotation],
+				DRPCUIDAnnotation:                  string(d.instance.UID),
+				rmnutil.IsCGEnabledAnnotation:      d.instance.GetAnnotations()[rmnutil.IsCGEnabledAnnotation],
+				rmnutil.UseVolSyncForPVCProtection: d.instance.GetAnnotations()[rmnutil.UseVolSyncForPVCProtection],


Other annotation have "Annotation" suffix. Maybe rename to "UseVolSyncAnnotation?

nirs · 2024-10-10T13:07:31Z

internal/controller/volsync/vshandler.go

@@ -2036,8 +2034,8 @@ func (v *VSHandler) createReadOnlyPVCFromSnapshot(rd *volsyncv1alpha1.Replicatio
 			pvc.Spec.AccessModes = accessModes
 			pvc.Spec.StorageClassName = rd.Spec.RsyncTLS.StorageClassName

-			// Only set when initially creating
-			pvc.Spec.DataSource = &snapshotRef
+			pvc.Spec.DataSource = snapshotRef


Why snapshotRef changed? I don't see any other change that explain this.

It changed to a pointer

nirs · 2024-10-10T13:18:24Z

internal/controller/volsync/vshandler.go

+	if util.IsRBDEnabledForVolSyncReplication(v.owner.GetAnnotations()) &&
+		protectedPVC.VolumeMode != nil &&
+		*protectedPVC.VolumeMode == corev1.PersistentVolumeBlock {
+		volumeMode = corev1.PersistentVolumeBlock


Indentation here is confusing - maybe indent the conditions more to make the block more clear?

if util.IsRBDEnabledForVolSyncReplication(v.owner.GetAnnotations()) && protectedPVC.VolumeMode != nil && *protectedPVC.VolumeMode == corev1.PersistentVolumeBlock { volumeMode = corev1.PersistentVolumeBlock }

Did you miss this comment?

nirs · 2024-10-10T13:21:00Z

internal/controller/volsync/vshandler_test.go

+					Expect(pvc.GetName()).To(Equal(rdSpec.ProtectedPVC.Name))
+					Expect(pvc.GetOwnerReferences()[0].Kind).To(Equal("ConfigMap"))
+					Expect(*pvc.Spec.VolumeMode).To(Equal(corev1.PersistentVolumeFilesystem))
+					Expect(k8sClient.Delete(ctx, pvc)).ToNot(HaveOccurred())


Cleanup must run in all cases. If AfterEach does this we should use it. If not we can do cleanup using defer.

ShyamsundarR

@BenamarMk we need a better way forward to support all PVCs as VolSync than using annotations. I am thinking we can create a DRPolicy that would only advertise Async PeerClasses that are VS only. This policy can hence be used, as VRG would be asked to protect PVCs for SCs with the VS and not VR method.

There are possibly other alternatives, but asking PVCs to be labelled would be a bit of a stretch for app devs at present. This is not the final solution I understand, but just throwing it out there for consideration in the future.

…resource Signed-off-by: Benamar Mekhissi <[email protected]>

…cross relevant components Signed-off-by: Benamar Mekhissi <[email protected]>

Signed-off-by: Benamar Mekhissi <[email protected]>

BenamarMk · 2024-11-12T11:48:27Z

@BenamarMk we need a better way forward to support all PVCs as VolSync than using annotations. I am thinking we can create a DRPolicy that would only advertise Async PeerClasses that are VS only. This policy can hence be used, as VRG would be asked to protect PVCs for SCs with the VS and not VR method.

Agreed - the intent was to explore a better solution for 4.19. Using an annotation to differentiate workloads isn’t necessarily a bad approach; it provides a lightweight, simple option where the user can just check a box when creating the workload. For now, this serves as a quick way to enable RBD workloads with VolSync, and we can think of a better solution for 4.19.

Signed-off-by: Benamar Mekhissi <[email protected]>

nirs · 2024-11-12T12:07:55Z

internal/controller/volumereplicationgroup_controller.go


-			break
+	if !pvcEnabledForVolSync {


Do we need the temporary variable? we can use:

if !util.IsPVCMarkedForVolSync(v.instance.GetAnnotations()) { ... }

nirs · 2024-11-12T12:11:35Z

internal/controller/util/misc.go

@@ -22,6 +22,9 @@ const (
 	OCMBackupLabelValue string = "ramen"

 	IsCGEnabledAnnotation = "drplacementcontrol.ramendr.openshift.io/is-cg-enabled"
+
+	// When this annotation is set to true, VolSync will protect RBD PVCs.


This can be any storage, RBD is a good example but we don't care which storage is this.

nirs · 2024-11-12T12:15:03Z

internal/controller/volsync/vshandler.go

+	if util.IsRBDEnabledForVolSyncReplication(v.owner.GetAnnotations()) &&
+		protectedPVC.VolumeMode != nil &&
+		*protectedPVC.VolumeMode == corev1.PersistentVolumeBlock {
+		volumeMode = corev1.PersistentVolumeBlock


Did you miss this comment?

nirs · 2024-11-12T12:16:07Z

internal/controller/volsync/vshandler_test.go

+					Expect(pvc.GetName()).To(Equal(rdSpecForBlockPVC.ProtectedPVC.Name))
+					Expect(pvc.GetOwnerReferences()[0].Kind).To(Equal("ConfigMap"))
+					Expect(*pvc.Spec.VolumeMode).To(Equal(corev1.PersistentVolumeBlock))
+					Expect(k8sClient.Delete(ctx, pvc)).ToNot(HaveOccurred())


Did you miss this comment?

nirs · 2024-11-12T12:17:29Z

internal/controller/volumereplicationgroup_controller.go


-			return nil
-		}
+	if !pvcEnabledForVolSync {


Same here, we could eliminate the temporary variable.

ShyamsundarR · 2024-11-12T13:25:05Z

Merging changes, open comments do not impact functionality at present.

nirs reviewed Oct 7, 2024

View reviewed changes

nirs requested a review from ELENAGER October 7, 2024 10:29

BenamarMk force-pushed the block-volsync-support branch 4 times, most recently from ab695f1 to 018f295 Compare October 9, 2024 15:35

nirs reviewed Oct 10, 2024

View reviewed changes

BenamarMk force-pushed the block-volsync-support branch from e0a3fd0 to 74f8093 Compare October 28, 2024 10:47

nirs mentioned this pull request Oct 31, 2024

e2e: Test volsync for block pvc #1631

Open

3 tasks

BenamarMk force-pushed the block-volsync-support branch from 74f8093 to 87df5ea Compare November 11, 2024 13:27

ELENAGER approved these changes Nov 11, 2024

View reviewed changes

ShyamsundarR approved these changes Nov 11, 2024

View reviewed changes

Benamar Mekhissi added 3 commits November 11, 2024 17:39

Add RBD support (Block/Filesystem VolumeMode) via annotation in DRPC …

a4da4fd

…resource Signed-off-by: Benamar Mekhissi <[email protected]>

Implement the necessary changes for non-CG Block volumeMode support a…

bbe7858

…cross relevant components Signed-off-by: Benamar Mekhissi <[email protected]>

Rename rmnutil to util

c12db7b

Signed-off-by: Benamar Mekhissi <[email protected]>

BenamarMk force-pushed the block-volsync-support branch from 87df5ea to a542889 Compare November 12, 2024 11:37

Rename annotation and other misc changes

6f83287

Signed-off-by: Benamar Mekhissi <[email protected]>

BenamarMk force-pushed the block-volsync-support branch from a542889 to 6f83287 Compare November 12, 2024 11:57

nirs reviewed Nov 12, 2024

View reviewed changes

ShyamsundarR merged commit 1f11958 into RamenDR:main Nov 12, 2024
20 checks passed

Add VolSync Support for PVCs with Block VolumeMode #1582

Add VolSync Support for PVCs with Block VolumeMode #1582

Conversation

BenamarMk commented Oct 2, 2024

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

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

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

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

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

BenamarMk commented Nov 12, 2024

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 commented Nov 12, 2024