Skip to content
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

move inter pod affinity predicate logic to its Filter plugin #86459

Merged
merged 1 commit into from Dec 21, 2019

Conversation

@ahg-g
Copy link
Member

ahg-g commented Dec 20, 2019

What type of PR is this?

/kind cleanup

Which issue(s) this PR fixes:
Fixes #86460

NONE

/cc @Huang-Wei @alculquicondor

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 20, 2019

[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:

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

@ahg-g ahg-g force-pushed the ahg-g:ahg1-affinity-pred branch from 28518b3 to 5515654 Dec 20, 2019
@ahg-g

This comment has been minimized.

Copy link
Member Author

ahg-g commented Dec 20, 2019

/retest

key string
value string
}
type topologyToMatchedTermCount map[topologyPair]int64

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Dec 20, 2019

Member

I don't like how we use map[string]map[string]int64 for scoring, but map[topologyPair]int64 for filtering. We should probably just pick one (the one that gives us the best performance). But that's for a follow up.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Author Member

yeah, priority and predicate evolved separately, make sense to unify them, let me see if I can do it in this PR.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Author Member

lets do that as a follow up

// preFilterStateKey is the key in CycleState to InterPodAffinity pre-computed data for Filtering.
// Using the name of the plugin will likely help us avoid collisions with other plugins.
preFilterStateKey = "PreFilter" + Name
// InterPodAffinity is a plugin that checks inter pod affinity

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Dec 20, 2019

Member

now I feel like this file is too big. But the plugin lives in a folder, so we can do something like:

interpodaffinity/
  plugin.go
  filtering.go
  scoring.go

WDYT?

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Author Member

sg, I was thinking the same thing.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Author Member

done.

}

for _, term := range terms {
namespaces := priorityutil.GetNamespacesFromPodAffinityTerm(pod, &term)

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Dec 20, 2019

Member

it looks like this block can be factored out and shared with postfilter?

@ahg-g ahg-g force-pushed the ahg-g:ahg1-affinity-pred branch 4 times, most recently from 670a4f0 to 976b886 Dec 20, 2019
@alculquicondor

This comment has been minimized.

Copy link
Member

alculquicondor commented Dec 20, 2019

great. Let's just wait for @Huang-Wei comments.

@ahg-g ahg-g force-pushed the ahg-g:ahg1-affinity-pred branch from 976b886 to 429448c Dec 20, 2019
@ahg-g

This comment has been minimized.

Copy link
Member Author

ahg-g commented Dec 20, 2019

/retest

1 similar comment
@ahg-g

This comment has been minimized.

Copy link
Member Author

ahg-g commented Dec 20, 2019

/retest

Copy link
Member

Huang-Wei left a comment

LGTM generally. Just one nit.

type InterPodAffinity struct {
sharedLister schedulerlisters.SharedLister
hardPodAffinityWeight int32
sync.Mutex

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Dec 20, 2019

Member

The mutex is never used. We defined local Mutexs in getTPMapMatchingExistingAntiAffinity and getTPMapMatchingIncomingAffinityAntiAffinity.

This comment has been minimized.

Copy link
@ahg-g

ahg-g Dec 20, 2019

Author Member

it is actually used by the score logic.

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Dec 20, 2019

Member

oh, true... A nit is to update getTPMapMatchingExistingAntiAffinity and getTPMapMatchingIncomingAffinityAntiAffinity as a method receiver, so we can use pl.Lock() and pl.Unlock() consistently. But it's pre-existing, so up to you.

@Huang-Wei

This comment has been minimized.

Copy link
Member

Huang-Wei commented Dec 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 21, 2019
k8s-ci-robot added a commit that referenced this pull request Dec 21, 2019
move inter pod affinity predicate logic to its Filter plugin
@k8s-ci-robot k8s-ci-robot merged commit 8737890 into kubernetes:master Dec 21, 2019
15 checks passed
15 checks passed
cla/linuxfoundation ahg-g authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
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-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot merged commit 429448c into kubernetes:master Dec 21, 2019
15 checks passed
15 checks passed
cla/linuxfoundation ahg-g authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
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-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot modified the milestone: v1.18 Dec 21, 2019
@ahg-g ahg-g deleted the ahg-g:ahg1-affinity-pred branch Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.