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

CONTAINERS_CONF envariable insufficient for testing #15413

Closed
edsantiago opened this issue Aug 22, 2022 · 12 comments
Closed

CONTAINERS_CONF envariable insufficient for testing #15413

edsantiago opened this issue Aug 22, 2022 · 12 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@edsantiago
Copy link
Member

It's not as clear as I'd like, but this is the containers.conf man page:

If the CONTAINERS_CONF path environment variable is set, just this path will be used. This is primarily used for testing.

To be 100% explicit: "just this path" means "podman will only read this file, ignoring all other containers.conf files in /usr/share/containers an /etc/containers.

This is causing headaches and gaps in our test coverage, specifically Ubuntu and RHEL8 which use cgroups v1:

# podman info --format '{{.Host.OCIRuntime.Name}}'
runc

! create a dummy empty containers.conf
# truncate --size=0 /tmp/containers-foo.conf
# CONTAINERS_CONF=/tmp/containers-foo.conf podman info --format '{{.Host.OCIRuntime.Name}}'
crun

There are a ton of system tests that write custom containers.conf files with stuff like userns="auto", dns-this-that, infra_image, events_logger. All of those, on Ubuntu and RHEL8, are being run with crun instead of the expected runc. (This probably doesn't matter with some options, but I bet it does with userns). It also means that podman is running with its full capability set.

This is really hard to fix. The options I see are:

  • make CONTAINERS_CONF mean "in addition to" not "exclusively". Nonstarter: it breaks compatibility (not a big deal on a test-only setting) but also breaks tests that require an exactly-tuned containers.conf.
  • Add a new element to the end of the default containers-conf-file read path, maybe something in /run. Or add a new CONTAINERS_CONF_PATH envariable that defines what that path should be. (IMHO: Ewwww)
  • Add a new CONTAINERS_CONF_EXTRA envariable whose meaning is, "first read all the defaults, then read this one"
  • Add a --dump-containers-conf option to podman, such that tests can use that to >/tmp/mycontainers.conf and edit that
  • Make it the test-writer's problem to write runtime engine, DefaultCapabilities, Logging, and every other possible necessary setting before invoking podman run
  • Have tests write their own overrides into /etc/containers/containers.conf.d or the rootless equivalent (Nope. One of my hard-fast- principles is never to vandalize a system. If test crashes, this would leave a user's system in a nonstandard, possibly unusable, state)

Suggestions (and fixes) welcome.

@mheon
Copy link
Member

mheon commented Aug 24, 2022

Of all these, --dump-containers-conf would be my preference (mainly because I can see people using this outside of the tests; on systems which for some reason don't have a containers.conf, generating one seems useful)

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

I am fine with that.

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

podman --dump-containers-conf ?

@edsantiago
Copy link
Member Author

On reflection ISTM to make more sense to add a command, not option. Probably hidden by default.

Or, hmmm, a suboption of podman info?

@mheon mheon added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 26, 2022
@vrothberg
Copy link
Member

podman info sounds like a nice idea. Everything can be hidden and the contents can be printed via --format {{.ContainersConf}} or something like that.

@rhatdan
Copy link
Member

rhatdan commented Sep 6, 2022

Do you want to just show the config file(s) used or dump the contents. Dumping the contents is easier.

@edsantiago
Copy link
Member Author

If I understand your question correctly, dump contents please. The idea is to do:

$ podman info --format='{{.ContainersConf}}' >/tmp/my-temp-containers.conf
$ env CONTAINERS_CONF=/tmp/my-temp-containers.conf podman run .....

...such that the second command will be 100% a NOP, because the tmp-conf file will be 100% identical to the configuration podman is running. The key bit is that one can then:

$ sed -ie 's/xxx=foo/xxx=bar/' /tmp/my-temp-containers.conf

...and change only one variable in the podman environment, while keeping everything else the same. This is imperative for things like changing events backend, because this:

$ cat >/tmp/foo.conf
events-backend=file
$ env CONTAINERS_CONF=/tmp/foo.conf podman run ...

...is super-duper-dangerous, because it completely disregards /etc/containers.conf and any other system settings. Including the CAP thingies.

@github-actions
Copy link

github-actions bot commented Oct 7, 2022

A friendly reminder that this issue had no activity for 30 days.

@edsantiago
Copy link
Member Author

ping, we really, really, really need this (for CNI/netavark testing)

@vrothberg
Copy link
Member

@rhatdan @mheon are you cool with the proposed change to podman info? @edsantiago is there something else you need?

@edsantiago
Copy link
Member Author

Actually... the more I think about this, the more I realize this won't work. Not without proper toml-editing tools (something like gron for json). Without a scriptable way to edit toml, there really isn't any point to this. So please stand down, let's leave this for an RFE some day when tools are available. Or we can even close it.

edsantiago added a commit to edsantiago/libpod that referenced this issue Mar 27, 2023
...not CONTAINERS_CONF. At least for most tests.

Nearly every system test currently using CONTAINERS_CONF=tmpfile
should be using CONTAINERS_CONF_OVERRIDE.

Simple reason: runtime (crun/runc), database_backend (bolt/sqlite),
logger, and other important settings from /etc/c.conf are not
usually written into the tmpfile. Those tests, therefore, are
not running podman as configured on the system.

Much more discussion: containers#15413

This PR is a prerequisite for enabling sqlite system tests. For
the sake of simplicity and sanity, I choose to submit the sqlite
switch as a separate PR once this passes and merges.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago
Copy link
Member Author

CONTAINERS_CONF_OVERRIDE seems to be an adequate solution for the problems reported here. Closing.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

No branches or pull requests

4 participants