Generates and labels `VolumeGroupSnapshotClass` #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 to k8sutils
- 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

RHSTOR-5795

TODO

Change to v1beta1 once available for VolumeGroupSnapshotClass #1150
Test complete workflow

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

/cc @rewantsoni

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?

rewantsoni · 2024-11-05T06:40:09Z

pkg/utils/k8sutils.go

+		return err
+	}
+
+	obj.SetResourceVersion("")


Could you add a comment reg why this is required?
something like:

k8s doesn't allow us to create objects when resourceVersion is set, as we are DeepCopying the object, the resource version also gets copied, hence we need to set it to empty before creating it

rewantsoni · 2024-11-05T06:41:10Z

internal/controller/storageclaim_controller.go

-				} else if resource.Name == "ceph-rbd" {
-					volumeSnapshotClass = r.getCephRBDVolumeSnapshotClass(data)
-				}
+				volumeSnapshotClass = r.getCephDriverVolumeSnapshotClass(resource.Labels, resource.Annotations)


Let's also set the Labels and annotation in CreateOrReplace for all resources

Madhu-1 · 2024-11-06T11:04:20Z

@raaizik can we split this in to Two PR's one for volumesnapshotclass and another one for volumegroupsnapshotclass? we can get the volumesnapshotclass PR merged and we can visit the volumegroupsnapshotclass PR once we have the feature in betav1 state.

raaizik · 2024-11-10T14:57:43Z

@raaizik can we split this in to Two PR's one for volumesnapshotclass and another one for volumegroupsnapshotclass? we can get the volumesnapshotclass PR merged and we can visit the volumegroupsnapshotclass PR once we have the feature in betav1 state.

Yes #266 @Madhu-1

internal/controller/storageclaim_controller.go

Madhu-1 · 2024-12-12T10:46:24Z

internal/controller/storageclaim_controller.go

@@ -141,6 +184,17 @@ func (r *StorageClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	r.ctx = ctrllog.IntoContext(ctx, r.log)
 	r.log.Info("Reconciling StorageClaim.")

+	crd := &metav1.PartialObjectMetadata{}
+	for _, crdName := range []string{volumeGroupSnapshotClassCrd, drClusterConfigCRDName} {


why do we have drClusterConfigCRDName isnt this PR only for VGSC?

Madhu-1 · 2024-12-12T10:48:47Z

internal/controller/storageclaim_controller.go

+						return nil
+					})
+					if err != nil {
+						return reconcile.Result{}, fmt.Errorf("failed to create or update StorageClass: %s", err)


failed to create or update StorageClass to failed to create or update VolumeGroupSnapshotClass

Madhu-1 · 2024-12-12T10:50:01Z

internal/controller/storageclaim_controller.go

+			Labels:      labels,
+			Annotations: annotations,
+		},
+		Driver:         templates.RBDDriverName,


This should be configurable as we need to support for both cephfs and rbd

Signed-off-by: raaizik <[email protected]>

openshift-ci · 2024-12-17T11:23:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: raaizik
Once this PR has been reviewed and has the lgtm label, please ask for approval from madhu-1. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rewantsoni

Could also test this PR on a cluster to see if the VGSC is created with the required fields

rewantsoni · 2024-12-18T10:40:34Z

internal/controller/storageclaim_controller.go

@@ -401,7 +454,35 @@ func (r *StorageClaimReconciler) reconcilePhases() (reconcile.Result, error) {
 					return nil
 				})
 				if err != nil {
-					return reconcile.Result{}, fmt.Errorf("failed to create or update VolumeSnapshotClass: %s", err)
+					return reconcile.Result{}, fmt.Errorf("failed to create or update StorageClass: %s", err)


Suggested change

return reconcile.Result{}, fmt.Errorf("failed to create or update StorageClass: %s", err)

return reconcile.Result{}, fmt.Errorf("failed to create or update VolumeSnapshotClass: %s", err)

You are under the VolumeSnapshotClass case here not StorageClass

rewantsoni · 2024-12-18T10:42:00Z

internal/controller/storageclaim_controller.go

+					data := map[string]string{}
+					err = json.Unmarshal(resource.Data, &data)
+					if err != nil {
+						return reconcile.Result{}, fmt.Errorf("failed to unmarshal StorageClaim configuration response: %v", err)
+					}
+					data["csi.storage.k8s.io/group-snapshotter-secret-namespace"] = r.OperatorNamespace
+					// generate a new clusterID for cephfs subvolumegroup, as
+					// storageclaim is clusterscoped resources using its
+					// hash as the clusterID
+					data["clusterID"] = r.storageClaimHash
+					driverName := templates.CephFsDriverName
+					if strings.Contains(strings.ToLower(resource.Name), "rbd") {
+						driverName = templates.RBDDriverName
+					}
+					volumeGroupSnapshotClass = r.getCephDriverVolumeGroupSnapshotClass(resource.Labels, resource.Annotations, driverName)
+					utils.AddAnnotation(volumeGroupSnapshotClass, storageClaimAnnotation, r.storageClaim.Name)


Let's move everything under CreateOrReplace

rewantsoni · 2024-12-18T10:43:11Z

internal/controller/storageclaim_controller.go

@@ -550,6 +636,20 @@ func (r *StorageClaimReconciler) getCephRBDVolumeSnapshotClass() *snapapi.Volume
 	return volumesnapshotclass
 }

+func (r *StorageClaimReconciler) getCephDriverVolumeGroupSnapshotClass(
+	labels map[string]string, annotations map[string]string, driver string) *groupsnapapi.VolumeGroupSnapshotClass {


We could add the labels/annotations directly in the CreateOrReplace

We might not need this function, we could have everything under CreateOrReplace.

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

Merged

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 2 times, most recently from ad64a3a to 15d83d2 Compare November 4, 2024 15:59

raaizik requested a review from rewantsoni November 4, 2024 16:41

rewantsoni suggested changes Nov 5, 2024

View reviewed changes

raaizik force-pushed the RHSTOR-5795 branch 2 times, most recently from 89f459e to 8865ae0 Compare November 5, 2024 13:54

raaizik requested a review from rewantsoni November 5, 2024 13:57

raaizik changed the title ~~Generates and labels Volume*SnapshotClass~~ Generates and labels VolumeSnapshotClass Nov 10, 2024

raaizik changed the title ~~Generates and labels VolumeSnapshotClass~~ Generates and labels VolumeGroupSnapshotClass Nov 10, 2024

openshift-merge-robot added the needs-rebase label Nov 12, 2024

raaizik force-pushed the RHSTOR-5795 branch from 8865ae0 to 82699b2 Compare November 12, 2024 13:33

openshift-merge-robot removed the needs-rebase label Nov 12, 2024

raaizik force-pushed the RHSTOR-5795 branch from 82699b2 to 523d103 Compare November 13, 2024 14:18

leelavg reviewed Nov 20, 2024

View reviewed changes

internal/controller/storageclaim_controller.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Nov 24, 2024

raaizik force-pushed the RHSTOR-5795 branch from 523d103 to 1652c39 Compare November 25, 2024 12:48

openshift-merge-robot removed the needs-rebase label Nov 25, 2024

raaizik force-pushed the RHSTOR-5795 branch 2 times, most recently from 6356b95 to 27dc75c Compare November 25, 2024 14:12

raaizik requested a review from leelavg November 25, 2024 14:12

raaizik force-pushed the RHSTOR-5795 branch 2 times, most recently from d1858ea to 5af8ff3 Compare December 11, 2024 10:49

Madhu-1 reviewed Dec 12, 2024

View reviewed changes

controllers: Generates VGSClass

212956b

Signed-off-by: raaizik <[email protected]>

raaizik force-pushed the RHSTOR-5795 branch from 5af8ff3 to 212956b Compare December 17, 2024 11:23

raaizik requested a review from Madhu-1 December 17, 2024 11:23

rewantsoni reviewed Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generates and labels `VolumeGroupSnapshotClass` #168

Generates and labels `VolumeGroupSnapshotClass` #168

raaizik commented Jun 3, 2024 •

edited

Loading

raaizik commented Jun 4, 2024

rewantsoni Jun 10, 2024

rewantsoni Jun 10, 2024

rewantsoni Nov 5, 2024

rewantsoni Nov 5, 2024

Madhu-1 commented Nov 6, 2024

raaizik commented Nov 10, 2024 •

edited

Loading

Madhu-1 Dec 12, 2024

Madhu-1 Dec 12, 2024

Madhu-1 Dec 12, 2024

openshift-ci bot commented Dec 17, 2024

rewantsoni left a comment

rewantsoni Dec 18, 2024

rewantsoni Dec 18, 2024

rewantsoni Dec 18, 2024

rewantsoni Dec 18, 2024

	return reconcile.Result{}, fmt.Errorf("failed to create or update StorageClass: %s", err)
	return reconcile.Result{}, fmt.Errorf("failed to create or update VolumeSnapshotClass: %s", err)

Generates and labels VolumeGroupSnapshotClass #168

Are you sure you want to change the base?

Generates and labels VolumeGroupSnapshotClass #168

Conversation

raaizik commented Jun 3, 2024 • edited Loading

Changes

TODO

raaizik commented Jun 4, 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

Madhu-1 commented Nov 6, 2024

raaizik commented Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 17, 2024

rewantsoni 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

Generates and labels `VolumeGroupSnapshotClass` #168

Generates and labels `VolumeGroupSnapshotClass` #168

raaizik commented Jun 3, 2024 •

edited

Loading

raaizik commented Nov 10, 2024 •

edited

Loading