Tests are failing with timeout error

[inknos]

Reproduces in GH and locally

1. pull upstream
2. make tests

tests get stuck at podman/tests/integration/test_containers.py::ContainersIntegrationTest::test_container_crud

See https://github.com/containers/podman-py/pull/420/checks?check_run_id=28860969899

[inknos]

After bisecting podman, this is the first commit where podman-py tests hang containers/podman@8a94331

[rhatdan]

@Luap99 PTAL

[Luap99]

I am not familiar with the python code here but if you do not call the wait endpoint then the linked commit should not change anything as the code is only used in there unless I am overlooking something.

Looking at your reproducer it doesn't even start the container so there is no reason to call wait either, and I cannot reproduce with your small python example either unless I did something wrong? I just copied your reproducer to a file in this repo and then just run it.

[inknos]

apologies I deleted the comment with the reproducer already because it was incorrect.

if you do not call the wait endpoint then the linked commit should not change anything as the code is only used in there unless I am overlooking something

your comment shed some light, now I actually have something that can reproduce it

from podman import PodmanClient
import subprocess

uri = "unix:///run/user/100/podman/podman.sock"
client = PodmanClient(base_uri=uri)

c = client.containers.create('nginx:latest', name="test", detach=True)
c.start()

works

c = client.containers.run('nginx:latest', name="test", detach=True)

hangs

I think I know where to look now. run calls start, wait and reload in sequence

[Luap99]

Yes that makes more sense, I was able to reproduce with the test case once I figure out the dependencies.

I see container.wait(condition=["running", "exited"]) being called there but I really really do not understand the purpose of this call. After start is done it is safe to assume the container is running (or well it could have any other state as well as in the meantime the process could have exited or any other podman command might have changed the state) so this call doesn't seem to do anything useful. In fact the caller asked for detach and we still wait?

[inknos]

not sure about this line. I found new issues related to getting the right status. it seems like the call will indeed wait forever since the status looks to be created every time.

>>> c = client.containers.create('nginx:latest', detach=True)
>>> c.status
'created'
>>> c.start()
>>> c.status
'created'

[Luap99]

In theory exited cannot happen in your reproducer however the wait condition should trigger on running as well so something must be wrong with that. The logic should return if any state was hit however it currently blocks forever.

I see what is causing this on the podman side and will look into a fix on the podman side tomorrow.

However I would recommend you drop the wait call there, it is not doing anything useful AFAICS.

the status looks to be created every time.

You will have to call reload() because the python code of course doesn't really understand state changes on the podman side, the API has no real way to update the state fields on its own so you must do an explicit reload which in turn does an inspect on the server and then just updates the attrs filed with that. c.state itself just returns self.attrs["State"]["Status"]

[jwhonce]

@Luap99 The wait call is blocking on the container on the server to be in the correct state. So a reload() is not required. These semantics are to be compatible.

[Luap99]

@Luap99 The wait call is blocking on the container on the server to be in the correct state.

Well if you call start the container is always running on the server, calling wait for running afterwards is just pointless.

[inknos]

Should the run function then call wait only in certain cases? I can see that for a detached container does not make much sense, but maybe it does for a container which is not detached. in my opinion, the behaviors should be consistent, so I would cut it tbh.

I think wait makes a lot of sense for scripting purposes, but not within the run function context. after all isn't run just create+start?

[Luap99]

In the non detached case it already calls wait() (without args so it waits for exit) later which is correct, exit_status = container.wait(). I think run should largely follow what podman run is doing.

[jwhonce]

I plead very bad git history, sorry guys. I added that condition without explanation. I want to say the idea was to have the reload() be correct and there was a race condition. I infer this because the wait() was added with integration tests comment.

Over the last three years the server semantics may have changed enough that the race condition no longer exists.

[inknos]

@jwhonce, we recently had an update in our docs where reload() was written explicitly for getting the status of a container. I believe wait can be removed and maybe reload too.

Actually, to improve getting the correct status, what about trying to reload directly within in the status property?

[Luap99]

Actually, to improve getting the correct status, what about trying to reload directly within in the status property?

Note that an inspect call is rather expensive. I am not sure how the code is used in general but if one does if c.status == "foo" || c.status == "bar" than having the status change in between might be a bad thing?

[inknos]

yeah, that can be an issue. let's not make the status property too smart