📖 Condition CAEP #3017
📖 Condition CAEP #3017
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
|
/hold for community consensus |
k8s-ci-robot
added
the
do-not-merge/hold
|
@fabriziopandini: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
| Once there will be an agreed design for the condition types, a second iteration will start | ||
| with the following goals: | ||
|
|
||
| 1. To define the first set of conditions on the different Cluster API objects. |
There was a problem hiding this comment.
I think this needs to be conflated with the first interaction. In order to come up with a sane data model we need to have first a clear understanding and view for each component needs.
There was a problem hiding this comment.
Agreed. Done
however, even if we are conflating iterations, please note those objectives are still non-goals
- To pre-define every possible condition.
- To define the exact semantic for every condition introduced in the proposal
This should be done by implementation PR once there is an agreement on data structure and on constraints/design principles for defining conditions.
|
|
||
| The goal of this first iteration is: | ||
|
|
||
| 1. To agree on a design for the condition types. |
There was a problem hiding this comment.
I think that to agree on a design for the condition types we need to include as goals a list of user stories traversing capi components which are not well communicated today but they will with the new conditions. Then in the "Implementation" section we can elaborate how we'll achieve these goals fleshing out where we set those conditions, which values and how the user can see them in a meaningful manner.
There was a problem hiding this comment.
I think we should preserve the distinction between goals and user stories, but I agree on expanding the implementation section with user stories and examples traversing CAPI components (see new commits)
| The `Severity` field MUST be set only when `Status=False` and it is designed to provide a standard classification | ||
| of possible conditions failure `Reason`. | ||
|
|
||
| Please note that the combination of `Reason` and `Severity` gives different meaning to a condition failure |
There was a problem hiding this comment.
Given my comment above about what a condition is, I wonder if Severity might be the wrong word, or even mechanism, for classifying conditions. The information encoded there (i.e. Info, Warning, Error) comes from logging, and even there some people dispute whether there is a need for more than Info and Error. Is there a more specific word, which applies to all conditions, which we can use? Or should we just treat Conditions as freeform messages and force callers to know which conditions they care about ahead of time? Could we have a convention for encoding whether a Condition is terminal or not in Type?
I would also be curious if this discussion has come up in SIG API Machinery...
Sorry if this note is confusing at all! I probably need to review @bgrant0607's conditions references in more detail...
There was a problem hiding this comment.
The information encoded there (i.e. Info, Warning, Error) comes from logging
True, but this is also a generally understood way to express:
- You are good
- You should take a look
- You have some problems
I wonder if Severity might be the wrong word
I have considered ReasonClass, ReasonCategory, but at the end opted for Severity because the focus here is in catching the user's attention.
But I'm more than happy to hear suggestions here...
should we just treat Conditions as freeform messages and force callers to know which conditions they care about ahead of time
free form message is difficult for consumers, so we are encoding ConditionTypes, Reason, and Severity to make it easier to deal with conditions
k8s-ci-robot
added
size/XL
|
@davidewatson @enxebre @ncdc @vincepri thanks for the feedback! |
|
Holding for 1 more week for async consensus |
|
The upstream conditions KEP has merged: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1623-standardize-conditions |
|
@vincepri @ncdc @benmoss @detiber I have addressed comment about risks of using a consistent polarity and relaxed naming convention rules, PTAL. WRT to the management of the "Unknown" state, I have found a consideration in the recent Kubernetes KEP that IMO clearly defines the problem:
I agree also on the proposed remediation, which is to require the condition author to clearly document the meaning of the "Unknown" state for each condition. TL;DR. even if "Unknown" might vary case by case and we should document this, consistent polarity clarifies the meaning of "True" and "False" and this is an improvement vs having different interpretations for all the three possible condition states. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, 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 |
|
Before merging, let's squash and change the status of the proposal to |
|
/milestone v0.3.x |
fabriziopandini
force-pushed
the
conditions-caep
branch
from
2918c84
to
cd81068
Compare
done |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it:
This PR brings the CAEP for introducing conditions in Cluster API
The discussion on the Google doc are addressed, with expectation for:
SeverityfieldObservedGernerationfieldMoving here for getting feedback from a broader adience
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):xref #3005
/assing @vincepri
/assing @ncdc