-
Notifications
You must be signed in to change notification settings - Fork 239
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
metadata: Add logic to fallback to quay if mirror is unreachable #3802
Conversation
pkg/crc/machine/bundle/metadata.go
Outdated
@@ -353,7 +353,12 @@ func Download(preset crcPreset.Preset, bundleURI string) (string, error) { | |||
// bundles, their sha256sums are known and can be checked. | |||
if bundleURI == constants.GetDefaultBundlePath(preset) { | |||
if preset == crcPreset.OpenShift || preset == crcPreset.Microshift { | |||
return DownloadDefault(preset) | |||
downloadedBundlePath, err := DownloadDefault(preset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to differentiate "not found" from other errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau looks like grab
package have StatusCodeError
type and can validated with IsStatusCodeError
which I now used.
StatusCodeError indicates that the server response had a status code that
// was not in the 200-299 range (after following any redirects).
f0f606b
to
504ffef
Compare
504ffef
to
048a2d7
Compare
048a2d7
to
f0e1b73
Compare
f0e1b73
to
d153865
Compare
This PR needs a rebase. |
d153865
to
39df44e
Compare
This remove some of code duplication.
39df44e
to
2833e21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, a few minor comments left.
logging.Infof("Using bundle path %s", bundlePath) | ||
return getPreflightChecks(experimentalFeatures, mode, bundlePath, preset) | ||
return getPreflightChecks(experimentalFeatures, mode, bundlePath, preset, enableBundleQuayFallback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, 4 arguments was already quite a few, 5 is starting to be too many args :-/
pkg/crc/machine/bundle/metadata.go
Outdated
@@ -346,7 +348,12 @@ func Download(preset crcPreset.Preset, bundleURI string) (string, error) { | |||
if bundleURI == constants.GetDefaultBundlePath(preset) { | |||
switch preset { | |||
case crcPreset.OpenShift, crcPreset.Microshift: | |||
return downloadDefault(preset) | |||
downloadedBundlePath, err := downloadDefault(preset) | |||
if grab.IsStatusCodeError(errors.Unwrap(err)) && enableBundleQuayFallback { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this could be errors.Is(err, grab.StatusCodeError)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau this is not work because as per errors.Is
it doesn't call the unwrap
on the target and the way we have this error it is wrapped around grabStatusCodeError
so it is not able to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Is
was introduced to deal with wrapped errors. Its documentation is clear about this. https://pkg.go.dev/errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but if I take a look to grab error package the implementation around StatusCodeError is type int and if I try to use errors.Is
on that it is not working until we know the statuscode. I think that is the reason it have IsStatusCodeError
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, errors.Is
checks for equality, not for type :-/ errors.As
checks for types, but is not as convenient as errors.Is
. If we keep this code, I think we should use errors.As
instead of hardcoding a single errors.Unwrap()
. Hardcoding this Unwrap()
will break if we move/refactor this code, and the level of wrapping is changed (eg if errors.Unwrap(errors.Unwrap())
becomes needed instead of errors.Unwrap()
).
However looking at downloadDefault()
, there are more situations than "the bundle file is missing/cannot be downloaded" when we want to fallback, for example if the signature is missing/invalid. IsStatusCodeError()
seems to be too restrictive? The initial version where the fallback always happened when there was an error may be a better option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau yeah that is more flexible, I can revert to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
As part of a9c0b4c we didn't add microshift preset. This PR add microshift preset to fetch default container image when it is available to `quay.io`.
2833e21
to
f1ed74e
Compare
@praveenkumar: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Since we are going to put bundles on quay and mirror where quay serve as fallback solution. This PR add a config flag `enable-bundle-quay-fallback` which allow user to download bundle from quay. ``` $ crc config set enable-bundle-quay-fallback true $ crc setup INFO Getting bundle for the CRC executable INFO Downloading bundle: /home/prkumar/.crc/cache/crc_microshift_libvirt_4.13.9_amd64.crcbundle... INFO Unable to download bundle from mirror, falling back to quay Getting image source signatures Copying blob d37da0367460 done Copying config f3021495d6 done Writing manifest to image destination Storing signatures INFO Extracting the image bundle layer... crc_microshift_libvirt_4.13.9_amd64.crcbundle: 1.46 GiB / 1.46 GiB [--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------] 100.00% INFO Verifying the bundle signature... INFO Uncompressing /home/prkumar/.crc/cache/crc_microshift_libvirt_4.13.9_amd64.crcbundle crc.qcow2: 4.28 GiB / 4.28 GiB [--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------] 100.00% oc: 141.85 MiB / 141.85 MiB [-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------] 100.00% Your system is correctly setup for using CRC. Use 'crc start' to start the instance ```
f1ed74e
to
fea84f0
Compare
@praveenkumar: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau 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 |
Since we are going to put bundles on quay and mirror where quay serve as
fallback solution. This PR makes user aware in case mirror fails to
download bundle.