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

Add a feature to the scheduler to score fewer than all nodes in every scheduling cycle #66733

Merged
merged 4 commits into from Aug 18, 2018

Conversation

@bsalamat
Contributor

bsalamat commented Jul 28, 2018

What this PR does / why we need it:
Today, the scheduler scores all the nodes in the cluster in every scheduling cycle (every time a posd is attempted). This feature implements a mechanism in the scheduler that allows scoring fewer than all nodes in the cluster. The scheduler stops searching for more nodes once the configured number of feasible nodes are found. This can help improve the scheduler's performance in large clusters (several hundred nodes and larger).
This PR also adds a new structure to the scheduler's cache, called NodeTree, that allows scheduler to iterate over various nodes in different zones in a cluster. This is needed to avoid scoring the same set of nodes in every scheduling cycle.

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 #66627

Special notes for your reviewer:
This is a large PR, but broken into a few logical commits. Reviewing would be easier if you review by commits.

Release note:

Add a feature to the scheduler to score fewer than all nodes in every scheduling cycle. This can improve performance of the scheduler in large clusters.
@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jul 28, 2018

Contributor

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@bsalamat

Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
Contributor

k8s-merge-robot commented Jul 28, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@bsalamat

Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels Jul 28, 2018

// NodeTree is a tree-like data structure that holds node names in each zone. Zone names are
// keys to "NodeTree.tree" and values of "NodeTree.tree" are arrays of node names.
type NodeTree struct {
tree map[string]*nodeArray // a map from zone (region-zone) to an array of nodes in the zone.

This comment has been minimized.

@lichuqiang

lichuqiang Jul 28, 2018

Member

Wondering if it better to make the key configurable, but not limited to "region-zone".
Users may want to divide nodes into other dimensions like rack.

@lichuqiang

lichuqiang Jul 28, 2018

Member

Wondering if it better to make the key configurable, but not limited to "region-zone".
Users may want to divide nodes into other dimensions like rack.

This comment has been minimized.

@bsalamat

bsalamat Jul 28, 2018

Contributor

That's a good idea to make the key configurable. Given the size of this PR, I think that would be a good follow-on PR.

@bsalamat

bsalamat Jul 28, 2018

Contributor

That's a good idea to make the key configurable. Given the size of this PR, I think that would be a good follow-on PR.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Jul 29, 2018

Contributor

I think the same logic could be used for labels meaning group node by labels, @bsalamat IIUC, you want this to be pushed to another PR, is that right?

@ravisantoshgudimetla

ravisantoshgudimetla Jul 29, 2018

Contributor

I think the same logic could be used for labels meaning group node by labels, @bsalamat IIUC, you want this to be pushed to another PR, is that right?

This comment has been minimized.

@bsalamat

bsalamat Jul 29, 2018

Contributor

Yes

@bsalamat

bsalamat Jul 29, 2018

Contributor

Yes

@@ -364,7 +383,7 @@ func (g *genericScheduler) findNodesThatFit(pod *v1.Pod, nodes []*v1.Node) ([]*v
checkNode := func(i int) {
var nodeCache *equivalence.NodeCache
nodeName := nodes[i].Name
nodeName := g.cache.NodeTree().Next()

This comment has been minimized.

@k82cn

k82cn Jul 28, 2018

Member

L408: should we find node by nodeName instead of nodes[i].

@k82cn

k82cn Jul 28, 2018

Member

L408: should we find node by nodeName instead of nodes[i].

This comment has been minimized.

@bsalamat

bsalamat Jul 28, 2018

Contributor

YES!

@bsalamat

bsalamat Jul 28, 2018

Contributor

YES!

@bsalamat

Thanks a lot, @wgliang, @lichuqiang, and @k82cn for your reviews. They made this PR better. I tried to address them all and sent two fixup commits for your review.

// NodeTree is a tree-like data structure that holds node names in each zone. Zone names are
// keys to "NodeTree.tree" and values of "NodeTree.tree" are arrays of node names.
type NodeTree struct {
tree map[string]*nodeArray // a map from zone (region-zone) to an array of nodes in the zone.

This comment has been minimized.

@bsalamat

bsalamat Jul 28, 2018

Contributor

That's a good idea to make the key configurable. Given the size of this PR, I think that would be a good follow-on PR.

@bsalamat

bsalamat Jul 28, 2018

Contributor

That's a good idea to make the key configurable. Given the size of this PR, I think that would be a good follow-on PR.

Show outdated Hide outdated pkg/scheduler/cache/node_tree.go
Show outdated Hide outdated pkg/scheduler/core/generic_scheduler.go

@bsalamat bsalamat added the lgtm label Aug 17, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 17, 2018

@jimangel

This comment has been minimized.

Show comment
Hide comment
@jimangel

jimangel Aug 17, 2018

Member

@bsalamat does this need/have a PR against the 1.12 docs branch? Looks like a major change. Thanks!

Member

jimangel commented Aug 17, 2018

@bsalamat does this need/have a PR against the 1.12 docs branch? Looks like a major change. Thanks!

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 17, 2018

Contributor

One comment about the flag. Otherwise sgtm.

Contributor

sttts commented Aug 17, 2018

One comment about the flag. Otherwise sgtm.

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Aug 17, 2018

Contributor

@jimangel Yes, I will write/update docs.

Contributor

bsalamat commented Aug 17, 2018

@jimangel Yes, I will write/update docs.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 17, 2018

Contributor

/lgtm
/approve

Contributor

sttts commented Aug 17, 2018

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 17, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, fejta, k82cn, sttts

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 17, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, fejta, k82cn, sttts

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

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 17, 2018

Contributor

New changes are detected. LGTM label has been removed.

Contributor

k8s-ci-robot commented Aug 17, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 17, 2018

@bsalamat bsalamat added the lgtm label Aug 17, 2018

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 17, 2018

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Aug 17, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@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. 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. If you want to cherry-pick this change to another branch, please follow the instructions here.

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

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation bsalamat 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

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

Merge pull request #67555 from wgliang/opt/improve-performance
Automatic merge from submit-queue (batch tested with PRs 67555, 68196). 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.

Not split nodes when searching for nodes but doing it all at once

**What this PR does / why we need it**:
Not split nodes when searching for nodes but doing it all at once.

**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 #

**Special notes for your reviewer**:
@bsalamat 

This is a follow up PR of #66733.

#66733 (comment)

**Release note**:

```release-note
Not split nodes when searching for nodes but doing it all at once.
```

k8s-publishing-bot added a commit to kubernetes/client-go that referenced this pull request Sep 4, 2018

Merge pull request #67555 from wgliang/opt/improve-performance
Automatic merge from submit-queue (batch tested with PRs 67555, 68196). 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.

Not split nodes when searching for nodes but doing it all at once

**What this PR does / why we need it**:
Not split nodes when searching for nodes but doing it all at once.

**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 #

**Special notes for your reviewer**:
@bsalamat

This is a follow up PR of #66733.

kubernetes/kubernetes#66733 (comment)

**Release note**:

```release-note
Not split nodes when searching for nodes but doing it all at once.
```

Kubernetes-commit: a0b457d0e5ed54646fd01eac877efcea5be3216d

sttts pushed a commit to sttts/client-go that referenced this pull request Sep 5, 2018

Merge pull request #67555 from wgliang/opt/improve-performance
Automatic merge from submit-queue (batch tested with PRs 67555, 68196). 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.

Not split nodes when searching for nodes but doing it all at once

**What this PR does / why we need it**:
Not split nodes when searching for nodes but doing it all at once.

**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 #

**Special notes for your reviewer**:
@bsalamat

This is a follow up PR of #66733.

kubernetes/kubernetes#66733 (comment)

**Release note**:

```release-note
Not split nodes when searching for nodes but doing it all at once.
```

Kubernetes-commit: a0b457d0e5ed54646fd01eac877efcea5be3216d

sttts pushed a commit to sttts/client-go that referenced this pull request Sep 5, 2018

Merge pull request #67555 from wgliang/opt/improve-performance
Automatic merge from submit-queue (batch tested with PRs 67555, 68196). 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.

Not split nodes when searching for nodes but doing it all at once

**What this PR does / why we need it**:
Not split nodes when searching for nodes but doing it all at once.

**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 #

**Special notes for your reviewer**:
@bsalamat

This is a follow up PR of #66733.

kubernetes/kubernetes#66733 (comment)

**Release note**:

```release-note
Not split nodes when searching for nodes but doing it all at once.
```

Kubernetes-commit: a0b457d0e5ed54646fd01eac877efcea5be3216d

k8s-publishing-bot added a commit to kubernetes/client-go that referenced this pull request Sep 6, 2018

Merge pull request #67555 from wgliang/opt/improve-performance
Automatic merge from submit-queue (batch tested with PRs 67555, 68196). 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.

Not split nodes when searching for nodes but doing it all at once

**What this PR does / why we need it**:
Not split nodes when searching for nodes but doing it all at once.

**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 #

**Special notes for your reviewer**:
@bsalamat

This is a follow up PR of #66733.

kubernetes/kubernetes#66733 (comment)

**Release note**:

```release-note
Not split nodes when searching for nodes but doing it all at once.
```

Kubernetes-commit: a0b457d0e5ed54646fd01eac877efcea5be3216d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment