Skip to content
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.

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

Improve Podman compatibility #414

Merged
merged 7 commits into from
Jun 22, 2022

Conversation

prskr
Copy link
Contributor

@prskr prskr commented Mar 8, 2022

What does this PR do?

Although Podman claims to be API compatible with Docker and for instance in #336 the consent seems to be let's wait until all problems are gone 😅 I thought I'd see what can be done as kind of a compromise.

The problems I could identify (and solve) are:

  • The current implementation assumes the default network name is bridge which isn't the case for Podman. Even worse, bridge is not even a valid network name with Podman because it's also a network mode and Podman does not allow the creation of networks conflicting with network modes.
  • The current implementation of the reaper has a hard coded path where it expects the Docker socket. Neither DOCKER_HOST env variable nor any other customization the testcontainers docs are describing are honored. This is also an issue with rootless Docker.
  • Podman requires at least one stream to be attached when a command is executed. It doesn't hurt to attach stdout and/or stderr but otherwise it breaks the exec wait strategy.

DefaultNetwork option

The DefaultNetwork option allows Podman users to configure the name of the default network. If it's not set the previous auto-detection is applied so no breaking change.

PodmanProvider provider type

Podman does not allow ambiguities between network mode names and network names therefore the default Podman network is called podman unlike the bridge network of Docker.
To be able to detect both cases without a breaking change the second ProviderType was introduced. Current test implementations will just behave like before but with this addition users of testcontainers-go can either explicitly set the provider type or setup a GenericProvider to their likes and use that one.

Honoring DOCKER_HOST and TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE

The DOCKER_HOST env variable was already considered when creating the Docker API client. Now its value is propagated (via the context.Context) to the reaper as a fallback if TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE isnt' set. If both env variables are empty the default socket path /var/run/docker.sock will be assumed.

Default NetworkMode if not set

In cases when the NetworkMode isn't set it will be set to bridge to avoid issues with Podman. Docker apparently assumes bridge if nothing's set therefore this shouldn't break anything either.

Parallel tests

To speed up the test suite I marked as many tests as possible to be executed in parallel. Some tests (like the one running in host mode) cannot be executed in parallel because that would result in port conflicts.

Compatibility state

Almost all tests are now compatible with Podman. Some are skipped because they are not compatible with Podman in rootless mode but this would be the same for Docker rootless.
Some docker-compose tests are failing in the CI pipeline with Podman 3.x but are running on my local machine with Podman 4.x so I assume they will be compatible eventually.

Changes in test suite

Add Podman pipeline

As stated by @mdelapenya testcontainers-java has a PoC of a Podman pipeline that I 'borrowed' 😄
They are installing Podman 3.x in rootless mode which has some consequences:

  • Docker-in-Docker does not work
  • the new Netavark/Aardvark network is not yet included

compose_test.go

  • Replace CLI calls withAPI calls to not depend on the availability of the docker CLI. This way validations are also possible against the Podman API.
  • Handle different naming schemes like <identifier>-<service-name>-<instance-count> vs <identifier>_<service-name>_<instance-count> by filtering with regular expressions

container_test.go

Use full qualified image names (e.g. docker.io/alpine) because some Podman distributions do not fallback to docker.io by default and this way the container image registry becomes explicit.

logconsumer_test.go

Skip test that cannot be executed with rootless Podman because it depends on Docker-in-Docker

docker_test.go

  • Use docker.io/nginx:alpine where possible
  • Use a custom nginx config opening port 8080 for host mode tests to be able to test them with rootless Podman
  • Unify container termination in all test cases and replace defer calls with *testing.T.Cleanup()
  • Set new Podman provider type to correctly handle default network bridge vs. podman
  • Use full qualified image names (means including registry)

testresources

  • Use full qualified image names in all Dockerfiles and docker-compose files
  • Add custom nginx highport config

I'm deeply sorry about the total size of the PR. If you have a good idea how to split it into different parts I will look into it! Some parts are for sure not necessary but I thought it would be good for example to unify how images are referenced, how containers are cleaned in test cases any many things more I stumbled upon while I was trying to get all tests working.
Also updating all GitHub actions could be moved to another PR but adding a new pipeline with outdated actions seemed weird and having different versions of the actions in different pipelines too so I also unified that 😅
Let me know what you think and I'm sure we can figure out something 😊

@prskr
Copy link
Contributor Author

prskr commented Mar 8, 2022

I just noticed there's also #407 which also adds TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE support - if that one's merged I will of course adopt these changes 🙈

@mdelapenya
Copy link
Member

I just noticed there's also #407 which also adds TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE support - if that one's merged I will of course adopt these changes 🙈

Hey @baez90 thanks again for the PR and the explanation to add context and motivation for it. It makes a difference in the review process.

I think #407 is super simple and straight forward, so we could merge that PR first and then rebase your code with that. Does it makes sense to you?

@prskr
Copy link
Contributor Author

prskr commented Mar 9, 2022

Sure thing, I think I've already an idea how to incorporate the changes from #407 with my own 😊

Glad to hear the descriptions are getting a bit better, I'm still learning but I've to admit that I really like the idea!

@mdelapenya
Copy link
Member

One thing that I'd like you to explore before merging this PR is the idea of running the entire test suite using podman instead of Docker. Do you want to contribute a Github Action workflow specific to it?

@prskr
Copy link
Contributor Author

prskr commented Mar 9, 2022

Interesting idea. I'll have a look into it how much I'd have to adjust in the current test suite to make this possible.
Frankly: some of docker-compose tests aren't working on my machine either way, hence might get a bit hard but I'll see what I can do!

Edit: at some point it might be necessary to detect if Podman or Docker is used...would you see that rather in the test suite or in the library code? I'd probably rather adopt the tests to cover some Podman specifics than making the DockerProvider Podman aware...what do you think?

@mdelapenya
Copy link
Member

The java folks have a PoC for Podman here: testcontainers/testcontainers-java#4583

@prskr
Copy link
Contributor Author

prskr commented Mar 18, 2022

awesome! Thanks for letting me know!
Regarding my previous question: do you mind me adopting the test suite to be able to distinguish between Podman and Docker? At least to be able to configure the DockerProvider with the right default network and stuff?

@fpozzobon
Copy link

That would be great to have this PR merged :)

@prskr
Copy link
Contributor Author

prskr commented Apr 7, 2022

I'm still both waiting for #407 to be merged and working on some improvements (as soon as I can spare some time) to - hopefully - run tests against Podman in the CI - although I'm still not sure if I'll be able to make it 100% compatible.

Sorry for the delay I think I should be able to work on this tomorrow 😊

@prskr prskr force-pushed the improve-podman-compatiblity branch from 1583528 to da54e7b Compare April 7, 2022 19:50
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #414 (c6e480f) into main (41a9d3d) will increase coverage by 1.31%.
The diff coverage is 59.20%.

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
+ Coverage   65.61%   66.92%   +1.31%     
==========================================
  Files          19       20       +1     
  Lines        1201     1823     +622     
==========================================
+ Hits          788     1220     +432     
- Misses        305      483     +178     
- Partials      108      120      +12     
Impacted Files Coverage Δ
network.go 0.00% <0.00%> (ø)
container.go 80.72% <33.33%> (-6.52%) ⬇️
compose.go 74.04% <46.42%> (-2.73%) ⬇️
reaper.go 76.85% <62.50%> (-2.84%) ⬇️
docker.go 68.58% <73.07%> (+2.75%) ⬆️
generic.go 67.85% <0.00%> (-2.15%) ⬇️
logger.go 100.00% <0.00%> (ø)
mounts.go 100.00% <0.00%> (ø)
testing.go 0.00% <0.00%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41a9d3d...c6e480f. Read the comment docs.

@prskr prskr force-pushed the improve-podman-compatiblity branch 7 times, most recently from 32897fb to 037f878 Compare April 11, 2022 14:42
@prskr
Copy link
Contributor Author

prskr commented Apr 11, 2022

@mdelapenya now that the pipeline does not fail anymore I'd consider this done. Let me know if I shall split this PR or anything else. I also updated the description to cover the latest changes.

I know that some tests in the Podman pipeline are still a bit flaky but I'd like to wait until Podman 4.x is ready for Ubuntu and revisit the pipeline then once more?

docker.go Show resolved Hide resolved
@prskr
Copy link
Contributor Author

prskr commented Apr 29, 2022

RC1 for Podman 4.1 mentions a fixed but that resolves the requirement to always set a bridge network for Podman containers because now the bridge network is assigned automatically again.

Does anyone know if the work around to add a new container at first to the bridge network and then to all others is still required? Becuase if not the whole PodmanProvider would become obsolete and this PR would become much easier and smaller again.

@mdelapenya any thoughts? How do you like to proceed with this PR and #407 ?

@prskr
Copy link
Contributor Author

prskr commented May 10, 2022

I just got the update to Podman 4.1 and it seems like the issue with the missing default network is now resolved. I probably could get rid of some code but the question is do we want to rely for Podman users to have the latest version or shall we test also against a 3.x version? @mdelapenya any thoughts?

@prskr prskr force-pushed the improve-podman-compatiblity branch from 037f878 to 21d8636 Compare May 10, 2022 04:59
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Hey @baez90 this is great and the code LGTM! Thank you very much for your time and dedication to support Podman.

I'll be honest: I need time to download the PR and play with Podman as I never did it in the past.

But the tests seems OK, which is a great step. Let me play with this version of the PR, and hopefully soon we will see this baby merged into the code.

Good job!

compose_test.go Show resolved Hide resolved
compose_test.go Outdated Show resolved Hide resolved
compose_test.go Show resolved Hide resolved
container_test.go Show resolved Hide resolved
docker_test.go Show resolved Hide resolved
docker_test.go Show resolved Hide resolved
docker_test.go Show resolved Hide resolved
network.go Show resolved Hide resolved
reaper.go Outdated Show resolved Hide resolved
@prskr prskr force-pushed the improve-podman-compatiblity branch 2 times, most recently from a9c2557 to 3b17b33 Compare May 11, 2022 19:31
@prskr
Copy link
Contributor Author

prskr commented May 11, 2022

Thank you very much for taking your time to review the PR and giving me feedback and everything!

From my side most things are done now. I've been thinking of removing the Podman provider again but without it the default network detection is still problematic.I don't really know if the special behavior to join a new container always to the bridge network first if it is among the networks the container should be joined to is still necessary or not and therefore don't really want to touch it especially because the PR is already so huge...

The current implementation works quite good with Podman 4.1. There are some tests still failing but those are not directly related to the Podman API compatibility but to the fact that Podman does not really handle a lot of parallel container operations really good (no daemon, file system locks for the BoltDB...). I raised an issue for the Docker API version problem, let's see what's coming from them but it shouldn't be a blocker for now I guess?

I'd like to add that not all changes I made are only necessary for Podman but also come in handy if someone's using Docker rootless (e.g. mounting the right socket to the reaper not always /var/run/docker.sock ).

Let me know if you encounter any problems or if I should change some things 😊

@prskr prskr force-pushed the improve-podman-compatiblity branch from 3b17b33 to 1cdb5b3 Compare May 13, 2022 11:29
@prskr prskr force-pushed the improve-podman-compatiblity branch 4 times, most recently from 1ecd98d to 64af8fe Compare June 2, 2022 19:05
@prskr
Copy link
Contributor Author

prskr commented Jun 2, 2022

@mdelapenya unfortunately I cannot reproduce your issue with Podman on Windows.
Some of the tests are working but most are also failing but not because of Podman directly but because it's 'rather complicated' to mount the npipe on Windows as a socket to ryuk. I'll see if I can install Go into the Podman WSL but that seems rather hacky.

I tuned the CI pipeline to install Podman 4.1 from Debian experimental (still no Podman 4 for Ubuntu 20.04 or 22.04) and it seems as this would work now as expected including running tests in parallel and without any 'continue on error' setting in place.

Apart from some more experiments with Podman on Windows I'd say this actually looks quite good from my side, what do you think?

@prskr
Copy link
Contributor Author

prskr commented Jun 20, 2022

@mdelapenya is there something missing here? Of course I'll rebase this PR once more if the other PR is merged. But at some point I'd really appreciate it to get this either merged or I'll close it as there seems to be no interest in this. I don't see any sense investing more time in keeping this up to date without any further progress/feedbackq on how to go on with this PR.

@mdelapenya
Copy link
Member

@mdelapenya is there something missing here? Of course I'll rebase this PR once more if the other PR is merged

We merged a PR regarding LogConsumer yesterday and caused the conflicts. Could you resolve them 🙏

But at some point I'd really appreciate it to get this either merged or I'll close it as there seems to be no interest in this. I don't see any sense investing more time in keeping this up to date without any further progress/feedbackq on how to go on with this PR.

@baez90 please forgive me. It IS of interest and I do appreciate your hard effort in contributing it. I shared my concerns about not being able to make it work on my machine, but it does not mean it's of no interest. I've been focused on smaller PRs with lower impact and less time to review.

Your PR represents the big star for the new release and I'd like to create a great experience using TC-go with Podman. So please please do not hesitate about the importance of your contributions. Sometimes maintainers need to juggle with company work, family time and open source 😞

- allow configuring the default network name - it's podman for Podman instead of bridge and 'bridge' is not a valid network name with Podman
- allow configuring an alternative Docker socket path for the reaper - this is also useful for rootless Docker
…c defaults

Set provider type for all docker_test.go test cases
Failures during test execution are ignored for now. Passing all tests can be achieved over time.
@prskr prskr force-pushed the improve-podman-compatiblity branch from 64af8fe to fb47fba Compare June 21, 2022 15:39
@mdelapenya
Copy link
Member

@baez90 I'd appreciate not pushing-force with every commit submission 🙏, as the mental effort of having to review the entire PR again after a push, instead of reviewing the incoming delta, is also exhausting 😔

Please don't get me wrong, it may be a personal preference, as I've seen this style in many other places. And as there are no contributing guidelines yet, is totally fine ☺️

I'm now AFK but will try my best with this PR ASAP

@prskr
Copy link
Contributor Author

prskr commented Jun 21, 2022

@mdelapenya I didn't change anything I only rebased the branch to resolve conflicts 😉
I absolutely know what you mean and normally I try to work in...well defined increments while working on a PR and only amend changes e.g. when working on the pipeline to avoid "for ***** sake - let the pipeline pass' commits 😄
From time to time I forget this and appreciate the reminder 😊

Regarding your concern testing the PR on MacOS: like I mentioned earlier I had similar problems running the test suite on Windows with Podman Machine due to limitations especially with the reaper because I couldn't find any possibility to mount native Windows pipes as Linux socket into the ryuk conainer. But other tests were running. I don't have access to a Mac so I can't say if there are similar problems to be expected with the way Podman is working on MacOS.

I already added the retry logic you mentioned in another PR (I think) to the Podman pipeline and I'm thinking of doing the same for the Docker pipeline as the last checks didn't even fail in code I touched 😅 (from what I can tell) and also only for Docker not for Podman...

Apart from that the main change is actually only how the default network is handled to ensure Docker gets its bridge network while Podman requires podman and I added some code to consider some environment variables although most of this is now already merged by other PRs 😅

I'm also sorry for the probably a bit harsh words earlier. It just happened to be a little bit frustrating.

ComposeVersion allows to format service names, volume names,... compatible to the installed docker-compose version.
Also parallel execution of tests is removed again as it makes more trouble than it helps, apparently.
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

It took a while, and sorry about that. I see you when you expressed your frustration, and as a maintainer it's my mission to keep you engaged, even more after this huge and fantastic contribution. Really well done

@mdelapenya mdelapenya requested a review from gianarb June 22, 2022 05:51
@mdelapenya
Copy link
Member

@gianarb I'd appreciate a final review before merging this one 🙏

@gianarb
Copy link
Member

gianarb commented Jun 22, 2022

This PR is a tremendous effort but it has everything it needs! Testing, a new set of CI jobs. For me it can be merged! Thanks

@gianarb gianarb added the feature New functionality or new behaviors on the existing one label Jun 22, 2022
@mdelapenya mdelapenya merged commit df722bc into testcontainers:main Jun 22, 2022
@mdelapenya
Copy link
Member

Big kudos to @baez90 for this humongous job!

Awesome!

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one podman Issues regarding podman.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants