Skip to content
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

Jump to bottom

do not remove volume dir when saveVolumeData fails #89464

Closed
wants to merge 1 commit into from
Closed

do not remove volume dir when saveVolumeData fails #89464

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions pkg/volume/csi/csi_attacher.go
View file
Open in desktop
Expand Up @@ -303,11 +303,9 @@ func (c *csiAttacher) MountDevice(spec *volume.Spec, devicePath string, deviceMo
volDataKey.driverName: csiSource.Driver,
}
if err = saveVolumeData(dataDir, volDataFileName, data); err != nil {
klog.Error(log("failed to save volume info data: %v", err))
if cleanErr := os.RemoveAll(dataDir); cleanErr != nil {
klog.Error(log("failed to remove dir after error [%s]: %v", dataDir, cleanErr))
}
return err
errMsg := log("failed to save volume info data: %v", err)
klog.Error(errMsg)
return errors.New(errMsg)
Copy link
Member

Choose a reason for hiding this comment

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

similar comment as below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removeMountDir() will be called in defer func here.

Copy link
Member

Choose a reason for hiding this comment

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

correct me if I am wrong but the defer function is not installed yet and hence will not be called when the function returns from here.

Copy link
Member

Choose a reason for hiding this comment

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

@fankangbest Can you please address this comment? I am happy to have unit test merged later on but we need this fixed before we can merge this.

}
defer func() {
// Only if there was an error and volume operation was considered
Expand Down
11 changes: 8 additions & 3 deletions pkg/volume/csi/csi_plugin.go
View file
Open in desktop
Expand Up @@ -420,10 +420,15 @@ func (p *csiPlugin) NewMounter(
volData[volDataKey.attachmentID] = attachID

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))
errorMsg := log("csi.NewMounter failed to save volume info data: %v", err)
klog.Error(errorMsg)

// attempt to cleanup volume mount dir.
if removeMountDirErr := removeMountDir(p, dir); removeMountDirErr != nil {
klog.Error(log("csi.NewMounter failed to remove mount dir [%s]: %v", dir, removeMountDirErr))
}
return nil, errors.New(log("failed to save volume info data: %v", err))

return nil, errors.New(errorMsg)
}

klog.V(4).Info(log("mounter created successfully"))
Expand Down