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 - 2. Calculating Predicates Metadata #77760

Merged
merged 6 commits into from Jul 24, 2019

Conversation

@Huang-Wei
Copy link
Member

commented May 10, 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 2nd PR of the "Even Pods Spread" KEP implementation.

  • build a new topologyPairsPodSpreadMap into PredicateMetadata
  • update ShallowCopy()
  • unit tests
  • a new set of utils using "chained" pattern to build API objects

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 10, 2019

/kind feature
/priority important-soon

@Huang-Wei Huang-Wei referenced this pull request May 10, 2019

Open

Umbrella issue for Even Pods Spread [Alpha] #77284

6 of 9 tasks complete

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-meta branch 2 times, most recently from d6a656b to 5ae8ed5 May 10, 2019

@Huang-Wei Huang-Wei changed the title Even Pods Spread - 2 Calculating Predicates Metadata Even Pods Spread - 2. Calculating Predicates Metadata May 13, 2019

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-meta branch from 5ae8ed5 to ccc0d59 May 21, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

/retest

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-meta branch from ccc0d59 to 7c98b44 May 21, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

/retest

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-meta branch from 7c98b44 to f1115ba Jun 14, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

/retest

}

func (topologyPairsMaps *topologyPairsMaps) removePod(deletedPod *v1.Pod) {
// add a topology pair holder if needed

This comment has been minimized.

Copy link
@draveness

draveness Jul 20, 2019

Member

nit: this comment should start with the function name.

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Jul 23, 2019

Author Member

It's usually tolerant for private functions.

podFullName := schedutil.GetPodFullName(pod)
if topologyPairsMaps.topologyPairToPods[pair] == nil {
topologyPairsMaps.topologyPairToPods[pair] = make(map[*v1.Pod]struct{})
if m.topologyPairToPods[pair] == nil {

This comment has been minimized.

Copy link
@draveness

draveness Jul 20, 2019

Member

nit: Could we replace this we addTopologyPairWithoutPods?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Jul 23, 2019

Author Member

Done.

@@ -19,7 +19,7 @@ package predicates
import (
"strings"

v1 "k8s.io/api/core/v1"

This comment has been minimized.

Copy link
@draveness

draveness Jul 20, 2019

Member

Can we revert this change since there is nothing updates else in this file and it is not a restriction that we should import without v1 prefix

@@ -21,7 +21,7 @@ import (

"time"

v1 "k8s.io/api/core/v1"

This comment has been minimized.

Copy link
@draveness

draveness Jul 20, 2019

Member

same here

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

That discussion was based on the context of switching to the {tpPair:count} solution. In that case, we don't hold a {podName:tpPairSet} structure.

I don't think we suggested that. Only to get rid of {tpPair:podSet}. I think we should still be able to hold {podName:tpPairSet}, which can even point to the same map (same address) for all the pods in the same node.

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

I don't think we suggested that. Only to get rid of {tpPair:podSet}. I think we should still be able to hold {podName:tpPairSet}, which can even point to the same map (same address) for all the pods in the same node.

That's an option. Let's see how it can benefit upon eliminating {tpPair:podSet}.

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I don't think we suggested that. Only to get rid of {tpPair:podSet}. I think we should still be able to hold {podName:tpPairSet}, which can even point to the same map (same address) for all the pods in the same node.

That's an option. Let's see how it can benefit upon eliminating {tpPair:podSet}.

sgtm

Huang-Wei added some commits Apr 30, 2019

EvenPodsSpread: PredicateMetadata initilization
- build a new `topologyPairsPodSpreadMap` into PredicateMetadata
- update ShallowCopy()
- unit tests
EvenPodsSpread: refactor topologyPairsPodSpreadMap
- update minMatchMap from []int32 to map[string]int32
EvenPodsSpread: refactor "chained" utils
- move "chanined" utils to pkg/scheduler/testing/utils.go so as to be re-used by all scheduler tests

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-pred-meta branch from d5e205a to f822487 Jul 23, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

/retest

1 similar comment
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

/retest

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

@alculquicondor: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@ahg-g
Copy link
Member

left a comment

/lgtm

Thanks for modifying it.

errCh.SendErrorWithCancel(err, cancel)
return
}
if ok {
// constraint.TopologyKey is already guaranteed to be present
pair := topologyPair{key: constraint.TopologyKey, value: node.Labels[constraint.TopologyKey]}

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 23, 2019

Member

You may also want to add that since we restrict a toplogyKey to exist in a single hard constraint, using topolgyKey as the key in this map is good enough to identify which exact constraint this pod is counted against (because otherwise the key to the map should be some sort of an identifier that combines the topologyKey and the label selector)

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

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 23, 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.

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 24, 2019

/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 k8s-ci-robot merged commit 87c3f51 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-meta branch Jul 24, 2019

for _, constraint := range constraints {
topologyPairsPodSpreadMap.topologyKeyToMinPodsMap[constraint.TopologyKey] = math.MaxInt32
}
for pair, podSet := range topologyPairsPodSpreadMap.topologyPairToPods {

This comment has been minimized.

Copy link
@tedyu

tedyu Jul 24, 2019

Contributor

Is it possible that value of some topologyPairsPodSpreadMap.topologyKeyToMinPodsMap is
math.MaxInt32 at the end of this loop ?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Jul 24, 2019

Author Member

Nope. The logic below ensures for each tp pair, they're valued with either empty podSet, or a concrete podSet:

// If needed, append topology pair without entry of pods.
// For example, on node-x, there is no pod matching spread constraints,
// but node-x should be also considered as a match (with match number 0)
// i.e. <node: node-x>: {}
for _, constraint := range constraints {
// constraint.TopologyKey is already guaranteed to be present
pair := topologyPair{
key: constraint.TopologyKey,
value: node.Labels[constraint.TopologyKey],
}
// addTopologyPairWithoutPods is a non-op if other pods match this pair
nodeTopologyMaps.addTopologyPairWithoutPods(pair)
}

Then in the above loop, the default value (MaxInt32) will be compared with len(podSet), so they won't remain MaxInt32 at the end of the loop.

topologyPairsPodSpreadMap.topologyKeyToMinPodsMap[constraint.TopologyKey] = math.MaxInt32
}
for pair, podSet := range topologyPairsPodSpreadMap.topologyPairToPods {
// TODO(Huang-Wei): short circuit unvisited portions of <topologyKey: any value>

This comment has been minimized.

Copy link
@tedyu

tedyu Jul 24, 2019

Contributor

I wonder what type of short circuiting you were thinking about.
Without short circuiting, the following comparison is performed and we continue to the next pair:

l < topologyPairsPodSpreadMap.topologyKeyToMinPodsMap[pair.key]

@alculquicondor alculquicondor referenced this pull request Jul 24, 2019

Closed

REQUEST: New membership for alculquicondor #1042

6 of 6 tasks complete

yuxiangqian added a commit to yuxiangqian/kubernetes that referenced this pull request Jul 31, 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.