Rootless 'podman exec' stopped working with containers created with '--uidmap' somewhere between 1b2f8679b864b882 (good) and 9b21f14eefae8ab7 (bad)

[debarshiray]

/kind bug

Description

Rootless podman exec stopped working somewhere between 1b2f867 (good) and 9b21f14 (bad) as can be seen when using the Silverblue toolbox. Starting from a clean state (ie., no images or containers present), this is what happens:

$ toolbox create
$ toolbox -v enter
...
...
toolbox: trying to exec /bin/bash in container fedora-toolbox-rishi:29
Error: cannot read conmon PID file "/run/user/1000/overlay-containers/36d44874d168fedf8effb849f28b2338e0916d960f1026f660a44e582a3536f4/userdata/conmon.pid": open /run/user/1000/overlay-containers/36d44874d168fedf8effb849f28b2338e0916d960f1026f660a44e582a3536f4/userdata/conmon.pid: permission denied

Note that in case you are running the toolbox script directly from Git, make sure that you have a /run/media directory. The toolbox RPM takes care of this.

drwxr-xr-x. 2 root root 40 Mar 15 17:18 /run/media

The command that's failing is:

$ podman --log-level debug exec -it fedora-toolbox-rishi:29 /bin/bash
INFO[0000] running as rootless                          
DEBU[0000] Initializing boltdb state at /home/rishi/.local/share/containers/storage/libpod/bolt_state.db 
DEBU[0000] Using graph driver overlay                   
DEBU[0000] Using graph root /home/rishi/.local/share/containers/storage 
DEBU[0000] Using run root /run/user/1000                
DEBU[0000] Using static dir /home/rishi/.local/share/containers/storage/libpod 
DEBU[0000] Using tmp dir /run/user/1000/libpod/tmp      
DEBU[0000] Using volume path /home/rishi/.local/share/containers/storage/volumes 
DEBU[0000] Set libpod namespace to ""                   
DEBU[0000] Not configuring container store              
ERRO[0000] cannot read conmon PID file "/run/user/1000/overlay-containers/36d44874d168fedf8effb849f28b2338e0916d960f1026f660a44e582a3536f4/userdata/conmon.pid": open /run/user/1000/overlay-containers/36d44874d168fedf8effb849f28b2338e0916d960f1026f660a44e582a3536f4/userdata/conmon.pid: permission denied

Output of podman version:

Version:            1.2.0-dev
RemoteAPI Version:  1
Go Version:         go1.11.5
OS/Arch:            linux/amd64

Output of podman info --debug:

debug:
  compiler: gc
  git commit: ""
  go version: go1.11.5
  podman version: 1.2.0-dev
host:
  BuildahVersion: 1.7.1
  Conmon:
    package: podman-1.2.0-12.dev.git9b21f14.fc29.x86_64
    path: /usr/libexec/podman/conmon
    version: 'conmon version 1.12.0-dev, commit: 90658d9dc6cbe4b23fd1c453a92405aac2ecb3e9'
  Distribution:
    distribution: fedora
    version: "29"
  MemFree: 315166720
  MemTotal: 8126992384
  OCIRuntime:
    package: runc-1.0.0-68.dev.git6635b4f.fc29.x86_64
    path: /usr/bin/runc
    version: |-
      runc version 1.0.0-rc6+dev
      commit: ef9132178ccc3d2775d4fb51f1e431f30cac1398-dirty
      spec: 1.0.1-dev
  SwapFree: 4130324480
  SwapTotal: 4133482496
  arch: amd64
  cpus: 4
  hostname: kolache
  kernel: 4.20.13-200.fc29.x86_64
  os: linux
  rootless: true
  uptime: 2h 26m 3.16s (Approximately 0.08 days)
insecure registries:
  registries: []
registries:
  registries:
  - docker.io
  - registry.fedoraproject.org
  - quay.io
  - registry.access.redhat.com
  - registry.centos.org
store:
  ConfigFile: /home/rishi/.config/containers/storage.conf
  ContainerStore:
    number: 1
  GraphDriverName: overlay
  GraphOptions:
  - overlay.mount_program=/usr/bin/fuse-overlayfs
  GraphRoot: /home/rishi/.local/share/containers/storage
  GraphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "false"
    Supports d_type: "true"
    Using metacopy: "false"
  ImageStore:
    number: 2
  RunRoot: /run/user/1000
  VolumePath: /home/rishi/.local/share/containers/storage/volumes

[debarshiray]

Bisecting further with the actual Git tree, instead of the RPM snapshots, I think this was caused by PR #2569:

commit 4a02713
Author: Giuseppe Scrivano [email protected]
Date: Thu Mar 7 08:14:22 2019 +0100

rootless: exec join the user+mount namespace

it is not enough to join the user namespace where the container is
running.  We also need to join the mount namespace so that we can
correctly look-up inside of the container rootfs.  This is necessary
to lookup the mounted /etc/passwd file when --user is specified.

Closes: https://github.com/containers/libpod/issues/2566

Signed-off-by: Giuseppe Scrivano <[email protected]>

[giuseppe]

thanks for the bisect. The patch helped to find the issue, which is a different one:

#2675

[giuseppe]

also, fedora-toolbox is a very interesting use case and it is stretching rootless containers to their limits :-)

once we have dedicated rootless tests, @debarshiray would you be interested to write some tests for Podman for the features you are using in fedora-toolbox? So we will avoid to introduce regressions in the future

[debarshiray]

thanks for the bisect. The patch helped to find the issue, which is a different one:

#2675

Nice.

Although, if I am not mistaken, doesn't this mean that rootless containers created with earlier versions of podman can no longer be used with podman exec?

I wonder if we can soften the blow somehow. One option that came to my mind was to not hard depend on the ConmonPidFile. Maybe we can fallback to getting the PID of the container's PID 1 through ctr.PID(), and then walking back up the process tree to find the conmon process using some heuristics? Although, I don't know what could happen if someone tricks the heuristics into a false positive.

[debarshiray]

also, fedora-toolbox is a very interesting use case and it is stretching rootless
containers to their limits :-)

once we have dedicated rootless tests, @debarshiray would you be interested to
write some tests for Podman for the features you are using in fedora-toolbox? So
we will avoid to introduce regressions in the future

Sure, I will be happy to. :)

[mheon]

Shouldn't break existing containers - we store the path in the database, so older containers should still use the old default they were created with, while new containers get the new, fixed default.

…

On Mon, Mar 18, 2019, 06:49 Debarshi Ray ***@***.***> wrote: also, fedora-toolbox is a very interesting use case and it is stretching rootless containers to their limits :-) once we have dedicated rootless tests, @debarshiray <https://github.com/debarshiray> would you be interested to write some tests for Podman for the features you are using in fedora-toolbox? So we will avoid to introduce regressions in the future Sure, I will be happy to. :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2673 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCM0TtZtBu0i3fBEuUbnYz5BOgv04ks5vX4togaJpZM4b3GLN> .

[debarshiray]

Shouldn't break existing containers - we store the path in the database,
so older containers should still use the old default they were created with,
while new containers get the new, fixed default.

Umm... but this isn't about the presence of the path to the conmon.pid file. The problem is that (the old path to) the conmod.pid file inside XDG_RUNTIME_DIR has permissions that are awkward for the current code, which leads to EPERM. The new default doesn't have this problem, but it's only going to take effect for freshly created containers.

I had earlier checked with the last known good snapshot from master (ie. commit 1b2f867), and now confirmed that it's true with the 1.1.2 release too. :)

If we can't soften the blow, then it would be good to have a narrower exit code that stands for "container needs to be re-created" so that scripts like the Silverblue toolbox can throw a less generic error message. Maybe we should even standardize such an exit code across all commands that work with an already created container?

[mheon]

Ahhh. I would have called that not fixing existing containers instead of breaking them, but you're 100% correct.

I don't know how we can easily detect this specific case; it could just as easily be someone accidentally running chown -r on a directory we use.

[debarshiray]

One could try to guess the cause of the failure by checking if the EPERM was a for the known-not-to-work path.

Is it possible to find out the path to the conmon.pid file from outside Podman? If yes, then the toolbox could try to do that itself. It should be reasonably OK because it knows that it doesn't override the path, so the combination of a path inside XDG_RUNTIME_DIR and EPERM would be a decent heuristic.

[mheon]

If we're not already reporting it in exec I would be fine with adding Conmon pidfile path there

[mheon]

And by exec I meant inspect. Need more coffee...

[debarshiray]

Ok, submitted #2698 to export ConmonPidFile in podman inspect for containers.

[debarshiray]

I added a more targetted hint to the error message from toolbox enter (see https://github.com/debarshiray/toolbox/pull/82) but I still wish people didn't have to throw away their existing containers. If nothing else, this would make it difficult to land podman-1.2.0 in stable Fedoras like 29 and 28.

[owtaylor]

Ahhh. I would have called that not fixing existing containers instead of breaking them, but you're 100% correct.

It seems precisely like breaking existing containers to me - as I understand the status after #2675 , if you create a container with podman < 1.2.0 [edit, fixed comparison], then unless this is resolved, upgrading to 1.2.0 makes this working container no longer work properly.

I don't know how we can easily detect this specific case; it could just as easily be someone accidentally running chown -r on a directory we use.

What about simply recreating ConmonPidFile when starting a stopped container?

[debarshiray]

I don't know how we can easily detect this specific case; it could just as easily be someone accidentally running chown -r on a directory we use.

What about simply recreating ConmonPidFile when starting a stopped container?

The problem is that #2675 writes the default ConmonPidFile path to the container's configuration if the user hadn't explicitly specified one. That makes things a bit difficult in terms of falling back to softer recovery modes.

I wonder if we can avoid writing the default path to the configuration when the user hadn't specified one explicitly, and keep plugging it in dynamically at run-time? If we can do that, then it might open up some possibilities that aren't as extreme as "re-create the container and start afresh". :)

I don't know if it will work, but something like "you have to re-start the container" is a lot nicer.

[mheon]

Not keeping the default in the database is arguably worse - we won't even be able to find the file for running containers that haven't restarted. This at least preserves consistency, in that we know where the file is with certainty, even if the location has incorrect permissions.

…

On Mon, Mar 18, 2019, 14:19 Debarshi Ray ***@***.***> wrote: I don't know how we can easily detect this specific case; it could just as easily be someone accidentally running chown -r on a directory we use. What about simply recreating ConmonPidFile when starting a stopped container? The problem is that #2675 <#2675> writes the default ConmonPidFile path to the container's configuration if the user hadn't explicitly specified one. That makes things a bit difficult in terms of falling back to softer recovery modes. I wonder if we can avoid writing the default path to the configuration when the user hadn't specified one explicitly, and keep plugging it in dynamically at run-time? If we can do that, then it might open up some possibilities that aren't as extreme as "re-create the container and start afresh". :) I don't know if it will work, but something like "you have to re-start the container" is a lot nicer. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2673 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCLmKhVqma_zYOE-DgUOX-IlhLK77ks5vX_TUgaJpZM4b3GLN> .

[mheon]

@owtaylor This changes nothing for containers created for previous versions of Podman - the new default only applies to new containers. Previous containers will use the old path. I don't see how that breaks old containers - if they are broken, they were broken before the patch.

[owtaylor]

@owtaylor This changes nothing for containers created for previous versions of Podman - the new default only applies to new containers. Previous containers will use the old path. I don't see how that breaks old containers - if they are broken, they were broken before the patch.

As I understand it, since #2569 the old path doesn't work any more - #2675 fixes that for new containers, but old containers keep using the conmon default location (in the CWD of the conmon execution)

Putting the new default directly into the args passed to conmon rather than writing them into the config file would seem better... then it would work for any container.

[owtaylor]

Putting the new default directly into the args passed to conmon rather than writing them into the config file would seem better... then it would work for any container.

Ah, I guess that wouldn't help locate the pid file for an existing running container instance - but I still think that a) the current code state will break the ability of people to keep using rootless containers they created with older versions of podman b) with some rearrangement of code, that isn't necessary.

[mheon]

I'm still convinced that's a bad idea - we risk losing track of where the PID file lives. It also won't fix your old containers - their configs will still have the old default path, so we'll still use it. We can't rewrite existing container configs, either - that's a limitation of libpod's immutable container model. Our best bet might be trying to make the c/storage rundir accessible (chown the rundir to a group we can access?) - that would let us fix things with a container restart, at least...

The bigger takeaway from this is that our rootless testing is massively inadequate, because this snuck in and nobody realized until toolbox started hitting it. I see two PRs open right not to run the full test suite as rootless, which is a good start.

[owtaylor]

It also won't fix your old containers - their configs will still have the old default path, so we'll still use it.

The old path was only written into the config since 1.1.0 - but yeah, 1.1.2 did end up in Fedora stable two weeks ago :-( so that makes it hard to handle all old containers. You could just check if the pidfile in the config file is inside the runtime dir...

The upgrade issue wouldn't be caught by more testing in the test suite, but certainly that would be a great start!

[giuseppe]

It seems precisely like breaking existing containers to me - as I understand the status after #2675 , if you create a container with podman < 1.2.0 [edit, fixed comparison], then unless this is resolved, upgrading to 1.2.0 makes this working container no longer work properly.

it changes only the default value. Existing containers will keep using their configuration that was created with the older podman version.

[debarshiray]

It seems precisely like breaking existing containers to me - as I understand
the status after #2675 , if you create a container with podman < 1.2.0 [edit, fixed comparison],
then unless this is resolved, upgrading to 1.2.0 makes this working container no longer work properly.

it changes only the default value. Existing containers will keep using their configuration that
was created with the older podman version.

Yes, containers created with podman <= 1.1.2 will keep using their existing configurations
but one cannot use podman exec on them after PR #2569 or commit 4a02713 if the containers were created with --uidmap but without --conmon-pidfile.

[mheon]

Given the permissions issues with conmon pidfiles from 1.1.x, I'm going to reopen this. We have a fix for new containers, but we need something to prevent regression on older containers.

[giuseppe]

we can add a workaround to fallback to the previous behaviour when we cannot read the conmon pid file. It is still broken with --user, but it is not a regression since it was already broken in the previous version.

What do you think?

[giuseppe]

PR for the workaround: #2762

[debarshiray]

we can add a workaround to fallback to the previous behaviour
when we cannot read the conmon pid file. It is still broken with
--user, but it is not a regression since it was already broken in the
previous version.

Makes sense to me!