Bug 1765756: UPSTREAM: 80004: Prefer to delete doubled-up pods of a ReplicaSet#24057
Merged
openshift-merge-robot merged 3 commits intoopenshift:masterfrom Nov 7, 2019
Conversation
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.
Skip the "Deployment should not disrupt a cloud load-balancer's connectivity during rollout" test if the number of nodes is less than 2; otherwise, set the deployment's replicas equal to the lesser of 5 and the number of nodes. The test would fail if there were fewer nodes than replicas, but the test needs at least 2 nodes, and the likelihood of failure absent the feature under test increases with the number of replicas, so it is desirable to set replicas to a higher value, within reason. Follow-up to commit 980b640. * vendor/k8s.io/kubernetes/test/e2e/apps/deployment.go: Skip the load-balancer connectivity test unless there are at least 2 nodes. (testRollingUpdateDeploymentWithLocalTrafficLoadBalancer): Set replicas to the min of 5 and the number of nodes.
openshift-ci-robot
added
the
size/XL
Miciah
changed the title
Contributor
|
/lgtm |
Miciah
changed the title
|
@Miciah: This pull request references Bugzilla bug 1765756, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@Miciah: This pull request references Bugzilla bug 1765756, which is valid. DetailsIn response to this:
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. |
Contributor
Author
|
I wonder whether I can do this: |
Contributor
Author
|
/test e2e-aws-scaleup-rhel7 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knobunc, Miciah, soltysh 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. |
|
@Miciah: All pull requests linked via external trackers have merged. Bugzilla bug 1765756 has been moved to the MODIFIED state. DetailsIn response to this:
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. |
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
7 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.
This commit brings back #23806, which was dropped during a rebase scuffle (see #24014 (comment)), and adds a fix for BZ#1765756.
UPSTREAM: 80004: Prefer to delete doubled-up pods of a ReplicaSet
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 forActivePodsWithRanks.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 and the new getPodsRankedByRelatedPodsOnSameNode function to take into account whether the pod is doubled up when sorting pods for deletion.(
getPodsRankedByRelatedPodsOnSameNode): New function. Return anActivePodsWithRanksvalue 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): SetOwnerReferenceson the ReplicaSet.(
newPod): Set a unique UID on the pod.(
byName): New type to sort pods by name.(
TestRelatedPodsLookup): New test forgetIndirectlyRelatedPods.(
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 bytestRollingUpdateDeploymentWithLocalTrafficLoadBalancer.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.UPSTREAM: 84339: Fix deployment e2e test at scale
UPSTREAM: 84568: test/e2e/apps: Skip or scale LB test per node count
Skip the "Deployment should not disrupt a cloud load-balancer's connectivity during rollout" test if the number of nodes is less than 2; otherwise, set the deployment's replicas equal to the lesser of 5 and the number of nodes.
The test would fail if there were fewer nodes than replicas, but the test needs at least 2 nodes, and the likelihood of failure absent the feature under test increases with the number of replicas, so it is desirable to set replicas to a higher value, within reason.
vendor/k8s.io/kubernetes/test/e2e/apps/deployment.go: Skip the load-balancer connectivity test unless there are at least 2 nodes.(
testRollingUpdateDeploymentWithLocalTrafficLoadBalancer): Setreplicasto the min of 5 and the number of nodes.This PR supersedes #24047.