add VolumeGroupSnapshotClass for CephFS and RBD #2859

ShravaniVangur · 2024-10-17T11:28:26Z

Testing creation of VolumeGroupSnapshotClass and related functionalities:

CSI-drivers require VolumeGroupSnapshotClass for VolumeGroupSnapshot creation.

NAME                                             DRIVER                                  DELETIONPOLICY   AGE
ocs-storagecluster-cephfsplugin-groupsnapclass   openshift-storage.cephfs.csi.ceph.com   Delete           50m
ocs-storagecluster-rbdplugin-groupsnapclass      openshift-storage.rbd.csi.ceph.com      Delete           50m

Description of ocs-storagecluster-cephfsplugin-groupsnapclass:

Name:             ocs-storagecluster-cephfsplugin-groupsnapclass
Namespace:        
Labels:           <none>
Annotations:      <none>
API Version:      groupsnapshot.storage.k8s.io/v1alpha1
Deletion Policy:  Delete
Driver:           openshift-storage.cephfs.csi.ceph.com
Kind:             VolumeGroupSnapshotClass
Metadata:
  Creation Timestamp:  2024-10-23T08:18:54Z
  Generation:          1
  Resource Version:    77802
  UID:                 bd8437f3-4c7c-4307-9826-14d026ce343a
Parameters:
  Cluster ID:                                             openshift-storage
  csi.storage.k8s.io/group-snapshotter-secret-name:       rook-csi-cephfs-provisioner
  csi.storage.k8s.io/group-snapshotter-secret-namespace:  openshift-storage
  Fs Name:                                                ocs-storagecluster-cephfilesystem
Events:                                                   <none>

On creating VolumeGroupSnapshot:

NAME                     READYTOUSE   VOLUMEGROUPSNAPSHOTCLASS                         VOLUMEGROUPSNAPSHOTCONTENT                              CREATIONTIME   AGE
cephfs-groupsnapshot01   true         ocs-storagecluster-cephfsplugin-groupsnapclass   groupsnapcontent-e5a91abd-52ae-4dac-a6d2-6ad1a3f4f524   7m47s          37m

Restoring CephFS volumegroupsnapshot to a new PVC (cephfs-pvc-01-restore here):

NAME                          STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS                  VOLUMEATTRIBUTESCLASS   AGE
cephfs-pvc-01                 Bound    pvc-29d36e86-62bc-4b24-b7e2-359b6a522c6b   1Gi        RWO            ocs-storagecluster-cephfs     <unset>                 56m
cephfs-pvc-01-restore         Bound    pvc-58173f95-bf0c-456d-9636-71e39783a16f   1Gi        RWX            ocs-storagecluster-cephfs     <unset>                 9s

Creating a pod to utilise the new PVC

NAME                   READY   STATUS    RESTARTS   AGE
csi-cephfs-vgsc-test   1/1     Running   0          113s

Madhu-1

/hold

The API is not available in Beta yet.

@nixpanic PTAL

Madhu-1 · 2024-10-18T06:59:17Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+	disable            bool
+}
+
+var driverName, driverValue string


any specific to have global variable?

No, changing them to local variables.

Madhu-1 · 2024-10-18T07:01:17Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+				break
+			}
+
+			r.Log.Info("Uninstall: Deleting GroupSnapshotClass.", "GroupSnapshotClass", klog.KRef(existing.Namespace, existing.Name))


we dont need to log Namespace for groupsnapshot as its a cluster scoped resouces

This is applicable for all the places where we are logging the groupsnapshot name

Madhu-1 · 2024-10-18T07:03:13Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+
+		vsc := vscc.groupSnapshotClass
+		existing := &groupsnapapi.VolumeGroupSnapshotClass{}
+		err := r.Client.Get(context.TODO(), types.NamespacedName{Name: vsc.Name, Namespace: vsc.Namespace}, existing)


instead of context.TODO use the context inside StorageClusterReconciler r.ctx

Madhu-1 · 2024-10-18T07:04:26Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+		case errors.IsNotFound(err):
+			r.Log.Info("Uninstall: GroupSnapshotClass not found, nothing to do.", "GroupSnapshotClass", klog.KRef(sc.Namespace, sc.Name))
+		default:
+			r.Log.Error(err, "Uninstall: Error while getting GroupSnapshotClass.", "GroupSnapshotClass", klog.KRef(sc.Namespace, sc.Name))


dont we need to retry if there is any deletion error? i see we are returning nil at the end of this function.

Yes adding that in the switch case

Madhu-1 · 2024-10-18T07:05:56Z

controllers/storagerequest/storagerequest_controller.go

@@ -186,6 +188,7 @@ func (r *StorageRequestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(&rookCephv1.CephClient{}, enqueueForOwner).
 		Watches(&storagev1.StorageClass{}, enqueueStorageConsumerRequest).
 		Watches(&snapapi.VolumeSnapshotClass{}, enqueueStorageConsumerRequest).
+		Watches(&groupsnapapi.VolumeGroupSnapshotClass{}, enqueueStorageConsumerRequest).


i think we dont need snapshot,groupsnapshot,pv,storageclass permission in storagerequest_controller. @leelavg can you please confirm it?

hmm, I didn't get the GH notification 🤔. Anyways, yes, we don't watch for these resources and need the struct to be encoded in gRPC response.

Just rechecked existing code and I don't think anything necessary for provider mode. do we need more info than below inaddition to changes to storageclaimcontroller in https://github.com/red-hat-storage/ocs-client-operator/pull/168/files?

ocs-operator/services/provider/server/server.go

Lines 730 to 735 in 1ea3ed4

&pb.ExternalResource{

Name: "ceph-rbd",

Kind: "VolumeGroupSnapshotClass",

Data: mustMarshal(map[string]string{

"csi.storage.k8s.io/group-snapshotter-secret-name": provisionerSecretName,

})},

Madhu-1 · 2024-10-18T07:07:46Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+		},
+		Driver: generateNameForSnapshotClassDriver(SnapshotterType(groupSnaphotterType)),
+		Parameters: map[string]string{
+			"clusterID":                     instance.Namespace,


where are we setting filesystem name and blockpool in the class?

it is set by the driverName and driverValue variables whose values are assigned by the setParameterBasedOnSnapshotterType function.

Right, this confused me a bit too, it is well hidden. driverName is not really a very suitable name, as the parameter is called fsName for CephFS or pool for RBD. But, I expect this to work correctly. Having a test that validates these kind of parameters would be nice to have.

nixpanic · 2024-10-18T15:36:52Z

The API is not available in Beta yet.

See kubernetes-csi/external-snapshotter#1150 for the BETA status PR.

Madhu-1 · 2024-10-23T13:30:39Z

we might need to have a VGSC CRD check as ocs-operator 4.18 needs to run on OCP 4.16 without any problem for EUS to EUS upgrade? @iamniting @Nikhil-Ladha thoughts?

Nikhil-Ladha · 2024-10-24T05:45:29Z

metrics/vendor/github.com/red-hat-storage/ocs-operator/v4/controllers/util/csi_options.go

 		return "ms_mode=secure"
 	}

-	// If Encryption is not enabled, but Compression or RequireMsgr2 is enabled, use prefer-crc mode
-	if sc.Spec.Network != nil && sc.Spec.Network.Connections != nil &&
-		((sc.Spec.Network.Connections.Compression != nil && sc.Spec.Network.Connections.Compression.Enabled) ||
-			sc.Spec.Network.Connections.RequireMsgr2) {
-		return "ms_mode=prefer-crc"
-	}
-
-	// Network spec always has higher precedence even in the External or Provider cluster. so they are checked first above
-
-	// None of Encryption, Compression, RequireMsgr2 are enabled on the StorageCluster
-	// If it's an External or Provider cluster, We don't require msgr2 by default so no mount options are needed
-	if sc.Spec.ExternalStorage.Enable || sc.Spec.AllowRemoteStorageConsumers {
-		return "ms_mode=legacy"
-	}
-	// If none of the above cases apply, We set RequireMsgr2 true by default on the cephcluster
-	// so we need to set the mount options to prefer-crc
+	// If encryption is not enabled, use prefer-crc mode


I think this auto-generated file change is already merged, please rebase your PR once to fix this.

Nikhil-Ladha · 2024-10-24T05:47:30Z

we might need to have a VGSC CRD check as ocs-operator 4.18 needs to run on OCP 4.16 without any problem for EUS to EUS upgrade? @iamniting @Nikhil-Ladha thoughts?

Yep, as going forward the plan will be updgrade ODF first, it is advisable to have these checks in place for new changes.
We can make use of the availCRD check that have been recently implemented to check for the CRD, before trying to create the SC.

Nikhil-Ladha

Break the PR into multiple commits, where as keep the generated changes into 1 commit, and the code changes into another.

iamniting · 2024-10-24T06:00:35Z

we might need to have a VGSC CRD check as ocs-operator 4.18 needs to run on OCP 4.16 without any problem for EUS to EUS upgrade? @iamniting @Nikhil-Ladha thoughts?

Yep, as going forward the plan will be updgrade ODF first, it is advisable to have these checks in place for new changes. We can make use of the availCRD check that have been recently implemented to check for the CRD, before trying to create the SC.

I agree we should have such checks, But let's not use availCrds If we are not watching the resource. Otherwise, our controller may restart. Lets have a plain check.

iamniting

Pls have a generated changes in the separate commit.

ShravaniVangur · 2024-10-24T18:36:07Z

I have added a plain check for VGSC CRD as well. Please do review it. @Madhu-1 @iamniting @Nikhil-Ladha

iamniting · 2024-10-28T11:58:44Z

controllers/storagecluster/reconcile.go

+			if vgsc {
+				objs = append(objs, &ocsGroupSnapshotClass{})
+			}


The check should be inside the ensure funcs not here.

iamniting · 2024-10-28T12:00:42Z

controllers/storagecluster/storagecluster_controller.go

@@ -244,6 +245,7 @@ func (r *StorageClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		).
 		Watches(&storagev1.StorageClass{}, enqueueStorageClusterRequest).
 		Watches(&volumesnapshotv1.VolumeSnapshotClass{}, enqueueStorageClusterRequest).
+		Watches(&volumegroupsnapshotv1a1.VolumeGroupSnapshotClass{}, enqueueStorageClusterRequest).


We can make use of available CRD feature as we are watching the resource.

iamniting

Can you pls fix tests?

nixpanic · 2024-11-14T09:36:46Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+		},
+		Driver: generateNameForSnapshotClassDriver(SnapshotterType(groupSnaphotterType)),
+		Parameters: map[string]string{
+			"clusterID":                     instance.Namespace,


Right, this confused me a bit too, it is well hidden. driverName is not really a very suitable name, as the parameter is called fsName for CephFS or pool for RBD. But, I expect this to work correctly. Having a test that validates these kind of parameters would be nice to have.

openshift-ci · 2024-12-12T10:12:22Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2024-12-12T10:12:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nixpanic, ShravaniVangur
Once this PR has been reviewed and has the lgtm label, please ask for approval from iamniting. 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

Madhu-1 · 2024-12-16T11:15:35Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+)
+
+type GroupSnapshotterType string


no need to export this type as its not used outside?

@Madhu-1 currently no but this would mean changing all the variable types from GroupSnapshotterType to string directly in functions such as newVolumeGroupSnapshotClass and in generate.go file as well. Is this the expected format?

Suggested change

type GroupSnapshotterType string

type groupSnapshotterType string

can we do something like this as its used outside of this file?

Madhu-1 · 2024-12-16T11:16:26Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+}
+
+func newVolumeGroupSnapshotClass(instance *ocsv1.StorageCluster, groupSnaphotterType GroupSnapshotterType) *groupsnapapi.VolumeGroupSnapshotClass {
+	driverType, driverValue := setParameterBasedOnSnapshotterType(instance, groupSnaphotterType)


instead of driverType and driverValue, it should be paramKey and paramValue?

Madhu-1 · 2024-12-16T11:21:31Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+
+	err := r.createGroupSnapshotClasses(vgsc)
+	if err != nil {
+		return reconcile.Result{}, nil


it should be returning err isnt it?

Madhu-1 · 2024-12-16T11:22:46Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

+		case errors.IsNotFound(err):
+			r.Log.Info("Uninstall: GroupSnapshotClass not found, nothing to do.", "GroupSnapshotClass", klog.KRef("", sc.Name))
+		default:
+			r.Log.Error(err, "Uninstall: Error while getting GroupSnapshotClass.", "GroupSnapshotClass", klog.KRef("", sc.Name))


dont we need to return err if something fails? same problem exists in storageclass and volumesnapshot class as well. @iamniting can you please confirm this?

@Madhu-1 For the ensureCreated function the error does need to be returned. For the ensureDeleted function the error is being returned in the first case. Only when it is not found we are logging it. This could be the case when r.AvailableCrds[VolumeGroupSnapshotClassCrdName] is false.

i think in that ase if will be errors.IsNotFound not any other error, this could leave us to a case where we might leave the stale classes due to some internal API server errors or something else. i will let @iamniting to confirm it, except this one everything else LGTM

CSI-drivers requires VolumeGroupSnapshotClass for VolumeGroupSnapshot. Signed-off-by: ShravaniVangur <[email protected]>

Updates the go mod dependencies. Signed-off-by: ShravaniVangur <[email protected]>

ShravaniVangur force-pushed the volgrp-snapclass branch 2 times, most recently from acdabd0 to de40e7b Compare October 18, 2024 06:15

Madhu-1 suggested changes Oct 18, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2024

openshift-ci bot assigned Madhu-1 Oct 18, 2024

Nikhil-Ladha reviewed Oct 24, 2024

View reviewed changes

iamniting requested changes Oct 24, 2024

View reviewed changes

openshift-ci bot assigned iamniting Oct 24, 2024

ShravaniVangur force-pushed the volgrp-snapclass branch from de40e7b to be56914 Compare October 24, 2024 17:26

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 24, 2024

ShravaniVangur force-pushed the volgrp-snapclass branch 2 times, most recently from 3ec52f3 to 6c2c34e Compare October 24, 2024 18:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

ShravaniVangur force-pushed the volgrp-snapclass branch from 6c2c34e to 1a7d990 Compare October 24, 2024 18:29

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

ShravaniVangur requested review from Nikhil-Ladha, iamniting and Madhu-1 October 24, 2024 18:36

iamniting requested changes Oct 28, 2024

View reviewed changes

ShravaniVangur force-pushed the volgrp-snapclass branch from 6379d30 to 71316e5 Compare October 30, 2024 07:48

ShravaniVangur requested a review from iamniting October 30, 2024 08:36

iamniting requested changes Nov 11, 2024

View reviewed changes

ShravaniVangur force-pushed the volgrp-snapclass branch from 71316e5 to 7b37de4 Compare November 13, 2024 05:27

ShravaniVangur force-pushed the volgrp-snapclass branch from 7b37de4 to 5a66a16 Compare November 13, 2024 05:57

ShravaniVangur requested a review from iamniting November 13, 2024 08:08

nixpanic approved these changes Nov 14, 2024

View reviewed changes

openshift-ci bot assigned nixpanic Nov 14, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2024

ShravaniVangur force-pushed the volgrp-snapclass branch from 42781ed to dba5744 Compare December 12, 2024 10:21

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2024

ShravaniVangur force-pushed the volgrp-snapclass branch from dba5744 to 58917de Compare December 12, 2024 10:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2024

ShravaniVangur force-pushed the volgrp-snapclass branch 5 times, most recently from bd9d6ce to b4de5dc Compare December 12, 2024 13:32

Madhu-1 reviewed Dec 16, 2024

View reviewed changes

ShravaniVangur added 2 commits December 17, 2024 17:10

add VolumeGroupSnapshotClass for CephFS and RBD

bedc668

CSI-drivers requires VolumeGroupSnapshotClass for VolumeGroupSnapshot. Signed-off-by: ShravaniVangur <[email protected]>

update go mod dependencies

1b08d24

Updates the go mod dependencies. Signed-off-by: ShravaniVangur <[email protected]>

ShravaniVangur force-pushed the volgrp-snapclass branch from b4de5dc to 1b08d24 Compare December 17, 2024 11:41

	&pb.ExternalResource{
	Name: "ceph-rbd",
	Kind: "VolumeGroupSnapshotClass",
	Data: mustMarshal(map[string]string{
	"csi.storage.k8s.io/group-snapshotter-secret-name": provisionerSecretName,
	})},

	type GroupSnapshotterType string
	type groupSnapshotterType string

add VolumeGroupSnapshotClass for CephFS and RBD #2859

Are you sure you want to change the base?

add VolumeGroupSnapshotClass for CephFS and RBD #2859

Conversation

ShravaniVangur commented Oct 17, 2024 • edited Loading

Testing creation of VolumeGroupSnapshotClass and related functionalities:

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

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

nixpanic commented Oct 18, 2024

Madhu-1 commented Oct 23, 2024

Choose a reason for hiding this comment

Nikhil-Ladha commented Oct 24, 2024

Nikhil-Ladha left a comment

Choose a reason for hiding this comment

iamniting commented Oct 24, 2024

iamniting left a comment

Choose a reason for hiding this comment

ShravaniVangur commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 12, 2024

openshift-ci bot commented Dec 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShravaniVangur commented Oct 17, 2024 •

edited

Loading