podman start --filter restart-policy=always : container state improper

[edsantiago]

Looks related to #22914, although probably not a regression (that one was a reliable panic, this is a flaky podman error):

[+0319s] not ok 151 [045] podman start --filter - start only containers that match the filter in 1914ms
...
<+010ms> # $ podman start --filter restart-policy=always   <<< first restart
<+268ms> # e214e6b649c557e0b24e610cb2bb986694f01f16e5545337195319b5f86763c8
         # 877d80fe7cfa11755af13fe02dbf4a359887ac6ca40e41b17f67b3d1c08392d9
         #
<+074ms> # $ podman start --filter restart-policy=always    <<<< second restart
<+351ms> # Error: unable to start container "e214e6b649c557e0b24e610cb2bb986694f01f16e5545337195319b5f86763c8": container e214e6b649c557e0b24e610cb2bb986694f01f16e5545337195319b5f86763c8 must be in Created or Stopped state to be started: container state improper

For extra credit, maybe someone could fix that error message to indicate the actual current container state

• fedora-39 : sys podman fedora-39 rootless host boltdb
  • PR CI: Use local cache registry #22726
    • 07-10 09:35 in [sys] [045] podman start --filter - start only containers that match the filter
    • 07-08 14:00 in [sys] [045] podman start --filter - start only containers that match the filter
• rawhide : sys podman rawhide rootless host sqlite
  • PR test composefs on rawhide #22425
    • 07-10 13:35 in [sys] [045] podman start --filter - start only containers that match the filter

x	x	x	x	x	x
sys(3)	podman(3)	fedora-39(2)	rootless(3)	host(3)	boltdb(2)
		rawhide(1)			sqlite(1)

[Luap99]

FYI reproduced locally, I didn't add a timer in my loop so not sure how long it took but it was somehwere around 30-60 mins

for i in {1..3}; do
    podman run --restart always --name c$i quay.io/libpod/testimage:20240123 true
done
while :; do bin/podman start --filter restart-policy=always || break ; done

I patched the binary to show the current state and it was running when it failed.

[Luap99]

I think I see the issue given that, our code does:
GetContainers() (with the given filter)
Then we check the state of the container we do not hold the container lock, if !running then start
In Start() we now lock the container, which means until we get the lock the state could have been changed, now that we are locked we check the state and throw the report error here because it is already running.

My fix would be to move the running state check into the locked Start() function
cc @mheon

[mheon]

I feel like locking the existing check would be most correct to not make Start() more complicated than it is right now, but it would also add an extra lock/unlock so I won't argue too hard

[Luap99]

I feel like locking the existing check would be most correct to not make Start() more complicated than it is right now, but it would also add an extra lock/unlock so I won't argue too hard

That wouldn't work because we must stay locked for both checks, if we unlock before then it again open ups the window for another state change.

[mheon]

Ew.

Maybe add a ErrCtrRunning, return that if the container is in running | paused state from Start, ignore it at the caller?

[Luap99]

I need to take a look tomorrow but if all callers then have to ignore it just complicates the code. But if some code paths want this to fail then a typed error sounds good to me.

[Luap99]

#23258
I will let my reproducer run for a while but I am pretty sure this will fix it.