(FACT-3202) Add is_virtual and virtual support for crio #2574
Conversation
|
Can one of the admins verify this patch? |
lollipopman
force-pushed
the
crio-container-support
branch
2 times, most recently
from
e93f832 to
0cd5606
Compare
|
Would love a review if any of the codeowners have a moment? |
|
friendly bump for a review please |
|
@joshcooper any chance you would have time to take a gander? |
|
friendly bump for a review! |
|
Would love a review, any takers? |
|
would love a review! |
|
friendly bump! |
|
Hi @lollipopman thanks for the PR, could you update your PR so there's no conflicts? Thanks! |
lollipopman
force-pushed
the
crio-container-support
branch
from
0cd5606 to
bae8f48
Compare
|
Happy to, pushed, thanks for looking @AriaXLi |
lollipopman
force-pushed
the
crio-container-support
branch
3 times, most recently
from
21afec5 to
a6f1604
Compare
|
@AriaXLi tests are passing, do you think you could take a look? thanks! |
|
@AriaXLi any chance you could look at this again, before it bitrots? <3 |
Prior to this commit the safe_readlines wrapper did not support the sep argument as supported by readlines.
lollipopman
force-pushed
the
crio-container-support
branch
from
a6f1604 to
15e7f17
Compare
|
@joshcooper patch updated, ready for another review |
|
Thank you @lollipopman looks good, just running tests |
Prior to this change facter returned:
$ facter is_virtual
false
$ facter virtual
physical
After this change facter returns:
$ facter is_virtual
true
$ facter virtual
crio
This change separates out reading pid 1's environment from proc and
reading the cgroup information. It also adds explicit support for podman
and returns container_other when the container runtime is not explicitly
supported.
lollipopman
force-pushed
the
crio-container-support
branch
from
15e7f17 to
7bc38cc
Compare
|
Tests look good, anything else to do @joshcooper? |
|
Thank you @lollipopman! |
| def post_resolve(fact_name, _options) | ||
| @fact_list.fetch(fact_name) { read_cgroup(fact_name) } | ||
| @fact_list.fetch(fact_name) do | ||
| read_environ(fact_name) || read_cgroup(fact_name) |
There was a problem hiding this comment.
If /proc/1/environ contains container=<value> where <value> is non-empty and is not one of the values in the case, then we set vm=container_other and never fall back to read_cgroup .
There was a problem hiding this comment.
true, perhaps read_environ() should only log a warning and not set the value to container_other
There was a problem hiding this comment.
@lollipopman prior to your change, we checked /proc/1/cgroups for docker
facter/lib/facter/resolvers/containers.rb
Line 39 in d47f879
And then 'lxc' or fell back to checking the container environment variable in /proc/1/environ
facter/lib/facter/resolvers/containers.rb
Line 42 in d47f879
Your commit reversed the order, so it looks in /proc/1/environ first.
facter/lib/facter/resolvers/containers.rb
Line 18 in 7bc38cc
Was that intentional? Would it be a problem if I reversed it?
One thing I didn't realize when I merged this is the VirtualDetector calls
So the Containers resolver can't hard code container_other https://github.com/puppetlabs/facter/blob/0656d9a34ce4790129f1fd6eba5bb4d49a9b9ad1/lib/facter/resolvers/containers.rb#L60C19-L60C34
There was a problem hiding this comment.
I don't think I had a strong reason for changing the order, and my fault for not adding a comment if I did. I assume my rationale was that the environment variable was more authoritative, but I don't think that argument is very strong at present. As for the hard coding of container_other I agree that should be dropped and just return nil for the fact.
There was a problem hiding this comment.
No worries @lollipopman, thanks for confirming!
|
Please fix this patch so that it doesn't warn when invoked on non-Linux systems (see issue 2742) |
5 participants
Prior to this change facter returned:
After this change facter returns:
This change separates out reading pid 1's environment from proc and reading the cgroup information. It also adds a container_other type when the container runtime is not explicitly supported.