New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anti affinity optimization #66948

Merged
merged 4 commits into from Aug 18, 2018

Conversation

@mohamed-mehany
Contributor

mohamed-mehany commented Aug 3, 2018

What this PR does / why we need it:
This pull request aims to optimize the performance of anti-affinity rules lookup of existing pods
This optimization maps the topology values to a list of pods running on nodes that match this value and store that map in the pod metadata. Accordingly, when validating anti-affinity rules of existing pods we will only check those running on nodes with similar topology values to the current candidate (node) for scheduling.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #63937

Special notes for your reviewer:
/sig scalability
/sig scheduling
Release note:

improve performance of anti-affinity predicate of default scheduler.
@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 3, 2018

Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Aug 3, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 3, 2018

Contributor

/assign @bsalamat

Contributor

mohamed-mehany commented Aug 3, 2018

/assign @bsalamat

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 3, 2018

Contributor

/check-cla

Contributor

mohamed-mehany commented Aug 3, 2018

/check-cla

@mohamed-mehany mohamed-mehany changed the title from [WIP] Anti affinity optimization to Anti affinity optimization Aug 3, 2018

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Aug 6, 2018

Contributor

/ok-to-test

Contributor

bsalamat commented Aug 6, 2018

/ok-to-test

@bsalamat

Thanks a lot, @mohamed-mehany! This is a very good start. I think the logic is generally correct, but can be optimized further. Please see my comments for more details.

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 7, 2018

Contributor

/retest

Contributor

mohamed-mehany commented Aug 7, 2018

/retest

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Aug 7, 2018

Member

would you show benchmark for the improvement?

Member

k82cn commented Aug 7, 2018

would you show benchmark for the improvement?

@@ -113,11 +113,6 @@ func predicateMetadataEquivalent(meta1, meta2 *predicateMetadata) error {
for !reflect.DeepEqual(meta1.podPorts, meta2.podPorts) {
return fmt.Errorf("podPorts are not equal")
}
sortAntiAffinityTerms(meta1.matchingAntiAffinityTerms)

This comment has been minimized.

@bsalamat

bsalamat Aug 7, 2018

Contributor

Please add similar checks for the two newly added structs topologyPairToAntiAffinityPods and antiAffinityPodsToTopologyPair here.

@bsalamat

bsalamat Aug 7, 2018

Contributor

Please add similar checks for the two newly added structs topologyPairToAntiAffinityPods and antiAffinityPodsToTopologyPair here.

Show outdated Hide outdated pkg/scheduler/algorithm/predicates/metadata.go
Show outdated Hide outdated pkg/scheduler/algorithm/predicates/metadata.go
Show outdated Hide outdated pkg/scheduler/algorithm/predicates/predicates.go
Show outdated Hide outdated pkg/scheduler/algorithm/predicates/predicates.go
@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 7, 2018

Contributor

/test pull-kubernetes-kubemark-e2e-gce-big

Contributor

mohamed-mehany commented Aug 7, 2018

/test pull-kubernetes-kubemark-e2e-gce-big

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Aug 8, 2018

Contributor

I tried running our AntiAffinity scheduler benchmark:

make test-integration WHAT=./test/integration/scheduler_perf GOFLAGS="-v=1" KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-run=xxx -bench BenchmarkSchedulingAntiAffinity/5000Nodes/0Pods"

and it seems that this PR in its current form does not improve performance when scheduling 2000 pods with AntiAffinity in a cluster with 5000 nodes. Here are the results when eCache is disabled:

With this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkSchedulingAntiAffinity/5000Nodes/0Pods-12         	    2000	 299407004 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	612.984s

Without this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkSchedulingAntiAffinity/5000Nodes/0Pods-12         	    2000	 298837178 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	611.690s
Contributor

bsalamat commented Aug 8, 2018

I tried running our AntiAffinity scheduler benchmark:

make test-integration WHAT=./test/integration/scheduler_perf GOFLAGS="-v=1" KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-run=xxx -bench BenchmarkSchedulingAntiAffinity/5000Nodes/0Pods"

and it seems that this PR in its current form does not improve performance when scheduling 2000 pods with AntiAffinity in a cluster with 5000 nodes. Here are the results when eCache is disabled:

With this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkSchedulingAntiAffinity/5000Nodes/0Pods-12         	    2000	 299407004 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	612.984s

Without this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkSchedulingAntiAffinity/5000Nodes/0Pods-12         	    2000	 298837178 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	611.690s
@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 9, 2018

Contributor

/test pull-kubernetes-e2e-kops-aws

Contributor

mohamed-mehany commented Aug 9, 2018

/test pull-kubernetes-e2e-kops-aws

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Aug 11, 2018

Contributor

I found sometime to profile the scheduler with and without the changes in this PR. When we run the BenchmarkSchedulingAntiAffinity without this PR, about 8% of the time is spent in satisfiesExistingPodsAntiAffinity and almost the rest is spent in satisfiesPodsAffinityAntiAffinity function. The latter is the function that checks the anti-affinity rules of the pod being scheduled. This PR optimizes satisfiesExistingPodsAntiAffinity which is a much smaller part of this benchmark. With this PR, satisfiesExistingPodsAntiAffinity has disappeared from the graphical output of pprof, which means that the function no longer takes a significant amount of time. So, this PR has worked and has optimized the function that it is targeting, but given that the function is not a large part of this particular benchmark, we don't see much performance improvement.
If we apply a similar optimization to checking anti-affinity rules of the pod being scheduled, we will probably see much better results.

Contributor

bsalamat commented Aug 11, 2018

I found sometime to profile the scheduler with and without the changes in this PR. When we run the BenchmarkSchedulingAntiAffinity without this PR, about 8% of the time is spent in satisfiesExistingPodsAntiAffinity and almost the rest is spent in satisfiesPodsAffinityAntiAffinity function. The latter is the function that checks the anti-affinity rules of the pod being scheduled. This PR optimizes satisfiesExistingPodsAntiAffinity which is a much smaller part of this benchmark. With this PR, satisfiesExistingPodsAntiAffinity has disappeared from the graphical output of pprof, which means that the function no longer takes a significant amount of time. So, this PR has worked and has optimized the function that it is targeting, but given that the function is not a large part of this particular benchmark, we don't see much performance improvement.
If we apply a similar optimization to checking anti-affinity rules of the pod being scheduled, we will probably see much better results.

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 13, 2018

Contributor

/test pull-kubernetes-integration

Contributor

mohamed-mehany commented Aug 13, 2018

/test pull-kubernetes-integration

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 14, 2018

Contributor

/test pull-kubernetes-e2e-gce-device-plugin-gpu

Contributor

mohamed-mehany commented Aug 14, 2018

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 15, 2018

Contributor

/test pull-kubernetes-integration

Contributor

mohamed-mehany commented Aug 15, 2018

/test pull-kubernetes-integration

@bsalamat

Thanks, @ahmad-diaa and @mohamed-mehany! Almost there. I have only a couple of minor comments.

@bsalamat

LGTM and Thanks again.
Please squash commits. We are ready to merge this PR.

@bsalamat

/lgtm

Thanks again!

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 18, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 18, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, mohamed-mehany

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Contributor

k8s-ci-robot commented Aug 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, mohamed-mehany

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mohamed-mehany

This comment has been minimized.

Show comment
Hide comment
@mohamed-mehany

mohamed-mehany Aug 18, 2018

Contributor

@bsalamat Thank you Bobby for your guidance and help 👍

Contributor

mohamed-mehany commented Aug 18, 2018

@bsalamat Thank you Bobby for your guidance and help 👍

@ahmad-diaa

This comment has been minimized.

Show comment
Hide comment
@ahmad-diaa

ahmad-diaa Aug 18, 2018

Contributor

@bsalamat Thank you so much for your effort, and guidance.

Contributor

ahmad-diaa commented Aug 18, 2018

@bsalamat Thank you so much for your effort, and guidance.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 18, 2018

Contributor

Automatic merge from submit-queue (batch tested with PRs 67041, 66948). If you want to cherry-pick this change to another branch, please follow the instructions here.

Contributor

k8s-merge-robot commented Aug 18, 2018

Automatic merge from submit-queue (batch tested with PRs 67041, 66948). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit ef388fe into kubernetes:master Aug 18, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation ahmad-diaa authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ahmad-diaa ahmad-diaa deleted the mohamed-mehany:anti-affinity-optimization branch Aug 18, 2018

@Huang-Wei Huang-Wei referenced this pull request Aug 28, 2018

Closed

REQUEST: New membership for Huang-Wei #47

6 of 6 tasks complete

k8s-merge-robot added a commit that referenced this pull request Sep 3, 2018

Merge pull request #67788 from mohamed-mehany/inter-pod-affinity-opti…
…mization

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Affinity/Anti-Affinity Optimization of Pod Being Scheduled

**What this PR does / why we need it**:
Following #66948, it was noticed that the applied optimizations for anti-affinity rules lookup of existing pods could be further applied to checking affinity and anti-affinity terms of the Pod being scheduled. This is done by mapping topology pairs to pods that potentially match the pod being scheduled instead of mapping nodes to matching pods, and accordingly the search space is reduced.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #67738

**Special notes for your reviewer**:
/sig scheduling
/sig scalability

**Release note**:

```release-note
Improve performance of Pod affinity/anti-affinity in the scheduler
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment