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

cephfs: remove snapshot protect/unprotect #4202

Merged
merged 2 commits into from
Nov 1, 2023
Merged

cephfs: remove snapshot protect/unprotect #4202

merged 2 commits into from
Nov 1, 2023

Conversation

iPraveenParihar
Copy link
Contributor

Describe what this PR does

This commit eliminates the code for protecting and unprotecting
snapshots, as the functionality to protect and unprotect snapshots
is being deprecated.

Is there anything that requires special attention

Is the change backward compatible? Yes

Are there concerns around backward compatibility? No

Fixes: #4169

@mergify mergify bot added the component/cephfs Issues related to CephFS label Oct 18, 2023
@iPraveenParihar
Copy link
Contributor Author

/test ci/centos/mini-e2e/k8s-1.27

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-protect-unprotect-snapshot branch 5 times, most recently from 61478a2 to 2511dd4 Compare October 27, 2023 06:47
@iPraveenParihar iPraveenParihar changed the title [WIP]cephfs: remove snapshot protect/unprotect cephfs: remove snapshot protect/unprotect Oct 27, 2023
@iPraveenParihar iPraveenParihar marked this pull request as ready for review October 27, 2023 07:25
@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-protect-unprotect-snapshot branch from 2511dd4 to 5a5e184 Compare October 30, 2023 09:14
Madhu-1
Madhu-1 reviewed Oct 30, 2023
View reviewed changes
internal/cephfs/core/clone.go
// if cloneErr is not nil we will unprotect the snapshot and delete the snapshot
var cloneErr error

defer func() {
if cloneErr != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR, if the Expand fails or the DeleteSnapshot fails we also need to remove the clone and the snapshot, which is missing, can you please address that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will address this in a separate PR.
Created a issue to track - #4218

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-protect-unprotect-snapshot branch from 5a5e184 to e688ab2 Compare October 30, 2023 10:23
@iPraveenParihar iPraveenParihar mentioned this pull request Oct 30, 2023
Closed
Madhu-1
Madhu-1 reviewed Oct 30, 2023
View reviewed changes
internal/cephfs/controllerserver.go Outdated
@@ -866,18 +852,8 @@ func (cs *ControllerServer) CreateSnapshot(

metadata := k8s.GetSnapshotMetadata(req.GetParameters())
if sid != nil {
// check snapshot is protected
protected := true
snapClient := core.NewSnapshot(parentVolOptions.GetConnection(), sid.FsSnapshotName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this client creation to inside if statement?

Copy link
Member

Choose a reason for hiding this comment

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

that won't work, snapclient is also used online 885 (a little below this if)

Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont think its used elsewhere inside the if sid the client is only used to set metadata for already found snapshot. please let me know if am missing something

Copy link
Collaborator

Choose a reason for hiding this comment

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

client creation can happen inside if len(metadata) != 0 {

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 dont think its used elsewhere inside the if sid the client is only used to set metadata for already found snapshot. please let me know if am missing something

yes, thats correct. @nixpanic mentioned the same

client creation can happen inside if len(metadata) != 0 {

I'll move under this if

nixpanic
nixpanic previously approved these changes Oct 30, 2023
View reviewed changes
internal/cephfs/controllerserver.go Outdated
@@ -866,18 +852,8 @@ func (cs *ControllerServer) CreateSnapshot(

metadata := k8s.GetSnapshotMetadata(req.GetParameters())
if sid != nil {
// check snapshot is protected
protected := true
snapClient := core.NewSnapshot(parentVolOptions.GetConnection(), sid.FsSnapshotName,
Copy link
Member

Choose a reason for hiding this comment

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

that won't work, snapclient is also used online 885 (a little below this if)

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-protect-unprotect-snapshot branch from e688ab2 to 6ee2c52 Compare October 31, 2023 09:37
@mergify mergify bot dismissed nixpanic’s stale review October 31, 2023 09:38

Pull request has been modified.

Madhu-1
Madhu-1 reviewed Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@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.

One last comment, missed that in last review 🤦🏻

@iPraveenParihar iPraveenParihar force-pushed the cephfs/remove-protect-unprotect-snapshot branch from 6ee2c52 to 1b52f60 Compare October 31, 2023 09:47
@nixpanic
Copy link
Member

@Mergifyio rebase

@iPraveenParihar @nixpanic
cephfs: remove snapshot protect/unprotect
1762ff3 
This commit eliminates the code for protecting and unprotecting
snapshots, as the functionality to protect and unprotect snapshots
is being deprecated.

Signed-off-by: Praveen M <m.praveen@ibm.com>
@iPraveenParihar @nixpanic
cleanup: removed unused SnapshotInfo from CheckSnapExists()
b2432e8 
Signed-off-by: Praveen M <m.praveen@ibm.com>
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 31, 2023

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the cephfs/remove-protect-unprotect-snapshot branch from 1b52f60 to b2432e8 Compare October 31, 2023 14:08
@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Oct 31, 2023
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Oct 31, 2023
@nixpanic
Copy link
Member

nixpanic commented Nov 1, 2023

/retest ci/centos/mini-e2e/k8s-1.26

@mergify mergify bot merged commit c09700b into ceph:devel Nov 1, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

Assignees
No one assigned
Labels
component/cephfs Issues related to CephFS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove code for Protect/Unprotect snapshot in CephFS
4 participants
@iPraveenParihar @nixpanic @ceph-csi-bot @Madhu-1