Skip to content

/continuity

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

fs: add WithAllowXAttrErrors CopyOpt #138

Merged
merged 3 commits into from Dec 3, 2018

Conversation

@AkihiroSuda
Copy link
Member

commented Oct 29, 2018

This option allows ignoring errors during copying xattr like security.selinux,
which is not always supported.

Reported in several issues including

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp
@AkihiroSuda AkihiroSuda referenced this pull request Oct 29, 2018
Closed
@AkihiroSuda

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 29, 2018

cc @tonistiigi
@AkihiroSuda AkihiroSuda referenced this pull request Oct 29, 2018
Closed

@AkihiroSuda AkihiroSuda force-pushed the AkihiroSuda:xattr branch from ea1dcc8 to 40874d2 Oct 29, 2018

@AkihiroSuda

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Oct 30, 2018

So the PR appeared to have nothing to do with the buildkit failure but should be still useful
@nixpanic
nixpanic suggested changes Nov 21, 2018
View changes
fs/copy.go Outdated Show resolved Hide resolved
AkihiroSuda and others added 2 commits Oct 29, 2018
@AkihiroSuda @nixpanic
fs: add WithAllowXAttrErrors CopyOpt 
This option allows ignoring errors during copying xattr like `security.selinux`,
which is not always supported.

Reported in several issues including
* openfaas/openfaas-cloud#312
* moby/buildkit#704
* genuinetools/img#45
* https://bugzilla.redhat.com/show_bug.cgi?id=1596918

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
cc6b518
@nixpanic
fs/copy: continue copying xattrs in case one fails 
Signed-off-by: Niels de Vos <ndevos@redhat.com>
Loading status checks…
194c23c
@nixpanic

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 21, 2018

Please consider this change on top of yours. It continues copying xattrs for a file, even if one of them fails (like security.selinux).

Feel free to incorporate the modification in an updated PR. Thanks!

@AkihiroSuda AkihiroSuda force-pushed the AkihiroSuda:xattr branch from 40874d2 to 194c23c Nov 22, 2018

@AkihiroSuda

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 22, 2018

@nixpanic Thanks, cherry-picked your commit
@AkihiroSuda

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 22, 2018

@dmcgowan @cpuguy83 PTAL?
@cpuguy83

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 22, 2018

Oh, nice!
Also related: moby/moby#38155
@cpuguy83

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 22, 2018

I think for this case, being able to pass in an error handler on file copy would be handy.

Something like:

func(err error) error

Where if the handler wants to ignore the error it would return nil.
@AkihiroSuda

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 22, 2018

Added type XAttrErrorHandler func(dst, src, xattrKey string, err error) error. PTAL.
@nixpanic
nixpanic approved these changes Nov 22, 2018
View changes
Copy link
Contributor

left a comment

LGTM, thanks!
nixpanic added a commit to nixpanic/external-storage that referenced this pull request Nov 22, 2018
@nixpanic
dep: use containerd/continuity#138 for xattr fixes
9c448cf
@nixpanic

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 30, 2018

Hey guys, any progress with reviewing/merging? This PR works well for me and I'd like to see it included so that I can consume it in kubernetes-incubator/external-storage#1013 and a future CSI driver.

Thanks!
@mikebrow
mikebrow reviewed Nov 30, 2018
View changes
fs/copy.go Outdated Show resolved Hide resolved
@mikebrow
mikebrow reviewed Nov 30, 2018
View changes
Copy link
Member

left a comment

see comment
@crosbymichael
crosbymichael approved these changes Nov 30, 2018
View changes
@AkihiroSuda
fs: add XAttrErrorHandler 
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Loading status checks…
a664d80

@AkihiroSuda AkihiroSuda force-pushed the AkihiroSuda:xattr branch from b83cbd2 to a664d80 Nov 30, 2018

@cpuguy83
cpuguy83 approved these changes Nov 30, 2018
View changes
@estesp
estesp approved these changes Dec 3, 2018
View changes
Copy link
Contributor

left a comment

LGTM

@estesp estesp merged commit 004b464 into containerd:master Dec 3, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crosbymichael crosbymichael

@mikebrow mikebrow

@nixpanic nixpanic

@cpuguy83 cpuguy83

@estesp estesp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@AkihiroSuda @nixpanic @cpuguy83 @crosbymichael @estesp @mikebrow
You can’t perform that action at this time.