Apply _netdev mount option in bind mount if available #68626
Conversation
k8s-ci-robot
added
sig/storage
k8s-ci-robot
added
release-note
_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
|
/test pull-kubernetes-integration |
|
/assign @saad-ali |
| @@ -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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's a bit confusing because it looks like for bindRemountOpts we are applying all the mount options from the global mount.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah - I logged #68797 to fix that as a follow up item.
|
|
||
| func checkForNetDev(options []string) bool { | ||
| for _, option := range options { | ||
| if option == "_netdev" { |
There was a problem hiding this comment.
Any issues with specifying this option on a non-systemd host?
There was a problem hiding this comment.
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.
| if option == "_netdev" { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
/approve @msau42 for LGTM |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
|
/kind bug |
k8s-ci-robot
added
kind/bug
_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
_netdevmount 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