Refactor and optimize preferred (anti) pod affinity #85959

ahg-g · 2019-12-05T17:46:28Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
Optimize preferred (anti) pod affinity by removing redundant creation of Selectors during metadata calculation, and eliminates unnecessary and excessive nodeinfo map lookups. Gain is roughly 25% on 5k clusters.

before

ahg@ahg1:~/go/src/k8s.io/kubernetes$ tail /tmp/affinity-master 
I1205 13:56:33.337355  222356 etcd.go:81] etcd already running at http://127.0.0.1:2379
goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkSchedulingPreferredPodAffinity/500Nodes/500Pods-12                 1000           2099450 ns/op
BenchmarkSchedulingPreferredPodAffinity/5000Nodes/15000Pods-12              1000          13419954 ns/op
PASS
ok      k8s.io/kubernetes/test/integration/scheduler_perf       204.199s
+++ [1205 13:59:57] Cleaning up etcd
+++ [1205 13:59:57] Integration test cleanup complete
ahg@ahg1:~/go/src/k8s.io/kubernetes$

after

ahg@ahg1:~/go/src/k8s.io/kubernetes$ tail /tmp/affinity-opt
I1205 14:04:00.566016  225809 etcd.go:81] etcd already running at http://127.0.0.1:2379
goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkSchedulingPreferredPodAffinity/500Nodes/500Pods-12                 1000           1909800 ns/op
BenchmarkSchedulingPreferredPodAffinity/5000Nodes/15000Pods-12              1000          10676680 ns/op
PASS
ok      k8s.io/kubernetes/test/integration/scheduler_perf       182.283s
+++ [1205 14:07:02] Cleaning up etcd
+++ [1205 14:07:02] Integration test cleanup complete

Does this PR introduce a user-facing change?:

NONE

/cc @Huang-Wei

k8s-ci-robot · 2019-12-05T17:48:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

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:

~~pkg/scheduler/OWNERS~~ [ahg-g]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahg-g · 2019-12-05T17:51:39Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

@@ -143,6 +166,75 @@ func CalculateInterPodAffinityPriorityReduce(pod *v1.Pod, meta interface{}, shar
 	return nil
 }

+func (p *podAffinityPriorityMap) processExistingPod(existingPod *v1.Pod, existingPodNodeInfo *schedulernodeinfo.NodeInfo) error {


this is just moving the processPod to a separate function instead of embedding it. This help showing it better in profiling.

ahg-g · 2019-12-05T19:58:29Z

/cc @alculquicondor

alculquicondor · 2019-12-06T13:52:40Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

 }

-func (p *podAffinityPriorityMap) processTerm(term *v1.PodAffinityTerm, podDefiningAffinityTerm, podToCheck *v1.Pod, fixedNode *v1.Node, weight int64) error {
-	namespaces := priorityutil.GetNamespacesFromPodAffinityTerm(podDefiningAffinityTerm, term)
+func getProcessedWeightedAffinityTerm(pod *v1.Pod, term *v1.PodAffinityTerm, weight int32) (*processedWeightedAffinityTerm, error) {


newProcessedWeightedAffinityTerm. And you could accept v1.WeightedPodAffinityTerm as a parameter.

can't accept WeightedPodAffinityTerm as a parameter because of https://github.com/kubernetes/kubernetes/pull/85959/files#diff-ac123860716bbbb4ed0f26cc27062bb7R201

alculquicondor · 2019-12-06T13:58:16Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

-		nodes:         nodes,
-		topologyScore: make(topologyPairToScore),
-	}
+type processedWeightedAffinityTerm struct {


I would just name it weightedAffinityTerm, to make it clear that it's our internal representation of the object v1.WeightedAffinityTerm

alculquicondor · 2019-12-06T14:45:01Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

+	pod                   *v1.Pod
+	affinityTerms         []*processedWeightedAffinityTerm
+	antiAffinityTerms     []*processedWeightedAffinityTerm
+	sharedLister          schedulerlisters.SharedLister


where is this being used?

alculquicondor · 2019-12-06T15:01:55Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

-		if err := p.processTerm(&term.PodAffinityTerm, podDefiningAffinityTerm, podToCheck, fixedNode, int64(term.Weight*int32(multiplier))); err != nil {
+func (p *podAffinityPriorityMap) processTerms(terms []*processedWeightedAffinityTerm, podDefiningAffinityTerm, podToCheck *v1.Pod, fixedNode *v1.Node, multiplier int) error {
+	for _, term := range terms {
+		if err := p.processTerm(term, podDefiningAffinityTerm, podToCheck, fixedNode, int64(term.weight*int32(multiplier))); err != nil {


passing the multiplier seems more readable to me.

Can we rename to updateScoreWithTerms or something like that?

passing the multiplier seems more readable to me.

done.

Can we rename to updateScoreWithTerms or something like that?

then we need to come up with a name for "processTerms" as well, let keep like this.

alculquicondor · 2019-12-06T15:11:21Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

+	return processedTerms, nil
+}
+
+func (p *podAffinityPriorityMap) processTerm(term *processedWeightedAffinityTerm, podDefiningAffinityTerm, podToCheck *v1.Pod, fixedNode *v1.Node, weight int64) error {


podDefiningAffinityTerm is unused :)

cool, that is because of the refactoring.

alculquicondor · 2019-12-06T15:13:24Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

-		return nil
+	pm := podAffinityPriorityMap{
+		topologyScore:         make(topologyPairToScore),
+		pod:                   pod,


is it worth storing this guy? I feel like it should just be passed as parameter.

we are computing this map from the perspective of this pod, so I think it is reasonable to have it. In fact passing it as a parameter means the functions of a specific podAffinityPriorityMap instance may accept different pods, which is not true.

ok, so now we only have a single function relying on it, I removed it :)

alculquicondor · 2019-12-06T16:30:31Z

/lgtm

/hold

for rebase

alculquicondor · 2019-12-06T16:20:43Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

 	hardPodAffinityWeight int32
 	sync.Mutex
 }

-type processedWeightedAffinityTerm struct {
+type weightedAffinityTerm struct {


worth adding a comment indicating that this the representation of v1.WeightedPodAffinityTerm

alculquicondor · 2019-12-06T16:23:54Z

pkg/scheduler/algorithm/priorities/interpod_affinity.go

@@ -292,14 +288,15 @@ func buildTopologyPairToScore(
 	processNode := func(i int) {
 		nodeInfo := allNodes[i]
 		if nodeInfo.Node() != nil {
+			// Unless the pod itself has affinity terms, We only need to process nodes hosting pods with affinity.


Here you are filtering pods, not nodes.

Unless the pod itself has affinity terms, we only need to process pods with affinity in the node

yup, fixed.

ahg-g

/lgtm

/hold

for rebase

squashed.

alculquicondor · 2019-12-06T18:01:35Z

/hold cancel

/lgtm

fejta-bot · 2019-12-06T23:07:43Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot · 2019-12-07T00:55:15Z

@ahg-g: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-conformance-kind-ipv6	7f2307b4e997650568fd49d3841c2ad59640bec9	link	`/test pull-kubernetes-conformance-kind-ipv6`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

ahg-g · 2019-12-07T01:26:32Z

/retest

k8s-ci-robot requested a review from Huang-Wei December 5, 2019 17:46

ahg-g force-pushed the ahg-affinity-opt branch from 7f2307b to bda0eb2 Compare December 5, 2019 17:47

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 5, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 5, 2019

ahg-g commented Dec 5, 2019

View reviewed changes

ahg-g force-pushed the ahg-affinity-opt branch from bda0eb2 to 1ce0733 Compare December 5, 2019 19:10

k8s-ci-robot requested a review from alculquicondor December 5, 2019 19:58

alculquicondor reviewed Dec 6, 2019

View reviewed changes

ahg-g force-pushed the ahg-affinity-opt branch 2 times, most recently from 9685623 to 36a0743 Compare December 6, 2019 16:10

k8s-ci-robot assigned alculquicondor Dec 6, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 6, 2019

alculquicondor reviewed Dec 6, 2019

View reviewed changes

ahg-g force-pushed the ahg-affinity-opt branch from 36a0743 to 7b8db19 Compare December 6, 2019 17:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2019

optimize preferred pod affinity

53be26e

ahg-g force-pushed the ahg-affinity-opt branch from 7b8db19 to 53be26e Compare December 6, 2019 17:59

ahg-g commented Dec 6, 2019

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 6, 2019

k8s-ci-robot merged commit 77a95dc into kubernetes:master Dec 7, 2019

k8s-ci-robot added this to the v1.18 milestone Dec 7, 2019

ahg-g deleted the ahg-affinity-opt branch January 10, 2020 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and optimize preferred (anti) pod affinity #85959

Refactor and optimize preferred (anti) pod affinity #85959

ahg-g commented Dec 5, 2019 •

edited

k8s-ci-robot commented Dec 5, 2019

ahg-g Dec 5, 2019

ahg-g commented Dec 5, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor commented Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

alculquicondor Dec 6, 2019

ahg-g Dec 6, 2019

ahg-g left a comment

alculquicondor commented Dec 6, 2019

fejta-bot commented Dec 6, 2019

k8s-ci-robot commented Dec 7, 2019 •

edited

ahg-g commented Dec 7, 2019

Refactor and optimize preferred (anti) pod affinity #85959

Refactor and optimize preferred (anti) pod affinity #85959

Conversation

ahg-g commented Dec 5, 2019 • edited

k8s-ci-robot commented Dec 5, 2019

Choose a reason for hiding this comment

ahg-g commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

alculquicondor commented Dec 6, 2019

fejta-bot commented Dec 6, 2019

k8s-ci-robot commented Dec 7, 2019 • edited

ahg-g commented Dec 7, 2019

ahg-g commented Dec 5, 2019 •

edited

k8s-ci-robot commented Dec 7, 2019 •

edited