Clarify ready #103782
Clarify ready #103782
Conversation
Clarifies and aligns the docs regarding ReadyReplicas in the ReplicaSetStatus, DeploymentStatus, StatefulSetStatus and the ReplicationControllerStatus.
Clarifies and aligns the docs regarding NumberReady in the DaemonSetStatus.
k8s-ci-robot
added
release-note-none
|
@matthiasgubler: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
Welcome @matthiasgubler! |
k8s-ci-robot
added
the
needs-triage
k8s-ci-robot
added
the
kind/api-change
|
Hi @matthiasgubler. 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 Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
matthiasgubler
force-pushed
the
clarify-ready
branch
from
8c35745
to
6d27bd8
Compare
| @@ -237,7 +237,7 @@ type StatefulSetStatus struct { | |||
| // replicas is the number of Pods created by the StatefulSet controller. | |||
| Replicas int32 `json:"replicas" protobuf:"varint,2,opt,name=replicas"` | |||
|
|
|||
| // readyReplicas is the number of Pods created by the StatefulSet controller that have a Ready Condition. | |||
| // Total number of pods created by this StatefulSet controller with a Ready Condition by passing the readinessProbe. | |||
There was a problem hiding this comment.
comment should start with the serialized field name (readyReplicas)
applies to all modified godoc lines
There was a problem hiding this comment.
Does this accurately describe behavior for pods without readiness probes? What about pods that set spec.readinessGates?
Referencing the Ready condition makes sense... I'm not sure denormalizing all the inputs that go into determining the Ready condition here makes sense (applies to all modified godoc lines)
There was a problem hiding this comment.
Does this accurately describe behavior for pods without readiness probes? What about pods that set spec.readinessGates?
Good point. I rechecked the code and in the file podutils.go the function IsPodReady basically just checks the Ready Condition. In the pkg/kubelet/status/generate.go file is the function GeneratePodReadyCondition defined, which checks, that every container has the status ready and all readinessGates are true.
According to the KEP documentation https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/580-pod-readiness-gates is ready defined as "ready condition and all readiness gates are true".
My suggestion would be to have a text like this:
| // Total number of pods created by this StatefulSet controller with a Ready Condition by passing the readinessProbe. | |
| // readyReplicas is the number of pods created by this StatefulSet controller with a Ready Condition and that are passing all readinessGates. |
Or would you suggest a rather generic description? Do you have another suggestion?
There was a problem hiding this comment.
I wouldn't try to describe everything that goes into determining the Ready condition here... just limit it to saying it's the count of pods created by this controller with a Ready condition
There was a problem hiding this comment.
👍
Next week I will commit a new version.
Co-authored-by: Jordan Liggitt <jordan@liggitt.net>
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
liggitt
added
the
tide/merge-method-squash
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, matthiasgubler 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 |
k8s-ci-robot
added
the
approved
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
|
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Clarifies and aligns the ready definition for the DeploymentStatus, DeploymentStatus, ReplicationControllerStatus, StatefulSetStatus, and DaemonSetStatus
Which issue(s) this PR fixes:
Fixes kubernetes/api#38
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: