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

Even Pods Spread - 3. Predicates Core #77828

Merged
merged 5 commits into from Jul 24, 2019

Conversation

@Huang-Wei
Copy link
Member

commented May 13, 2019

What type of PR is this?

/sig scheduling
/hold

/assign @bsalamat
/cc @krmayankk

What this PR does / why we need it:

This is the 3rd PR of the "Even Pods Spread" KEP implementation. After this PR, users can run workloads using "hard topologySpreadConstraints". (Preemption and Priority support PRs will sent out later)

  • Define a new Predicate to supports API spec whenUnsatisfiable: DoNotSchedule
  • Core Predicate logic
  • unit tests on EvenPodsSpread predicate
  • unit tests on genericScheduler.Schedule()

Which issue(s) this PR fixes:

Part of #77284.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

(will document all the changes in one place)

NONE
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

/kind feature
/priority important-soon

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

/test pull-kubernetes-node-e2e

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from 0e8b271 to 1a0c215 May 13, 2019

@krmayankk
Copy link
Contributor

left a comment

i took a quick first pass over it. More comments will help understand the core logic

Show resolved Hide resolved pkg/scheduler/algorithm/predicates/metadata.go Outdated
// (2) minimum match number of each hard spread constraint
type topologyPairsPodSpreadMap struct {
minMatchMap map[string]int32
*topologyPairsMaps

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 21, 2019

Contributor

is it necessary to use an existing struct from anti affinity ? I was thinking when we deprecate it, it might be easier to cleanup the code

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei May 21, 2019

Author Member

topologyPairsMaps is a common data structure, and is not limited to work for affinity only.

return false, nil, fmt.Errorf("node not found")
}

constraints := getHardTopologySpreadConstraints(pod)

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 21, 2019

Contributor

why does this only get the hard topology constraints ?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei May 21, 2019

Author Member

As predicates only care about hard constraints :) In Priority logic, it will take care of soft constraints.


var topologyPairsPodSpreadMap *topologyPairsPodSpreadMap
if predicateMeta, ok := meta.(*predicateMetadata); ok {
topologyPairsPodSpreadMap = predicateMeta.topologyPairsPodSpreadMap

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 21, 2019

Contributor

when does this get populated ?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei May 21, 2019

Author Member

As usual, at the beginning of Predicates phase.

p.Spec.Affinity.NodeAffinity = &v1.NodeAffinity{}
}
nodeSelector := MakeNodeSelector().NotIn(key, vals).Obj()
p.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution = nodeSelector

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 21, 2019

Contributor

why do we care about the affinity fields in this PR ?I would try to keep this feature completely separate from the affinity feature

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from 1a0c215 to f9a2ab8 Jun 14, 2019

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from f9a2ab8 to c2202d4 Jul 2, 2019

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from c2202d4 to 216a821 Jul 3, 2019

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from 9c4a63e to 003b757 Jul 21, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

/retest

@@ -140,3 +140,227 @@ func MoreImportantPod(pod1, pod2 interface{}) bool {
}
return GetPodStartTime(pod1.(*v1.Pod)).Before(GetPodStartTime(pod2.(*v1.Pod)))
}

// Utilities below to build API objects using a "chained" manner

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Jul 23, 2019

Member

Shouldn't these be on pkg/scheduler/testing? Also, I don't see them being used in this PR.

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Jul 23, 2019

Author Member

Geez... My bad on rebasing, again. Thanks for the catch @alculquicondor. Updated.

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

/retest

@alculquicondor
Copy link
Member

left a comment

LGTM

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from 4f32fea to 6d89b4c Jul 24, 2019

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Jul 24, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

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

if predicateMeta, ok := meta.(*predicateMetadata); ok {
topologyPairsPodSpreadMap = predicateMeta.topologyPairsPodSpreadMap
} else { // We don't have precomputed metadata. We have to follow a slow path to check spread constraints.
// TODO(Huang-Wei): get it implemented

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 24, 2019

Member

can you please create an issue to track this? we also need an issue to track optimizing the implementation by using counters instead of pod lists in the maps.

},
},
}
for _, tt := range tests {

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 24, 2019

Member

can we add a test where an existing pod has labels that matches only one constraint of an incoming pod, basically test the case we discussed in #77760 (comment)

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Jul 24, 2019

Author Member

They're added in the test TestEvenPodsSpreadPredicate_MultipleConstraints. You can check out commit "EvenPodsSpread: update 'selfMatch' logic".

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 24, 2019

Member

nice!

@@ -36,6 +36,7 @@ import (
schedulerapi "k8s.io/kubernetes/pkg/scheduler/api"
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
schedulertesting "k8s.io/kubernetes/pkg/scheduler/testing"
st "k8s.io/kubernetes/pkg/scheduler/testing"

This comment has been minimized.

Copy link
@draveness

draveness Jul 24, 2019

Member

seems like a duplicate import :)

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Jul 24, 2019

Author Member

Good catch! Updated (removed the above import as st is shorter to make the code compact)

Huang-Wei added some commits Apr 30, 2019

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-core branch from 6d89b4c to 1822085 Jul 24, 2019

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

/hold cancel

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

@ahg-g comments addressed. PTAL.

@ahg-g

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 24, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 24, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

/retest

@alculquicondor alculquicondor referenced this pull request Jul 24, 2019

Closed

REQUEST: New membership for alculquicondor #1042

6 of 6 tasks complete

@k8s-ci-robot k8s-ci-robot merged commit d83cf5f into kubernetes:master Jul 24, 2019

22 of 23 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation Huang-Wei authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
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-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
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
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@Huang-Wei Huang-Wei deleted the Huang-Wei:eps-pred-core branch Jul 24, 2019

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.