Allow for xattr copy failure for vfs#45420
Conversation
vfs is declared to work with any filesystem, but after moby@31f654a it's no longer working with NFS. As the extended attribute support depends on filesystem and if we do copy it in vfs and do not allow failure, that would essentially mean that vfs does NOT support all filesystems but only those that support xattr. So we should just try to copy security.capabilities and allow for failure. In this way, vfs come back to the state of being able to run on any filesystem as declared in https://docs.docker.com/storage/storagedriver/select-storage-driver/. Fixes moby#45417 Signed-off-by: Chen Qi <[email protected]>
thaJeztah
added
area/storage
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
NACKing for design review. The change which introduced this "regression" fixed a real bug in the VFS driver:
Allowing the VFS driver to work on filesystems which do not support xattrs will seemingly work at first but will cause hard-to-diagnose problems for containers which depend on file capabilities for proper functioning.
| // The copyOpaqueXattrs controls if "trusted.overlay.opaque" xattrs are copied. | ||
| // Passing false disables copying "trusted.overlay.opaque" xattrs. | ||
| func DirCopy(srcDir, dstDir string, copyMode Mode, copyOpaqueXattrs bool) error { | ||
| func DirCopy(srcDir, dstDir string, copyMode Mode, copyOpaqueXattrs bool, allowXattrFailure bool) error { |
There was a problem hiding this comment.
Can we make this a daemon option?
There was a problem hiding this comment.
Yeah, I'd be okay with a storage-opt or something like that, so long as it's opt-in and flagged in docker info so we can identify if any bug reports are self-inflicted
There was a problem hiding this comment.
I agree. I'll check how to do it and make that option opted in by default.
@corhere I know the previous commit solved a real problem for images with xattrs. However, at the same time, it also introduced a real regression for images with no xattrs. How can an image fail for xattr copying when it does not have any? @AkihiroSuda Daemon option is a good idea. I'll do it :) |
That's a very, very good point. I was too focused on the implications with the vfs driver, and didn't give enough consideration to the issue being with DirCopy. In that case, I don't think a daemon or As for my criticisms of using the vfs graph driver on xattr-unsupported filesystems, I'm really taking issue with the unarchiver ignoring |
4 participants
vfs is declared to work with any filesystem, but after 31f654a it's no longer working with NFS.
As the extended attribute support depends on filesystem and if we do copy it in vfs and do not allow failure, that would essentially mean that vfs does NOT support all filesystems but only those that support xattr.
So we should just try to copy security.capabilities and allow for failure. In this way, vfs come back to the state of being able to run on any filesystem as declared in https://docs.docker.com/storage/storagedriver/select-storage-driver/.
Fixes #45417
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)