From 68d79d0e0bbdfc8af06f67cb1c838a516f583674 Mon Sep 17 00:00:00 2001 From: Pawan Prakash Sharma Date: Tue, 6 Apr 2021 20:03:00 +0530 Subject: [PATCH] feat(zfspv): remove finalizer that is owned by ZFS-LocalPV (#303) We set the Finalizer to nil while handling the delete event, instead, we should try to destroy the volume when there are no user finalizers set. User might have added his own finalizers and we should not try to destroy the volumes until those user finalizers are removed. Signed-off-by: Pawan --- pkg/mgmt/snapshot/snapshot.go | 16 +++++++++---- pkg/mgmt/volume/volume.go | 19 +++++++++------ pkg/zfs/mount.go | 2 +- pkg/zfs/volume.go | 45 ++++++++++++++++++++++++++--------- 4 files changed, 58 insertions(+), 24 deletions(-) diff --git a/pkg/mgmt/snapshot/snapshot.go b/pkg/mgmt/snapshot/snapshot.go index e793f7c..7378d1a 100644 --- a/pkg/mgmt/snapshot/snapshot.go +++ b/pkg/mgmt/snapshot/snapshot.go @@ -77,14 +77,20 @@ func (c *SnapController) syncSnap(snap *apis.ZFSSnapshot) error { var err error // ZFSSnapshot should be deleted. Check if deletion timestamp is set if c.isDeletionCandidate(snap) { - err = zfs.DestroySnapshot(snap) - if err == nil { - zfs.RemoveSnapFinalizer(snap) + userFin := zfs.GetUserFinalizers(snap.Finalizers) + if len(userFin) == 0 { + // destroy only if other finalizers have been removed + err = zfs.DestroySnapshot(snap) + if err == nil { + err = zfs.RemoveSnapFinalizer(snap) + } + } else { + return fmt.Errorf("snapshot: can not destroy, waiting for finalizers to be removed %v", userFin) } } else { - // if finalizer is not set then it means we are creating + // if status is not Ready then it means we are creating // the zfs snapshot. - if snap.Finalizers == nil { + if snap.Status.State != zfs.ZFSStatusReady { err = zfs.CreateSnapshot(snap) if err == nil { err = zfs.UpdateSnapInfo(snap) diff --git a/pkg/mgmt/volume/volume.go b/pkg/mgmt/volume/volume.go index 0edc219..997812b 100644 --- a/pkg/mgmt/volume/volume.go +++ b/pkg/mgmt/volume/volume.go @@ -78,15 +78,20 @@ func (c *ZVController) syncZV(zv *apis.ZFSVolume) error { var err error // ZFS Volume should be deleted. Check if deletion timestamp is set if c.isDeletionCandidate(zv) { - err = zfs.DestroyVolume(zv) - if err == nil { - zfs.RemoveZvolFinalizer(zv) + userFin := zfs.GetUserFinalizers(zv.Finalizers) + if len(userFin) == 0 { + // destroy only if other finalizers have been removed + err = zfs.DestroyVolume(zv) + if err == nil { + err = zfs.RemoveVolumeFinalizer(zv) + } + } else { + return fmt.Errorf("volume: can not destroy, waiting for finalizers to be removed %v", userFin) } } else { - // if finalizer is not set then it means we are creating - // the volume. And if it is set then volume has already been - // created and this event is for property change only. - if zv.Finalizers != nil { + // if volume has already been created and its state is Ready + // then this event is for property change only. + if zfs.IsVolumeReady(zv) { err = zfs.SetVolumeProp(zv) } else { if len(zv.Spec.SnapName) > 0 { diff --git a/pkg/zfs/mount.go b/pkg/zfs/mount.go index 943ba38..9aee842 100644 --- a/pkg/zfs/mount.go +++ b/pkg/zfs/mount.go @@ -134,7 +134,7 @@ func verifyMountRequest(vol *apis.ZFSVolume, mountpath string) (bool, error) { vol.Spec.OwnerNodeID != NodeID { return false, status.Error(codes.Internal, "verifyMount: volume is owned by different node") } - if vol.Finalizers == nil { + if !IsVolumeReady(vol) { return false, status.Error(codes.Internal, "verifyMount: volume is not ready to be mounted") } diff --git a/pkg/zfs/volume.go b/pkg/zfs/volume.go index 27ec350..dcca9d7 100644 --- a/pkg/zfs/volume.go +++ b/pkg/zfs/volume.go @@ -233,10 +233,6 @@ func UpdateZvolInfo(vol *apis.ZFSVolume, status string) error { finalizers := []string{} labels := map[string]string{ZFSNodeKey: NodeID} - if vol.Finalizers != nil { - return nil - } - switch status { case ZFSStatusReady: finalizers = append(finalizers, ZFSFinalizer) @@ -255,8 +251,8 @@ func UpdateZvolInfo(vol *apis.ZFSVolume, status string) error { return err } -// RemoveZvolFinalizer adds finalizer to ZFSVolume CR -func RemoveZvolFinalizer(vol *apis.ZFSVolume) error { +// RemoveVolumeFinalizer removes finalizer from ZFSVolume CR +func RemoveVolumeFinalizer(vol *apis.ZFSVolume) error { vol.Finalizers = nil _, err := volbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).Update(vol) @@ -290,10 +286,6 @@ func UpdateSnapInfo(snap *apis.ZFSSnapshot) error { finalizers := []string{ZFSFinalizer} labels := map[string]string{ZFSNodeKey: NodeID} - if snap.Finalizers != nil { - return nil - } - newSnap, err := snapbuilder.BuildFrom(snap). WithFinalizer(finalizers). WithLabels(labels).Build() @@ -310,7 +302,7 @@ func UpdateSnapInfo(snap *apis.ZFSSnapshot) error { return err } -// RemoveSnapFinalizer adds finalizer to ZFSSnapshot CR +// RemoveSnapFinalizer removes finalizer from ZFSSnapshot CR func RemoveSnapFinalizer(snap *apis.ZFSSnapshot) error { snap.Finalizers = nil @@ -358,3 +350,34 @@ func UpdateRestoreInfo(rstr *apis.ZFSRestore, status apis.ZFSRestoreStatus) erro _, err = restorebuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).Update(newRstr) return err } + +// GetUserFinalizers returns all the finalizers present on the ZFSVolume object +// execpt the one owned by ZFS node daemonset. We also need to ignore the foregroundDeletion +// finalizer as this will be present becasue of the foreground cascading deletion +func GetUserFinalizers(finalizers []string) []string { + var userFin []string + for _, fin := range finalizers { + if fin != ZFSFinalizer && + fin != "foregroundDeletion" { + userFin = append(userFin, fin) + } + } + return userFin +} + +// IsVolumeReady returns true if volume is Ready +func IsVolumeReady(vol *apis.ZFSVolume) bool { + + if vol.Status.State == ZFSStatusReady { + return true + } + + // For older volumes, there was no Status field + // so checking the node finalizer to make sure volume is Ready + for _, fin := range vol.Finalizers { + if fin == ZFSFinalizer { + return true + } + } + return false +}