Dont remove volumes when saveVolumeData fails #96021
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
sig/storage
huffmanca
force-pushed
the
dont-remove-volume
branch
from
b71d8e8
to
a115fb4
Compare
|
/retest |
huffmanca
changed the title
|
/assign gnufied |
| if err := saveVolumeData(dataDir, volDataFileName, volData); err != nil { | ||
| if removeErr := os.RemoveAll(dataDir); removeErr != nil { | ||
| klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, removeErr)) | ||
| err = saveVolumeData(dataDir, volDataFileName, volData) |
There was a problem hiding this comment.
should we do same thing in this file for other places where saveVolumeData is being called followed up by os.RemoveAll? This applies to NewBlockVolumeMapper call.
There was a problem hiding this comment.
Is this directory same or different than the one used in MountDevice?
There was a problem hiding this comment.
As best I can tell from testing with the hostpath driver, these appear to be different directories:
csi_plugin:created path successfully [/var/lib/kubelet/pods/103bb9c7-0a9f-4958-bb6a-8d05f64420f5/volumes/kubernetes.io~csi/pvc-21228cc8-5669-4c0d-bbdc-d7b4e19dfb79]csi_attacher:created target path successfully [/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-22c63cbf-4e2c-4ca6-b269-d5e44a1f3718/globalmount]
|
@huffmanca have we tried unit testing this stuff? it might be difficult but worth giving it a shot I think. |
huffmanca
force-pushed
the
dont-remove-volume
branch
from
a115fb4
to
7132ab9
Compare
huffmanca
force-pushed
the
dont-remove-volume
branch
from
7132ab9
to
bee1aa2
Compare
k8s-ci-robot
added
size/L
huffmanca
force-pushed
the
dont-remove-volume
branch
from
bee1aa2
to
5956d09
Compare
pkg/volume/csi/csi_attacher.go
Outdated
| defer func() { | ||
| // Only if there was an error and volume operation was considered | ||
| // finished, we should remove the directory. | ||
| if err != nil && volumetypes.IsOperationFinishedError(err) { | ||
| // clean up metadata | ||
| klog.Errorf(log("attacher.MountDevice failed: %v", err)) | ||
| if err := removeMountDir(c.plugin, deviceMountPath); err != nil { | ||
| if err := os.RemoveAll(dataDir); err != nil { |
There was a problem hiding this comment.
Isn't using os.RemoveAll opposite of what we set out to fix? This means if - saving the volDataFileName fails then we will still end up calling rm -rf on entire data directory?
huffmanca
force-pushed
the
dont-remove-volume
branch
from
5956d09
to
15da65d
Compare
|
/retest |
1 similar comment
|
/retest |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/priority-important-soon |
|
@huffmanca can you add a release-note plz? I think this is important enough of a change that should be covered in release notes. |
k8s-ci-robot
added
release-note
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: huffmanca, msau42 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest Review the full test history for this PR. Silence the bot with an |
…021-upstream-release-1.20 Automated cherry pick of #96021: Dont remove volumes when saveVolumeData fails
…021-upstream-release-1.18 Automated cherry pick of #96021 upstream release 1.18
…021-upstream-release-1.19 Automated cherry pick of #96021 upstream release 1.19
What type of PR is this?
/kind bug
What this PR does / why we need it:
This is a continuation of #89464 . It incorporates that author's commit while also adjusting the location of the
defer()function to correctly remove.Which issue(s) this PR fixes:
Fixes # #89281
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: