UPSTREAM: 80004: Prefer to delete doubled-up pods of a ReplicaSet#23806
Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom Oct 24, 2019
Conversation
openshift-ci-robot
added
the
size/L
Miciah
force-pushed
the
UPSTREAM-80004-prefer-to-delete-doubled-up-pods-of-a-replicaset
branch
from
7a53570 to
507bcd8
Compare
openshift-ci-robot
added
size/XL
Miciah
force-pushed
the
UPSTREAM-80004-prefer-to-delete-doubled-up-pods-of-a-replicaset
branch
4 times, most recently
from
cb2160e to
3a71e71
Compare
When scaling down a ReplicaSet, delete doubled up replicas first, where a "doubled up replica" is defined as one that is on the same node as an active replica belonging to a related ReplicaSet. ReplicaSets are considered "related" if they have a common controller (typically a Deployment). The intention of this change is to make a rolling update of a Deployment scale down the old ReplicaSet as it scales up the new ReplicaSet by deleting pods from the old ReplicaSet that are colocated with ready pods of the new ReplicaSet. This change in the behavior of rolling updates can be combined with pod affinity rules to preserve the locality of a Deployment's pods over rollout. A specific scenario that benefits from this change is when a Deployment's pods are exposed by a Service that has type "LoadBalancer" and external traffic policy "Local". In this scenario, the load balancer uses health checks to determine whether it should forward traffic for the Service to a particular node. If the node has no local endpoints for the Service, the health check will fail for that node. Eventually, the load balancer will stop forwarding traffic to that node. In the meantime, the service proxy drops traffic for that Service. Thus, in order to reduce risk of dropping traffic during a rolling update, it is desirable preserve node locality of endpoints. * vendor/k8s.io/kubernetes/pkg/controller/controller_utils.go (ActivePodsWithRanks): New type to sort pods using a given ranking. * vendor/k8s.io/kubernetes/pkg/controller/controller_utils_test.go (TestSortingActivePodsWithRanks): New test for ActivePodsWithRanks. * vendor/k8s.io/kubernetes/pkg/controller/replicaset/replica_set.go (getReplicaSetsWithSameController): New method. Given a ReplicaSet, return all ReplicaSets that have the same owner. (manageReplicas): Call getIndirectlyRelatedPods, and pass its result to getPodsToDelete. (getIndirectlyRelatedPods): New method. Given a ReplicaSet, return all pods that are owned by any ReplicaSet with the same owner. (getPodsToDelete): Add an argument for related pods. Use related pods and the new getPodsRankedByRelatedPodsOnSameNode function to take into account whether the pod is doubled up when sorting pods for deletion. (getPodsRankedByRelatedPodsOnSameNode): New function. Return an ActivePodsWithRanks value that wraps the given slice of pods and computes ranks where each pod's rank is equal to the number of active related pods that are colocated on the same node. * vendor/k8s.io/kubernetes/pkg/controller/replicaset/replica_set_test.go (newReplicaSet): Set OwnerReferences on the ReplicaSet. (newPod): Set a unique UID on the pod. (byName): New type to sort pods by name. (TestRelatedPodsLookup): New test for getIndirectlyRelatedPods. (TestGetPodsToDelete): Augment the "various pod phases and conditions, diff = len(pods)" test case to ensure that scale-down still selects doubled-up pods if there are not enough other pods to scale down. Add a "various pod phases and conditions, diff = len(pods), relatedPods empty" test case to verify that getPodsToDelete works even if related pods could not be determined. Add a "ready and colocated with another ready pod vs not colocated, diff < len(pods)" test case to verify that a doubled-up pod gets preferred for deletion. Augment the "various pod phases and conditions, diff < len(pods)" test case to ensure that not-ready pods are preferred over ready but doubled-up pods. * vendor/k8s.io/kubernetes/pkg/controller/replicaset/BUILD: Regenerate. * vendor/k8s.io/kubernetes/test/e2e/apps/deployment.go (testRollingUpdateDeploymentWithLocalTrafficLoadBalancer): New end-to-end test. Create a deployment with a rolling update strategy and affinity rules and a load balancer with "Local" external traffic policy, and verify that set of nodes with local endponts for the service remains unchanged during rollouts. (setAffinity): New helper, used by testRollingUpdateDeploymentWithLocalTrafficLoadBalancer. * vendor/k8s.io/kubernetes/test/e2e/apps/types.go (AgnhostImageName) (AgnhostImage): New constants for the agnhost image. * vendor/k8s.io/kubernetes/test/e2e/framework/service/jig.go (GetEndpointNodes): Factor building the set of node names out... (GetEndpointNodeNames): ...into this new method.
Miciah
force-pushed
the
UPSTREAM-80004-prefer-to-delete-doubled-up-pods-of-a-replicaset
branch
from
3a71e71 to
aa70dc2
Compare
Contributor
Author
|
Looks like problems pulling from the internal registry. |
Contributor
|
@smarterclayton PTAL, we need to get this in soon. |
Contributor
|
/lgtm |
Contributor
|
/retest |
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, Miciah, smarterclayton 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 |
openshift-ci-robot
added
the
approved
Contributor
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
Contributor
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
I lost that commit during rebase, let me know if we want to bring it back. |
Contributor
Author
|
Can you be more specific? What failure are you saying this PR caused, and why? Looking at a half dozen failures, the most common symptom that I see is that new nodes do not become ready but rather report, "runtime network not ready: NetworkReady=false reason:NetworkPluginNotReady message:Network plugin returns error: Missing CNI default network". Is that the error you are saying this PR caused? How did you trace it to this PR? |
Contributor
Author
|
Ah, are you referring to https://bugzilla.redhat.com/show_bug.cgi?id=1765756? |
Contributor
Contributor
Author
|
#24057 brings this commit back and adds a fix for https://bugzilla.redhat.com/show_bug.cgi?id=1765756. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When scaling down a ReplicaSet, delete doubled up replicas first, where a "doubled up replica" is defined as one that is on the same node as an active replica belonging to a related ReplicaSet. ReplicaSets are considered "related" if they have a common controller (typically a Deployment).
My intention with this change is to make a rolling update of a Deployment scale down the old ReplicaSet as it scales up the new ReplicaSet by deleting pods from the old ReplicaSet that are colocated with ready pods of the new ReplicaSet. This change in the behavior of rolling updates can be combined with pod affinity rules to preserve the locality of a Deployment's pods over rollout.
A specific scenario that benefits from this change is when a Deployment's pods are exposed by a Service that has type "LoadBalancer" and external traffic policy "Local". In this scenario, the load balancer uses health checks to determine whether it should forward traffic for the Service to a particular node. If the node has no local endpoints for the Service, the health check will fail for that node. Eventually, the load balancer will stop forwarding traffic to that node. In the meantime, the service proxy drops traffic for that Service. Thus, in order to reduce risk of dropping traffic during a rolling update, it is desirable preserve node locality of endpoints.
vendor/k8s.io/kubernetes/pkg/controller/controller_utils.go(ActivePodsWithRanks): New type tosort pods using a given ranking.
vendor/k8s.io/kubernetes/pkg/controller/replicaset/replica_set.go(getReplicaSetsWithSameController): New method. Given a ReplicaSet, return all ReplicaSets that have the same owner.(
manageReplicas): CallgetIndirectlyRelatedPods, and pass its result togetPodsToDelete.(
getIndirectlyRelatedPods): New method. Given a ReplicaSet, return all pods that are owned by any ReplicaSet with the same owner.(
getPodsToDelete): Add an argument for related pods. Use related pods andActivePodsWithRanksto take into account whether the pod is doubled up.vendor/k8s.io/kubernetes/pkg/controller/replicaset/replica_set_test.go(newReplicaSet): SetOwnerReferenceson the ReplicaSet.(
newPod): Set a unique UID on the pod.(
byName): New type to sort pods by name.(
TestRelatedPodsLookup): New test forgetIndirectlyRelatedPods.(
TestGetPodsToDelete): Add a "ready and colocated with another ready pod vs not colocated, diff < len(pods)" test case to verify that a doubled-up pod gets preferred for deletion. Augment the "various pod phases and conditions, diff = len(pods)" test case to ensure that scale-down still selects doubled-up pods if there are not enough other pods to scale down. Augment the "various pod phases and conditions, diff < len(pods)" test case to ensure that not-ready pods are preferred over ready but doubled-up pods.vendor/k8s.io/kubernetes/pkg/controller/replicaset/BUILD: Regenerate.