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

Prioritize addtionalHelperBinariesDir over default dirs #1754

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

ashley-cui
Copy link
Member

When addtionalHelperBinariesDir is set, first search that path for helper binaries, then the default locations.

Closes: containers/podman#20808

@ashley-cui
Copy link
Member Author

Tested by building with HELPER_BINARIES_DIR="/opt/podman/qemu/bin"

With this fix:

acui@Ashleys-Mini podman % ps aux | grep qemu
acui             12489   0.2  0.3 412252816  22240 s002  S     1:51PM   1:05.48 /opt/podman/qemu/bin/qemu-system-aarch64

Without this fix:

acui@Ashleys-Mini podman % ps aux | grep qemu  
acui             13480   2.0  0.2 412195168  18208 s002  S     2:06PM   1:03.42 /opt/homebrew/bin/qemu-system-aarch64--

@ashley-cui
Copy link
Member Author

@benoitf PTAL

@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

LGTM
/approve
@containers/podman-maintainers PTAL
This should be back ported

Copy link
Contributor

openshift-ci bot commented Nov 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

@cfergeau PTAL

@mheon
Copy link
Member

mheon commented Nov 29, 2023

Sure, LGTM

@cfergeau
Copy link

I would also change

 var defaultHelperBinariesDir = []string{
+       // Relative to the binary directory
+       "$BINDIR/../libexec/podman",
        // Homebrew install paths
        "/usr/local/opt/podman/libexec/podman",
        "/opt/homebrew/opt/podman/libexec/podman",
@@ -42,6 +44,4 @@ var defaultHelperBinariesDir = []string{
        "/usr/local/lib/podman",
        "/usr/libexec/podman",
        "/usr/lib/podman",
-       // Relative to the binary directory
-       "$BINDIR/../libexec/podman",
 }

but also LGTM

@benoitf
Copy link

benoitf commented Nov 29, 2023

hello, I patched the file in podman vendor/github.com/containers/common/pkg/config/default.go and I did create the installer but it does work only for gvproxy in my case

I used

make ARCH=aarch64 NO_CODESIGN=1 pkginstaller

then installed it

if I run it, now I can see that it's using gvproxy from the /opt/podman/bin folder but qemu is still launched from brew

benoitf          50363  37.9  2.0 441821360 1987296 s002  S    10:55AM   0:26.68 /opt/homebrew/bin/qemu-system-aarch64 -m 30895 -smp 5 -fw_cfg ....
benoitf          50362   0.2  0.0 409236784  31904 s002  S    10:55AM   0:00.11 /opt/podman/qemu/bin/gvproxy -debug -mtu 1500 ....

also I'm not sure to follow why it's using /opt/podman/qemu/bin/gvproxy and not /opt/podman/bin/gvproxy but at least it's using one binary from the installer and not from the system

@benoitf
Copy link

benoitf commented Nov 29, 2023

update: it works with a fresh machine, but not with an existing machine so somehow the path to qemu is persisted in the machine configuration

I don't know if it's a feature or a bug but I would expect that it should not store the path to the qemu binary

When addtionalHelperBinariesDir is set, first search that path for helper binaries, then the default locations.

Signed-off-by: Ashley Cui <acui@redhat.com>
@ashley-cui
Copy link
Member Author

@cfergeau Fixed, thanks!

@benoitf Currently we do store the path to the binary in the config. I think finding the binary on start time and not init time is something we should consider, but that work is in podman anyway, so it wouldn't be fixed by this PR. Maybe we open an issue on that?

@cfergeau
Copy link

/lgtm

Copy link
Contributor

openshift-ci bot commented Nov 29, 2023

@cfergeau: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@baude
Copy link
Member

baude commented Nov 29, 2023

I think finding the binary on start time and not init time is something we should consider,

this will be part of podman 5; i dont intend to change the behavior of the config files for podman 4.x. while this seems obvious to do this not that PD is part of the equation, the fully qualified binary path was done to protect against the opposite of this problem ... that is to say, a new qemu binary is found somewhere and chosen over the known working one.

@baude
Copy link
Member

baude commented Nov 29, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 29, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 3b9abaa into containers:main Nov 29, 2023
7 checks passed
@benoitf
Copy link

benoitf commented Nov 29, 2023

I'm fine to not have that in 4.x but if it's taken into account for 5.x

@benoitf
Copy link

benoitf commented Nov 29, 2023

@ashley-cui do you know when it'll land into podman main branch (because it's using an old version of containers/common there)

@baude
Copy link
Member

baude commented Nov 29, 2023

@ashley-cui do you know when it'll land into podman main branch (because it's using an old version of containers/common there)

it can be vendored whenever you want ... the main branch is podman 5 now

@ashley-cui
Copy link
Member Author

@benoitf
Copy link

benoitf commented Nov 29, 2023

@baude yes I'm using podman main branch this week (5.0.0-dev) 👍 (just I think it's more convenient when the patch is already included and not having to include it by myself)

@ashley-cui thanks

@benoitf
Copy link

benoitf commented Dec 6, 2023

hello it does not seem to have been backported to 4.8.x branch

@baude
Copy link
Member

baude commented Dec 6, 2023

was it asked for ?

@baude
Copy link
Member

baude commented Dec 6, 2023

actually history suggestions you were fine with that ??

@benoitf
Copy link

benoitf commented Dec 6, 2023

it has been asked there
#1754 (comment)

I replied that I was OK for the storage of qemu binary location in the machine definition for 4.x and that it could wait for 5.x

@Luap99
Copy link
Member

Luap99 commented Dec 7, 2023

/cherry-pick v0.57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prefer podman's installer binary (gvproxy, qemu) over system wide binaries
7 participants