Skip to content

make delete volume idempotent when node is removed from cluster #413

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

make delete volume idempotent when node is removed from cluster #413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions pkg/driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
corev1 "k8s.io/api/core/v1"
k8serror "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
kubeinformers "k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -365,7 +366,7 @@ func CreateLVMVolume(ctx context.Context, req *csi.CreateVolumeRequest,
}
klog.Infof("scheduling the volume %s/%s on node %s",
params.VgPattern.String(), volName, owner)

volObj, err := volbuilder.NewBuilder().
WithName(volName).
WithCapacity(capacity).
Expand Down Expand Up @@ -472,6 +473,28 @@ func (cs *controller) DeleteVolume(
return csipayload.NewDeleteVolumeResponseBuilder().Build(), nil
}

// Checks if k8s node is present. If the check fails with any other reason then NotFound.
// We will return true and assume node is present.
func isNodePresent(nodeId string) bool {
cfg, err := k8sapi.Config().Get()
if err != nil {
return true
}

kubeClient, err := kubernetes.NewForConfig(cfg)
if err != nil {
return true
}

_, err = kubeClient.CoreV1().Nodes().Get(context.TODO(), nodeId, metav1.GetOptions{})
if err != nil {
if k8serror.IsNotFound(err) {
return false
}
}
return true
}

func (cs *controller) deleteVolume(ctx context.Context, volumeID string) error {
klog.Infof("received request to delete volume %q", volumeID)
vol, err := lvm.GetLVMVolume(volumeID)
Expand All @@ -482,7 +505,6 @@ func (cs *controller) deleteVolume(ctx context.Context, volumeID string) error {
return errors.Wrapf(err,
"failed to get volume for {%s}", volumeID)
}

// if volume is not already triggered for deletion, delete the volume.
// otherwise, just wait for the existing deletion operation to complete.
if vol.GetDeletionTimestamp() == nil {
Expand All @@ -491,6 +513,17 @@ func (cs *controller) deleteVolume(ctx context.Context, volumeID string) error {
"failed to handle delete volume request for {%s}", volumeID)
}
}
present := isNodePresent(vol.Spec.OwnerNodeID)
Copy link
Member

@tiagolobocastro tiagolobocastro Nov 6, 2025

Choose a reason for hiding this comment

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

what if the finalizer is already removed? Then we don't need to run this?

Copy link
Member Author

@abhilashshetty04 abhilashshetty04 Nov 7, 2025

Choose a reason for hiding this comment

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

finalizer is removed from the node-agent when lvremove succeeds. Here we will validate if node is not present. If not, finalizer will not be removed. so will remove finalizer from here.

Copy link
Member

@tiagolobocastro tiagolobocastro Nov 7, 2025

Choose a reason for hiding this comment

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

If the lvm volume has additional finalisers (eg: added by user) but we've already removed ours, on a previous csi call, then we don't need to do this again
We only need to do this stage if our finaliser is present

Copy link
Member Author

@abhilashshetty04 abhilashshetty04 Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Right now this removes all Finalizers. I am using the function which is called by node-agent once lvremove is done.

https://github.com/openebs/lvm-localpv/blob/develop/pkg/lvm/volume.go#L217C1-L223C2

lvmvolume CR is managed by localpv-lvm. Users may not add any finalizers onto it.

if !present {
klog.Warningf("Removing finalizer as node %s is not present in cluster", vol.Spec.OwnerNodeID)
if err = lvm.RemoveVolFinalizer(vol); err != nil {
return err
}
if err = lvm.VerifyLVMVolumeDestroy(volumeID); err != nil {
return err
}
return nil
}
if err = lvm.WaitForLVMVolumeDestroy(ctx, volumeID); err != nil {
return err
}
Expand Down
22 changes: 21 additions & 1 deletion pkg/lvm/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func ProvisionVolume(vol *apis.LVMVolume) (*apis.LVMVolume, error) {
func DeleteVolume(volumeID string) (err error) {
err = volbuilder.NewKubeclient().WithNamespace(LvmNamespace).Delete(volumeID)
if err == nil {
klog.Infof("deprovisioned volume %s", volumeID)
klog.Infof("deprovisioning volume %s", volumeID)
}

return
Expand Down Expand Up @@ -164,6 +164,23 @@ func WaitForLVMVolumeDestroy(ctx context.Context, volumeID string) error {
}
}

// Verifies if lvmvolume CR got removed after removing finaliser.
func VerifyLVMVolumeDestroy(volumeID string) error {
for i := 0; i < 10; i++ {
_, err := GetLVMVolume(volumeID)
if err != nil {
if k8serror.IsNotFound(err) {
return nil
}
return status.Errorf(codes.Internal,
"lvmvolume: destroy wait failed, not able to get the volume %s %s", volumeID, err.Error())
}
time.Sleep(time.Duration(100 * time.Millisecond))
}
return status.Errorf(codes.FailedPrecondition,
"lvmvolume: %s not removed after removing finaliser", volumeID)
}

// GetLVMVolumeState returns LVMVolume OwnerNode and State for
// the given volume. CreateVolume request may call it again and
// again until volume is "Ready".
Expand Down Expand Up @@ -219,6 +236,9 @@ func RemoveVolFinalizer(vol *apis.LVMVolume) error {
vol.Finalizers = nil

_, err := volbuilder.NewKubeclient().WithNamespace(LvmNamespace).Update(vol)
if err != nil {
klog.Infof("Finalizer removed successfully for %s", vol.Name)
Copy link
Member

@tiagolobocastro tiagolobocastro Nov 6, 2025

Choose a reason for hiding this comment

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

Do we need this as info or is debug fine?

Copy link
Member Author

@abhilashshetty04 abhilashshetty04 Nov 7, 2025

Choose a reason for hiding this comment

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

Will not reach this code if node is present. Would be better to have it in log by default.

Copy link
Member

@tiagolobocastro tiagolobocastro Nov 7, 2025

Choose a reason for hiding this comment

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

Don't we already have the message from above where we call it if the node is not present?
Oh also this check seems reversed, if err != nil then we didn't remove the finalizer?

Copy link
Member Author

@abhilashshetty04 abhilashshetty04 Nov 10, 2025

Choose a reason for hiding this comment

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

Yes, err != nil means we fail to remove finalizer, we return err. If not, we move on to check if lvmvolume CR is deleted.

}
return err
}

Expand Down
Loading