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

Allow unmounting bind-mounted directories. #49118

Merged
merged 1 commit into from Jul 24, 2017
Merged

Allow unmounting bind-mounted directories. #49118

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
20 changes: 6 additions & 14 deletions pkg/volume/flexvolume/unmounter.go
View file
Open in desktop
Expand Up @@ -51,23 +51,15 @@ func (f *flexVolumeUnmounter) TearDownAt(dir string) error {
return nil
}

notmnt, err := isNotMounted(f.mounter, dir)
call := f.plugin.NewDriverCall(unmountCmd)
call.Append(dir)
_, err := call.Run()
if isCmdNotSupportedErr(err) {
err = (*unmounterDefaults)(f).TearDownAt(dir)
}
if err != nil {
return err
}
if notmnt {
glog.Warningf("Warning: Path: %v already unmounted", dir)
} else {
call := f.plugin.NewDriverCall(unmountCmd)
call.Append(dir)
_, err := call.Run()
if isCmdNotSupportedErr(err) {
err = (*unmounterDefaults)(f).TearDownAt(dir)
}
if err != nil {
return err
}
}

// Flexvolume driver may remove the directory. Ignore if it does.
Copy link
Member

Choose a reason for hiding this comment

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

is this assuming it is a directory as well? is that going to be problematic if the flex plugin was mounting a file or dir that should not be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't add any removal here, do we?
The file / directory which was mounted is not really worked with in this function, the dir is the mountpoint. And the goal here is to just run the driver to do the unmount, without checking whether the mountpoint seems mounted.
Of course, the driver can do whatever checks it needs to do to be extra safe.
If you are worried about this check being removed, what we could do is check whether during mounting, the mountpoint started to look like mountpoint (== the filesystem id of the mountpoint and of its parent got different). But I'm not sure if we'd be able to store/persist that check result until the unmount time.

Copy link
Member

Choose a reason for hiding this comment

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

I meant the existing os.Remove(dir) call 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.

Ah. Well, it's a directory / location that was created in the pod's directory by the driver during mounting, to have the mountpoint exist, in https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/flexvolume/util.go#L71. It's per-driver per-mountpoint named.
I can't think of use-case when leaving it behind might be useful. It's not like this was the location where any other software might expect for the data to keep on living.

Copy link
Member

Choose a reason for hiding this comment

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

ok

if pathExists, pathErr := util.PathExists(dir); pathErr != nil {
Expand Down