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

update detect_virt to work on recent lxc/d #101

Closed
wants to merge 1 commit into from
Closed

update detect_virt to work on recent lxc/d #101

wants to merge 1 commit into from

Conversation

sbromberger
Copy link

@sbromberger sbromberger commented Dec 24, 2022

detect_virt does not set VIRTUALIZATION in later versions of lxd containers. I added a second check that should work.

This might be able to be better optimized (that is, right now, both checks are run regardless of the output of the first check), but as this is probably an infrequently-run function, it shouldn't be a big deal.

I've confirmed this is working on bare-metal void (where it's not set) and in void lxd containers (v5.9, where it is set).

`detect_virt` does not set `VIRTUALIZATION` in later versions of lxd containers. I added a second check that should work.

This might be able to be better optimized (that is, right now, both checks are run regardless of the output of the first check), but as this is probably an infrequently-run function, it shouldn't be a big deal.
@sbromberger sbromberger changed the title update detect_virt to work on modern lxc/d update detect_virt to work on recent lxc/d Dec 24, 2022
@@ -30,6 +30,7 @@ emergency_shell() {
detect_virt() {
# Detect LXC (and other) containers
[ -z "${container+x}" ] || export VIRTUALIZATION=1
[ -r /proc/1/environ ] && grep -qa "container=lx" /proc/1/environ && export VIRTUALIZATION=1
Copy link
Contributor

Choose a reason for hiding this comment

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

The preceding line is supposed to catch this case... is it not doing that? What happens to the container variable?

Copy link
Author

@sbromberger sbromberger Dec 24, 2022

Choose a reason for hiding this comment

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

if I'm reading it correctly, L32 just checks to see if the container environment variable is set. This doesn't happen (anymore) in recent versions of LXD, though my quick investigation suggests it was once set within containers.

EDITED to correct misconception.

Copy link
Author

@sbromberger sbromberger Dec 24, 2022

Choose a reason for hiding this comment

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

seth@hydrogen:~ $ [ -z "${container+x}" ] || echo "VIRTUAL"
seth@hydrogen:~ $ env | grep -i container
seth@hydrogen:~ $ 

(hydrogen is a container.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you 100% sure this is not an LXD VM? When/why did they change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still seeing this in their docs: "The initial environment of PID1 is blank except for container=lxc which can be used by the init system to detect the runtime."

https://linuxcontainers.org/lxd/docs/latest/container-environment/#pid1

Copy link
Author

@sbromberger sbromberger Dec 24, 2022

Choose a reason for hiding this comment

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

Further evidence that this is not making it from runit (PID1) to the run script:

root         1     0  0 13:58 ?        00:00:00 runit
root        38     1  0 13:58 ?        00:00:00 runsvdir -P /run/runit/runsvdir/current
...
root      4631    38  0 15:10 ?        00:00:00 runsv docker
...
root      6977    38  0 15:22 ?        00:00:00 runsv testseth
bash-5.1# cat /proc/1/environ
container=lxc
...
bash-5.1# cat /proc/38/environ 
PATH=/usr/bin:/usr/sbin 

Copy link
Author

Choose a reason for hiding this comment

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

Found the issue with some help from gurus:

/etc/runit/1 sets VIRTUALIZATION - this is confirmed. Then /etc/runit/2 calls svrundir but it blanks the environment out before running it. This is why svrundir can't find VIRTUALIZATION.

So, I think the quickest and most correct thing to do is pass VIRTUALIZATION into the svrundir call in /etc/runit/2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm VIRTUALIZATION is sort of a terrible name IMO. Not sure if we want to export it. See #45.

Indeed this file is really only supposed to set the variable for the "core services" / scripts. Exporting the container variable verbatim may be another option. Or IS_CONTAINER=1 as in #45.

Copy link
Author

@sbromberger sbromberger Dec 24, 2022

Choose a reason for hiding this comment

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

I'm happy to bikeshed the name, but I'd really like to get this fix in soon-ish if possible since it's blocking void-linux/void-packages#41273. See #102 for a first cut. We can easily change it there so that VIRTUALIZATION only appears in runit/1.

This was a heck of a rabbit-hole! Thank you for all your help and patience.

Copy link
Author

Choose a reason for hiding this comment

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

Also - I think that if we (you) decide to adopt the changes in #45, then there should be a consistent way of checking for this variable. Right now we're assigning a numeric value to it, but all the checks I've seen are checking for non-zero length. That is, VIRTUALIZATION=0 will evaluate to true in most of the checks. This is probably not what we want, especially if we're calling it IS_CONTAINER which directly implies a boolean.

@sbromberger
Copy link
Author

Closing because #102 is where it's at.

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.

2 participants