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

Carry 9684: Support --storage-opt for aufs graph driver #10390

Closed
wants to merge 7 commits into from

Conversation

tiborvass
Copy link
Contributor

dqminh and others added 7 commits January 27, 2015 14:13
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
Signed-off-by: Daniel, Dao Quang Minh <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
@crosbymichael
Copy link
Contributor

LGTM

1 similar comment
@jessfraz
Copy link
Contributor

LGTM

@SvenDowideit
Copy link
Contributor

@tiborvass can we please link to the dirperm1 long explanation from the known issues in the docs/sources/release-notes.md

also, while the description of using that option is technically correct, it doesn't make it obvious that there is a disadvantage to using the dirperm1 option - and so the a user would reasonably ask - why don't we make that the default, thus fixing the long standing bug?

@tiborvass
Copy link
Contributor Author

@SvenDowideit what do you suggest? This PR is supposed to close #783. Should we still keep that in the known issues? Let me know what you'd like to see and I'll add it.

@dqminh
Copy link
Contributor

dqminh commented Feb 6, 2015

@SvenDowideit @tiborvass afaik only ubuntu 14.10 and after carry the dirperm1 patches by default, or if you build the aufs patches yourself. So i dont think it can be the default yet.

Perhaps we should add a note to #783 to tell people about the workaround ?

@tiborvass
Copy link
Contributor Author

@dqminh so there are no downsides? It's just that if it's not available there's no fix.

I think this PR fixes that issue. Ping @SvenDowideit

@dqminh
Copy link
Contributor

dqminh commented Feb 6, 2015

@tiborvass Yes, if it's not available, you cant use it :D

I think that in Docker usecase, there's no downside. Not enable dirperm1 prevents user from changing permission bits of a directory in lower layer to a broader level, but you can already do it in other graph drivers so this point is moot.
The (pro) side effect of dirperm1 is that it's a bit faster since it doesnt have to walk all the layers to check for permissions.

@estesp
Copy link
Contributor

estesp commented Feb 6, 2015

@tiborvass I think @dqminh's point is probably what Docker should focus on as far as the "why dirperm1 makes sense to enable" --for those on recent Ubuntu distros who can take advantage. The fact that the bug is extremely confusing when you hit it (I remember trying to help @duglin figure out a Dockerfile that worked until you switched the order of a chmod and an ADD last fall! very confusing because the standard ls -l perms look right, but you can't access the files/dirs), not to mention the fact that switching graph drivers makes the "bug" go away. So, dirperm1 where available makes AUFS backend have the same COW/layer characteristics as the other graph drivers.

@tiborvass
Copy link
Contributor Author

@dqminh @estesp so would it make sense to enable it by default? Does it mean we should remove it from --storage-opt ?

@estesp
Copy link
Contributor

estesp commented Feb 6, 2015

@dqminh stated it isn't available until Ubuntu 14.10, so other than the fact that it may not be supported depending on base OS, I believe on by default is a reasonable approach.

My only other thought: would we want to allow for it to be disabled in case there is a strange use case scenario where someone is relying on current behavior?

@tiborvass
Copy link
Contributor Author

@estesp your last question is basically a better formulation of "what's the downside of using dirperm1?", and what I've been told so far is that there is none.

@estesp
Copy link
Contributor

estesp commented Feb 6, 2015

@tiborvass how is this for a not-so-helpful response: there is no downside unless someone saw an upside to weird permissions errors from Dockerfile ordering problems :-} I don't know how to answer that without changing the default and seeing who screams. I would hope no one, and looking at the long painful history of #783 (and all its dups and related issues), you would think changing the default will make a lot of people very very happy.

@tiborvass
Copy link
Contributor Author

Thanks @estesp! I'll wait for @dqminh's answer too.
If anyone has opinions on this, now is the time!

@tiborvass
Copy link
Contributor Author

@dqminh I should probably do something like in #10534

@dqminh
Copy link
Contributor

dqminh commented Feb 7, 2015

@tiborvass #10534 case is different though, because afair directio is widely supported ( back to ubuntu 12.04 ), while dirperm1 is only supported since 14.10 ( ubuntu is probably the only distribution that is shipping aufs by default afaik ).

Enabling dirperm1 by default will then make the older systems not able to run aufs driver unless they recompile the aufs extension, which is sort of bad because ubuntu LTS stack falls into those systems.

I think it's safer for dirperm1 to be opted in for now, and maybe enabled by default after 15.04.

@tiborvass
Copy link
Contributor Author

@dqminh but isn't there a way to detect whether dirperm1 is available and enable it if it is?

@dqminh
Copy link
Contributor

dqminh commented Feb 7, 2015

Hmm, maybe we can also test mounting with dirperm1 at first mount to detect
if dirperm1 is supported ? If we fail to set dirperm1, then retry without
dirperm1 for the rest of the ops. How does it sound ?

On Sunday, 8 February 2015, Tibor Vass [email protected] wrote:

@dqminh https://github.com/dqminh but isn't there a way to detect
whether dirperm1 is available and enable it if it is?


Reply to this email directly or view it on GitHub
#10390 (comment).

@tiborvass
Copy link
Contributor Author

@dqminh IANTM, but sure why not!

Ping @crosbymichael @tianon @unclejack

@unclejack
Copy link
Contributor

@tiborvass Thanks for letting me know about this.

Perhaps we could test if this dirperm1 feature is available during the aufs init phase? @dqminh I'd be +1 to doing that check as you've also written above.

We should try to detect this on the fly and print a warning if it's not supported. I think this is the best approach and that it would be a better experience for the user.

@tiborvass
Copy link
Contributor Author

@unclejack Thanks for your input! I'll see what I can do to do that, but if someone wants to send me a PR, i'll cherry-pick their commits onto this branch :)

@tianon
Copy link
Member

tianon commented Feb 9, 2015

I think if we find a good way to check for dirperm1 in the code (and thus prefer it), we should also add CONFIG_AUFS_DLGT to check-config.sh in the AUFS section (http://sourceforge.net/p/aufs/mailman/message/18822722/). 👍

@SvenDowideit
Copy link
Contributor

@tiborvass http://docs.docker.com/release-notes/#known-issues lists the problem atm, and this is (if i understand correctly) a fix which may not be available.

so yes, I'd keep the known issue there for now, but add a link from it to the docs you added about the mount-opt - pointing out that it needs settings that can be checked with check-config.sh.

Additionally - @tianon can we make sure boot2docker has things set up right?

@icecrime
Copy link
Contributor

icecrime commented Mar 7, 2015

Ping @tiborvass: it's unclear if you have something left to do here, or if this should be moved to 3-code-review.

@tiborvass
Copy link
Contributor Author

@icecrime thanks for the reminder, I do have something to do here :( Will try to update shortly

@tiborvass
Copy link
Contributor Author

I'll reopen when I update today, sorry about the delay

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

Successfully merging this pull request may close these issues.

unexpected file permission error in container