fix(channels): only treat known ready conditions as health signals - #18657
kubernetes-prow[bot] merged 2 commits into
Conversation
isHealthy() marked an object unhealthy on any status condition with a "False" status, regardless of the condition type. Controllers routinely publish conditions that carry no readiness meaning: KEDA sets "Paused" to "False" on every ScaledObject and ScaledJob it is actively scaling, and a healthy Node reports "MemoryPressure", "DiskPressure" and "PIDPressure" as "False". Such objects were permanently unhealthy, so an addon containing one could never be applied successfully. Since kops 1.36 the kops-channels pod reports readiness from the result of the last apply iteration, so this also leaves the pod permanently not ready and cluster validation failing. Only consider condition types whose "True" status means the object is healthy, and ignore the rest. Abnormal-true conditions are deliberately not interpreted, matching the previous behaviour for those. Signed-off-by: Joakim Olsson <joakim@unbound.se>
|
Welcome @argoyle! |
|
Hi @argoyle. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
hakman
left a comment
There was a problem hiding this comment.
This fixes the KEDA and Node cases, but it can hide known resource-specific failures. A Deployment may have Available=True while Progressing=False/ProgressDeadlineExceeded; Gateways use Accepted and Programmed.
Could we add a small GroupKind-specific condition map for:
apps/Deployment:ProgressingGateway:Accepted,ProgrammedGatewayClass:Accepted
I’d also remove or reword the stale kstatus TODO.
| "PodScheduled", // Pod | ||
| "Ready", // Pod, Node, and the convention for custom resources | ||
| ) | ||
|
|
There was a problem hiding this comment.
| +var readyConditionTypesByGroupKind = map[schema.GroupKind]sets.Set[string]{ | |
| {Group: "apps", Kind: "Deployment"}: sets.New("Progressing"), | |
| {Group: "gateway.networking.k8s.io", Kind: "Gateway"}: sets.New("Accepted", "Programmed"), | |
| {Group: "gateway.networking.k8s.io", Kind: "GatewayClass"}: sets.New("Accepted"), | |
| } | |
| } | ||
|
|
||
| // TODO: Check conditionType? | ||
| if !readyConditionTypes.Has(conditionType) { |
There was a problem hiding this comment.
| if !readyConditionTypes.Has(conditionType) { | |
| if !readyConditionTypes.Has(conditionType) && | |
| !readyConditionTypesByGroupKind[gvk.GroupKind()].Has(conditionType) { |
| ) | ||
|
|
||
| // isHealthy reports whether the object should be considered "healthy" | ||
| // TODO: Replace with kstatus library |
There was a problem hiding this comment.
| // TODO: Replace with kstatus library |
A Deployment can report "Available" as "True" while "Progressing" is "False" with reason ProgressDeadlineExceeded, and Gateway API resources signal readiness through "Accepted" and "Programmed" rather than through a "Ready" condition. Reading only the kind-independent conditions would report all of those as healthy. Add a GroupKind-keyed map of extra readiness conditions, consulted on top of the kind-independent set, and reword the stale kstatus TODO. Signed-off-by: Joakim Olsson <joakim@unbound.se>
|
Thanks — the Deployment case is a real hole, Added var readyConditionTypesByGroupKind = map[schema.GroupKind]sets.Set[string]{
{Group: "apps", Kind: "Deployment"}: sets.New("Progressing"),
{Group: "gateway.networking.k8s.io", Kind: "Gateway"}: sets.New("Accepted", "Programmed"),
{Group: "gateway.networking.k8s.io", Kind: "GatewayClass"}: sets.New("Accepted"),
}Keyed on the full The New test cases: a Deployment with |
|
/ok-to-test |
|
/test pull-kops-aws-upgrade-k135-ko135-to-k136-kolatest-many-addons |
|
Thanks for the report and for the fix. |
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
…-upstream-release-1.36 Automated cherry pick of #18657: fix(channels): only treat known ready conditions as health signals
What this PR does / why we need it:
isHealthy()marked an object unhealthy on anystatus.conditionsentry with a"False"status, regardless of the condition type — the code carried a// TODO: Check conditionType?at exactly that spot.Plenty of controllers publish conditions where
"False"carries no readiness meaning:Paused: "False"on everyScaledObject/ScaledJobit is actively scaling, plusActive: "False"while idle andFallback: "False"when no fallback is configured.NodereportsMemoryPressure,DiskPressureandPIDPressureas"False".Objects like these are permanently unhealthy, so any addon containing one can never be applied successfully:
Since #18433 the kops-channels pod derives its readiness from the result of the last apply iteration, so this now also leaves the pod permanently not ready and
kops validate clusterfailing:This PR only considers condition types whose
"True"status means the object is healthy (Available,Ready,Established, …) and ignores everything else.Abnormal-true conditions (
Degraded,ReplicaFailure, the Node pressure conditions) are deliberately not interpreted as failures. That matches the current behaviour — a"True"status has never marked an object unhealthy — and keeps this change to fixing the false negatives rather than introducing new ways for an apply to fail.The
// TODO: Replace with kstatus libraryaboveisHealthy()still stands; this is a targeted fix, not that replacement.Which issue(s) this PR fixes:
Special notes for your reviewer:
Adds
health_test.go, which the package did not have. The table covers the KEDA and Node cases above, plus the existing behaviours worth pinning down: deletion timestamp, absent and nullstatus.conditions, the kinds short-circuited by GVK, an unavailable Deployment, and a CRD withNamesAccepted: "False". Reverting the type check turns 3 of the cases red.