Move kep to implementable, specify opt-in and out #1697
Conversation
alenkacz
requested review from
gerred,
kensipe,
nfnt and
zen-dog
as code owners
keps/0034-instance-health.md
Outdated
| @@ -106,10 +104,16 @@ From all these resources it would compute readiness based on `Condition: Ready`, | |||
|
|
|||
| For operators installing their app via e.g. `Deployment`, there’s no need at this point to also check the Status of the underlying Pods because Deployment `Status` already mirrors the readiness of the underlying Pods. This is true to all higher-level types like StatefulSet etc. | |||
|
|
|||
| To mitigate situation when a plan adds a resource we don't want to include in `Ready` condition, it's possible to opt-out from this when adding annotation `kudo.dev/allow-readiness-computation:false` | |||
There was a problem hiding this comment.
Hmmm... I'm not sure allow-readiness-computation is the right way... This feels like we would have a default value of "true" if this annotation is not present, which feels odd to me. I would prefer something that defaults to "false" if it's not there:
What about ignore-readiness-computation:true or skip-for-readiness-computation:true?
There was a problem hiding this comment.
I don't mind the default being true but I'll switch to your suggestion :) I like both
keps/0034-instance-health.md
Outdated
|
|
||
| ### Future work | ||
|
|
||
| To support even more complex cases and operator, we should think about adding possibility to specify `kudo.dev/readiness-strategy:selected` which will switch readiness computation to OPT-IN mode. With this one set, only resources with `kudo.dev/allow-readiness-computation:true` will be considered for readiness computation. |
There was a problem hiding this comment.
Ok, having read this the annotation from above makes a little more sense, still... Maybe we can find something that works with better defaults? Maybe we need two different annotations for opt-in and opt-out?
There was a problem hiding this comment.
Alright I kept both in place (two different options).
There was a problem hiding this comment.
LGTM, though I agree with @ANeumann82's comment to rename allow-readiness-computation.
keps/0034-instance-health.md
Outdated
| @@ -106,10 +104,18 @@ From all these resources it would compute readiness based on `Condition: Ready`, | |||
|
|
|||
| For operators installing their app via e.g. `Deployment`, there’s no need at this point to also check the Status of the underlying Pods because Deployment `Status` already mirrors the readiness of the underlying Pods. This is true to all higher-level types like StatefulSet etc. | |||
|
|
|||
| To mitigate situation when a plan adds a resource we don't want to include in `Ready` condition, it's possible to opt-out from this when adding annotation `kudo.dev/skip-for-readiness-computation:true` | |||
There was a problem hiding this comment.
| To mitigate situation when a plan adds a resource we don't want to include in `Ready` condition, it's possible to opt-out from this when adding annotation `kudo.dev/skip-for-readiness-computation:true` | |
| To mitigate situation when a plan adds a resource we don't want to include in `Ready` condition, it's possible to opt-out from this by adding annotation `kudo.dev/skip-for-readiness-computation:true` |
Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
Signed-off-by: Alena Varkockova varkockova.a@gmail.com
What this PR does / why we need it:
Move the KEP to implementable, specify some more left-overs from review.
Please take time to review also the KEP as a whole as this moves it to implementable.