crc-org / crc

CRC is a tool to help you run containers. It manages a local OpenShift 4.x cluster, Microshift or a Podman VM optimized for testing and development purposes
https://crc.dev
Apache License 2.0
1.26k stars 242 forks source link

cache: Add condition to check binary name with arch suffix #4267

Closed praveenkumar closed 4 months ago

praveenkumar commented 4 months ago

As part of 926c78, libvirt driver download url change to have arch info and keeping the binary name as it is but as part of cmd/embeded.go it get the list using basename of uri and match with binary name which causing the following warning when crc binary have embedded driver and admin helper bit.

WARN Executable name is crc-driver-libvirt but extracted file name is crc-driver-libvirt-amd64

With this PR we are adding the condition where filelist and expected binary name match even it have arch info as suffix.

Testing

Try same with this PR and you shouldn't see this warning message.

openshift-ci[bot] commented 4 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from praveenkumar. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/crc-org/crc/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
cfergeau commented 4 months ago

I'd favour something like this if this works:

diff --git a/pkg/crc/cache/cache_linux.go b/pkg/crc/cache/cache_linux.go
index cfb623b3e..226816521 100644
--- a/pkg/crc/cache/cache_linux.go
+++ b/pkg/crc/cache/cache_linux.go
@@ -5,7 +5,10 @@ import (
 )

 func NewMachineDriverLibvirtCache() *Cache {
-       return newCache(libvirt.MachineDriverPath(), libvirt.MachineDriverDownloadURL, libvirt.MachineDriverVersion, getCurrentLibvirtDriverVersion)
+       cache := newCache(libvirt.MachineDriverPath(), libvirt.MachineDriverDownloadURL, libvirt.MachineDriverVersion, getCurrentLibvirtDriverVersion)
+       // upstream binaries are suffixed with the arch (crc-driver-libvirt-amd64)
+       // crc caches the binary as crc-driver-libvirt as it's the name expected by github.com/crc-org/machine
+       cache.ignoreNameMismatch = true
 }

 func getCurrentLibvirtDriverVersion(executablePath string) (string, error) {

Or to update the machine code to run crc-driver-libvirt-$arch instead of crc-driver-libvirt

praveenkumar commented 4 months ago

I'd favour something like this if this works:


diff --git a/pkg/crc/cache/cache_linux.go b/pkg/crc/cache/cache_linux.go
index cfb623b3e..226816521 100644
--- a/pkg/crc/cache/cache_linux.go
+++ b/pkg/crc/cache/cache_linux.go
@@ -5,7 +5,10 @@ import (
 )

 func NewMachineDriverLibvirtCache() *Cache {
-       return newCache(libvirt.MachineDriverPath(), libvirt.MachineDriverDownloadURL, libvirt.MachineDriverVersion, getCurrentLibvirtDriverVersion)
+       cache := newCache(libvirt.MachineDriverPath(), libvirt.MachineDriverDownloadURL, libvirt.MachineDriverVersion, getCurrentLibvirtDriverVersion)
+       // upstream binaries are suffixed with the arch (crc-driver-libvirt-amd64)
+       // crc caches the binary as crc-driver-libvirt as it's the name expected by github.com/crc-org/machine
+       cache.ignoreNameMismatch = true
 }

This mean we are ignoring the binary name, I think current PR is better that this one where we are just testing if binary name have arch suffix.

Or to update the machine code to run crc-driver-libvirt-$arch instead of crc-driver-libvirt

I do agree on that, but since this week is code freeze and we do need to deliver this time (hopefully without delay for 4.16) and if we make this change which means release of new version of crc-driver-libvirt. I can do that beginning of next sprint and then revert this change?

openshift-ci[bot] commented 4 months ago

@praveenkumar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 71d3b774c339adc21a6ec45fb125e8312d1d7371 link false /test security
ci/prow/e2e-crc 71d3b774c339adc21a6ec45fb125e8312d1d7371 link true /test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
cfergeau commented 4 months ago

if we make this change which means release of new version of crc-driver-libvirt.

This only needs changes in github.com/crc-org/machine, and to pick up these changes in crc, I filed https://github.com/crc-org/crc/pull/4268 for this.

cfergeau commented 4 months ago

we do need to deliver this time (hopefully without delay for 4.16)

If this warning only shows up when downloading a helper, and not when the helper is embedded, then it's not a problem if a fix is not present in the release.

praveenkumar commented 4 months ago

/close

In favor of #4268

openshift-ci[bot] commented 4 months ago

@praveenkumar: Closed this PR.

In response to [this](https://github.com/crc-org/crc/pull/4267#issuecomment-2230132942): >/close > >In favor of #4268 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.