Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add VolumeGroupSnapshotClass for CephFS and RBD #2859

Merged
merged 2 commits into from
Jan 27, 2025
Merged

add VolumeGroupSnapshotClass for CephFS and RBD #2859

merged 2 commits into from
Jan 27, 2025

Conversation

ShravaniVangur
Copy link
Contributor

@ShravaniVangur ShravaniVangur commented Oct 17, 2024 •
edited
Loading

Testing creation of VolumeGroupSnapshotClass and related functionalities

(BETA Version):

  • 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/v1beta1
Deletion Policy:  Delete
Driver:           openshift-storage.cephfs.csi.ceph.com
Kind:             VolumeGroupSnapshotClass
Metadata:
  Creation Timestamp:  2025-01-22T19:19:19Z
  Generation:          1
  Resource Version:    63461
  UID:                 267d766b-5e1e-4dad-bb1c-35dcda95fcc9
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>                                           <none>
  • On creating VolumeGroupSnapshot:
NAME                     READYTOUSE   VOLUMEGROUPSNAPSHOTCLASS                         VOLUMEGROUPSNAPSHOTCONTENT                              CREATIONTIME   AGE
cephfs-groupsnapshot-01   true         ocs-storagecluster-cephfsplugin-groupsnapclass   groupsnapcontent-95ea256b-486d-4e24-9888-3d749a08ae74   15m            15m
  • 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-ec6b96ce-1b95-4e68-9a23-50d7950df0a1   1Gi        RWO            ocs-storagecluster-cephfs     <unset>                 17m
cephfs-pvc-01-restore         Bound    pvc-2f8ba30b-6e4d-4f67-8b23-fcbc8e323fcc   1Gi        RWX            ocs-storagecluster-cephfs     <unset>                 6m31s
  • Creating a pod to utilise the new PVC
NAME                  READY   STATUS    RESTARTS   AGE
csi-cephfs-vgsc-pod   1/1     Running   0          39s

@ShravaniVangur ShravaniVangur force-pushed the volgrp-snapclass branch 2 times, most recently from acdabd0 to de40e7b Compare October 18, 2024 06:15
Madhu-1
Madhu-1 suggested changes Oct 18, 2024
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

/hold

The API is not available in Beta yet.

@nixpanic PTAL

controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated Show resolved Hide resolved
controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated Show resolved Hide resolved
controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated Show resolved Hide resolved
controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated Show resolved Hide resolved
controllers/storagerequest/storagerequest_controller.go Outdated Show resolved Hide resolved
controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated Show resolved Hide resolved
@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 Oct 18, 2024
@nixpanic
Copy link
Member

The API is not available in Beta yet.

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

@Madhu-1
Copy link
Member

Madhu-1 commented Oct 23, 2024

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
Copy link
Member

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
Nikhil-Ladha reviewed Oct 24, 2024
View reviewed changes
Copy link
Member

@Nikhil-Ladha Nikhil-Ladha left a comment

Choose a reason for hiding this comment

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

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

@iamniting
Copy link
Member

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
iamniting requested changes Oct 24, 2024
View reviewed changes
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.

Pls have a generated changes in the separate commit.

@openshift-merge-robot 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 ShravaniVangur force-pushed the volgrp-snapclass branch 2 times, most recently from 3ec52f3 to 6c2c34e Compare October 24, 2024 18:14
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024
@openshift-merge-robot 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
Copy link
Contributor Author

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

iamniting
iamniting requested changes Oct 28, 2024
View reviewed changes
controllers/storagecluster/reconcile.go Outdated Show resolved Hide resolved
controllers/storagecluster/storagecluster_controller.go Outdated Show resolved Hide resolved
iamniting
iamniting requested changes Nov 11, 2024
View reviewed changes
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.

Can you pls fix tests?

Madhu-1
Madhu-1 reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM, @iamniting PTAL

controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated Show resolved Hide resolved
@iamniting
Copy link
Member

@ShravaniVangur Is the code tested? Also, is updating the GroupSnapshotClass allowed?

@ShravaniVangur ??

@ShravaniVangur
Copy link
Contributor Author

@iamniting

@ShravaniVangur Is the code tested? Also, is updating the GroupSnapshotClass allowed?

Yes I have tested the code with the beta version and have updated the results in the description.
Since the beta version is available we can update the GroupSnapshotClass.

@raaizik raaizik mentioned this pull request Jan 23, 2025
Open
2 tasks
iamniting
iamniting requested changes Jan 27, 2025
View reviewed changes
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.

Pls take a look at the failing tests

@ShravaniVangur ShravaniVangur force-pushed the volgrp-snapclass branch 4 times, most recently from e0248ac to ce65bb2 Compare January 27, 2025 08:29
@ShravaniVangur
add VolumeGroupSnapshotClass for CephFS and RBD
97e8c42 
CSI-drivers requires VolumeGroupSnapshotClass for VolumeGroupSnapshot.

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

Signed-off-by: ShravaniVangur <[email protected]>
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, nixpanic, 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 Jan 27, 2025
@ShravaniVangur
Copy link
Contributor Author

/test ocs-operator-bundle-e2e-aws

@ShravaniVangur
Copy link
Contributor Author

/remove-hold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit edb31a8 into red-hat-storage:main Jan 27, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malayparida2000 malayparida2000 malayparida2000 left review comments

@leelavg leelavg leelavg left review comments

@Madhu-1 Madhu-1 Madhu-1 requested changes

@iamniting iamniting iamniting approved these changes

@nixpanic nixpanic nixpanic approved these changes

@Nikhil-Ladha Nikhil-Ladha Awaiting requested review from Nikhil-Ladha

Assignees

@nixpanic nixpanic

@Madhu-1 Madhu-1

@iamniting iamniting

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ShravaniVangur @nixpanic @Madhu-1 @Nikhil-Ladha @iamniting @leelavg @malayparida2000 @openshift-merge-robot