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

Support running inside Toolbx containers #3370

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

debarshiray
Copy link

@debarshiray debarshiray commented Oct 2, 2023

Toolbx containers are Podman containers that are designed to be used as interactive command line environments for development and troubleshooting the host operating system. Therefore, a well working sos(1) will improve the troubleshooting experience inside these containers.

Currently, those trying to use Toolbx containers for troubleshooting with sos report have been setting the HOST environment variable inside their containers.

The problem with doing that is that HOST is a very generic name without any namespacing. Not every user is going to use sos report, and it can easily conflict with a variable of the same name being used for a different purpose. This is similar to the NAME and VERSION environment variables that used to be set inside Toolbx containers due to outdated or wrong information in Fedora's container guidelines. They were a constant source of complaints and were recently fixed. The same logic applies to HOST.

Note that there is an ongoing effort to change the implementation of the toolbox RPM in various Red Hat products from github.com/coreos/toolbox to github.com/containers/toolbox or Toolbx. The latter was inspired by the former, and is the more widely used of the two. eg., Toolbx is installed by default on Fedora CoreOS, Silverblue and Workstation.

So far, this change has been made in Red Hat Enterprise Linux 8.5 onwards, but not in other products like Red Hat OpenShift Container Platform.

@debarshiray
Copy link
Author

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname email@example.com?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3370
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@debarshiray
Copy link
Author

I made sure that flake8 sos and nosetests -v tests/unittests/ pass, but this avocado invocation is giving a bunch of errors and failures inside a rootful container:

PYTHONPATH=tests/ avocado run --test-runner=runner -t stageone tests/{cleaner,collect,report,vendor}_tests

I don't know what to do about that.

@TurboTurtle
Copy link
Member

Just an initial comment here, not necessarily reflective of the code change in the PR at this point: we can add support for toolbox/toolbx, but it has to conform to two major points:

  1. it cannot prevent/break existing support for containers (see example below)
  2. it cannot have sos make changes to the host

For reference, on RHEL the recommendation is (or at least what the recommendation was as of June/July) to use the support-tools container image. E.G. podman container runlabel RUN registry.redhat.io/rhel8/support-tools as this completely sets up the container environment as needed to collect an sos report from the host.

Outside of all this, we can certainly look at renaming the env var we look at, and have a period of transition support before ultimately dropping the use of HOST, if needed.

@bmr-cymru
Copy link
Member

The HOST environment variable goes all the way back to the earliest container support in sos:

commit 63805ed15d63ddfebb06cd03f96f310bbf60d3b2
Author: Bryn M. Reeves <bmr@redhat.com>
Date:   Mon Jan 26 15:36:40 2015 -0500

    [policies] add container support to Red Hat policy
    
    Check for the presence of container-specific environment variables
    and set _host_sysroot if present:
    
      container_uuid=UUID
      HOST=/path
    
    If both a container environment variable and HOST are present run
    the PackageManager query command in a chroot.
    
    Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

It's what we were asked to check for at the time (for the Red Hat support-tools container). We would have preferred something like HOST_SYSROOT, or even better with some prefix like RH_HOST_SYSROOT but this was communicated to us a the time as the environment variable we should check for. Getting changes coordinated for the different moving parts has proven challenging in the past.

If that's changed since and there's some better variable to check for then it'd be fine to update to that, but as @TurboTurtle says we need to maintain compatibility for existing environments.

@debarshiray
Copy link
Author

@TurboTurtle @bmr-cymru Thanks for the review and the historical context.

it cannot prevent/break existing support for containers (see example below)

I didn't get this part. Does it break existing containers? Only Toolbx containers can have /run/.toolboxenv. So the second patch should only set the HOST environment variable on Toolbx containers and leave others alone. The first one only changes the container detection logic to something that will work inside a sudo(8) session.

it cannot have sos make changes to the host

All I know is that people who are trying to use sos report through the support-tools image want to set HOST to a location inside the container that has the entire / from the host operating system bind mounted. Regardless of whether sos(1) should change the host or not, people are already using it this way today.

I am just the Toolbx maintainer who is pushing back against requests to have the HOST environment variable set in the container for the reasons I mentioned above.

To me, the best option is to teach sos(1) how to work best inside a Toolbx container. I confess that I have no idea what best is, so I just replicated what the sos(1) users were asking for. :)

I think it will be best if you discuss what the end result should be with folks like @control-d who actually use or want to use sos report inside Toolbx containers. Then I can help make that happen.

Currently, if 'sos report' is run inside a rootful Toolbx [1] container
with the HOST environment variable set, it creates the report inside the
host operating system's /var/tmp, which is at $HOST/var/tmp inside the
container:
  # toolbox enter
  ⬢# HOST=/run/host sos report
  ...
  Your sosreport has been generated and saved in:
          /run/host/var/tmp/sosreport-toolbox-2023-10-01-trpwqii.tar.xz
  ...

However, if it's run as 'sudo sos report' inside a rootless Toolbx
container with the HOST environment variable set, it creates the report
inside the container's /var/tmp:
  $ toolbox enter
  ⬢$ sudo su -
  ⬢# HOST=/run/host sos report
  ...
  Your sosreport has been generated and saved in:
          /var/tmp/sosreport-toolbox-2023-10-01-nwjqcff.tar.xz
  ...

Toolbx [1] containers are ultimately Podman containers that are designed
to be used as interactive command line environments for development and
troubleshooting the host operating system.  So, one can replicate the
above with a podman(1) invocation as well.

This happens because the 'container' environment variable isn't set
inside the sudo(8) session.  Instead of relying on environment
variables, which often go missing in unexpected ways, it will be better
to check for the /run/.containerenv and /.dockerenv stamp files that
identify Podman and Docker containers respectively.

[1] https://containertoolbx.org/
    https://github.com/containers/toolbox

Resolves: sosreport#3370

Signed-off-by: Debarshi Ray <debarshir@gnome.org>
Toolbx [1] containers are Podman containers that are designed to be
used as interactive command line environments for development and
troubleshooting the host operating system.  Currently, those trying to
use them for troubleshooting with 'sos report' have been setting the
HOST environment variable inside their containers.

It will improve the troubleshooting experience if sos(1) could detect
that it's running inside a Toolbx container and automatically configure
itself to do the right thing.

Toolbx containers can be distinguished from other Podman containers by
the presence of the /run/.toolboxenv file.

[1] https://containertoolbx.org/
    https://github.com/containers/toolbox

Resolves: sosreport#3370

Signed-off-by: Debarshi Ray <debarshir@gnome.org>
@debarshiray
Copy link
Author

Updated the branch to fix a single-double quote mix-up.

@debarshiray
Copy link
Author

Ping @control-d @travier

@TurboTurtle
Copy link
Member

it cannot prevent/break existing support for containers (see example below)

I didn't get this part. Does it break existing containers? Only Toolbx containers can have /run/.toolboxenv. So the second patch should only set the HOST environment variable on Toolbx containers and leave others alone. The first one only changes the container detection logic to something that will work inside a sudo(8) session.

No, I wasn't saying this patch breaks it - just a general thinking-out-loud that we would want to support both toolbx and support-tools methods.

it cannot have sos make changes to the host

All I know is that people who are trying to use sos report through the support-tools image want to set HOST to a location inside the container that has the entire / from the host operating system bind mounted. Regardless of whether sos(1) should change the host or not, people are already using it this way today.

I am just the Toolbx maintainer who is pushing back against requests to have the HOST environment variable set in the container for the reasons I mentioned above.

Ack, and agreed there should be a better-named env var here. But as Bryn mentioned above our hands were a bit tied at the initial implementation.

To me, the best option is to teach sos(1) how to work best inside a Toolbx container. I confess that I have no idea what best is, so I just replicated what the sos(1) users were asking for. :)

I think it will be best if you discuss what the end result should be with folks like @control-d who actually use or want to use sos report inside Toolbx containers. Then I can help make that happen.

Ack, sure. Ideally in my mind what we have is a situation where a user does not need to do anything special themselves to get a toolbx container running sos to collect from the host, but can optionally do something to cause sos to collect from within the container. We do that today for support-tools via the RUN label on the image, i.e. podman container runlabel RUN registry.redhat.io/rhel8/support-tools, but users can either launch the image manually or unset the HOST variable to do a container-focused collection (admittedly a much less utilized use case, but one that was explicitly requested from RH Support).

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.

3 participants