Generates and labels `Volume*SnapshotClass` #168

raaizik · 2024-06-03T12:41:23Z

Changes

Have storage claim controller own VolumeGroupSnapshotClass with the required labels for RDR added to both VGSC and VSC.
Adds Available CRDs functionality
- Resolves compatibility issue
Upgrades k8s-csi
- Requires go >= 1.22.0 which uses client-go v0.30 which works with CR v0.18 and CG v0.14

raaizik · 2024-06-04T13:50:41Z

rewantsoni · 2024-06-10T04:57:47Z

controllers/storageclaim_controller.go

+				labels["ramendr.openshift.io/replicationID"] = r.storageClaimHash
+				labels["ramendr.openshift.io/storageID"] = storageID
 				if resource.Name == "cephfs" {
 					volumeSnapshotClass = r.getCephFSVolumeSnapshotClass(data)


The labels are not being applied to the cephfs or rbd VolumeSnapshotClass

Suggested change

labels["ramendr.openshift.io/replicationID"] = r.storageClaimHash

labels["ramendr.openshift.io/storageID"] = storageID

if resource.Name == "cephfs" {

volumeSnapshotClass = r.getCephFSVolumeSnapshotClass(data)

labels["ramendr.openshift.io/replicationID"] = r.storageClaimHash

labels["ramendr.openshift.io/storageID"] = storageID

if resource.Name == "cephfs" {

volumeSnapshotClass = r.getCephFSVolumeSnapshotClass(data,labels)

rewantsoni · 2024-06-10T05:01:01Z

controllers/storageclaim_controller.go

+	if reflect.DeepEqual(existing.Parameters, volumeGroupSnapshotClass.Parameters) {
+		return nil
+	}


Should we also check for labels?

raaizik · 2024-06-17T08:25:04Z

Makefile

yq isn't recognized anymore. (bash: yq: command not found)

nb-ohad · 2024-06-20T10:13:40Z

controllers/storageclaim_controller.go

 			err = json.Unmarshal(resource.Data, &data)
 			if err != nil {
 				return reconcile.Result{}, fmt.Errorf("failed to unmarshal StorageClaim configuration response: %v", err)
 			}

+			// SID for RamenDR
+			storageID := getMD5Hash(string(r.storageClaim.UID)) + r.storageClaimHash


Storage ID does not need to be distinct across clusters, only the Replication ID needs to be.
But even if the requirement was true for Storage ID, a UUID is assumed to be globally unique (by definition).

Suggested change

storageID := getMD5Hash(string(r.storageClaim.UID)) + r.storageClaimHash

storageID := r.storageClaim.UID

And please move the definition of the storage ID var outside of the for, there is no need to redefine it on every iteration as it is constant for the entire reconciliation.

nb-ohad · 2024-06-20T10:27:31Z

controllers/storageclaim_controller.go

+				labels["ramendr.openshift.io/storageID"] = storageID
+				volumeSnapshotClass = r.getCephDriverVolumeSnapshotClass(resource.Name, data)
+				utils.AddLabels(volumeSnapshotClass, labels)


If we are only concerned with a single-label why do we need the labels map var in the first place

Suggested change

labels["ramendr.openshift.io/storageID"] = storageID

volumeSnapshotClass = r.getCephDriverVolumeSnapshotClass(resource.Name, data)

utils.AddLabels(volumeSnapshotClass, labels)

volumeSnapshotClass = r.getCephDriverVolumeSnapshotClass(resource.Name, data)

utils.AddLabel(volumeSnapshotClass, "ramendr.openshift.io/storageID", storageID)

nb-ohad · 2024-06-20T10:38:47Z

controllers/storageclaim_controller.go

+				labels["ramendr.openshift.io/storageID"] = storageID
+				volumeGroupSnapshotClass = r.getCephDriverVolumeGroupSnapshotClass(resource.Name, data)
+				utils.AddLabels(volumeGroupSnapshotClass, labels)


Same comment as the one above regarding AddLabels

nb-ohad · 2024-06-20T10:40:12Z

controllers/storageclaim_controller.go

+				// generate a new clusterID for cephfs subvolumegroup, as
+				// storageclaim is clusterscoped resources using its
+				// hash as the clusterID
+				data["clusterID"] = r.storageClaimHash


@raaizik I am confused.
If we can decide on a cluster ID locally why do we need to send it from the provider (as implemented in this PR: red-hat-storage/ocs-operator#2661)?

As are we ➡️ comment
Edit: removed from provider

nb-ohad · 2024-06-20T10:43:56Z

controllers/storageclaim_controller.go

+	labelsEqual := reflect.DeepEqual(existing.Labels, volumeSnapshotClass.Labels)
+	if reflect.DeepEqual(existing.Parameters, volumeSnapshotClass.Parameters) && labelsEqual {


Let's make the code more readable by aligning both the params and label equality checks
Also, we are setting annotations so we should also consider checking equality there as well

Suggested change

labelsEqual := reflect.DeepEqual(existing.Labels, volumeSnapshotClass.Labels)

if reflect.DeepEqual(existing.Parameters, volumeSnapshotClass.Parameters) && labelsEqual {

areLabelsEqual := eflect.DeepEqual(existing.Labels, volumeSnapshotClass.Labels)

areAnnotationsEqual := eflect.DeepEqual(existing.Annotations, volumeSnapshotClass.Annotations)

areParamsEqual := reflect.DeepEqual(existing.Parameters, volumeSnapshotClass.Parameters)

if areLabelsEqual && areAnnotationsEqual && areParamsEqual {

Edit (leela): Fixed the suggestion rendering

nb-ohad · 2024-06-20T10:50:36Z

controllers/storageclaim_controller.go

-	if existing.UID != "" {
-		// Since we have to update the existing VolumeSnapshotClass, so we will delete the existing VolumeSnapshotClass and create a new one.
-		r.log.Info("VolumeSnapshotClass needs to be updated, deleting it.", "Name", existing.Name)
+	if labelsEqual {


This check does not make sense.
At this point in the code, it is already established that an update is needed (because of label changes or params changes)

The only concern left is if we need to delete the existing one or not!

Your current approach has a couple of bugs:

If the VolumeSnapshotClass didn't exist then your labelsEqual will be true and you will jump to the lease which will try to update instead of create

if both labels and params changed you will jump to the else and try to update instead of deleting and creating

With that in mind, I would prefer to keep the code simple and always delete and recreate, even in cases where we don't have to delete. Trying to do it differently will just lead to many edge cases that we can easily miss and the overhead in deleting and recreating is negligible as we don't assume many label changes

Looks like your opinions are conflicting ➡️ comment @Madhu-1

@nb-ohad i dont agree to delete the resource when its not required, we do have update for that case, these resources have special meaning we should use what is required when instead of having a common case to delete and recreate always if there is a change but it doesn't require to be deleted.

@Madhu-1 This is going to be an evergrowing pit of bugs, even the current implementation is missing very important edge cases. I am not predicting too many label, and annotation changes over the lifetime of these resources and want to avoid very common eng mistakes that result in non-so-simple bugs when an Eng touches this code.

I believe it's pretty straightforward because we already know what will change and what will not change. However, I haven't reviewed this part of the code thoroughly, so I may be missing some edge cases. My suggestion is to update the metadata if it changes, and to use delete/create for anything else. If you believe there are potential issues, we can resort to using delete/create, but I still prefer updating in certain cases.

Changed back to no update.

nb-ohad · 2024-06-20T10:53:42Z

controllers/storageclaim_controller.go

@@ -651,6 +673,70 @@ func (r *StorageClaimReconciler) hasVolumeSnapshotContents() (bool, error) {
 	return false, nil
 }

+func (r *StorageClaimReconciler) getCephDriverVolumeGroupSnapshotClass(driverType string,


Can we please move this function near the getCephDriverVolumeSnapshotClass function? both of them have similar concerns and I would prefer to have both of them at a similar location in the file

nb-ohad · 2024-06-20T10:59:08Z

controllers/storageclaim_controller.go

+	if err := r.own(volumeGroupSnapshotClass); err != nil {
+		return fmt.Errorf("failed to own volumegroupsnapshotclass: %v", err)
+	}


Isn't volumeGroupSnapshotClass (and volumeSnapshotClass) a cluster-wide resource? In that case, you cannot own it by a namespace scoped resource (StorageClaim resource). We are already working around that using the storage claim annotation. So let's remove this own call.

nb-ohad · 2024-06-20T10:59:19Z

controllers/storageclaim_controller.go

+func (r *StorageClaimReconciler) createOrReplaceVolumeGroupSnapshotClass(volumeGroupSnapshotClass *groupsnapapi.VolumeGroupSnapshotClass) error {
+	existing := &groupsnapapi.VolumeGroupSnapshotClass{}
+	existing.Name = r.storageClaim.Name
+
+	if err := r.own(volumeGroupSnapshotClass); err != nil {
+		return fmt.Errorf("failed to own volumegroupsnapshotclass: %v", err)
+	}
+
+	if err := r.get(existing); err != nil && !errors.IsNotFound(err) {
+		return fmt.Errorf("failed to get VolumeGroupSnapshotClass: %v", err)
+	}
+
+	// If present then compare the existing VolumeGroupSnapshotClass parameters and labels with
+	// the received VolumeGroupSnapshotClass parameters and labels, and only proceed if they differ.
+	labelsEqual := reflect.DeepEqual(existing.Labels, volumeGroupSnapshotClass.Labels)
+	if reflect.DeepEqual(existing.Parameters, volumeGroupSnapshotClass.Parameters) && labelsEqual {
+		return nil
+	}
+
+	if labelsEqual {
+		// VolumeGroupSnapshotClass already exists, but parameters have changed. Delete the existing VolumeGroupSnapshotClass and create a new one.
+		if existing.UID != "" {
+			// Since we have to update the existing VolumeGroupSnapshotClass, so we will delete the existing VolumeGroupSnapshotClass and create a new one.
+			r.log.Info("VolumeGroupSnapshotClass needs to be updated, deleting it.", "Name", existing.Name)
+
+			// Delete the VolumeGroupSnapshotClass.
+			if err := r.delete(existing); err != nil {
+				r.log.Error(err, "Failed to delete VolumeGroupSnapshotClass.", "Name", existing.Name)
+				return fmt.Errorf("failed to delete VolumeGroupSnapshotClass: %v", err)
+			}
+		}
+		r.log.Info("Creating VolumeGroupSnapshotClass.", "Name", existing.Name)
+		if err := r.Client.Create(r.ctx, volumeGroupSnapshotClass); err != nil {
+			return fmt.Errorf("failed to create VolumeGroupSnapshotClass: %v", err)
+		}
+	} else {
+		r.log.Info("Updating VolumeGroupSnapshotClass", "Name", existing.Name)
+		if err := r.Client.Update(r.ctx, volumeGroupSnapshotClass); err != nil {
+			return fmt.Errorf("failed to update VolumeGroupSnapshotClass: %v", err)
+		}
+	}
+
+	return nil
+}


This function is almost an identical duplication of the createOrReplaceVolumeSnapshotClass.
Instead of creating a duplicate where Bugz might be repeating, can we please generalize to a
func (r *StorageClaimReconciler) createOrReplace(client object.Client) error

controllers/storageclaim_controller.go

Madhu-1

some nits, LGTM

Madhu-1 · 2024-07-01T11:33:37Z

controllers/storageclaim_controller.go

@@ -207,6 +219,20 @@ func (r *StorageClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	return result, nil
 }

+func (r *StorageClaimReconciler) mapCRDAvailability(crdNames ...string) map[string]bool {


Do we need to make crdNames as input? instead of that can we have it as var inside this method only?

What do you mean? We need to be able to send to this function all the CRD names that need to go through this flow path.

Madhu-1 · 2024-07-01T11:34:43Z

controllers/storageclaim_controller.go

+	var driverName string
+	if driverType == "cephfs" {
+		driverName = csi.GetCephFSDriverName()
+	} else {


can you please add the case of rbd here?

Madhu-1 · 2024-07-01T11:35:20Z

controllers/storageclaim_controller.go

-	if err != nil {
-		return fmt.Errorf("failed to create StorageClass: %v", err)
+	r.log.Info("Creating "+classKind+".", "Name", types.NamespacedName{
+		Namespace: class.GetNamespace(),


namespace is not required as its clusterScoped resouces

And how about for StoageClass? Seems like it did have its namespace mentioned in the logs:

r.log.Info("StorageClass needs to be updated, deleting it.", "StorageClass", klog.KRef(storageClass.Namespace, existing.Name))

rewantsoni · 2024-07-01T12:00:34Z

controllers/storageclaim_controller.go

-func (r *StorageClaimReconciler) createOrReplaceStorageClass(storageClass *storagev1.StorageClass) error {
-	existing := &storagev1.StorageClass{}
-	existing.Name = r.storageClaim.Name
+func (r *StorageClaimReconciler) createOrReplaceClass(class client.Object) error {


Shouldn't we get the existing before the switch case? existingParameters will always be empty in this case.
We should,

set the name to existing, get it based on the object kind

compare the existing parameter/labels/annotations to the desired

if equal break

own the desired

if UID is not nil delete it

create the desired

Could we also rename class client.Object to a better name? maybe to desired?

I like class since they're all classes

rewantsoni · 2024-07-01T12:01:19Z

controllers/storageclaim_controller.go

@@ -207,6 +218,20 @@ func (r *StorageClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	return result, nil
 }

+func (r *StorageClaimReconciler) mapCRDAvailability(crdNames ...string) map[string]bool {


maybe move this to a utility package?

Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>

openshift-ci bot added the do-not-merge/work-in-progress label Jun 3, 2024

pull-request-size bot added the size/M label Jun 3, 2024

raaizik force-pushed the RHSTOR-5795 branch 4 times, most recently from d68c5d2 to 7e993b3 Compare June 3, 2024 13:59

pull-request-size bot added size/L and removed size/M labels Jun 3, 2024

raaizik force-pushed the RHSTOR-5795 branch from 7e993b3 to 15b46ec Compare June 3, 2024 14:00

pull-request-size bot added size/M and removed size/L labels Jun 3, 2024

raaizik force-pushed the RHSTOR-5795 branch 2 times, most recently from 38c58d7 to b148266 Compare June 4, 2024 10:21

raaizik changed the title ~~[WIP] Generates VolumeGroupSnapshotClass~~ [WIP] Generates and labels Volume*SnapshotClass Jun 4, 2024

raaizik changed the title ~~[WIP] Generates and labels Volume*SnapshotClass~~ Generates and labels Volume*SnapshotClass Jun 4, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 4, 2024

openshift-ci bot requested a review from rewantsoni June 4, 2024 13:50

raaizik force-pushed the RHSTOR-5795 branch from b148266 to da0d4b5 Compare June 4, 2024 14:53

raaizik changed the title ~~Generates and labels Volume*SnapshotClass~~ [WIP] Generates and labels Volume*SnapshotClass Jun 5, 2024

openshift-ci bot added the do-not-merge/work-in-progress label Jun 5, 2024

raaizik changed the title ~~[WIP] Generates and labels Volume*SnapshotClass~~ Generates and labels Volume*SnapshotClass Jun 6, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 6, 2024

raaizik force-pushed the RHSTOR-5795 branch 2 times, most recently from 63d1586 to 0af9ddc Compare June 6, 2024 13:04

raaizik mentioned this pull request Jun 9, 2024

Sends VolumeGroupSnapshotClass to client red-hat-storage/ocs-operator#2661

Open

rewantsoni suggested changes Jun 10, 2024

View reviewed changes

openshift-ci bot assigned rewantsoni Jun 10, 2024

raaizik force-pushed the RHSTOR-5795 branch from 0af9ddc to 099b7a8 Compare June 10, 2024 07:51

pull-request-size bot removed the size/M label Jun 10, 2024

raaizik force-pushed the RHSTOR-5795 branch 7 times, most recently from d3c6028 to bc8957d Compare June 17, 2024 07:58

raaizik requested a review from Madhu-1 June 17, 2024 08:01

raaizik commented Jun 17, 2024

View reviewed changes

raaizik force-pushed the RHSTOR-5795 branch from 53b7268 to 5a84b64 Compare June 17, 2024 09:02

raaizik requested a review from leelavg June 17, 2024 09:09

nb-ohad requested changes Jun 20, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Jun 20, 2024

raaizik force-pushed the RHSTOR-5795 branch from 69d1b79 to a89cad2 Compare June 25, 2024 13:33

raaizik requested a review from nb-ohad June 25, 2024 13:39

raaizik force-pushed the RHSTOR-5795 branch 4 times, most recently from ff475ee to 4d46063 Compare July 1, 2024 09:48

Madhu-1 reviewed Jul 1, 2024

View reviewed changes

controllers/storageclaim_controller.go Outdated Show resolved Hide resolved

controllers/storageclaim_controller.go Outdated Show resolved Hide resolved

controllers/storageclaim_controller.go Outdated Show resolved Hide resolved

controllers/storageclaim_controller.go Show resolved Hide resolved

raaizik force-pushed the RHSTOR-5795 branch 2 times, most recently from c042c9c to fc5325c Compare July 1, 2024 11:29

raaizik requested a review from Madhu-1 July 1, 2024 11:30

Madhu-1 reviewed Jul 1, 2024

View reviewed changes

raaizik force-pushed the RHSTOR-5795 branch from fc5325c to a7f5fbe Compare July 1, 2024 11:50

rewantsoni suggested changes Jul 1, 2024

View reviewed changes

raaizik added 2 commits July 1, 2024 15:22

controllers: Generates VGSClass

e18b7d9

Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>

godeps: upgrade go, cr, cg for k8s-csi

d29d96b

Signed-off-by: raaizik <132667934+raaizik@users.noreply.github.com>

raaizik force-pushed the RHSTOR-5795 branch from a7f5fbe to d29d96b Compare July 1, 2024 12:22

raaizik requested a review from Madhu-1 July 1, 2024 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generates and labels `Volume*SnapshotClass` #168

Generates and labels `Volume*SnapshotClass` #168

raaizik commented Jun 3, 2024 •

edited

Loading

raaizik commented Jun 4, 2024

rewantsoni Jun 10, 2024

rewantsoni Jun 10, 2024

raaizik Jun 17, 2024 •

edited

Loading

nb-ohad Jun 20, 2024

nb-ohad Jun 20, 2024

nb-ohad Jun 20, 2024

nb-ohad Jun 20, 2024

raaizik Jun 20, 2024 •

edited

Loading

nb-ohad Jun 20, 2024 •

edited by leelavg

Loading

nb-ohad Jun 20, 2024

raaizik Jun 20, 2024 •

edited

Loading

Madhu-1 Jun 24, 2024

nb-ohad Jun 25, 2024

Madhu-1 Jun 25, 2024

raaizik Jun 25, 2024

nb-ohad Jun 20, 2024

nb-ohad Jun 20, 2024

nb-ohad Jun 20, 2024

Madhu-1 left a comment

Madhu-1 Jul 1, 2024

raaizik Jul 1, 2024

Madhu-1 Jul 1, 2024

Madhu-1 Jul 1, 2024

raaizik Jul 1, 2024

rewantsoni Jul 1, 2024 •

edited

Loading

raaizik Jul 1, 2024

rewantsoni Jul 1, 2024 •

edited

Loading

	storageID := getMD5Hash(string(r.storageClaim.UID)) + r.storageClaimHash
	storageID := r.storageClaim.UID

		labelsEqual := reflect.DeepEqual(existing.Labels, volumeSnapshotClass.Labels)
		if reflect.DeepEqual(existing.Parameters, volumeSnapshotClass.Parameters) && labelsEqual {

-	labelsEqual := reflect.DeepEqual(existing.Labels, volumeSnapshotClass.Labels)
-	if reflect.DeepEqual(existing.Parameters, volumeSnapshotClass.Parameters) && labelsEqual {
+	areLabelsEqual := eflect.DeepEqual(existing.Labels, volumeSnapshotClass.Labels)
+	areAnnotationsEqual := eflect.DeepEqual(existing.Annotations, volumeSnapshotClass.Annotations)
+	areParamsEqual := reflect.DeepEqual(existing.Parameters, volumeSnapshotClass.Parameters)
+	if areLabelsEqual && areAnnotationsEqual && areParamsEqual {

Generates and labels Volume*SnapshotClass #168

Are you sure you want to change the base?

Generates and labels Volume*SnapshotClass #168

Conversation

raaizik commented Jun 3, 2024 • edited Loading

Changes

raaizik commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raaizik Jun 17, 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

raaizik Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

nb-ohad Jun 20, 2024 • edited by leelavg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raaizik Jun 20, 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

Choose a reason for hiding this comment

Madhu-1 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

Choose a reason for hiding this comment

rewantsoni Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewantsoni Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Generates and labels `Volume*SnapshotClass` #168

Generates and labels `Volume*SnapshotClass` #168

raaizik commented Jun 3, 2024 •

edited

Loading

raaizik Jun 17, 2024 •

edited

Loading

raaizik Jun 20, 2024 •

edited

Loading

nb-ohad Jun 20, 2024 •

edited by leelavg

Loading

raaizik Jun 20, 2024 •

edited

Loading

rewantsoni Jul 1, 2024 •

edited

Loading

rewantsoni Jul 1, 2024 •

edited

Loading