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.

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

Allow unmounting bind-mounted directories. #49118

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

adelton
Copy link
Contributor

@adelton adelton commented Jul 18, 2017

What this PR does / why we need it:

For files, we cannot use path/..;
we could use filepath.Dir but for bind-mounted, isNotMounted which calls IsLikelyNotMountPoint would not work anyway.
Let's just have the driver do the work.

Addressing

Error: UnmountVolume.TearDown failed for volume "..." (volume.spec.Name: "...") pod "..." (UID: "...") with: lstat /path/.../test-flex/..: not a directory

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

N/A

Special notes for your reviewer:

N/A

Release note:

Fix a bug with binding mount directories and files using flexVolumes

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @adelton. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 18, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 18, 2017
@pweil-
Copy link
Contributor

pweil- commented Jul 18, 2017

@kubernetes/sig-storage-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 18, 2017
@liggitt
Copy link
Member

liggitt commented Jul 18, 2017

Definitely needs an accompanying test. Does everything except unmounting already work for bindmounted files?

@jsafrane
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 18, 2017
@adelton
Copy link
Contributor Author

adelton commented Jul 18, 2017

I should note that even with bind-mounted directory (not file), things are currently broken and the error would be

unmounter.go:59] Warning: Path: /path/.../test-flex already unmounted

@adelton
Copy link
Contributor Author

adelton commented Jul 18, 2017

Alternatively, we could walk /proc/mounts in that isNotMounted ... but the plugin can decide to just copy something there and not do any mounting at all, so it might be best to just call the driver and let it do the right thing.

@adelton
Copy link
Contributor Author

adelton commented Jul 18, 2017

Definitely needs an accompanying test.

What should such a test do? We cannot run mount -o bind in tests ... so would merely doing nothing during mount (or creating some file in the target directory) and then seeing unmount not failing be enough?

Does everything except unmounting already work for bindmounted files?

I cannot guarantee that "everything". With this change, I'm able to use flexVolumes to bind-mount (and unmount) files and directories to my containers, that's what I was after.

@liggitt
Copy link
Member

liggitt commented Jul 18, 2017

What should such a test do? We cannot run mount -o bind in tests ... so would merely doing nothing during mount (or creating some file in the target directory) and then seeing unmount not failing be enough?

Yeah, exercise the data you expect to see plumbed through to the flex plugin, to make sure nothing in between blocks it with an error.

With this change, I'm able to use flexVolumes to bind-mount (and unmount) files and directories to my containers, that's what I was after.

Just for posterity, can you include the config (pod spec and flex plugin) you used to do this? Separately, that would be great to include in documentation/examples.

@adelton
Copy link
Contributor Author

adelton commented Jul 18, 2017

test-flex-pod.json:

{
    "kind": "Pod",
    "apiVersion": "v1",
    "metadata": {
        "name": "test-flex",
        "creationTimestamp": null,
        "labels": {
            "app": "test-flex"
        }
    },
    "spec": {
        "volumes": [
            {
                "name": "test-flex",
                "flexVolume": {
                    "driver": "example/bash"
                }
            }
        ],
        "containers": [
            {
                "name": "test-flex",
                "image": "registry.access.redhat.com/rhel7",
                "imagePullPolicy": "Never",
                "command": [
                    "/usr/bin/sleep",
                    "infinity"
                ],
                "volumeMounts": [
                    {
                        "name": "test-flex",
                        "mountPath": "/flex"
                    }
                ]
            }
        ]
    }
}

/usr/libexec/kubernetes/kubelet-plugins/volume/exec/example~bash/bash:

#!/bin/bash
case "$1" in
	init|detach|mountdevice)
		echo '{ "status": "Success" }'
	;;
	getvolumename)
		echo '{ "status": "Success", "volumeName": "'$(hostname)'" }'
	;;
	attach|waitforattach)
		echo '{ "status": "Success", "device": "/opt/data/file" }'
	;;
	isattached)
		echo '{ "status": "Success", "attached": true }'
	;;
	mount)
		file=/opt/data/file
		if [ -d "$file" ] ; then
			mkdir -p "$2"
		elif [ -f "$file" ] ; then
			test -d "$2" && rmdir "$2"
			touch "$2"
		fi
		mount -o bind "$file" "$2"
		echo '{ "status": "Success" }'
	;;
	unmount)
		umount -f "$2"
		rmdir "$2" || rm -f "$2"
		echo '{ "status": "Success" }'
	;;
esac

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

@adelton
Copy link
Contributor Author

adelton commented Jul 19, 2017

About the tests:

looking into ./pkg/volume/flexvolume/flexvolume_test.go there is execScriptTempl2 there, implementing mount and unmount, but it does not seem to be used anywhere. The code which was using it was removed in 0d2af70. Was removal of that code correct?

In execScriptTempl1, if I apply

diff --git a/pkg/volume/flexvolume/flexvolume_test.go b/pkg/volume/flexvolume/flexvolume_test.go
index 44af4fc..0367fee 100644
--- a/pkg/volume/flexvolume/flexvolume_test.go
+++ b/pkg/volume/flexvolume/flexvolume_test.go
@@ -42,23 +42,23 @@ PATH=$2
 if [ "$1" == "attach" -a $# -eq 2 ]; then
   echo -n '{
     "device": "{{.DevicePath}}",
-    "status": "Success"
+    "status": "Successx"
   }'
   exit 0
 elif [ "$1" == "detach" -a $# -eq 2 ]; then
   echo -n '{
-    "status": "Success"
+    "status": "Successx"
   }'
   exit 0
 elif [ "$1" == "getvolumename" -a $# -eq 4 ]; then
   echo -n '{
-    "status": "Success",
+    "status": "Successx",
     "volume": "fakevolume"
   }'
   exit 0
 elif [ "$1" == "isattached" -a $# -eq 2 ]; then
   echo -n '{
-    "status": "Success",
+    "status": "Successx",
     "attached": true
   }'
   exit 0

running make test WHAT=./pkg/volume/flexvolume GOFLAGS="-v" still passes. Is it expected that the tests pass even if the script does not return correct success?

I think I'm trying to asses the general state of the tests before extending them (or bringing back tests that used to be there).

@adelton
Copy link
Contributor Author

adelton commented Jul 19, 2017

Another question about the pkg/volume/flexvolume/flexvolume_test.go -- when I make the init in execScriptTempl1, the test fails with

=== RUN   TestCanSupport
E0719 13:48:19.829849    2289 driver-call.go:232] init command failed, status: Successx, reason: 
--- FAIL: TestCanSupport (0.00s)
	flexvolume_test.go:180: Can't find the plugin by name
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x11fc5b5]

Is that expected that after the first failure, things can just SIGSEGV, or should even the tests be defensive and something like

diff --git a/pkg/volume/flexvolume/flexvolume_test.go b/pkg/volume/flexvolume/flexvolume_test.go
index 44af4fc..f47412a 100644
--- a/pkg/volume/flexvolume/flexvolume_test.go
+++ b/pkg/volume/flexvolume/flexvolume_test.go
@@ -177,7 +177,7 @@ func TestCanSupport(t *testing.T) {
        plugMgr.InitPlugins(ProbeVolumePlugins(tmpDir), volumetest.NewFakeVolumeHost("fake", nil, nil))
        plugin, err := plugMgr.FindPluginByName("flexvolume-kubernetes.io/fakeAttacher")
        if err != nil {
-               t.Errorf("Can't find the plugin by name")
+               t.Fatalf("Can't find the plugin by name")
        }
        if plugin.GetPluginName() != "flexvolume-kubernetes.io/fakeAttacher" {
                t.Errorf("Wrong name: %s", plugin.GetPluginName())

is called for?

@liggitt
Copy link
Member

liggitt commented Jul 19, 2017

Is that expected that after the first failure, things can just SIGSEGV, or should even the tests be defensive and something like

in a table test, depends how fatal the error is... if you can clean up inside the loop and run other tests and get useful info, just t.Errorf() and continue

@adelton
Copy link
Contributor Author

adelton commented Jul 19, 2017

The rest of

, plugin is used in every call, so if it's nil, nothing else in TestCanSupport can pass, really.

@adelton
Copy link
Contributor Author

adelton commented Jul 19, 2017

Filed #49203 for that now.

@jsafrane
Copy link
Member

Currently Kubernetes expects that a volume is mounted as a file system to a pod. For example, Flex documentation says:

<driver executable> mount <mount dir> <json options>

It's not a <mount file>. There is an ongoing discussion about how to support block devices in Kubernetes. It may end up in exactly the same approach as you use here, but for now a volume should be a filesystem. The best would be to mount a tmpfs to mount-dir and bind your file there.

@adelton
Copy link
Contributor Author

adelton commented Jul 20, 2017

Do you say that bind-mounting in general (even directory) is not expected to work, or just bind-mounting files?

Can you point me to the discussion?

@jsafrane
Copy link
Member

Do you say that bind-mounting in general (even directory) is not expected to work, or just bind-mounting files?

Bind-mounting directories should work.

Block storage discussion: https://docs.google.com/document/d/1XeNFxc89C54psYqz4RErk1xcso0wMrKtSfWqI6POvaU/edit#heading=h.54andwjaz4be

@jsafrane
Copy link
Member

Bind-mounting directories should work.

Testing that now, clearing a bind-mounted dir fails because isNotMounted does not catch these bound directories as mounts as long as the parent is on the same filesystem as the bound directory.

@adelton, are you interested in bind-mounted dirs in flex volumes? Edit the commit message to add support for bound directories, the code itself is OK.

@jsafrane
Copy link
Member

/assign

@adelton
Copy link
Contributor Author

adelton commented Jul 20, 2017

Bind-mounting directories should work.

Mounting works, unmounting bind-mounted directory fails though, with

unmounter.go:59] Warning: Path: /path/.../test-flex already unmounted

because the code does not detect the bind-mountpoint where the filesystem id is the same both for the mountpoint and for the parent directory -- and that's the logic that the check uses to find out if something is mounted on a directory or not.

This patch obviously fixes that issue as well.

Will you be willing to consider this change if commit message was fixed to talk about directories, not files?

@adelton
Copy link
Contributor Author

adelton commented Jul 20, 2017

Ah, I've added comment along the same lines as you. Will fix the commit message.

Any opinion about the state of the tests there, namely the execScriptTempl2?

@msau42
Copy link
Member

msau42 commented Jul 20, 2017

We had this issue with directories (not mount points) for local volumes too. We fixed it by adding a new IsNotMountPoint() that has to iterate through all the mounts on the system. Performance could be slow if there are a lot of mounts on the system, but we decided to take that hit for now, and can look into caching later.

@msau42
Copy link
Member

msau42 commented Jul 20, 2017

PR #48402 added the new unmount method.

@jsafrane
Copy link
Member

@msau42, IMO a flex volume plugin does not need to mount anything at all, it can mimic Secrets or ConfigMap with magic files, just with source of the files outside of Kubernetes control.

For bind-mounted directories, the isNotMounted which calls
IsLikelyNotMountPoint fails because the filesystem of the mounted
location and the parent directory are the same.

Addressing:
unmounter.go:59] Warning: Path: /path/.../test-dir already unmounted
@adelton
Copy link
Contributor Author

adelton commented Jul 24, 2017

Fixed the commit message to only talk about bind-mounted directories, rebase on master -> 6b7d4b7.

@adelton adelton changed the title Allow unmounting bind-mounted files. Allow unmounting bind-mounted directories. Jul 24, 2017
@adelton
Copy link
Contributor Author

adelton commented Jul 24, 2017

/retest

@jsafrane
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2017
@jsafrane
Copy link
Member

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adelton, jsafrane

Associated issue requirement bypassed by: jsafrane

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2017
@jsafrane
Copy link
Member

/retest

1 similar comment
@jsafrane
Copy link
Member

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49444, 47864, 48584, 49395, 49118)

@k8s-github-robot k8s-github-robot merged commit 1feb0fa into kubernetes:master Jul 24, 2017
@resouer
Copy link
Contributor

resouer commented Aug 13, 2017

@jsafrane Will this be cherry picked into 1.7, or it targets to 1.8?

Currently, I have to mount a tmpfs into hostPath to fool IsLikelyNotMountPoint, would like to see this been released.

@adelton
Copy link
Contributor Author

adelton commented Aug 14, 2017

I've filed #50596 cherry pick for 1.7 now.

@wojtek-t wojtek-t added this to the v1.7 milestone Aug 23, 2017
@wojtek-t wojtek-t added cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Aug 23, 2017
k8s-github-robot pushed a commit that referenced this pull request Aug 24, 2017
Automatic merge from submit-queue

Automated cherry pick of #49118 upstream release 1.7

Cherry pick of #49118  on release-1.7.

#49118 : Allow unmounting bind-mounted directories

```release-note
It is now posible to use flexVolumes to bind mount directories and files.
```
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.