Fix cleanup of volume metadata json file. #65323
Conversation
Create the json file with metadata as the last item, when everything else is ready, so we don't need to clean up the file in all error cases in this function.
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
size/S
pkg/volume/csi/csi_attacher.go
Outdated
| } | ||
| if err := saveVolumeData(dataDir, volDataFileName, data); err != nil { | ||
| glog.Error(log("failed to save volume info data: %v", err)) | ||
| if err := os.RemoveAll(dataDir); err != nil { |
There was a problem hiding this comment.
Do all the error returns above also need to RemoveAll(dataDir)? Maybe this cleanup should be in a defer function that only runs if err != nil ?
There was a problem hiding this comment.
MkdirAll() is just a few lines above and there is no return in between. I'll move it even closer to NodeStageVolume with some comment.
k8s-ci-robot
added
size/M
|
Moved staging directory creating closer to |
|
/hold This PR makes is actually worse. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel Now the json file is created early and cleaned in defer. @msau42, PTAL |
k8s-ci-robot
removed
the
do-not-merge/hold
pkg/volume/csi/csi_attacher.go
Outdated
| // clean up metadata | ||
| glog.Errorf(log("attacher.MountDevice failed: %v", err)) | ||
| if err := removeMountDir(c.plugin, deviceMountPath); err != nil { | ||
| glog.Error(log("attacher.MountDevice failed to remove mount dir after a NodeStageVolume() error [%s]: %v", deviceMountPath, err)) |
There was a problem hiding this comment.
It may not always be a NodeStageVolume() call that errored
There was a problem hiding this comment.
Updated the log message
| defer func() { | ||
| if err != nil { | ||
| // clean up metadata | ||
| glog.Errorf(log("attacher.MountDevice failed: %v", err)) |
There was a problem hiding this comment.
Also should all the early error returns set err first so that the correct error will be logged here? For example L311, 322, 331, 336, 345
There was a problem hiding this comment.
Fixed 322, 331 336 and 345. I can't see anything wrong in 311.
There was a problem hiding this comment.
My question regarding 311, 332, 337: is there a reason to log almost the same message first before returning the error, which will then be logged again in the defer?
There was a problem hiding this comment.
I see, I removed these error logs.
jsafrane
force-pushed
the
fix-csi-json
branch
3 times, most recently
from
c62d16b to
9069efe
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, 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 |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 65404, 65323, 65468). If you want to cherry-pick this change to another branch, please follow the instructions here. |
#65323-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. Cherry pick of #64882 #65323 on release-1.11. #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. ```release-note Fixed cleanup of CSI metadata files. ```
#65323-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. Cherry pick of #64882 #65323 on release-1.10. #64882: Fix UnmountDevice with deleted pod. #65323: Fix cleanup of volume metadata json file. ```release-note Fixed cleanup of CSI metadata files. ```
Create the json file with metadata as the last item, when everything else is ready, so we don't need to clean up the file in all error cases in this function.
Fixes #65322
Release note:
/assign @saad-ali @vladimirvivien