RFC for Degraded NodePool Status Condition

[jigisha620]

Description

Adding RFC for Degraded NodePool Status Condition.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jigisha620
Once this PR has been reviewed and has the lgtm label, please assign bwagner5 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @jigisha620. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[coveralls]

Pull Request Test Coverage Report for Build 12718791708

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 4 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 81.184%

---

Files with Coverage Reduction	New Missed Lines	%
pkg/controllers/disruption/drift.go	2	89.66%
pkg/scheduling/requirements.go	2	98.01%

Totals
Change from base Build 12718181288:	-0.02%
Covered Lines:	9082
Relevant Lines:	11187

---

💛 - Coveralls

[jmdeal]

Checkpointing

[jmdeal]

Could you elaborate on what type of failures are being masked? As for it being wrong, I'm wondering if we should only consider degraded unknown or true. Maybe we don't ever transition it to false?

[jonathan-innis]

I think like Jason has called out online and offline, I would make our motivation front and center here. Why do we think that there is a need for something like this to exist? Does it make tracking down failures to NodePools easier? Does alarming get easier with this kind of a setup?

[jonathan-innis]

I think listing out tangible, real faillure types that this will catch will help illuminate how we design this.

[jonathan-innis]

One thing that I still feel like would be better here is if we considered flipping the polarity of this condition type at all -- Degraded: False meaning that it's healthy feels a bit weird to me but I get that we have to come up with some other word besides "Degraded" that isn't "Ready" and probably isn't "Healthy" to really reflect what this condition is evaluating

[jonathan-innis]

nit: It's slightly confusing to call this "Degraded: Unknown". The only reason that I say that is becasue this doesn't necessarily mean that we transition the condition to Unknown when the condition is already set -- I know this is said above but I did find it a tad semantically odd as I was reading through this and trying to parse-out the design

[saurav-agarwalla]

One thing that I had discussed with Reed is making Reason a more structured object and putting a serialized string output of that here since I understand that this has to be a string. That way, we can expose more details including error codes mentioning the reason behind the degradation, expose resource IDs/dependents causing it as well as have more than one reason for the degradation. Making it a structured object will also allow us to parse it better for metrics.