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 container runtime detection #32309

Merged
merged 1 commit into from
Apr 13, 2023
Merged

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Mar 31, 2023

Fully resolve the container runtime once and for all. It costs us one more call when the rootless info is not needed but I prefer things to be resolved entirely once, rather than using patterns that are a bit brittle.

Fixes #32246

@Karm any chance you could have a look at this? I /think/ it's the last rabbit hole. It's probably worth having a look at the result, rather than the diff as the diff is not very readable. Things should be much simpler to follow.

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Nice!

@gastaldi gastaldi requested a review from Karm March 31, 2023 17:02
@Karm
Copy link
Member

Karm commented Mar 31, 2023

I will whip up the test env (Windows Podman, Windows Docker, Windows both, Windows none, Linux none, Linux Docker, Linux Docker rootless, Linux Podman, Linux Podman rootless) and report back next week...
I'll try to stitch together at least a rudimentary CI Windows Podman x Docker on the workstation I resurrected for this purpose.

@Karm
Copy link
Member

Karm commented Mar 31, 2023

It costs us one more call when the rootless info is not needed

I'll record the calls again. One is fine. One each class init or something like in the past...not so much.

e.g. info is an order of magnitude more costly (hundreds ms) than version (tens of ms) on my system. The same for podman on another contemporary XEON (Icelake) system.

I usually run test suites en bloc, so things tend to add up...

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Apr 10, 2023

If it is decided we want this, it will need a rebase

@geoand geoand added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Apr 10, 2023
Fully resolve the container runtime once and for all. It costs us one
more call when the rootless info is not needed but I prefer things to be
resolved entirely once, rather than using patterns that are a bit
brittle.

Fixes quarkusio#32246
@Karm Karm force-pushed the rootless-detection branch from 49d764b to f2f24be Compare April 13, 2023 12:46
@Karm
Copy link
Member

Karm commented Apr 13, 2023

My Quarkus time was eaten by the AWT monster. I'm switching context to this now... I rebased the branch.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 13, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gastaldi gastaldi merged commit bbe0ade into quarkusio:main Apr 13, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone Apr 13, 2023
@Karm
Copy link
Member

Karm commented Apr 13, 2023

👍 https://karms.biz/pastebin/podman-linux-lkjfdlkfsh.txt

  • Windows Docker and Podman cool too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core kind/bugfix triage/needs-rebase This PR needs to be rebased first because it has merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quarkus doesn't detect Docker rootless anymore
4 participants