Add description of replica controller scaledown sort logic #26993
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Deploy preview for kubernetes-io-vnext-staging processing. Building with commit 10162f7 https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6052698d4a5dfa0007b4f7f4 |
k8s-ci-robot
added
language/en
|
@damemi thank you for opening this! /assign |
k8s-ci-robot
added
the
sig/apps
| is unknown, and a pod whose phase is unknown comes before a running pod. | ||
| 3. If exactly one of the pods is ready, the pod that is not ready comes | ||
| before the ready pod. | ||
| 4. If controller.kubernetes.io/pod-deletion-cost annotation is set, then |
There was a problem hiding this comment.
FYI @ahg-g
Maybe we can have a single docs PR for both KEPs
There was a problem hiding this comment.
actually lets keep them separate, the other PR is ready for review.
There was a problem hiding this comment.
BTW I'd be happy to see this level of detail covered in the reference section of the docs.
| @@ -310,6 +310,33 @@ assuming that the number of replicas is not also changed). | |||
| A ReplicaSet can be easily scaled up or down by simply updating the `.spec.replicas` field. The ReplicaSet controller | |||
| ensures that a desired number of Pods with a matching label selector are available and operational. | |||
|
|
|||
| When scaling down, the ReplicaSet controller chooses which pods to delete by sorting the available pods to | |||
There was a problem hiding this comment.
I would argue that this is too detailed to be useful to users. Also we should avoid locking the criteria in a way that users start relying on it, making it harder to change in the future. In particular, we should state that these are preferences and not something that is guaranteed.
Important downscale selection criteria IMO:
- Pending (including unscheduled) pods.
- pod-deletion-cost annotation (probably worth stating the default)
- too many pods in a node (this is the rank, although not sure if we should include it in the documentation).
- lower running time (bucketed)
And if they all match, selection is random (the UIDs is 100% an implementation detail that shouldn't go in the documentation).
damemi
force-pushed
the
dev-1.21-random-downscale
branch
from
d238302 to
9655963
Compare
|
Updated this description to be less implementation detailed following @alculquicondor's suggestion in #26993 (comment) |
| When scaling down, the ReplicaSet controller chooses which pods to delete by sorting the available pods to | ||
| prioritize scaling down pods based on the following general algorithm: | ||
| 1. Pending (and unschedulable) pods are scaled down first | ||
| 2. If controller.kubernetes.io/pod-deletion-cost annotation is set, then |
There was a problem hiding this comment.
We should also list controller.kubernetes.io/pod-deletion-cost in https://kubernetes.io/docs/reference/labels-annotations-taints/
There was a problem hiding this comment.
I will add that in #26739
@alculquicondor we should do that for the indexed job annotation too.
| 1. Pending (and unschedulable) pods are scaled down first | ||
| 2. If controller.kubernetes.io/pod-deletion-cost annotation is set, then | ||
| the pod with the lower value will come first. | ||
| 3. If the pods' ranks differ, the pod with greater rank comes before the pod |
There was a problem hiding this comment.
rank is an internal detail to the controller utils. The rank we use in replicaset controller is the number of pods in a node.
There was a problem hiding this comment.
Would this be better as just something like 3. Pods on nodes with greater replicas come before pods on nodes with fewer replicas?
There was a problem hiding this comment.
Sounds good. But should it be greater or more? Or perhaps greater number?
There was a problem hiding this comment.
Updated with more to keep it clear :)
damemi
force-pushed
the
dev-1.21-random-downscale
branch
from
9655963 to
10162f7
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7f583552244a1cf8783d3487be4c5fc82f89c75d
|
|
Hi @damemi , thank you for having your Doc PR ready for review, friendly reminder about the upcoming doc related dates for the 1.21 release:
|
|
@PI-Victor is there anything else this needs to be Ready to Merge? It looks like concerns were all addressed and /lgtm from sig-scheduling. @soltysh does this look good from apps? |
|
from my perspective it looks like all concerns were addressed. |
|
Thank you for the updates. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: reylejano 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
This copies the description from the replica controller's internal scale-down logic (https://github.com/damemi/kubernetes/blob/a8d105ab724ccdb45b4ff380d84fd356e833991e/pkg/controller/controller_utils.go#L787-L815)
This was prompted as part of kubernetes/enhancements#2185 and kubernetes/kubernetes#99212