detect if inside Docker container #428
Conversation
|
@tduffield review? |
|
I haven't tried it personally but it makes sense. +1 |
|
Cool. @paulczar can you share the verification that you've done for this? As long as we're comfortable with the existing coverage we can get this in. |
| @@ -170,9 +174,9 @@ def lxc_version_exists? | |||
| # Kernel docs, https://www.kernel.org/doc/Documentation/cgroups | |||
| if File.exists?("/proc/self/cgroup") | |||
| if File.read("/proc/self/cgroup") =~ %r{^\d+:[^:]+:/(lxc|docker)/.+$} | |||
There was a problem hiding this comment.
There's a set of users who may be using certain versions of docker and are switching on these values returning LXC who would be broken by this (presuming they exist).
Do we know if previous to Docker 0.9, when Docker used LXC, if it said docker or lxc here? What about Docker 0.9+ with libcontainer? I'd like to see a unit test for each case that makes it clear what happens in those cases.
@atomic-penguin, any thoughts on this ?
There was a problem hiding this comment.
@btm the check for the files /.dockerenv or /.dockerinit look more reliable than what we did in the past. It looks like /.dockerinit was added around July 2013, and has been around since v0.5.1. It wasn't something that I knew about when I submitted the original pull request to add to Ohai. As far as I can tell examining the code in this PR, this would provide better Docker detection that what we currently have, and I do not see that it breaks backwards compatibility.
Had to look back at my OHAI-551 notes. Prior to 0.9, Docker used the cgroup namespace /lxc/. Post 0.9, it used the cgroup namespace /docker/. The spec test removed in this PR appears to be incorrect, and is certainly my fault. There may have been good reason for it at the time, since we didn't yet have the virtualization.systems hash.
For docker guests, the presence of the /.dockerinit || /.dockerenv should detect most Docker instances excepting early first six month alpha builds (<= v0.5.0) of Docker.
There was a problem hiding this comment.
The theoretically backward compat concern would be that if you were using docker and it's reporting docker in /etc/self/cgroup, so virtualization[:system] is getting set to lxc, and you've got recipes that are switching on that. Once we ship this, virtualization[:system] is going to report docker instead and that code won't run.
There was a problem hiding this comment.
@tduffield @atomic-penguin @paulczar so, I think this only would affect a couple people, if any, but it's a breaking change and therefore semver says we should wait to ship it. Could you give this a second thought and potentially disagree?
The alternative maybe is to leave this part of the code alone for now (adding in the dockerinit feature), then separately changing this code to work this way. I'm not sure how useful that is to anyone, but perhaps if the dockerinit stuff is more reliable than cgroups, i.e. maybe sometimes the cgroup file doesn't exist, we at least get better coverage.
|
I'm a little concerned there may be a breaking change in changing the existing lxc detection code, otherwise looks good. |
|
Just another opinion. This is minor version and is intending to work "provide better Docker detection that what we currently have". The lxc concern - is there a test that can be done to see if there is an issue? |
|
@tjsoftworks regarding testing, the code is pretty clear that the ohai used to report "lxc" when it saw docker there, and now it will report docker. I don't know how many people that would affect, and I agree that the behavior in this patch is more correct, but that change technically needs to wait. In the interim we could take the dockerinit patch as that happens later and wouldn't change that behavior, but if /proc/self/cgroup still contains docker on those systems, you're going to keep getting lxc from ohai until the next major release of ohai. |
|
@paulczar if you would split the added dockerinit feature into a separate pull request, I think we'd take that now. |
|
This could go into master, as we now have a 7-stable branch of ohai. Then if someone wanted the dockerinit stuff to go into a 7.x.y release, they could provide a separate pull request for just that code. @tyler-ball and I have reasoned about this and think this is probably good to go into master now that we have a new 7-stable branch. @sersut, agree? |
|
Currently |
|
Some confusion here from all the comments, do I need to change anything? or is it okay as is? |
|
@sersut Because this is a breaking change, can we bump master to 8.0.0 when we merge this to master? @paulczar If we merge this to a new greater version (like 8.0.0) the whole change can go in. If we want to merge this to a 7.x.x branch, we would have to make changes to maintain backwards compatibility. |
|
@paulczar it would be convenient to have this branch as two separate commits. One would add the .dockerinit feature, and the second change the existing cgroup behavior. |
|
rereading the code, I don't think it makes sense to break it out into two commits. the .dockerinit feature is unlikely to ever be used because it should be matched by the cgroup checks, it's just there to extend coverage for when/if docker supports something weird like solaris zones. |
|
After a bunch of discussion in office hours and in chef's hipchat, we should just ship this. The matrix of possible semver compliance options is just absurd and this isn't going to break a lot of people. The number of people it may break is less horrible than the semver options IMHO. |
No description provided.