Apply _netdev mount option in bind mount if available #68626

gnufied · 2018-09-13T16:29:14Z

_netdev mount option is a userspace mount option and
isn't copied over when bind mount is created and remount
also does not copies it over and hence must be explicitly
use with bind mount

Without _netdev mount option, a bind mount might not get unmounted before network is stopped on a node and hence soft-shutting down the node will not work.

Fixes #68692

/sig storage

cc @jingxu97 @msau42

Apply _netdev mount option on bind mount

_netdev mount option is a userspace mount option and isn't copied over when bind mount is created and remount also does not copies it over and hence must be explicitly used with bind mount

gnufied · 2018-09-14T13:53:20Z

/test pull-kubernetes-integration

gnufied · 2018-09-14T21:13:53Z

/assign @saad-ali

saad-ali · 2018-09-14T21:43:05Z

pkg/util/mount/mount.go

@@ -286,7 +286,7 @@ func IsNotMountPoint(mounter Interface, file string) (bool, error) {
 // use in case of bind mount, due to the fact that bind mount doesn't respect mount options.
 // The list equals:
 //   options - 'bind' + 'remount' (no duplicate)
-func isBind(options []string) (bool, []string) {
+func isBind(options []string) (bool, []string, []string) {


Why is this being special cased in the bind mount? Shouldn't it be a mount option for the global mount? That way the bind mounts can be simple?

The problem is - when we bind mount we are dropping ALL mount options supplied by global mount. That is because traditionally Kernel drops most mount options when bind mounting something. So even if global mount did use _netdev, the bind mount will not use it.

So, this fix makes sure that - if global mount had _netdev, then we must carry that option when creating bind mount. We ourselves don't supply _netdev but we expect that mount option to come from global mount (either via storageClass mount option or plugin uses _netdev as one of the default mount options).

can we just apply all mount options we applied to global mount to the bind mount too? Then we don't have to special case netdev

can we just apply all mount options we applied to global mount to the bind mount too? Then we don't have to special case netdev

We should start with just special casing. Suddenly having all mount options apply to bind may have unexpected results.

It's a bit confusing because it looks like for bindRemountOpts we are applying all the mount options from the global mount.

The problem is - remount also does not copy _netdev mount option over, if original bind mount did not had it. I am somewhat skeptical about using remount to apply mount options to be honest. We know that - mount options that Kernel knows about are automatically copied over from original mount point when bind mount is created. We don't need remount for that.

And userspace mount options aren't copied over on remount.

Where is the "_netdev" mount option coming from since it's not from the global mount?

I see that:

bind options will return "bind" and "_netdev" if it's set

bind remount options will return "bind", "remount", and all other options (including "_netdev")

Do we really need to special case the "_netdev" in the first bind mount if the remount is going to already include it?

Do we really need to special case the "_netdev" in the first bind mount if the remount is going to already include it?

Yeah we do need to. If first bind mount did not include _netdev mount option then _netdev mount option on remount has no effect and it isn't applied.

Thanks for clarifying. This looks fine then. Another issue I found is that all other plugins besides rbd are not using storageclass mount options in SetUp, so this fix will only work for rbd. We can handle the other plugins separately.

Yeah - I logged #68797 to fix that as a follow up item.

saad-ali · 2018-09-14T21:43:26Z

pkg/util/mount/mount.go

+
+func checkForNetDev(options []string) bool {
+	for _, option := range options {
+		if option == "_netdev" {


Any issues with specifying this option on a non-systemd host?

Thing is - this PR does not add _netdev mount option by default, it just says if user or plugin author has specified _netdev mount option, I will try to honor it while creating bind mount.

saad-ali · 2018-09-14T21:44:21Z

pkg/util/mount/mount.go

+		if option == "_netdev" {
+			return true
+		}
+	}


NFS has a similar issue, where we can't unmount if share (server is disabled or network issues). Can we apply this to NFS as well?

may be. But systemd at least already knows that NFS mounts are using network. This problem is more severe in case of block storage volume types, which appear as regular block device to systemd but actually are networked and hence must be unmounted before network is toredown during shutdown sequence.

The premise of the PR isn't for Kubernetes to apply _netdev whenever it can, but it is to honor that mount option, if user or plugin author has chosen to use it while creating bind mounts.

saad-ali · 2018-09-18T03:21:04Z

/approve

@msau42 for LGTM

msau42 · 2018-09-18T17:58:11Z

/lgtm

k8s-ci-robot · 2018-09-18T17:58:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42, saad-ali

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:

~~pkg/util/mount/OWNERS~~ [saad-ali]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nikhita · 2018-09-25T08:43:12Z

/kind bug

k8s-ci-robot requested review from jsafrane and saad-ali September 13, 2018 16:29

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 13, 2018

Apply _netdev mount option in bind mount if available

e881a29

_netdev mount option is a userspace mount option and isn't copied over when bind mount is created and remount also does not copies it over and hence must be explicitly used with bind mount

gnufied force-pushed the fix-netdev-mount-opt branch from 1870c6d to e881a29 Compare September 13, 2018 17:47

k8s-ci-robot assigned saad-ali Sep 14, 2018

saad-ali reviewed Sep 14, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2018

k8s-ci-robot assigned msau42 Sep 18, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2018

gnufied mentioned this pull request Sep 18, 2018

Setup call to volume plugins should include user and plugin provided mount options #68797

Open

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 25, 2018

k8s-ci-robot merged commit c7a67b3 into kubernetes:master Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply _netdev mount option in bind mount if available #68626

Apply _netdev mount option in bind mount if available #68626

gnufied commented Sep 13, 2018 •

edited

Loading

gnufied commented Sep 14, 2018

gnufied commented Sep 14, 2018

saad-ali Sep 14, 2018

gnufied Sep 17, 2018

msau42 Sep 17, 2018

saad-ali Sep 18, 2018

msau42 Sep 18, 2018

gnufied Sep 18, 2018

msau42 Sep 18, 2018

gnufied Sep 18, 2018

msau42 Sep 18, 2018

gnufied Sep 18, 2018

saad-ali Sep 14, 2018

gnufied Sep 17, 2018

saad-ali Sep 18, 2018

saad-ali Sep 14, 2018

gnufied Sep 17, 2018

saad-ali commented Sep 18, 2018

msau42 commented Sep 18, 2018

k8s-ci-robot commented Sep 18, 2018

nikhita commented Sep 25, 2018

Apply _netdev mount option in bind mount if available #68626

Apply _netdev mount option in bind mount if available #68626

Conversation

gnufied commented Sep 13, 2018 • edited Loading

gnufied commented Sep 14, 2018

gnufied commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Sep 18, 2018

msau42 commented Sep 18, 2018

k8s-ci-robot commented Sep 18, 2018

nikhita commented Sep 25, 2018

gnufied commented Sep 13, 2018 •

edited

Loading