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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add VolumeGroupSnapshotClass for CephFS and RBD #2859

wants to merge 2 commits into from

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?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 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 Dec 12, 2024
@ShravaniVangur ShravaniVangur force-pushed the volgrp-snapclass branch 5 times, most recently from bd9d6ce to b4de5dc Compare December 12, 2024 13:32
@iamniting
Copy link
Member

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

malayparida2000
malayparida2000 reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@malayparida2000 malayparida2000 left a comment

Choose a reason for hiding this comment

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

@ShravaniVangur Is this intended to go in 4.18? If yes we would need a Jira Bug.
If this is intended for 4.19 then if there is a Jira Story/Upstream Issue for this please attach it here.

Please mark as resolved the conversations which have been already resolved, Normally that should be done by the reviewer but here too many open conversations is making it confusing to take a final look.

@Madhu-1 still has a hold on this PR, please discuss with him about the reason & get it resolved.

@ShravaniVangur
Copy link
Contributor Author

@malayparida2000

@ShravaniVangur Is this intended to go in 4.18? If yes we would need a Jira Bug. If this is intended for 4.19 then if there is a Jira Story/Upstream Issue for this please attach it here.

Jira issues: Cephfs and RBD.

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 23, 2025
View reviewed changes
controllers/storagecluster/volumegroupsnapshotterclasses.go Outdated
for _, vgsc := range vgscs {
sc := vgsc.groupSnapshotClass
existing := groupsnapapi.VolumeGroupSnapshotClass{}
err := r.Client.Get(r.ctx, types.NamespacedName{Name: sc.Name, Namespace: sc.Namespace}, &existing)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to do a GET call? We can just make a DELETE call, and if it is not found, we can ignore it. There is no need for a GET and DELETE call.

controllers/storagecluster/volumegroupsnapshotterclasses.go
type GroupSnapshotClassConfiguration struct {
groupSnapshotClass *groupsnapapi.VolumeGroupSnapshotClass
reconcileStrategy ReconcileStrategy
disable bool
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest adding a comment for the disable field saying it was added for a future purpose. When we add an API or some other mechanism to disable the VGSC, this field will be used to control it.

controllers/storagecluster/generate.go Outdated
return "pool", generateNameForCephBlockPool(instance)
}
return "fsName", generateNameForCephFilesystem(instance)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not understand what change is expected here?

@ShravaniVangur
add VolumeGroupSnapshotClass for CephFS and RBD
b46e235 
CSI-drivers requires VolumeGroupSnapshotClass for VolumeGroupSnapshot.

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

Signed-off-by: ShravaniVangur <[email protected]>
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

@nixpanic nixpanic nixpanic approved these changes

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

@iamniting iamniting Awaiting requested review from iamniting

Requested changes must be addressed to merge this pull request.

Assignees

@nixpanic nixpanic

@Madhu-1 Madhu-1

@iamniting iamniting

Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
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