Repository navigation
fix: don't default null replicas/readyReplicas in Readiness checks - #4905
Conversation
kubernetes-prow
Bot
added
cncf-cla: yes
27ea3a3 to
ae9a305
Compare
|
Fixed two of the regression tests (nullReadyReplicas cases). They used replicas=3, which passed under both old and new code and didn't actually catch the bug. Changed to replicas=0, which is the case where the old default (readyReplicas=null becomes 0) would wrongly report ready. |
isReplicaSetReady and isReplicationControllerReady silently treated a null spec.replicas as 1 and null status.readyReplicas as 0, which can misreport readiness. isStatefulSetReady already had this exact issue fixed by a maintainer in a prior review - this applies the same fix to the two remaining copy-pasted methods, plus regression tests.
ae9a305 to
b9afedb
Compare
|
Some of this is philosophical, I guess. Is a replica set with zero replicas "ready" or not? Can you check what |
|
Checked client-go's WaitForReadyReplicaSet - it does *(rs.Spec.Replicas) == rs.Status.ReadyReplicas, so a 0-replica RS is ready there (0 == 0). But: ReadyReplicas is Pushed an update: null spec.replicas still returns false (client-go doesn't guard against that either, so we're just safer there), but null readyReplicas is now treated as 0, matching what it actually means on the wire. Added tests for both cases. |
readyReplicas is omitempty on a plain int in the real API, so it's omitted from JSON whenever it's genuinely 0 - not just when unreported. Treating null as 0 (rather than 'not ready') matches client-go's own WaitForReadyReplicaSet check and fixes a case flagged in review where a zero-replica ReplicaSet was incorrectly reported not-ready.
ed42a60 to
0c31c25
Compare
|
Pushed the client-go matching fix and formatting cleanup - ready for another look whenever you have a chance. Thanks! |
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, sureshmelvinsigera 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 |
kubernetes-prow
Bot
added
the
approved
af5d347
into
kubernetes-client:master
isReplicaSetReady and isReplicationControllerReady silently treated a null spec.replicas as 1 and null status.readyReplicas as 0, which can misreport readiness. isStatefulSetReady already had this exact issue fixed by a maintainer in a prior review - this applies the same fix to the two remaining copy-pasted methods, plus regression tests.
See bc40df9 ("Addressed comment") for the original StatefulSet fix this mirrors.