[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@Luap99 @rhatdan @edsantiago WDYT?

@Luap99 any idea why I am observing the following in shell completion?

podman (RUN-1873) $ ./bin/podman --module
sub/sub.conf  test.conf     

podman (RUN-1873) $ ./bin/podman --module t
test/               transfer.md         troubleshooting.md 

Above it's perfectly capable of detecting the files in the XDG_CONFIG_HOME but it's unable to complete and will pick a path in CWD instead.

EDIT need to fix your comment before and will retry.

Another problem I just realized, I think we must pass --module this down to the cleanup command to ensure [engine] options are the same for the cleanup process and the one that started the container. For example it would be very important to call the right oci runtime if one was set in a module.

Now doing this sounds easy but there is one big problem with that, if a user starts a container with a module then removes the module file, the cleanup command will fail immediately without doing anything which will be very problematic. Now this is of course a corner case but one that could be impossible to debug as there are almost never logs from the cleanup command. I think there are other cases were this is already a problem, i.e. adding a options that fail config validation so this is not exactly a new problem but one to keep in mind.
So maybe it is fine for now to accept this risk but I think we need a design for a more fault tolerant cleanup process.

A weird thing happened when I tried testing completion:

$ mkdir -p /tmp/fakehome/.config/containers/containers.conf.modules
$ touch /tmp/fakehome/.config/containers/containers.conf.modules/sdfsdf.conf

$ XDG_CONFIG_HOME=/tmp/fakehome/.config bin/podman __completeNoDesc --module ''
sdfsdf.conf            <------- this is good
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

$ XDG_CONFIG_HOME=/tmp/fakehome/.config bin/podman __completeNoDesc --module 'x'
Failed to obtain podman configuration: could not resolve module "x": 3 errors occurred:
        * stat /tmp/fakehome/.config/containers/containers.conf.modules/x: no such file or directory
        * stat /etc/containers/containers.conf.modules/x: no such file or directory
        * stat /usr/share/containers/containers.conf.modules/x: no such file or directory

A weird thing happened when I tried testing completion:

Nice catch, Ed. Thank you! I think that explains my observation as well. Explanation: modules should not be loaded during shell completion.

Pushed a new version with the comments addressed. Still not ready to go but I'll be buried in meetings for a while.

So maybe it is fine for now to accept this risk but I think we need a design for a more fault tolerant cleanup process.

I honestly never thought of it before but see how this can cause hard to debug issues. Mind opening an issue for it?

@rhatdan @Luap99 @edsantiago PTanotherL

Once I have your blessings, we can merge containers/common#1599 and properly vendor c/common in here.

Another problem I just realized, I think we must pass --module this down to the cleanup command to ensure [engine] options are the same for the cleanup process and the one that started the container. For example it would be very important to call the right oci runtime if one was set in a module.

This is still open and important, especially when a container uses --restart=always.

Also I see no mentions in the docs that --module is not supported on the remote client.

This is still open and important, especially when a container uses --restart=always.

Thanks! I will add that. Can you elaborate on the --restart=always case? What could break? I don't find something from the top of my head but an example may even be helpful for a test.

Can you elaborate on the --restart=always case? What could break?

For example host_containers_internal_ip can only be set in containers.conf and will effect the generated /etc/hosts in the container. Because that option is not stored in the container config it will only be read when we write /etc/hosts. Now we write /etc/hosts on each container start so the one restarted from within the cleanup process could result in a different hosts file which would be bad. I am sure there are more options that would be effected by this.

This is still open and important, especially when a container uses --restart=always.

Done ✔️

Also added a regression test: https://github.com/containers/podman/pull/19567/files#diff-07cf7ec60df1f145e45014e0a31d374cde4b1fb4f28cf3999c4e13293b6dc541R127-R131

Fresh thought: do we need to worry/care about people who try sudo podman --module X with X in their homedir? Probably not a problem for the original requesters, but we have no way of knowing who will be using this.

Was any consideration given to using .d for the new modules directory?

What is the reason for /usr/share/...? The only one I can imagine is for a distro to include preconfigured modules... which sounds kind of alarming TBH. Has that been fully thought out?

Would you consider adding a few more tests?

You also might want to fix some stutter. One of them should be easy, the other, not so much.

Fresh thought: do we need to worry/care about people who try sudo podman --module X with X in their homedir? Probably not a problem for the original requesters, but we have no way of knowing who will be using this.

HOME is always ignored by Podman when running as root. I think modules should remain consistent with that.

Was any consideration given to using .d for the new modules directory?

Yes. Mixing with .d breaks separation of concerns. .d configs are always loaded and modules should only be loaded on demand, so we needed a new directory to avoid user errors.

What is the reason for /usr/share/...? The only one I can imagine is for a distro to include preconfigured modules... which sounds kind of alarming TBH.

You are right on spot. Imagine an nvidia.conf module being shipped in an RP

Has that been fully thought out?

I hope so. I mean at that point we had weeks of conversations with the requester, a reviewed design document and the other PR in containers/common. If you don't feel comfortable with the current implementation, please block before merging the PR.

Would you consider adding a few more tests?

Will do, thanks a lot!

Now vendoring c/common@main.

@rhatdan @Luap99 PTanotherL
Let's not merge without @edsantiago's blessing

Fresh thought: do we need to worry/care about people who try sudo podman --module X with X in their homedir?

HOME is always ignored by Podman when running as root. I think modules should remain consistent with that.

Sorry, I didn't express myself right. I was thinking of someone creating ~/..../module/foo, using --module foo, then later (after they've grown used to it, and forgotten that it's in their homedir) trying sudo podman --module foo and getting a surprise. IMO sudo podman has enough surprises already, one more is not a big deal. I just want to make sure we're aware of this beforehand.

Please don't gate on me. I'm really uncomfortable with this concept, but tests LGTM (thanks for adding those) and I can't find any specific showstoppers.

Sorry, I didn't express myself right. I was thinking of someone creating ~/..../module/foo, using --module foo, then later (after they've grown used to it, and forgotten that it's in their homedir) trying sudo podman --module foo and getting a surprise. IMO sudo podman has enough surprises already, one more is not a big deal. I just want to make sure we're aware of this beforehand.

I am curious what others think. So far, rootful Podman ignores all rootless configs. Same for other tools ignoring settings in XDG_CONFIG_HOME.

I don't understand the Windows error (https://github.com/containers/podman/pull/19567/checks?check_run_id=15935251777). @Luap99 @edsantiago do you know what's going on?

I don't understand the Windows error (https://github.com/containers/podman/pull/19567/checks?check_run_id=15935251777). @Luap99 @edsantiago do you know what's going on?

c/common changed the machine defaults, I think you need to merge + rebase onto #19596

Sorry, I didn't express myself right. I was thinking of someone creating ~/..../module/foo, using --module foo, then later (after they've grown used to it, and forgotten that it's in their homedir) trying sudo podman --module foo and getting a surprise. IMO sudo podman has enough surprises already, one more is not a big deal. I just want to make sure we're aware of this beforehand.

I am curious what others think. So far, rootful Podman ignores all rootless configs. Same for other tools ignoring settings in XDG_CONFIG_HOME.

I don't understand the issue here, sudo is running as root so of course it should ignore rootless configurations. I don't see surprise factor? Like would you expect sudo vim to load the rootless .vimrc? To me the behaviour of ignoring rootless configurations when running as root is the normal expected thing.

It's not about what I expect, it's about end users. We seem to get frequent "sudo doesn't work" questions, and this is just one more thing for less-experienced users to trip on. Anyhow, enough. We can live with that.

OK, we're green.

LGTM

/lgtm