✨ Add support negative polarity conditions #10550
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
util/conditions/setter.go
Outdated
| } | ||
|
|
||
| // PositiveFalseCondition returns a negative polarity condition with Status=false and the given type (Status=False has a positive meaning). | ||
| func PositiveFalseCondition(t clusterv1.ConditionType) *clusterv1.Condition { |
There was a problem hiding this comment.
Why can't this have a reason and message?
There was a problem hiding this comment.
Back in time we introduced all those helpers to enforce a sort of consistency, and what we have to express when status is good (TrueCondition) does not allow to express reason and message & severity.
This func mirror the same for a condition with negative polarity(when status is False but with a positive meaning).
In other words, I tried to keep things consistent with what we have.
There was a problem hiding this comment.
Sometimes it can be awkward to require a reason, but there are cases where I think it can make sense.
I guess something like Available as a condition, True status, reason would be AsExpected maybe? But does it also make sense without?
But yeah, in general, I wonder if we should just have the reason and message allowed in all cases, rather than being prescriptive?
Are there kube conventions that say you should or shouldn't set reason/message in certain cases?
There was a problem hiding this comment.
If a user wants flexibility, he/she/they can already do this today using the Set helper.
But as far as I know, in the last 4 years almost everyone used "prescriptive" helpers.
WRT to API conventions, before digging into "reason", let's keep in mind that the scope of this PR is to make one step toward API compliance, not the entire path 😉
Based on my understanding, reason is another area where the thinking about convention evolved over time. The current field description in corev1 types sort of captures it:
// reason contains a programmatic identifier indicating the reason for the condition's last transition.
// Producers of specific condition types may define expected values and meanings for this field,
// and whether the values are considered a guaranteed API.
// The value should be a CamelCase string.
// This field may not be empty.
My reading of this is that community is moving towards having reason everywhere, but they recognize producers of condition might have adopted different approaches.
CAPI as of today has two main differences from the API conventions WRT to how we handle reasons:
- From a semantic point of view, we use reason to "explain" the current state, not the last transition (in some case the diff is subtle, but in controller we just observe what is the current state and surface it in conditions, we do not track how do we get to the current state)
- We set reason only when condition have a negative meaning (as far as I remember setting a reason for positive state was considered redoundant, but it was 4 years ago so I might be wrong...)
This PR intentionally does not mean to change what CAPI today is doing WRT to reasons, because this could imply a way bigger impact that probably we should defer to when we cut a new API version.
There was a problem hiding this comment.
This PR intentionally does not mean to change what CAPI today is doing WRT to reasons, because this could imply a way bigger impact that probably we should defer to when we cut a new API version.
This feels sensible for keeping the scope small here 😄
For my 0.02c: this approach 'feels' more right to me (less surprising)
From a semantic point of view, we use reason to "explain" the current state, not the last transition (in some case the diff is subtle, but in controller we just observe what is the current state and surface it in conditions, we do not track how do we get to the current state)
There was a problem hiding this comment.
I also prefer explaining the current state not the transition. I'm not sure how we would even do this if a controller should be able to calculate the status from scratch. How could it even know what the previous state was
(explaining transitions in conditions also sounds very much like describing the "edges" of reconciliations)
util/conditions/setter.go
Outdated
| @@ -96,6 +96,25 @@ func FalseCondition(t clusterv1.ConditionType, reason string, severity clusterv1 | |||
| } | |||
| } | |||
|
|
|||
| // NegativeTrueCondition returns a negative polarity condition with Status=True and the given type (Status=True has a negative meaning). | |||
| func NegativeTrueCondition(t clusterv1.ConditionType, reason string, severity clusterv1.ConditionSeverity, messageFormat string, messageArgs ...interface{}) *clusterv1.Condition { | |||
There was a problem hiding this comment.
why do NegativeTrueCondition and PositiveFalseCondition include the polarity in the func name?
Since all they are doing is set values in a clusterv1.Condition type wouldn't they be called just TrueCondition / falseCondition and let higher invoker decide on polarity?
There was a problem hiding this comment.
See #10550 (comment), but the intent is to keep things consistent with what we have.
Just a note about the approach you are suggesting about let higher invoker decide on polarity: this is not in discussion.
The difference is that:
- If we change the existing TrueCondition & FalseCondition, this will impact everyone and we end up to defer to users the task to enforce the same consistency that today we have with this helpers
- With the proposed change instead, the users specific helpers for the NegativePolarity, which are consistent with the existing one for positive polarity
Last, but not least, there is already a Set helper with allow the users to decide which field to set (but in this case, it also imply the task to enforce consistency across the codebase/all the providers)
util/conditions/setter.go
Outdated
| } | ||
|
|
||
| // PositiveFalseCondition returns a negative polarity condition with Status=false and the given type (Status=False has a positive meaning). | ||
| func PositiveFalseCondition(t clusterv1.ConditionType) *clusterv1.Condition { |
There was a problem hiding this comment.
Sometimes it can be awkward to require a reason, but there are cases where I think it can make sense.
I guess something like Available as a condition, True status, reason would be AsExpected maybe? But does it also make sense without?
But yeah, in general, I wonder if we should just have the reason and message allowed in all cases, rather than being prescriptive?
Are there kube conventions that say you should or shouldn't set reason/message in certain cases?
|
What are the next steps here? Do we want to discuss the Positive/Negative nomenclature in more depth? I'm not personally a huge fan of it and would rather users didn't have to worry, ie we could have helpers that allow you just to specify the state true or false explicitly. But I can also see that this would make a major departure from where we are now and understand the desire to make a small step here to allow more flexibility in conditions. The PR does this in a sympathetic way and unblocks what is required. I think the naming, and perhaps eventually removing the connotation of positive/negative could be done at a later point once we've understood more about the usage pattern of the negative/neutral condition types being introduced |
|
@JoelSpeed on the positive side there is agreement on the idea to support negative polarity conditions, that's already a good point to be. The only thing we are discussing are the new helpers: Probably, the most quick way forward is to just drop them, and let users willing to use negative polarity conditions to create Condition types "freely" use the Set(Condition) helper. From the other side of the coin, having helpers really helped users so far in keeping things consistent across all providers, and I still see value in investing some time to figure out new names for helpers. Another option, if we all agree that there is value in having helpers, is to start with the current names (or a variation of them we quickly agree upon) and eventually iterate in the future. |
|
Might be using more explicit names for helpers might help: (and BTW, the fact that we add helpers don't prevent users to create Condition types "freely" use the Set(Condition) helper) |
|
Definitely less confusing |
|
I think the new names (above) are good and will make more sense to someone who's not read this PR :) I also think they're good enough for now as a starting point, and can be iterated later on too if we find people are confused by them! Do we have documentation on the helpers already? (outside of the proposal?) |
|
ACK, I will try get back to this next week. |
|
Ack, I'm happy with the proposed approach forward, please update when you have a chance |
|
@sbueringer @vincepri @JoelSpeed @enxebre @theobarberbany |
|
Also LGTM :) |
|
/lgtm My summary would be
|
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 0729ef4fe54d9bba3f5336be19bf52cf5472fee1
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 this PR does / why we need it:
This PR is an iteration on the condition proposal & corresponding utils allowing the implementation of conditions with negative polarity, which is a step toward latest K8s guidelines about conditions
Which issue(s) this PR fixes:
Part of #7635
/area api
cc @JoelSpeed @vincepri @theobarberbany