Skip to content

add private volumeGroupSnapshotClass Api #3231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

ShravaniVangur
Copy link
Contributor

@ShravaniVangur ShravaniVangur commented May 16, 2025

Updating the ocs-operator to import the private volumegroupsnapshotclass api,enabling access to and utilization of the private fork of the sidecar available at https://github.com/red-hat-storage/external-snapshotter.

VolumeGroupSnapshotClass Creation PR: Ocs-Client-Operator

Result:

NAME                                       DRIVER                                  DELETIONPOLICY   AGE
ocs-storagecluster-cephfs-groupsnapclass   [openshift-storage.cephfs.csi.ceph.com](http://openshift-storage.cephfs.csi.ceph.com/)   Delete           13s

With featuregate enabled:

oc get volumegroupsnapshotclass.groupsnapshot.storage.openshift.io
NAME                                       DRIVER                                  DELETIONPOLICY   AGE
ocs-storagecluster-cephfs-groupsnapclass   openshift-storage.cephfs.csi.ceph.com   Delete           12m

oc get volumegroupsnapshotclass.groupsnapshot.storage.k8s.io      
NAME                                         DRIVER                                  DELETIONPOLICY   AGE
ocs-storagecluster-ceph-rbd-groupsnapclass   openshift-storage.rbd.csi.ceph.com      Delete           80s
ocs-storagecluster-cephfs-groupsnapclass     openshift-storage.cephfs.csi.ceph.com   Delete           80s
Name:             ocs-storagecluster-cephfs-groupsnapclass
Namespace:
Labels:           [ramendr.openshift.io/storageid=4b6a45a67974c61dea673244ea12f915](http://ramendr.openshift.io/storageid=4b6a45a67974c61dea673244ea12f915)
Annotations:      <none>
API Version:      [groupsnapshot.storage.openshift.io/v1beta1](http://groupsnapshot.storage.openshift.io/v1beta1)
Deletion Policy:  Delete
Driver:           [openshift-storage.cephfs.csi.ceph.com](http://openshift-storage.cephfs.csi.ceph.com/)
Kind:             VolumeGroupSnapshotClass
Metadata:
  Creation Timestamp:  2025-06-16T18:12:03Z
  Generation:          1
  Owner References:
    API Version:           [ocs.openshift.io/v1alpha1](http://ocs.openshift.io/v1alpha1)
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  StorageClient
    Name:                  ocs-storagecluster
    UID:                   55c8cd31-df79-4193-91c1-6b818a08e979
  Resource Version:        463031
  UID:                     469c1d49-b433-4275-842c-bf75f86db3ff
Parameters:
  Cluster ID:                                             openshift-storage
  [csi.storage.k8s.io/group-snapshotter-secret-name](http://csi.storage.k8s.io/group-snapshotter-secret-name):       cephfs-provisioner-1b5eca0f-5ab0-4272-bd1a-3c83536dfeee
  [csi.storage.k8s.io/group-snapshotter-secret-namespace](http://csi.storage.k8s.io/group-snapshotter-secret-namespace):  openshift-storage
  Fs Name:                                                ocs-storagecluster-cephfilesystem
Events:                                                   <none>

Copy link
Contributor

openshift-ci bot commented May 16, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2025
@ShravaniVangur ShravaniVangur force-pushed the rhstor7247-ocs branch 3 times, most recently from 62b40c0 to e245e10 Compare May 28, 2025 12:48
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2025
@ShravaniVangur ShravaniVangur force-pushed the rhstor7247-ocs branch 4 times, most recently from c18b915 to afdefc9 Compare June 10, 2025 12:50
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2025
@ShravaniVangur ShravaniVangur force-pushed the rhstor7247-ocs branch 3 times, most recently from bc4165b to d9e9f1f Compare June 10, 2025 16:52
@ShravaniVangur ShravaniVangur marked this pull request as ready for review June 11, 2025 07:00
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2025
@ShravaniVangur
Copy link
Contributor Author

/hold For testing

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2025
@ShravaniVangur ShravaniVangur force-pushed the rhstor7247-ocs branch 7 times, most recently from fbd7319 to 74829d3 Compare June 17, 2025 02:31
@ShravaniVangur
Copy link
Contributor Author

/unhold

Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second commit is not about the vendor changes only, It is about all generated changes. I would like to see them separate if possible.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2025

resources := getKubeResourcesForClass(
consumer.Spec.VolumeGroupSnapshotClasses,
"VolumeGroupSnapshotClass",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have the whole class display name here?volumegroupsnapshotclass.groupsnapshot.storage.openshift.io.

@@ -117,6 +117,33 @@ var (
},
}
}
createOdfVolumeGroupSnapshotClassCRD = func() *extv1.CustomResourceDefinition {
pluralName := "volumegroupsnapshotclasses"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are using this only once, we can hardcode it

Copy link
Contributor

@leelavg leelavg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as controllers/storagecluster/odfvolumegroupsnapshotterclasses.go is copy of existing similar code not leaving comments on that to behave the same way of public vgsc reconciliation.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2025
@leelavg
Copy link
Contributor

leelavg commented Jul 17, 2025

@ShravaniVangur there is a actual test failure in CI, ptal.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2025
@ShravaniVangur
Copy link
Contributor Author

/test ocs-operator-bundle-e2e-aws

@agarwal-mudit agarwal-mudit added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2025
@leelavg
Copy link
Contributor

leelavg commented Jul 22, 2025

based on offline discussion, below is the required fix

diff --git a/services/provider/server/server.go b/services/provider/server/server.go
index f1bdf9c6c..744bd17b9 100644
--- a/services/provider/server/server.go
+++ b/services/provider/server/server.go
@@ -2079,7 +2079,7 @@ func getKubeResourcesForClass[T CommonClassSpecAccessors](
                klog.Warningf("Encountered unsupported provisioner in %s: %s", classDisplayName, srcName)
            } else if errors.Is(err, util.UnsupportedDriver) {
                klog.Warningf("Encountered unsupported driver in %s: %s", classDisplayName, srcName)
-           } else if srcKubeObj == nil {
+           } else if reflect.ValueOf(srcKubeObj).IsNil() {
                klog.Warningf("The name %s does not points to a builtin or an existing %s, skipping", classDisplayName, srcName)
            } else if srcKubeObj.GetLabels()[util.ExternalClassLabelKey] == "true" {
                klog.Warningf("The %s is an external %s, skipping", srcName, classDisplayName)
@@ -2087,7 +2087,7 @@ func getKubeResourcesForClass[T CommonClassSpecAccessors](
                srcClassCache[srcName] = srcKubeObj
            }
        }
-       if srcKubeObj != nil {
+       if _, exist := srcClassCache[srcName]; exist {
            distKubeObj := srcKubeObj.DeepCopyObject().(client.Object)
            distKubeObj.SetName(destName)
            kubeResources = append(kubeResources, distKubeObj)

RC is very well explained in https://www.jerf.org/iri/post/2957/

"a nil interface" and "an interface containing nil" are not the same nil. One nil is of the interface's type, and the other nil is contained in the interface but has a specific other type that is not the interface.

Creates the resources for  private vgsc api.

Signed-off-by: ShravaniVangur <[email protected]>
Vendor changes

Signed-off-by: ShravaniVangur <[email protected]>
Signed-off-by: ShravaniVangur <[email protected]>
@leelavg
Copy link
Contributor

leelavg commented Jul 23, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2025
Copy link
Contributor

openshift-ci bot commented Jul 23, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, leelavg, ShravaniVangur

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit e9e6230 into red-hat-storage:main Jul 23, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants