feat: add GCP CPUS_ALL_REGIONS quota error #2792
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: typeid 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 |
| - name: GCPQuotaSSDTotalGBExceeded | ||
| searchRegexStrings: | ||
| - "Quota \'SSD_TOTAL_GB\' exceeded" | ||
| - "Quota 'SSD_TOTAL_GB' exceeded" |
There was a problem hiding this comment.
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.
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 data out 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 []string that is the installLogRegex.SearchRegexStrings array.
But interpreting a backslash preceding a single quote should resolve to just ... a single quote. See here for "proof": notice that the regexes p1 and p2 render the same under %q (which is %s surrounded by double quotes), and thus unsurprisingly yield the same results from Match(), which, again unsurprisingly, matches when the subject string has single quotes, but not when it doesn't, and not when it has explicit backslashes.
We are seeing clusters stuck provisioning while the error doesn't get caught
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.)
There was a problem hiding this comment.
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?
Right, I was seeing clusters failing with Quota 'SSD_TOTAL_GB' exceeded that 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.
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-regexes configmap 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.
(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.
I tried to track down the source of that error message and its printer does come with a salient caveat:
// Format formats the message as a string.
// This method is only intended for human consumption and ignores errors.
// Do not depend on the output being stable. Its output will change across
// different builds of your program, even when using the same version of the
// protobuf module.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2792 +/- ##
=======================================
Coverage 50.34% 50.34%
=======================================
Files 279 279
Lines 34169 34169
=======================================
Hits 17201 17201
Misses 15614 15614
Partials 1354 1354
🚀 New features to boost your workflow:
|
|
/test security We've resolved these CVEs. |
|
@typeid: all tests passed! 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
This PR adds
CPUS_ALL_REGIONSas known quota error for GCP and attempts to fix theSSD_TOTAL_GBerror, which currently doesn't properly get caught.