Add mechanism to override drainability status #6196
Conversation
k8s-ci-robot
added
kind/feature
|
/assign x13n |
k8s-ci-robot
added
size/L
artemvmin
force-pushed
the
drain-blocking-rule
branch
from
71d2546 to
8b9f8a1
Compare
| }, | ||
| "override no match": { | ||
| rules: Rules{ | ||
| fakeRule{drainability.Status{ |
There was a problem hiding this comment.
nit: How about drainability.NewDrainableStatus(drainability.OverridingOnly(drainability.SkipDrain))? Or is it not really easier to read?
There was a problem hiding this comment.
I think they're both quite readable. I would argue the opposite and say that the constructors in the drainability package are unnecessary boilerplate. Since drainability.Status is already exported, the following are identical:
status := drainability.NewBlockedStatus(reason, err)
status := &drainability.Status{
Reason: reason,
Error: err,
}
The latter is better because it's explicit about parameters and requires no additional boilerplate when new fields are added to the struct.
| } | ||
| for _, candidate := range candidates { | ||
| for _, override := range candidate.Overrides { | ||
| if status.Outcome == override { |
There was a problem hiding this comment.
Maybe worth logging the fact that override happened? At least at V(5)? This logic becomes non-trivial with this change, so some observability wouldn't hurt.
There was a problem hiding this comment.
Great idea. Added a Name() to the Rule interface to make this log more useful.
artemvmin
force-pushed
the
drain-blocking-rule
branch
from
3fbaa8f to
a0d56b3
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: artemvmin, 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
The interface changed in kubernetes#6196 but the Rules class didn't
The interface changed in kubernetes#6196 but the Rules class didn't
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds a mechanism for one rule to selectively override other rules. This allows for custom rules that override some outcomes (e.g. BlockDrain -> DrainOk) while not overriding others (e.g. SkipDrain).
Does this PR introduce a user-facing change?