Skip to content

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

Merged
vdemeester merged 1 commit intomoby:masterfrom
dmandalidis:xattr-fix
Dec 12, 2018
Merged

Ignore xattr errors on copy (fixes #38155)#38316
vdemeester merged 1 commit intomoby:masterfrom
dmandalidis:xattr-fix

Conversation

@dmandalidis
Copy link
Copy Markdown
Contributor

@dmandalidis dmandalidis commented Dec 4, 2018 •
edited by cpuguy83
Loading

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
Copy Markdown
Contributor Author

dmandalidis commented Dec 4, 2018

powerpc failure seems irrelevant

cpuguy83
cpuguy83 requested changes Dec 4, 2018
View reviewed changes
Copy link
Copy Markdown
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.

container/container_unix.go Outdated
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Dec 4, 2018

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
Copy Markdown
Contributor Author

@dmandalidis dmandalidis Dec 5, 2018

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
Copy Markdown

codecov bot commented Dec 5, 2018 •
edited
Loading

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

AkihiroSuda
AkihiroSuda reviewed Dec 5, 2018
View reviewed changes
container/container_unix.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Dec 5, 2018

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
Copy Markdown
Contributor Author

@dmandalidis dmandalidis Dec 5, 2018

Choose a reason for hiding this comment

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

fixed

cpuguy83
cpuguy83 approved these changes Dec 6, 2018
View reviewed changes
Copy link
Copy Markdown
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

@dmandalidis
Ignore xattr ENOTSUP errors on copy (fixes #38155)
d0192ae 
Signed-off-by: Dimitris Mandalidis <dimitris.mandalidis@gmail.com>
vdemeester
vdemeester approved these changes Dec 12, 2018
View reviewed changes
Copy link
Copy Markdown
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 🐯

@vdemeester vdemeester merged commit f34727f into moby:master Dec 12, 2018
@thaJeztah thaJeztah mentioned this pull request Dec 12, 2018
Merged
@tyhi tyhi mentioned this pull request Dec 12, 2018
Closed
This was referenced Dec 12, 2018
Merged
Merged
@antonmoeriksson antonmoeriksson mentioned this pull request Dec 12, 2018
Closed
@mumoshu mumoshu mentioned this pull request Apr 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dmandalidis @vdemeester @cpuguy83 @AkihiroSuda @yongtang @GordonTheTurtle