Skip to content

Conversation

@milanbalazs
Copy link
Contributor

Currently if the detach=True and remove=True parameters are specified in the same command, the container is not deleted.

Reproduce the issue:

from podman import PodmanClient

uri = "unix:///run/user/1000/podman/podman.sock"

with PodmanClient(base_url=uri) as client:
    c = client.containers.run("nginx", deatch=True, remove=True, command="/bin/false")

    print(c.status)

With this change if the above parameters are provided then the container object is returned and the container removing (wait method is included) is started on a separated thread.

Fix for:

Signed-off-by: Milan Balazs <milanbalazs01@gmail.com>
@rhatdan
Copy link
Member

rhatdan commented Sep 27, 2024

These options should be sent to the server, to tell it to remove the container when it is finished, I don't think it is needs to be done on the client side?

@milanbalazs
Copy link
Contributor Author

milanbalazs commented Sep 27, 2024

These options should be sent to the server, to tell it to remove the container when it is finished, I don't think it is needs to be done on the client side?

I agree that would be an elegant solution, but based on the API documentation the described options are not implemented on server side (API):

Currently the containers.start method handles only the detachKeys parameter and the API also waits only this query parameter:

def start(self, **kwargs) -> None:
"""Start processes in container.
Keyword Args:
detach_keys: Override the key sequence for detaching a container (Podman only)
"""
response = self.client.post(
f"/containers/{self.id}/start", params={"detachKeys": kwargs.get("detach_keys")}
)
response.raise_for_status()

In my opinion, if you would like to pass these parameters to server side (via API), then some changes are needed on server side as well (at least in the API component).

If it's enough to solve this issue on the client side, then my PR can solve it. It's up to you. :)

@rhatdan
Copy link
Member

rhatdan commented Sep 30, 2024

OK
LGTM

@inknos
Copy link
Contributor

inknos commented Oct 7, 2024

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inknos, milanbalazs

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Oct 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 8335410 into containers:main Oct 7, 2024
16 checks passed
Alex-Izquierdo added a commit to ansible/eda-server that referenced this pull request Mar 26, 2025
…708 (#1248)

We were creating the container with "remove=True". This was due to the
first design where we had a worker attached to the container and we
relied on podman to get deleted the container once it is stopped. We
found that this parameter did not work properly in the podman client, so
we added a cleanup logic anyway. After the refactor of activations, this
was never needed, since we fetch the logs in the monitor after stop the
container, before remove it.

Now after the upgrade of the podman client, this [has been fixed
](containers/podman-py#435 the container is
actually removed when it is stopped. Our cleanup logic run under a
conditional "if container.exists()" so we can incur in race conditions
since there is a time window between podman stops the container and it
is get deleted, then, we can enter in the conditional and later raises
an exception because the container was deleted in the meantime.

Even in the successful case (we don't enter in the condition) now the
container gets deleted once it is stopped there is a potential loss of
the last output not gathered by the monitor.

1. Remove nested try/except block, it is a bug because NotFound
exception is also an ApiError exception.
2. Don't create the container with "remove=True"
3. Consolidate log level messages to warning. 

Jira: https://issues.redhat.com/browse/AAP-42708

Signed-off-by: Alex <aizquier@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants