From 5f5f78f55f0ad43bd99c93f6bd1507547824a998 Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Tue, 22 Jun 2021 18:58:17 +0530 Subject: [PATCH 01/11] fix(controller): prevent volume deletion if snapshot exists Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 26 ++++++++++++++++++++++++-- pkg/zfs/volume.go | 9 +++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 10a306a5f..8a59aeefa 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -954,7 +954,7 @@ func (cs *controller) ListVolumes( } func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) error { - volumeID := req.GetVolumeId() + volumeID := strings.ToLower(req.GetVolumeId()) if volumeID == "" { return status.Error( codes.InvalidArgument, @@ -962,7 +962,29 @@ func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) erro ) } - err := cs.validateRequest( + // volume should not be deleted if there are snapshots present for the volume + snapList, err := zfs.GetSnapshotForVolume(volumeID) + if err != nil { + return status.Errorf( + codes.NotFound, + "failed to handle delete volume request for {%s}, "+ + "validation failed checking for snapshots. Error: %s", + req.VolumeId, + err.Error(), + ) + } + + // delete is not supported if there are any snapshots present for the volume + if len(snapList.Items) != 0 { + return status.Errorf( + codes.Internal, + "failed to handle delete volume request for {%s} with %d snapshots", + req.VolumeId, + len(snapList.Items), + ) + } + + err = cs.validateRequest( csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, ) if err != nil { diff --git a/pkg/zfs/volume.go b/pkg/zfs/volume.go index 3055be9a7..49dc2694d 100644 --- a/pkg/zfs/volume.go +++ b/pkg/zfs/volume.go @@ -299,6 +299,15 @@ func GetZFSSnapshot(snapID string) (*apis.ZFSSnapshot, error) { return snap, err } +// GetSnapshotForVolume fetches all the snapshots for the given volume +func GetSnapshotForVolume(volumeID string) (*apis.ZFSSnapshotList, error) { + listOptions := metav1.ListOptions{ + LabelSelector: ZFSVolKey + "=" + volumeID, + } + snapList, err := snapbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).List(listOptions) + return snapList, err +} + // GetZFSSnapshotStatus returns ZFSSnapshot status func GetZFSSnapshotStatus(snapID string) (string, error) { getOptions := metav1.GetOptions{} From b37de6d34af8be86458a6074326d89d7e10c2ff7 Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Thu, 24 Jun 2021 13:52:13 +0530 Subject: [PATCH 02/11] fix(controller): prevent volume/snapshot deleteion if clone exists Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 66 +++++++++++++++++++++++++++++++++++----- pkg/zfs/volume.go | 31 +++++++++++++++++-- 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 8a59aeefa..7273736dc 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -788,12 +788,12 @@ func (cs *controller) DeleteSnapshot( req *csi.DeleteSnapshotRequest, ) (*csi.DeleteSnapshotResponse, error) { - if req.SnapshotId == "" { - return nil, status.Errorf(codes.InvalidArgument, "DeleteSnapshot: empty snapshotID") - } - klog.Infof("DeleteSnapshot request for %s", req.SnapshotId) + if err := cs.validateDeleteSnapshotReq(req); err != nil { + return nil, err + } + // snapshodID is formed as @ // parsing them here snapshotID := strings.Split(req.SnapshotId, "@") @@ -963,13 +963,13 @@ func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) erro } // volume should not be deleted if there are snapshots present for the volume - snapList, err := zfs.GetSnapshotForVolume(volumeID) + snapList, err := zfs.GetSnapshotsForVolume(volumeID) if err != nil { return status.Errorf( codes.NotFound, "failed to handle delete volume request for {%s}, "+ "validation failed checking for snapshots. Error: %s", - req.VolumeId, + req.GetVolumeId(), err.Error(), ) } @@ -979,17 +979,40 @@ func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) erro return status.Errorf( codes.Internal, "failed to handle delete volume request for {%s} with %d snapshots", - req.VolumeId, + req.GetVolumeId(), len(snapList.Items), ) } + // volume should not be deleted if there are clones present for the volume + cloneList, err := zfs.GetClonesForVolume(volumeID) + if err != nil { + return status.Errorf( + codes.NotFound, + "failed to handle delete volume request for {%s}, "+ + "validation failed checking for clones. Error: %s", + req.GetVolumeId(), + err.Error(), + ) + } + + // delete is not supported if there are any clones present for the volume + if len(cloneList.Items) != 0 { + return status.Errorf( + codes.Internal, + "failed to handle delete volume request for {%s} with %d clones", + req.GetVolumeId(), + len(cloneList.Items), + ) + } + err = cs.validateRequest( csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, ) if err != nil { return errors.Wrapf( err, + "failed to handle delete volume request for {%s} : validation failed", volumeID, ) @@ -997,6 +1020,35 @@ func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) erro return nil } +func (cs *controller) validateDeleteSnapshotReq(req *csi.DeleteSnapshotRequest) error { + if req.GetSnapshotId() == "" { + return status.Errorf(codes.InvalidArgument, "DeleteSnapshot: empty snapshotID") + } + + cloneList, err := zfs.GetClonesForSnapshot(req.SnapshotId) + if err != nil { + return status.Errorf( + codes.NotFound, + "failed to handle delete snapshot request for {%s}, "+ + "validation failed checking for existing clones. Error: %s", + req.GetSnapshotId(), + err.Error(), + ) + } + + // snapshot delete is not supported if clones exist for this snapshot + if len(cloneList.Items) != 0 { + return status.Errorf( + codes.Internal, + "failed to handle delete volume request for {%s} with %d clones", + req.GetSnapshotId(), + len(cloneList.Items), + ) + } + + return nil +} + // IsSupportedVolumeCapabilityAccessMode valides the requested access mode func IsSupportedVolumeCapabilityAccessMode( accessMode csi.VolumeCapability_AccessMode_Mode, diff --git a/pkg/zfs/volume.go b/pkg/zfs/volume.go index 49dc2694d..c98863aee 100644 --- a/pkg/zfs/volume.go +++ b/pkg/zfs/volume.go @@ -299,8 +299,8 @@ func GetZFSSnapshot(snapID string) (*apis.ZFSSnapshot, error) { return snap, err } -// GetSnapshotForVolume fetches all the snapshots for the given volume -func GetSnapshotForVolume(volumeID string) (*apis.ZFSSnapshotList, error) { +// GetSnapshotsForVolume fetches all the snapshots for the given volume +func GetSnapshotsForVolume(volumeID string) (*apis.ZFSSnapshotList, error) { listOptions := metav1.ListOptions{ LabelSelector: ZFSVolKey + "=" + volumeID, } @@ -308,6 +308,33 @@ func GetSnapshotForVolume(volumeID string) (*apis.ZFSSnapshotList, error) { return snapList, err } +// GetClonesForVolume lists all the clone volumes for the given volume +func GetClonesForVolume(volumeName string) (*apis.ZFSVolumeList, error) { + listOptions := metav1.ListOptions{ + LabelSelector: ZFSSrcVolKey + "=" + volumeName, + } + cloneList, err := volbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).List(listOptions) + return cloneList, err +} + +// GetClonesForSnapshot lists all the clone volumes for the given snapshot +func GetClonesForSnapshot(snapID string) (*apis.ZFSVolumeList, error) { + zfsVolList, err := volbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).List(metav1.ListOptions{}) + if err != nil { + return nil, err + } + + listBuilder := volbuilder.ListBuilderFrom(*zfsVolList) + listBuilder.WithFilter(func(vol *volbuilder.ZFSVolume) bool { + if vol.Object.Spec.SnapName == snapID { + return true + } + return false + }) + + return listBuilder.List(), nil +} + // GetZFSSnapshotStatus returns ZFSSnapshot status func GetZFSSnapshotStatus(snapID string) (string, error) { getOptions := metav1.GetOptions{} From 047bf7f3203b05c0e556e134668e12da91b371aa Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Thu, 24 Jun 2021 13:54:32 +0530 Subject: [PATCH 03/11] add changelog Signed-off-by: Akhil Mohan --- changelogs/unreleased/350-akhilerm | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/350-akhilerm diff --git a/changelogs/unreleased/350-akhilerm b/changelogs/unreleased/350-akhilerm new file mode 100644 index 000000000..b0ccdf3ff --- /dev/null +++ b/changelogs/unreleased/350-akhilerm @@ -0,0 +1 @@ +add validation to prevent volume/snapshot deletion if there is a dependent snapshot/clone \ No newline at end of file From e84d6dc3274326463bc85bfbc99ecf5ad26ebe3f Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Thu, 24 Jun 2021 14:00:38 +0530 Subject: [PATCH 04/11] fix typo Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 7273736dc..4cd437142 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -1040,7 +1040,7 @@ func (cs *controller) validateDeleteSnapshotReq(req *csi.DeleteSnapshotRequest) if len(cloneList.Items) != 0 { return status.Errorf( codes.Internal, - "failed to handle delete volume request for {%s} with %d clones", + "failed to handle delete snapshot request for {%s} with %d clones", req.GetSnapshotId(), len(cloneList.Items), ) From b0ff0ba1752810f39ee391eab07ef76ec1e9de70 Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Thu, 24 Jun 2021 14:08:50 +0530 Subject: [PATCH 05/11] short circuit expression Signed-off-by: Akhil Mohan --- pkg/zfs/volume.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/zfs/volume.go b/pkg/zfs/volume.go index c98863aee..3660fa07d 100644 --- a/pkg/zfs/volume.go +++ b/pkg/zfs/volume.go @@ -326,10 +326,7 @@ func GetClonesForSnapshot(snapID string) (*apis.ZFSVolumeList, error) { listBuilder := volbuilder.ListBuilderFrom(*zfsVolList) listBuilder.WithFilter(func(vol *volbuilder.ZFSVolume) bool { - if vol.Object.Spec.SnapName == snapID { - return true - } - return false + return vol.Object.Spec.SnapName == snapID }) return listBuilder.List(), nil From 75854f206f31fa9b0d21fa792302d0d6d07fdd2a Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Mon, 28 Jun 2021 11:06:46 +0530 Subject: [PATCH 06/11] add wait for snapshot destroy method Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 4cd437142..c95a89ccd 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -181,6 +181,21 @@ func waitForVolDestroy(volname string) error { } } +func waitForSnapDestroy(snapName string) error { + for { + _, err := zfs.GetZFSSnapshot(snapName) + if err != nil { + if k8serror.IsNotFound(err) { + return nil + } + return status.Errorf(codes.Internal, + "zfs: destroy wait failed, not able to get the snapshot %s %s", snapName, err.Error()) + } + time.Sleep(time.Second) + klog.Infof("waiting for snapshot to be destroyed %s", snapName) + } +} + func waitForReadySnapshot(snapname string) error { for { snap, err := zfs.GetZFSSnapshot(snapname) @@ -801,7 +816,9 @@ func (cs *controller) DeleteSnapshot( // should succeed when an invalid snapshot id is used return &csi.DeleteSnapshotResponse{}, nil } - if err := zfs.DeleteSnapshot(snapshotID[1]); err != nil { + + snapName := snapshotID[1] + if err := zfs.DeleteSnapshot(snapName); err != nil { return nil, status.Errorf( codes.Internal, "failed to handle DeleteSnapshot for %s, {%s}", @@ -809,6 +826,11 @@ func (cs *controller) DeleteSnapshot( err.Error(), ) } + + if err := waitForSnapDestroy(snapName); err != nil { + return nil, err + } + return &csi.DeleteSnapshotResponse{}, nil } From 67a7686aca36abd5075a9b568f343c1d817189fd Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Tue, 29 Jun 2021 14:38:16 +0530 Subject: [PATCH 07/11] remove clone validation Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 44 ---------------------------------------- pkg/zfs/volume.go | 24 ---------------------- 2 files changed, 68 deletions(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index c95a89ccd..6ea64eaa9 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -830,7 +830,6 @@ func (cs *controller) DeleteSnapshot( if err := waitForSnapDestroy(snapName); err != nil { return nil, err } - return &csi.DeleteSnapshotResponse{}, nil } @@ -1006,28 +1005,6 @@ func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) erro ) } - // volume should not be deleted if there are clones present for the volume - cloneList, err := zfs.GetClonesForVolume(volumeID) - if err != nil { - return status.Errorf( - codes.NotFound, - "failed to handle delete volume request for {%s}, "+ - "validation failed checking for clones. Error: %s", - req.GetVolumeId(), - err.Error(), - ) - } - - // delete is not supported if there are any clones present for the volume - if len(cloneList.Items) != 0 { - return status.Errorf( - codes.Internal, - "failed to handle delete volume request for {%s} with %d clones", - req.GetVolumeId(), - len(cloneList.Items), - ) - } - err = cs.validateRequest( csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, ) @@ -1047,27 +1024,6 @@ func (cs *controller) validateDeleteSnapshotReq(req *csi.DeleteSnapshotRequest) return status.Errorf(codes.InvalidArgument, "DeleteSnapshot: empty snapshotID") } - cloneList, err := zfs.GetClonesForSnapshot(req.SnapshotId) - if err != nil { - return status.Errorf( - codes.NotFound, - "failed to handle delete snapshot request for {%s}, "+ - "validation failed checking for existing clones. Error: %s", - req.GetSnapshotId(), - err.Error(), - ) - } - - // snapshot delete is not supported if clones exist for this snapshot - if len(cloneList.Items) != 0 { - return status.Errorf( - codes.Internal, - "failed to handle delete snapshot request for {%s} with %d clones", - req.GetSnapshotId(), - len(cloneList.Items), - ) - } - return nil } diff --git a/pkg/zfs/volume.go b/pkg/zfs/volume.go index 3660fa07d..47ba4532b 100644 --- a/pkg/zfs/volume.go +++ b/pkg/zfs/volume.go @@ -308,30 +308,6 @@ func GetSnapshotsForVolume(volumeID string) (*apis.ZFSSnapshotList, error) { return snapList, err } -// GetClonesForVolume lists all the clone volumes for the given volume -func GetClonesForVolume(volumeName string) (*apis.ZFSVolumeList, error) { - listOptions := metav1.ListOptions{ - LabelSelector: ZFSSrcVolKey + "=" + volumeName, - } - cloneList, err := volbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).List(listOptions) - return cloneList, err -} - -// GetClonesForSnapshot lists all the clone volumes for the given snapshot -func GetClonesForSnapshot(snapID string) (*apis.ZFSVolumeList, error) { - zfsVolList, err := volbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).List(metav1.ListOptions{}) - if err != nil { - return nil, err - } - - listBuilder := volbuilder.ListBuilderFrom(*zfsVolList) - listBuilder.WithFilter(func(vol *volbuilder.ZFSVolume) bool { - return vol.Object.Spec.SnapName == snapID - }) - - return listBuilder.List(), nil -} - // GetZFSSnapshotStatus returns ZFSSnapshot status func GetZFSSnapshotStatus(snapID string) (string, error) { getOptions := metav1.GetOptions{} From 4669caec2d1ab814315ecd0e0fc35f429694a140 Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Tue, 29 Jun 2021 16:15:10 +0530 Subject: [PATCH 08/11] remove waitForSnapDestroy Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 6ea64eaa9..15c4670e0 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -181,21 +181,6 @@ func waitForVolDestroy(volname string) error { } } -func waitForSnapDestroy(snapName string) error { - for { - _, err := zfs.GetZFSSnapshot(snapName) - if err != nil { - if k8serror.IsNotFound(err) { - return nil - } - return status.Errorf(codes.Internal, - "zfs: destroy wait failed, not able to get the snapshot %s %s", snapName, err.Error()) - } - time.Sleep(time.Second) - klog.Infof("waiting for snapshot to be destroyed %s", snapName) - } -} - func waitForReadySnapshot(snapname string) error { for { snap, err := zfs.GetZFSSnapshot(snapname) @@ -827,9 +812,6 @@ func (cs *controller) DeleteSnapshot( ) } - if err := waitForSnapDestroy(snapName); err != nil { - return nil, err - } return &csi.DeleteSnapshotResponse{}, nil } @@ -1011,7 +993,6 @@ func (cs *controller) validateDeleteVolumeReq(req *csi.DeleteVolumeRequest) erro if err != nil { return errors.Wrapf( err, - "failed to handle delete volume request for {%s} : validation failed", volumeID, ) From 508db5a4fd7c43171c7eea1228ea74c7dc742010 Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Tue, 29 Jun 2021 16:16:31 +0530 Subject: [PATCH 09/11] remove empty line Signed-off-by: Akhil Mohan --- pkg/driver/controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 15c4670e0..9f5787d2a 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -811,7 +811,6 @@ func (cs *controller) DeleteSnapshot( err.Error(), ) } - return &csi.DeleteSnapshotResponse{}, nil } From 4030e49d62c0268694fc29983d1273f920ec985a Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Tue, 29 Jun 2021 16:17:24 +0530 Subject: [PATCH 10/11] update changelog Signed-off-by: Akhil Mohan --- changelogs/unreleased/350-akhilerm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/350-akhilerm b/changelogs/unreleased/350-akhilerm index b0ccdf3ff..d63a2ad5a 100644 --- a/changelogs/unreleased/350-akhilerm +++ b/changelogs/unreleased/350-akhilerm @@ -1 +1 @@ -add validation to prevent volume/snapshot deletion if there is a dependent snapshot/clone \ No newline at end of file +add validation to prevent volume deletion if there is a dependent snapshot \ No newline at end of file From 3ecbbcc1d8c9993a3bce4a3538e4e8723d0ab122 Mon Sep 17 00:00:00 2001 From: Akhil Mohan Date: Tue, 29 Jun 2021 17:22:01 +0530 Subject: [PATCH 11/11] updated sanity tests to 4.2.0 Signed-off-by: Akhil Mohan --- ci/sanity.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/sanity.sh b/ci/sanity.sh index b49af393e..eed106e06 100755 --- a/ci/sanity.sh +++ b/ci/sanity.sh @@ -56,7 +56,7 @@ EOT CSI_TEST_REPO="https://github.com/$test_repo/csi-test.git" CSI_REPO_PATH="$(go env GOPATH)/src/github.com/$test_repo/csi-test" if [ ! -d "$CSI_REPO_PATH" ] ; then - git clone -b "v4.0.1" "$CSI_TEST_REPO" "$CSI_REPO_PATH" + git clone -b "v4.2.0" "$CSI_TEST_REPO" "$CSI_REPO_PATH" else cd "$CSI_REPO_PATH" git pull "$CSI_REPO_PATH"