rosetta: add option to ignore rosetta failure when installation cancelled #202
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
lstocchi
force-pushed
the
i160
branch
2 times, most recently
from
39cc8c3
to
92a0169
Compare
There was a problem hiding this comment.
Haven't tested this extensively, but looks good to me.
It would be nice to update the documentation as well to mention the new option, and to add a Fixes https://github.com/crc-org/vfkit/issues/160 in the commit log so that the corresponding issue is automatically fixed.
pkg/vf/rosetta_arm64.go
Outdated
| return fmt.Errorf("rosetta is not installed") | ||
| } | ||
| log.Debugf("installing rosetta") | ||
| if err := vz.LinuxRosettaDirectoryShareInstallRosetta(); err != nil { | ||
| if err := doInstallRosetta(); err != nil { | ||
| if dev.IgnoreIfMissing && strings.Contains(err.Error(), fmt.Sprintf("VZErrorDomain Code=%d", vz.ErrorOperationCancelled)) { |
There was a problem hiding this comment.
Instead of strings.Contains, maybe you can make use of errors.As to cast err to vz.NSError ( https://go.dev/blog/go1.13-errors ), and then check nsErr.Domain and nsErr.Code?
It will be a bit more verbose, but in a way "more correct".
However, maybe we want to ignore all errors in this situation, even failures to install not related to a cancellation?
There was a problem hiding this comment.
I think I tried the errors.As without success but you made a nice observation, I agree we should ignore all errors, makes sense. Updating it
pkg/vf/rosetta_arm64.go
Outdated
| if err := vz.LinuxRosettaDirectoryShareInstallRosetta(); err != nil { | ||
| if err := doInstallRosetta(); err != nil { | ||
| if dev.IgnoreIfMissing && strings.Contains(err.Error(), fmt.Sprintf("VZErrorDomain Code=%d", vz.ErrorOperationCancelled)) { | ||
| log.Debugf("user cancelled rosetta installation but ignoreIfMissing option is true") |
There was a problem hiding this comment.
Is there a way for (eg) podman to detect the rosetta installation failed? If not, we need to print something to stdout or stderr, which is not great, but will allow podman to know what happened.
There was a problem hiding this comment.
So, not sure what to do here and what's the suggested way to handle it. I just added a generic message to the stdout end the error on stderr
| defer func() { | ||
| checkRosettaDirectoryShareAvailability = origCheckRosettaDirectoryShareAvailability | ||
| doInstallRosetta = origDoInstallRosetta | ||
| }() |
There was a problem hiding this comment.
Not sure you need to restore the initial values after running the test? I'd expect each xxx_test.go file to run in its own process (to be checked though ;)
There was a problem hiding this comment.
So, I've been digging on it a bit and I didn't find any clear response but by looking at the code it seems that each test is run by launching a go routine https://github.com/golang/go/blob/182bdbb1e1d62fe53f8386e7b02c7738898d373b/src/testing/testing.go#L555 and from what I read all goroutines within a Go program share the same process so all tests run in the same process.
By creating a new test file I can see it to be true so I think it's better to keep the defer to avoid problems for future tests.
|
Forgot to update the documentation, updating it |
…lled
this adds a ignore-if-missing option that allows vfkit to not fail if the rosetta installation do not succeed for some reason (e.g. user cancelled installation).
So far, if the rosetta installation was cancelled or failed vfkit exited with an error. This behavior still exists if ignore-if-missing option is not set or it is false.
This will be used by podman bc there could be a fallback and rosetta could also be not mandatory containers/podman#23153
it fixes crc-org#160
This adds a ignore-if-missing option that allows vfkit to not fail if the rosetta installation is cancelled by the user.
So far, if the rosetta installation was cancelled vfkit exited with an error. This behavior still exists if ignore-if-missing option is not set.
it fixes #160