use schedule instead of VolumeReplicationClass #121

raghavendrabhat · 2021-07-19T15:00:31Z

VolumeReplicationClass field in VolumeReplicationGroup is
nothing but a string. Instead of that, use a schedule which
is then used in combination with the storage driver for the
pvc (obtained from storageclass) by the VolumeReplicationGroup
to determine the appropriate VolumeReplicationClass to use.

TODO:

HUB/DRPC (Previously AVR):

modify DRPolicy to include schedule along with DR prepared
clusters
Make DRPC (previously AVR) refer to the DRPolicy schedule
while creating VolumeReplicationGroup and use that as the
schedule for VolumeReplicationGroup.

Managed Cluster:

While reconciling VolumeReplicationGroup, for each of the
PVC obtained via label selection, do this
- storagedriver = StorageDriver(StorageClass(pvc))
- Get All the VolumeReplicationClasses in the cluster
- For each VolumeReplicationClass (chosen via label selector)
  if (replicationdriver == storagedriver and
  replicationSchedule == vrg.Spec.Schedule)
  chosenReplicationClass = VolumeReplicationClass

Signed-off-by: Raghavendra M raghavendra@redhat.com

VolumeReplicationClass field in VolumeReplicationGroup is nothing but a string. Instead of that, use a schedule which is then used in combination with the storage driver for the pvc (obtained from storageclass) by the VolumeReplicationGroup to determine the appropriate VolumeReplicationClass to use. TODO: HUB/DRPC (Previously AVR): - modify DRPolicy to include schedule along with DR prepared clusters - Make DRPC (previously AVR) refer to the DRPolicy schedule while creating VolumeReplicationGroup and use that as the schedule for VolumeReplicationGroup. Managed Cluster: - While reconciling VolumeReplicationGroup, for each of the PVC obtained via label selection, do this - storagedriver = StorageDriver(StorageClass(pvc)) - Get All the VolumeReplicationClasses in the cluster - For each VolumeReplicationClass (chosen via label selector) if (replicationdriver == storagedriver and replicationSchedule == vrg.Spec.Schedule) chosenReplicationClass = VolumeReplicationClass Signed-off-by: Raghavendra M <raghavendra@redhat.com>

BenamarMk · 2021-07-19T15:29:22Z

controllers/drplacementcontrol_controller.go

@@ -233,21 +234,36 @@ func (r *DRPlacementControlReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return ctrl.Result{}, err
 	}

+	schedule, err := r.getSchedule(ctx, drpc)


DRPolicy object will be saved in the DRPCInstance. I think you should use it (not yet merged), that way, you will not have to get the DRPolicy multiple times, one for the Schedule and another time for the ReplicationClassSelector

Ack! Also as it stands this is being fetched too soon. BUT if DRPolicy is fetched and stored in drpc (which is almost there in #120 ) then these functions can be removed.

BenamarMk · 2021-07-19T15:31:21Z

controllers/drplacementcontrol_controller.go

+
+	*replicationClassSelector = clusterPeers.Spec.ReplicationClassSelector
+
+	return replicationClassSelector, nil


Why don't you just return the copy of clusterPeers.Spec.ReplicationClassSelector instead of creating an instance in line 473 and then assignment in line 481?

BenamarMk · 2021-07-19T15:33:05Z

controllers/drplacementcontrol_controller.go

@@ -438,6 +520,8 @@ type DRPCInstance struct {
 	userPlacementRule    *plrv1.PlacementRule
 	drpcPlacementRule    *plrv1.PlacementRule
 	mwu                  rmnutil.MWUtil
+	schedule             *string
+	replclassselect      *metav1.LabelSelector


why isn't this variable following the camel case convention?

(Future) Just a note here, the DRPCInstance is becoming huge. We should refactor it.

BenamarMk · 2021-07-19T15:37:29Z

controllers/volumereplicationgroup_controller.go

@@ -1109,6 +1147,11 @@ func (v *VRGInstance) updateVR(volRep *volrep.VolumeReplication,

 // createVR creates a VolumeReplication CR with a PVC as its data source.
 func (v *VRGInstance) createVR(vrNamespacedName types.NamespacedName, state volrep.ReplicationState) error {
+	volumeReplicationClass, err := v.selectVolumeReplicationClass(vrNamespacedName)
+	if err != nil {
+		return fmt.Errorf("failed to find the appropriate VolumeReplicationClass (%s)", v.instance.Name)


Isn't the error returned here is lost? Shouldn't be included in the message?

BenamarMk · 2021-07-19T15:39:45Z

controllers/volumereplicationgroup_controller.go

+	if err := v.reconciler.Get(v.ctx, types.NamespacedName{Name: *scName}, storageClass); err != nil {
+		v.log.Info(fmt.Sprintf("Failed to get the storageclass %s", *scName))
+
+		return className, fmt.Errorf("failed to get the storageclass with name %s",


Same here, the err information is lost

BenamarMk · 2021-07-19T15:41:45Z

controllers/volumereplicationgroup_controller.go

+			continue
+		}
+
+		schedule, found := replicationClass.Spec.Parameters["schedule"]


"schedule" is used in another placed. Please define as global const.

BenamarMk · 2021-07-19T15:51:10Z

controllers/volumereplicationgroup_controller.go

+		schedule, found := replicationClass.Spec.Parameters["schedule"]
+		if !found {
+			// As of now, the provisioner matches. Use this as the best guess
+			// until a replicationClass with schedule match is found. i.e.


What if you never find a schedule but a provisioner matched, is the className assignment here still valid?

Ack! If we choose this we will exit with a non-empty classname that does not match the schedule, let's match both driver name and schedule to select the volReplClass.

ShyamsundarR · 2021-07-20T11:45:41Z

controllers/drplacementcontrol_controller.go

@@ -414,7 +496,7 @@ func (r *DRPlacementControlReconciler) addClusterPeersToPlacementRule(ctx contex
 	}

 	if len(clusterPeers.Spec.ClusterNames) == 0 {
-		return fmt.Errorf("invalid DRPolicy configuration. Name %s", clusterPeers.Name)
+		return fmt.Errorf("invalid DRPolicy configuration. No clusters given. Name %s", clusterPeers.Name)


(nit) errors as a single line with no caps please, these are included into other logs/error messages and hence the standard

ShyamsundarR · 2021-07-20T11:48:34Z

controllers/drplacementcontrol_controller.go

@@ -233,21 +234,36 @@ func (r *DRPlacementControlReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return ctrl.Result{}, err
 	}

+	schedule, err := r.getSchedule(ctx, drpc)


Ack! Also as it stands this is being fetched too soon. BUT if DRPolicy is fetched and stored in drpc (which is almost there in #120 ) then these functions can be removed.

ShyamsundarR · 2021-07-20T11:52:52Z

controllers/volumereplicationgroup_controller.go

+func (v *VRGInstance) updateReplicationClassList() error {
+	labelSelector := v.instance.Spec.ReplicationClassSelector
+
+	v.log.Info("Fetching PersistentVolumeClaims", "labeled", labels.Set(labelSelector.MatchLabels))


(nit) message should read "Fetching VolumeReplicationClass" rather than PVC.

ShyamsundarR · 2021-07-20T12:00:04Z

controllers/volumereplicationgroup_controller.go

+	}
+
+	pvc := &corev1.PersistentVolumeClaim{}
+	if err := v.reconciler.Get(v.ctx, namespacedName, pvc); err != nil {


We already listed the PVCs and should not be "getting" them again. Reduce APi calls, and reuse fetched PVCs instead here (or in a higher function and pass the required replClass to lower functions, from an abstraction POV)

ShyamsundarR · 2021-07-20T12:02:09Z

controllers/volumereplicationgroup_controller.go

+		schedule, found := replicationClass.Spec.Parameters["schedule"]
+		if !found {
+			// As of now, the provisioner matches. Use this as the best guess
+			// until a replicationClass with schedule match is found. i.e.


Ack! If we choose this we will exit with a non-empty classname that does not match the schedule, let's match both driver name and schedule to select the volReplClass.

ShyamsundarR · 2021-07-20T12:02:47Z

controllers/volumereplicationgroup_controller.go

@@ -385,6 +403,26 @@ func (v *VRGInstance) updatePVCList() error {
 	return nil
 }

+func (v *VRGInstance) updateReplicationClassList() error {
+	labelSelector := v.instance.Spec.ReplicationClassSelector


(test) does empty label selector return all classes here?

ShyamsundarR · 2021-07-20T12:03:47Z

controllers/volumereplicationgroup_controller.go

@@ -322,6 +327,19 @@ func (v *VRGInstance) processVRG() (ctrl.Result, error) {
 		return ctrl.Result{Requeue: true}, nil
 	}

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


(optimize) can this be postponed till we need it the first time? IOW, make the call when we need it, so that we are done with other checks like ready for primary/secondary etc.?

ShyamsundarR · 2021-07-20T12:07:46Z

controllers/volumereplicationgroup_controller.go

@@ -228,6 +229,8 @@ func filterPVC(mgr manager.Manager, pvc *corev1.PersistentVolumeClaim, log logr.
 // +kubebuilder:rbac:groups=ramendr.openshift.io,resources=volumereplicationgroups/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=ramendr.openshift.io,resources=volumereplicationgroups/finalizers,verbs=update
 // +kubebuilder:rbac:groups=replication.storage.openshift.io,resources=volumereplications,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=replication.storage.openshift.io,resources=volumereplicationclasses,verbs=get;list;watch;update;patch


(heads up) The file ramen/config/dr_cluster/rbac/role.yaml also has to be updated when roles change, as it is a partial copy of ramen/config/rbac/role.yaml to suit VRG reconciler needs (or ramen-dr-cluster needs)

ShyamsundarR · 2021-07-20T12:08:55Z

api/v1alpha1/drpolicy_types.go

+	// can be used to schedule replication to occur at regular, time-based
+	// intervals.
+	//+kubebuilder:validation:Pattern=`^(\d+|\*)(/\d+)?(\s+(\d+|\*)(/\d+)?){4}$`
+	Schedule *string `json:"schedule"`


ramen/config/samples/ramendr_v1alpha1_drpolicy.yaml needs to be updated.

ShyamsundarR · 2021-07-20T12:09:13Z

api/v1alpha1/volumereplicationgroup_types.go

+	// that are scanned to select an appropriate VolumeReplicationClass
+	// for the VolumeReplication resource.
+	//+optional
+	ReplicationClassSelector metav1.LabelSelector `json:"replicationClassSelector,omitempty"`


ramen/config/samples/ramendr_v1alpha1_volumereplicationgroup.yaml needs to be updated

hatfieldbrian · 2021-07-20T23:08:51Z

controllers/drplacementcontrol_controller_test.go

@@ -185,8 +187,8 @@ func updateManagedClusterViewWithVRG(mcv *fndv2.ManagedClusterView, replicationS
 		TypeMeta:   metav1.TypeMeta{Kind: "VolumeReplicationGroup", APIVersion: "ramendr.openshift.io/v1alpha1"},
 		ObjectMeta: metav1.ObjectMeta{Name: DRPCName, Namespace: DRPCNamespaceName},
 		Spec: rmn.VolumeReplicationGroupSpec{
-			VolumeReplicationClass: "volume-rep-class",


ocm-minikube-ramen.sh creates this object, but it seems it is no longer required, so pull request #166 removes the creation and deletion of it.

raghavendrabhat · 2021-08-03T18:47:44Z

#182 has been submitted which has the changes rebased on top of #120 and also review comments addressed.

BenamarMk reviewed Jul 19, 2021

View reviewed changes

ShyamsundarR reviewed Jul 20, 2021

View reviewed changes

hatfieldbrian reviewed Jul 20, 2021

View reviewed changes

hatfieldbrian mentioned this pull request Jul 20, 2021

ocm-minikube-ramen.sh: volume-rep-class remove #166

Merged

raghavendrabhat closed this Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use schedule instead of VolumeReplicationClass #121

use schedule instead of VolumeReplicationClass #121

raghavendrabhat commented Jul 19, 2021

BenamarMk Jul 19, 2021

ShyamsundarR Jul 20, 2021

BenamarMk Jul 19, 2021 •

edited

Loading

BenamarMk Jul 19, 2021

BenamarMk Jul 19, 2021

BenamarMk Jul 19, 2021

BenamarMk Jul 19, 2021

BenamarMk Jul 19, 2021

BenamarMk Jul 19, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

ShyamsundarR Jul 20, 2021

hatfieldbrian Jul 20, 2021

raghavendrabhat commented Aug 3, 2021


		*replicationClassSelector = clusterPeers.Spec.ReplicationClassSelector

		return replicationClassSelector, nil

use schedule instead of VolumeReplicationClass #121

use schedule instead of VolumeReplicationClass #121

Conversation

raghavendrabhat commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenamarMk Jul 19, 2021 • 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

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

raghavendrabhat commented Aug 3, 2021

BenamarMk Jul 19, 2021 •

edited

Loading