cephfs: remove snapshot protect/unprotect #4202
Conversation
|
/test ci/centos/mini-e2e/k8s-1.27 |
iPraveenParihar
force-pushed
the
cephfs/remove-protect-unprotect-snapshot
branch
5 times, most recently
from
61478a2 to
2511dd4
Compare
iPraveenParihar
changed the title
iPraveenParihar
force-pushed
the
cephfs/remove-protect-unprotect-snapshot
branch
from
2511dd4 to
5a5e184
Compare
| // if cloneErr is not nil we will unprotect the snapshot and delete the snapshot | ||
| var cloneErr error | ||
|
|
||
| defer func() { | ||
| if cloneErr != nil { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Will address this in a separate PR.
Created a issue to track - #4218
iPraveenParihar
force-pushed
the
cephfs/remove-protect-unprotect-snapshot
branch
from
5a5e184 to
e688ab2
Compare
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, | |||
There was a problem hiding this comment.
Move this client creation to inside if statement?
There was a problem hiding this comment.
that won't work, snapclient is also used online 885 (a little below this if)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
client creation can happen inside if len(metadata) != 0 {
There was a problem hiding this comment.
i dont think its used elsewhere inside the
if sidthe 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
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, | |||
There was a problem hiding this comment.
that won't work, snapclient is also used online 885 (a little below this if)
iPraveenParihar
force-pushed
the
cephfs/remove-protect-unprotect-snapshot
branch
from
e688ab2 to
6ee2c52
Compare
mergify
bot
dismissed
nixpanic’s stale review
Pull request has been modified.
iPraveenParihar
force-pushed
the
cephfs/remove-protect-unprotect-snapshot
branch
from
6ee2c52 to
1b52f60
Compare
|
@Mergifyio rebase |
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>
Signed-off-by: Praveen M <m.praveen@ibm.com>
✅ Branch has been successfully rebased |
nixpanic
force-pushed
the
cephfs/remove-protect-unprotect-snapshot
branch
from
1b52f60 to
b2432e8
Compare
|
/test ci/centos/k8s-e2e-external-storage/1.27 |
|
/test ci/centos/k8s-e2e-external-storage/1.26 |
|
/test ci/centos/k8s-e2e-external-storage/1.28 |
|
/test ci/centos/mini-e2e-helm/k8s-1.27 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e-helm/k8s-1.26 |
|
/test ci/centos/mini-e2e-helm/k8s-1.28 |
|
/test ci/centos/mini-e2e/k8s-1.27 |
|
/test ci/centos/mini-e2e/k8s-1.26 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e/k8s-1.28 |
|
/retest ci/centos/mini-e2e/k8s-1.26 |
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