Skip to content

Enhance support for Podman API in Docker client #8349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
May 23, 2023
Merged

Enhance support for Podman API in Docker client #8349

merged 7 commits into from
May 23, 2023

Conversation

whummer
Copy link
Member

@whummer whummer commented May 22, 2023 •
edited
Loading

Enhance support for targeting Podman API with our Docker client. Running on Podman has been increasingly requested by some customers recently, and during testing we've identified some gaps and discrepancies between the Docker API and Podman API responses (see also previous PR #8333).

Design Notes

This PR aims to be a starting point to address Podman support more systematically, and also have it covered by tests (we can reuse our excellent Docker client test suite for that purpose). Some/most of the existing Docker client tests are already working against Podman, and a new test marker @skip_for_podman has been introduced to temporarily skip failing tests for now. The goal would be to make all test pass over time, and then remove the @skip_for_podman marker.

The initial idea was to create a derived PodmanClient and inherit the functionality from the Docker client, but it turns out that the two platforms are sufficiently similar to handle both targets in a single client implementation. The majority of compatibility switches (especially in the CmdDockerClient class) is to handle slightly different response payloads or error messages returned by the docker vs podman commands.

The PR introduces a new CI build workflow to run the Podman Docker client tests in a separate build, whenever any of the relevant files are changing.

Patch vs minor release

Currently the changes are targeting a simple patch release and are not behind a feature flag (arguably shouldn't be breaking, as so far we didn't officially support Podman, and our Docker test coverage is pretty decent). I'm open to discussion, though, whether we want to add the compatibility mode behind a feature flag for now. 👍

Summary of changes

  • adjust the logic in the Docker CLI and SDK clients to support both, Docker and Podman API responses
  • add strip_wellknown_repo_prefixes utility to strip off well-known image repo names like localhost/ or docker.io/library/ which are added by the Podman API, but not by Docker
  • introduce a new CI build workflow to run the Podman Docker client tests in a separate build

@whummer whummer added the semver: patch Non-breaking changes which can be included in patch releases label May 22, 2023
@whummer whummer force-pushed the test-podman branch 12 times, most recently from 0d60dba to 0ffa62a Compare May 22, 2023 20:29
@whummer whummer force-pushed the test-podman branch 2 times, most recently from 42b0b25 to 25110d1 Compare May 22, 2023 21:51
@whummer whummer changed the title [wip] Enhance support for podman API in Docker client [wip] Enhance support for Podman API in Docker client May 22, 2023
@whummer whummer force-pushed the test-podman branch 5 times, most recently from aa5ff5b to bf1d596 Compare May 23, 2023 00:17
@whummer whummer force-pushed the test-podman branch 4 times, most recently from 1a471a5 to 5b86a3b Compare May 23, 2023 09:27
@coveralls
Copy link

Coverage Status

Coverage: 82.26% (-0.007%) from 82.267% when pulling 5b86a3b on test-podman into b1b3ac4 on master.

@whummer whummer force-pushed the test-podman branch 2 times, most recently from a7a26fc to 98fdb6e Compare May 23, 2023 12:12
@whummer whummer force-pushed the test-podman branch 2 times, most recently from 0490d44 to d24f54e Compare May 23, 2023 12:50
@whummer whummer force-pushed the test-podman branch 2 times, most recently from 49f2b26 to 4990a44 Compare May 23, 2023 13:35
@whummer whummer changed the title [wip] Enhance support for Podman API in Docker client Enhance support for Podman API in Docker client May 23, 2023
@whummer
Copy link
Member Author

whummer commented May 23, 2023 •
edited
Loading

test_gateway_server unit test still failing (unrelated, also failing on master):

FAILED tests/unit/http_/test_hypercorn.py::test_gateway_server - RuntimeError...

@whummer whummer marked this pull request as ready for review May 23, 2023 13:57
@whummer whummer requested review from alexrashed and thrau May 23, 2023 13:57
whummer
whummer commented May 23, 2023
View reviewed changes
tests/integration/conftest.py
@@ -180,21 +179,6 @@ def localstack_runtime():
return


@pytest.fixture
Copy link
Member Author

Choose a reason for hiding this comment

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

minor refactoring: moving this fixture to API Gateway tests module (no functional change)

thrau
thrau approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Love to see the podman support being tested 👍

I'm OK merging it as is for now. Having said that, at this point, I would have preferred introdcing a PodmanContainerClient class implementation that inherits from the docker sdk client, but isolates the peculiarities of podman into the implementation, rather than putting something like strip_wellknown_repo_prefixes into the base abstraction.

Anyway, nice work!

@github-actions
Copy link

github-actions bot commented May 23, 2023 •
edited
Loading

LocalStack Community integration with Pro

2 067 tests   1 785 ✔️  1h 16m 55s ⏱️
       2 suites     282 💤
       2 files           0 ❌

Results for commit 3529694.

♻️ This comment has been updated with latest results.

alexrashed
alexrashed approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

LGTM! I agree that we should create a separate client class in the next iteration :)
Concerning the bump type: I would say it's a minor change. But this doesn't mean that it needs to be behind a feature flag right now. We currently only block breaking changes from being merged to master.

@whummer
Copy link
Member Author

whummer commented May 23, 2023 •
edited
Loading

@thrau @alexrashed +1 , totally agree - let's pull it out into a separate client in the next iteration. Was very much on the fence whether to extend the base interface here (especially the strip_wellknown_repo_prefixes is indeed a bit painful!).

We'll need to extend the logic in create_docker_client() a bit to detect whether to create a Docker or a Podman client (e.g., by inspecting the system info, and allowing an override via config variable). We'll also need to consider the fact that we allow lazy initialization of the client in case the API is intermittently down (this is where a config override could help). Good plan for the next iteration! 👌 (I also added a TODO comment in the code now..)

@whummer whummer merged commit 29bc673 into master May 23, 2023
@whummer whummer deleted the test-podman branch May 23, 2023 16:23
@alexrashed alexrashed mentioned this pull request Sep 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

@thrau thrau thrau approved these changes

@dfangl dfangl Awaiting requested review from dfangl

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@whummer @coveralls @alexrashed @thrau