Remove the 'external-provisioner.volume.kubernetes.io/finalizer' #1755
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ikolomiyets The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @ikolomiyets! |
|
Hi @ikolomiyets. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
jiayiliu-amz
left a comment
jiayiliu-amz
left a comment
There was a problem hiding this comment.
Having the csi driver manipulate PV finalizers violates the CSI architecture. The external-provisioner sidecar should manage finalizers based on DeleteVolume's return value https://kubernetes.io/blog/2025/05/05/kubernetes-v1-33-prevent-persistentvolume-leaks-when-deleting-out-of-order-graduate-to-ga/#how-does-it-work
This approach also creates potential race conditions between the driver and external-provisioner on removing the finalizer, which I'm not sure is a problem or not.
I suggest we instead focus on making the DeleteVolume call to return success when the underlying resource has been cleaned up so the external-provisioner can remove the PV finalizer itself. The current issue is that DeleteVolume does not handle "already delete" scenario correctly in multiple places (e.g. EPIPE on unmounting, AP not existing etc.)
Thank you for raising the issue and publishing the PR! Please feel free to follow up on the discussion or publish new revisions. Please also feel free to let us know if you prefer this to be fixed by the EFS team, as I do believe this is a bug in the csi driver code.
pkg/driver/controller.go
Outdated
| // Attempt to remove finalizer, if fails, just report an error and go on | ||
| err = removePVFinalizer(ctx, cloud.DefaultKubernetesAPIClient, volId) |
There was a problem hiding this comment.
We should not try to deal with the PV finalizer in the csi driver, which would be a break of abstraction in my opinion, because the PVFinalizer should be owned by the csi sidecar and the csi drvier does not need to know about it.
I think the fix should focus on making the DeleteVolume API being able to correctly handle "already deleted" cases (including AP already deleted, error code 32 on unmount (EPIPE) etc.) so DeleteVolume will return success and the PVFinalizer is removed by the sidecar (external-provisioner).
There was a problem hiding this comment.
This makes perfect sense. I did not familiarise myself with what's each CSI component responsibility, hence took a shortcut.
I revert all the changes except two: one which skips deferred attempt to unmount if access point does not exist, i.e. when it never mounted; and the other by the looks of it unreachable error handler.
k8s-ci-robot
added
size/XS
pkg/driver/controller.go
Outdated
| err = os.RemoveAll(apRootPath) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Could not delete access point root directory %q: %v", accessPoint.AccessPointRootDir, err) | ||
| } |
There was a problem hiding this comment.
we missed the "error is NotExist or NotDir" scenario, which we can just consider as the directory has been removed (no need to fail the DeleteVolume). That is
stat_result = Stat(apRootPath)
if stat_result.ok && stat_result.is_dir:
RemoveAll(apRootPath)
else if stat_result.not_exist || !stat_result.is_dir:
skip deletion (already gone)
else:
fail (real error like permission denied that we don't know the status of the dir)
There was a problem hiding this comment.
one last place I see have issue is https://github.com/kubernetes-sigs/aws-efs-csi-driver/blob/master/pkg/driver/mounter.go#L63 that
func (m *NodeMounter) IsLikelyNotMountPoint(target string) (bool, error) {
notMnt, err := m.MounterForceUnmounter.IsLikelyNotMountPoint(target)
if err != nil {
if os.IsNotExist(err) {
return false, nil
}
return false, err
}
return notMnt, nil
}
if the mount point does not exist, we should return TRUE for isNotMounted. It would be nice to also fix it together in this PR
There was a problem hiding this comment.
By the way, please squash your changes to a single commit. Thank you
There was a problem hiding this comment.
I've moved code responsible for removal of directory to a separate function. To me it is easier to read. Hope it does not break any guidelines.
jiayiliu-amz
left a comment
jiayiliu-amz
left a comment
There was a problem hiding this comment.
Proposed 2 other places that we should treat the volume deletion as completed
k8s-ci-robot
added
size/L
3 participants
Is this a bug fix or adding new feature?
My original thinking was that this is a bug, since it is a controller's responsibility to remove the finalizer. However, I would say that for the lack of the better word it is a "missing feature" as per original comment in issue #1753.
What is this PR about? / Why do we need it?
Since Kubernetes v1.33 the "Prevent PersistentVolume Leaks When Deleting out of Order" has been graduated to GA. As part of the feature, external provisioner adds an external-provisioner.volume.kubernetes.io/finalizer' to the Persistent Volume in question.
In relation to the EFS driver it manifests itself in the PV remaining in "Terminating" state unless the corresponding finalizer is removed manually.
This PR ensures that above finalizer is removed from the PV in question upon successful deletion of the volume.
What testing is done?
Unit tests are added to the function that is responsible for the removal of the PV's finalizer.
I also test it rigorously in my test cluster.