feat(install): Adds support for podman(compose) #3673
Conversation
…mpatibility with docker
There was a problem hiding this comment.
Quite surprised (positively!) that the changes needed are so few! Added a few comments and on top of those, we definitely should have a test using podman now to make sure this actually works and does not break in the future (can help with this if you need me).
Thanks so much for taking the effort!
|
FYI tests failed with the following error:
|
I agree that having a test would be great. In the best case, we can run the whole suite for both docker and podman. I'm just unsure about the environment this is run on and would welcome your input and ideas very much! |
…pose build; Substitutes occurences of
|
Have converted back to draft, as there are some tweaks to make, still. |
|
@DuncanConroy Let us know if you need help in terms of anything, we'd be happy to help you. Having this PR really made us happy :) |
| @@ -29,6 +29,8 @@ Options: | |||
| --no-report-self-hosted-issues | |||
| Do not report error and performance data about your | |||
| self-hosted instance upstream to Sentry. | |||
| --container-engine-podman | |||
There was a problem hiding this comment.
Same as above: how about this just being --container-engine with a default value of docker?
There was a problem hiding this comment.
Not sure I understand your intent. This was thought as a switch to activate podman over docker. We can surely remove the autodetection and completely rely on this switch. I'd still keep it --container-engine-podman, because just using --container-engine would possibly be misleading?
There was a problem hiding this comment.
@DuncanConroy sorry for not being clearer. What I had in mind was making --container-engine a flag to pass in a custom value as in --container-engine <engine> where we default to docker (so when the flag is not passed, we will assume docker).
Although we would only support podman and docker at start, this should keep the API unchanged if we ever add new engines like runc or similar.
There was a problem hiding this comment.
Not sure how to do this with bash, tbh. :D
There was a problem hiding this comment.
I'll see what I can do about this 😅
There was a problem hiding this comment.
It is quite simple... unless you want to validate argument :)
Something like
case "$1" in
<...>
--container-engine) CONTAINER_ENGINE="$2"; shift ;;Modifies docker-compose.yml with pull_policy: never, which also works for podman-compose.
…ait_ready function
what is "nofile restriction"?
okay 👍 |
ah you mean nofile limits on linux.. |
|
Ah I see the problem now. |
|
Another bug in podman-compose containers/podman-compose#1118 I stepped on. |
|
ughh I wanna skip the test |
|
@BYK @aminvakil hey, I wanna ship this as soon as possible. is it okay if we skip the actual integration tests, and just trying to run |
| create_command="$CONTAINER_ENGINE volume create" | ||
| if [ "$CONTAINER_ENGINE" = "podman" ]; then | ||
| create_command="$create_command --ignore $1" | ||
| else | ||
| create_command="$create_command --name=$1" | ||
| fi | ||
|
|
||
| $create_command | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Okay, so the state is:
|
I'll try testing this branch on a clean environment with podman and report back. |
|
Initial try fails with: seems like an error from my environment. I just installed podman, podman-compose and executed |
|
Yea. Recent podmans require register host in image name |
|
Look at https://github.com/vllm-project/vllm/pull/19236/files for reference |
There was a problem hiding this comment.
@doc-sheet Thanks, yeah it worked.
Full log:
https://gist.github.com/aminvakil/336d2e6afa7652a9740f8ebb5fb77306
|
Let's wait for Burak's approval then. |
|
Another thought, I'm wondering whether we should silent these "errors": |
|
Honestly, I'm not a fan of the idea of such a large patch getting landed for something we are not testing nor not many people are using. If this cannot become stable, can we consider making it a patch file like you did for external kafka? Working on this made me realize that |
What do you think about stating this as experimental on release notes or when it's getting used? I have no idea how many users use podman though. |
|
+1 for stating this as experimental instead of putting this to optional-modifications. |
|
@aldy505 @aminvakil maybe I'm blowing this out of proportion but I still feel like the amount of changes and added complexity needed to support Podman (and not even completely and reliably!) seems way over the value add for (apparently) a very small number of people. One may argue this is a chicken & egg problem but I'm not sure if we should try to address such a problem? My take away is: we got asked for Podman support many years ago and we said "may be in the future?". That future kind of came around and we spent more than a week to make that happen and it still doesn't work reliably. Let's cut the losses. The PR will be there if we (or someone else) wants to pick this up again later. |
There was a problem hiding this comment.
@BYK Honestly I don't know how many users use podman and need support for it.
Also can you elaborate on working reliably? The integration test which is failing, is exactly using docker-compose and will fail.
That being said I agree with you not merging it as it is unless tests get fixed as well, therefore removing my approval.
We keep bumping into edge cases in podman or more accurately |
Adds support for podman(compose), while maintaining compatibility with docker
Introduces a new script to detect podman vs. docker. Distinguishes between docker and podman minimum versions and substitutes uses of docker with a variable instead.
Closes #369
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.