search

spiffe / spiffe-csi

Container Storage Interface components for SPIFFE
Apache License 2.0
54 stars 21 forks source link

Handle error scenarios gracefully in NodeUnpublishVolume #161

Closed swamibluedata closed 9 months ago

swamibluedata commented 9 months ago

This fixes

https://github.com/spiffe/spiffe-csi/issues/158

Issue unmount if and only if the target folder is present and is a valid mount point.

Issue Remove call if and only if the target folder exists

If the mount point doesn't exist, report success, which will allow kubelet to continue with pod termination sequence

Signed-off-by: Swami Viswanathan <swami.viswanathan@hpe.com>
swamibluedata commented 9 months ago

Addressed a PR comment from the closed PR

https://github.com/spiffe/spiffe-csi/pull/160

azdagron commented 9 months ago

Hey @swamibluedata! Thanks for this contribution! If you could fix the DCO on your commit, that would be great. You can click on the "details" link to the right of the failed action.

In your case it looks like the Signed-off-by is set to one email, but the author of the commit is another. You may need to do a git commit --amend -s --reset-author to fix the commit before force pushing.

azdagron commented 9 months ago

Looks like at least one unit test is also failing and may need to be updated based on the new behavior.

The failing integration test is unrelated (the v1.21.14 test is failing across the board at the moment).

swamibluedata commented 9 months ago

Hey @swamibluedata! Thanks for this contribution! If you could fix the DCO on your commit, that would be great. You can click on the "details" link to the right of the failed action.

In your case it looks like the Signed-off-by is set to one email, but the author of the commit is another. You may need to do a git commit --amend -s --reset-author to fix the commit before force pushing.

Got it and will need to address the test case as well.

swamibluedata commented 9 months ago

Looks like at least one unit test is also failing and may need to be updated based on the new behavior.

The failing integration test is unrelated (the v1.21.14 test is failing across the board at the moment).

Both the new "unable to verify mount point" error and an existing error "unable to unmount" are going to be tricky to test.

For "unable to verify mount point", i need to make enumerateMounts fail For "unable to mount", i can run a process that opens a file on that mountpoint which will prevent unmount

Thinking

azdagron commented 9 months ago

The unit tests for the driver package replace the various functions with those where they can inject different conditions. That's probably good enough for the unit tests. At some point we'll wire up more test cases into the integration test, which is running in a kind cluster, but for now I think unit test coverage give us the same test fidelity we have now.

swamibluedata commented 9 months ago

Hey @swamibluedata! Thanks for this contribution! If you could fix the DCO on your commit, that would be great. You can click on the "details" link to the right of the failed action.

In your case it looks like the Signed-off-by is set to one email, but the author of the commit is another. You may need to do a git commit --amend -s --reset-author to fix the commit before force pushing.

Updated the commit, still complaining :(

https://github.com/spiffe/spiffe-csi/pull/161/commits/37bb8fa045048021611a2362cdd1dd0c33162e28

azdagron commented 9 months ago

Looks like the last thing to fix is a linting error. You need to run goimports on one of the files you edited (setting up your editor to do this automatically when saving is usually the easiest path).

swamibluedata commented 9 months ago

@azdagron anything i need to address