-
Notifications
You must be signed in to change notification settings - Fork 256
feat: add GCP CPUS_ALL_REGIONS quota error #2792
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
Open
typeid
wants to merge
1
commit into
openshift:master
Choose a base branch
from
typeid:add_gcp_errors
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm unsure why the test for this succeeds. We are seeing clusters stuck provisioning while the error doesn't get caught - my best bet is on the extra backslashes here, but I can't figure out why the unit tests pass.
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.
There's several layers of parsing going on here. I don't have a good explanation, but here's a walkthrough of how I think it should be working:
In the production code path, we read the configmap yaml from bindata, decode it into a runtime object like this before applying it to the target cluster. In the unit test we use the same decoder, but we're reading from the file system rather than from bindata.
In the prod code we're applying the object to the cluster. In the test we're seeding the fake client with it.
We pull the
dataout of the ConfigMap the same way in both paths (which is kind of the point of the unit test). We should be processing that content as a single monolithic string, wherein the backslashes are (so far) preserved. I think when we do this unmarshal is where they would be interpreted, as the individual regex gets stuffed into an actual element in the[]stringthat is theinstallLogRegex.SearchRegexStringsarray.But interpreting a backslash preceding a single quote should resolve to just ... a single quote. See here for "proof": notice that the regexes
p1andp2render the same under%q(which is%ssurrounded by double quotes), and thus unsurprisingly yield the same results fromMatch(), which, again unsurprisingly, matches when the subject string has single quotes, but not when it doesn't, and not when it has explicit backslashes.Can you please clarify? You're saying that you're seeing installation failures with a message that should be getting trapped by this regex, but it's not? It's showing up as a generic failure or something? If you manually change the regex in the configmap, does that fix it? (You may have to scale down hive-operator first so we don't revert your change.)
Uh oh!
There was an error while loading. Please reload this page.
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.
Right, I was seeing clusters failing with
Quota 'SSD_TOTAL_GB' exceededthat didn't get caught/mapped to the error. I'm unable to reproduce this easily though, and I believe just removing the backslashes might work - worst case it doesn't and keeps us in the same state.Happy to remove this from the PR though if we want to dig further into the issue.
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 sounds like we don't know whether removing the backslashes will work... which casts doubt on whether your new thing will work either :)
If you can't easily reproduce the quota problems, I'm not sure how we're going to prove either one.
What we could do (and it's really not a terrible process in general) is add both regexes to your
additional-install-log-regexesconfigmap and try to / wait for repro. (Though I'm guessing that's not really any easier to experiment with than this one as I imagine it's in a git repo somewhere anyway?)LMK if you're interested in that idea. If not, I don't super have a problem with iterating in this repo. As long as you don't e.g. completely bork the yaml syntax and cause a parsing error, there's really no harm in getting it wrong.
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.
(Idly wondering whether the single quotes in the actual message from AWS are "smart" -- something other than ASCII 0x27...)
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.
I tried to track down the source of that error message and its printer does come with a salient caveat: