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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions functions
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

}

deactivate_vgs() {
Expand Down