-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[image-builder] Detect "429 - Too Many Request" and bubble up as "Unavailable" #20112
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
Conversation
@@ -624,6 +624,9 @@ func (o *Orchestrator) getAbsoluteImageRef(ctx context.Context, ref string, allo | |||
} | |||
return "", status.Error(codes.Unauthenticated, "cannot resolve image") | |||
} | |||
if resolve.TooManyRequestsMatcher(err) { | |||
return "", status.Errorf(codes.Unavailable, "upstream registry responds with 'too many request': %v", err) | |||
} | |||
if err != nil { | |||
return "", status.Errorf(codes.Internal, "cannot resolve image: %v", err) |
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.
Code LGTM 🚀
For further reviewers, this is how we detect imageBuildFailedUser
gitpod/components/server/src/workspace/workspace-starter.ts
Lines 1300 to 1306 in 4524c19
const looksLikeUserError = (msg: string): boolean => { | |
return ( | |
msg.startsWith("build failed:") || | |
msg.includes("headless task failed:") || | |
msg.includes("cannot resolve image") | |
); | |
}; |
and error message was
13 INTERNAL: cannot resolve image: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/gitpod/workspace-full/xxx: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
Error string | ||
Ref string | ||
Error string | ||
ErrorMatcher func(error) bool |
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.
It seems uintuitive too me that the ErrorMatcher is part of the Expectation. I would have expected it in the test case definition i.e. L 41
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.
It seems uintuitive too me that the ErrorMatcher is part of the Expectation
I somewhat agree: I would greatly prefer to specify an "positive" Error string
we can simply compare with. But I'm lagging knowledge as to how to construct it in a way that it 💯 resembles the error we get from containerd
.
If you have any ideas, please let me know
Given that, making it part of the Expectation
makes sense as it overrides the Error
check: it's just another way to check whether reality matches our expectation.
/unhold |
Description
Make sure that "429" errors are not tagged as
imageBuildFailedUser
inserver
.Related Issue(s)
Fixes ENT-644
How to test
Documentation
Preview status
gitpod:summary
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh
. If enabled,with-preview
andwith-large-vm
will be enabled./hold