Skip to content

Conversation

whummer
Copy link
Member

@whummer whummer commented May 18, 2023

Fix extraction of container Image when using podman. We have some customers who are using podman (with Docker not being available), and one of the issues that has surfaced is that the list_containers(..) Docker SDK call fails when using podman.

image

The root cause is that certain versions of podman use a slightly different API response format for listing containers: containers/podman#8329 . In particular, the Image attribute of containers is "sha256:..." for Docker, but it contains the actual image name/tag like "myimage:mytag-123" for podman.

A simple integration test is added which simulates the podman API response via a monkey patch. Without the fix in the PR, this test would fail, and the patch in docker_sdk_client.py makes the test pass (and should fix the customer issue, too).

We can consider contributing this fix to the upstream repo, too - but for now the goal is to get a fix out quickly to our users.

The PR also removes a few unused strings in constants.py. 🧹

@whummer whummer requested a review from simonrw May 18, 2023 11:36
@whummer whummer added the semver: patch Non-breaking changes which can be included in patch releases label May 18, 2023
@github-actions
Copy link

LocalStack Community integration with Pro

2 047 tests   1 763 ✔️  1h 22m 10s ⏱️
       2 suites     284 💤
       2 files           0

Results for commit 3aa8eaf.

Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'm glad we have a test covering the behaviour, and if it fixes a discrepancy between docker and podman, then it's all great!

Comment on lines 722 to 729
def container_init(self, attrs=None, *args, **kwargs):
# Simulate podman API response, Docker returns "sha:..." for Image, podman returns "<image-name>:<tag>".
# See https://github.com/containers/podman/issues/8329
attrs["Image"] = image_name
container_init_orig(self, attrs=attrs, *args, **kwargs)

# apply patch to simulate podman behavior
container_init_orig = Container.__init__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably my issue, but I find it really confusing to have a variable used (container_init_orig) before it has been defined. I know Python's scoping rules allow this, but still it's weird. Not that you need to change it, just raising for discussion.

... Or you could move the function definition to in between lines 728 and 729?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've gotten so used to Python's scoping that I don't notice the difference anymore (although you already pointed it out to me several times!). 😄 But makes sense, updated it! 👍

@whummer whummer merged commit d79658c into master May 18, 2023
@whummer whummer deleted the podman-image-fix branch May 18, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants