[overlay] add configurable mount options to overlay snapshotter #8676
Conversation
snapshots/overlay/overlay.go
Outdated
| // NOTE: Options are not applied to bind mounts. | ||
| func WithMountOptions(options []string) Opt { | ||
| return func(config *SnapshotterConfig) error { | ||
| config.mountOptions = options |
There was a problem hiding this comment.
| config.mountOptions = options | |
| config.mountOptions = append(config.mountOptions, options) |
Is it better to use an append operation here?
| if err != nil { | ||
| logrus.WithError(err).Warnf("cannot detect whether \"userxattr\" option needs to be used, assuming to be %v", userxattr) | ||
|
|
||
| if !hasOption(config.mountOptions, "userxattr", false) { |
There was a problem hiding this comment.
Looks like you may silently enable userxattr even if a user didn't want it, I mean didn't specify it. Is it better to allow a user to choose and if option is specified we check whether kernel supports it, etc or to completely rely on a user's choice without extra checks?
There was a problem hiding this comment.
Is it better to allow a user to choose and if option is specified we check whether kernel supports it, etc or to completely rely on a user's choice without extra checks?
No, that's a breaking change.
There was a problem hiding this comment.
No, that's a breaking change.
IIUC, this will be a backward compatibility issue. Anyway, in this case there's no way to "unset" userxattr option, even though by default it is "on" this PR adds a possibility to "unset" options.
Another case I see may appear with conflicting options userxattr|metacopy|redirect_{dir,follow,etc.}, even though for time being this may be omitted but later this will be the case, because you will need to break this behavior, otherwise it looks like there's no way to support metacopy|redirect_dir at least as it is now.
There was a problem hiding this comment.
In the user namespace it is essential to specify userxattr.
torvalds/linux@2d2f2d7
Outside the user namespace it shouldn't be specified (and actually not specified in this PR).
There was a problem hiding this comment.
If options conflict with userxattr, a check could be added for it. I would suggest that be done in a follow up though since this change is scoped to adding the options and not any behavior change or new functionality.
There was a problem hiding this comment.
Outside the user namespace it shouldn't be specified (and actually not specified in this PR).
As I can see, this PR opens the possibility to do that, e.g. mount_options=userxattr,... :D
If options conflict with userxattr, a check could be added for it. I would suggest that be done in a follow up though since this change is scoped to adding the options and not any behavior change or new functionality
Ok, the last moment from me -- what about to update the documentation (not sure whether it has anything about which options containerd supports and which doesn't) to prevent a user to specify any conflicting options.
Also, looks like it is reasonable to at least warn a user that even though this option didn't specify it is enabled.
There was a problem hiding this comment.
The documentation can be updated in a later PR. We don't always immediately document new features like this, especially as you pointed out, there may be edge cases to figure out that should be pointed out to users.
Allows default mount options to be provided through configuration. Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
overlay-options
branch
from
67f62c7 to
d115129
Compare
fuweid
added
the
cherry-picked/1.6.x
estesp
added
the
cherry-picked/1.7.x
…/main Merge upstream containerd/main at commit 5d1ab01 into ado fork-external/main Related work items: containerd#7944, containerd#8174, containerd#8334, containerd#8362, containerd#8572, containerd#8582, containerd#8588, containerd#8605, containerd#8606, containerd#8617, containerd#8619, containerd#8626, containerd#8627, containerd#8633, containerd#8637, containerd#8641, containerd#8643, containerd#8645, containerd#8652, containerd#8667, containerd#8672, containerd#8673, containerd#8676, containerd#8680, containerd#8684, containerd#8685, containerd#8692, containerd#8696, containerd#8697, containerd#8701, containerd#8708, containerd#8717, containerd#8726, containerd#8728, containerd#8729, containerd#8731, containerd#8732, containerd#8740, containerd#8752, containerd#8758, containerd#8762, containerd#8764
Allows default mount options to be provided through configuration. This allows configurations to use options supported by the kernel without requiring an update to the overlay plugin.
Note: this still wouldn't be appropriate for the volatile option, an additional change would be necessary to strip the "volatile" option on temp mounts. Such as https://github.com/containerd/containerd/pull/8402/files#diff-5adff636c3210a8801cbc84d4431a5ee66e8bab1985a88fd59c6e895b604914c