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

Ignore xattr errors on copy (fixes #38155) #38316

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Ignore xattr errors on copy (fixes #38155) #38316

merged 1 commit into from
Dec 12, 2018

Conversation

dmandalidis
Copy link
Contributor

@dmandalidis dmandalidis commented Dec 4, 2018

Signed-off-by: Dimitris Mandalidis dimitris.mandalidis@gmail.com

- What I did
Allowed copy an existing folder, ignoring xattr set errors when the target filesystem doesn't support xattr.

- How I did it
Updated containerd/continuity to take advantage of newly-introduced CopyDirOpt. Passed fs.WithAllowXAttrErrors at fs.CopyDir of container_unix.go#copyExistingContents to ignore xattr set errors

- How to verify it

  • Setup an empty NFS share
  • Build a docker image with a dummy folder containing a simple file
  • Run the image as a service in a SELinux-enabled installation and map the dummy folder to the empty NFS share.
  • File should be copied without problems although NFS doesn't support extended attributes

- Description for the changelog
Ignore extended attributes set errors during copy.

Fixes #38155

@dmandalidis
Copy link
Contributor Author

powerpc failure seems irrelevant

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Just a question on how we are handling the errors. Right now it will ignore all errors but I think to fix the bug we may only want to ignore on "not supported" and possibly permissions errors... but there may be other cases I am unaware of.

@@ -399,7 +399,7 @@ func copyExistingContents(source, destination string) error {
// destination is not empty, do not copy
return nil
}
return fs.CopyDir(destination, source)
return fs.CopyDir(destination, source, fs.WithAllowXAttrErrors())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make our own handler here and explicitly check for ENOTSUP or possibly EPERM.
WDYT? @AkihiroSuda ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 imo the best approach would be to check-before-set, especially for ENOTSUP. In that way we could horizontally control whether we fail for unsupported operations or not. However, I 've sent a change reflecting your comment to have an example to discuss. Intuitively, it doesn't look very nice as I think we 're going too deep, but I 've no other arguments against this since syscall errno is also examined elsewhere.

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@3e44f58). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38316   +/-   ##
=========================================
  Coverage          ?   36.56%           
=========================================
  Files             ?      610           
  Lines             ?    45281           
  Branches          ?        0           
=========================================
  Hits              ?    16555           
  Misses            ?    26399           
  Partials          ?     2327

// are not supported
func ignoreUnsupportedXAttrs() fs.CopyDirOpt {
xeh := func(dst, src, xattrKey string, err error) error {
if err != syscall.ENOTSUP {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors.Cause(err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Dimitris Mandalidis <dimitris.mandalidis@gmail.com>
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants