Add additional error codes for invalid reservations to GCE client #6093
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
area/cluster-autoscaler
size/S
| regexp.MustCompile("only compatible with Aggregate Reservations"), | ||
| regexp.MustCompile("instance configuration is optimized for ([a-z0-9*-]+) workloads"), | ||
| regexp.MustCompile("should be a ([a-zA-Z0-9*-]+) VM Family for instance with ([a-zA-Z0-9*\\ -]*) machine type"), | ||
| regexp.MustCompile("is not supported for aggregate reservations. It must be one of"), |
There was a problem hiding this comment.
nit: can you check this with a prefix "VM Family: %s "? Makes it a little bit clearer
| regexp.MustCompile("must use ReservationAffinity of"), | ||
| regexp.MustCompile("The reservation must exist in the same project as the instance"), | ||
| regexp.MustCompile("only compatible with Aggregate Reservations"), | ||
| regexp.MustCompile("instance configuration is optimized for ([a-z0-9*-]+) workloads"), |
There was a problem hiding this comment.
Please use the other part of the error message: "Please target a reservation with workload_type" (makes it clear that the error is about reservations and what's it about).
| regexp.MustCompile("The reservation must exist in the same project as the instance"), | ||
| regexp.MustCompile("only compatible with Aggregate Reservations"), | ||
| regexp.MustCompile("instance configuration is optimized for ([a-z0-9*-]+) workloads"), | ||
| regexp.MustCompile("should be a ([a-zA-Z0-9*-]+) VM Family for instance with ([a-zA-Z0-9*\\ -]*) machine type"), |
There was a problem hiding this comment.
nit: can you check this with a prefix "AggregateReservation VMFamily: %s?
| "must use ReservationAffinity of", | ||
| "The reservation must exist in the same project as the instance", | ||
| "only compatible with Aggregate Reservations", | ||
| regexReservationErrors := []*regexp.Regexp{ |
There was a problem hiding this comment.
This list is static. Can we compile the regexps once, rather than on every call to this function?
There was a problem hiding this comment.
Moved the regexp slice to a var block.
| for _, rErr := range reservationErrors { | ||
| if strings.Contains(errorMessage, rErr) { | ||
| for _, re := range regexReservationErrors { | ||
| if re.MatchString(errorMessage) { |
There was a problem hiding this comment.
I'm concerned that this change will make the matching more restrictive.
Old checkers uses a contains logic, wheres now we're doing a full-match (I think).
This could potentially break existing logic where we would have stacktrace to the errorMessage?
Can we explore if we can do a regex contains?
There was a problem hiding this comment.
It will work: in order to do a full match you'd have to include ^ and $ in the regex string. It's not anchored by default.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbostan, x13n 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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds more error codes related to the reservations increasing the visibility of GCE Errors.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: