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

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

Merged
merged 1 commit into from Sep 4, 2018

Conversation

@wgliang
Member

wgliang commented Aug 18, 2018

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:

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

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Aug 18, 2018

Member

/sig scheduling

Member

wgliang commented Aug 18, 2018

/sig scheduling

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Aug 22, 2018

Member

/assign @bsalamat
PTAL

Member

wgliang commented Aug 22, 2018

/assign @bsalamat
PTAL

@misterikkit

I don't think we should try too hard to mimic the Parallelize API that is being replaced here. If there's a pattern that works better for us here, let's go with it.

@misterikkit

I don't think we should try too hard to mimic the Parallelize API that is being replaced here. If there's a pattern that works better for us here, let's go with it.

I apologize. I didn't notice that you were adding the util in client-go. Given that, it makes lots of sense to mimic the Parallelize API.

Indeed, I understand what you mean, maybe a callback function can do the same, just like bsalamat said. And using Context may be too complicated for a public function.

I don't believe that Context should be considered "too complicated" for a public function. In this case, it offers features that we don't need, so a stop channel would also work. (Passing a stop channel around is a very common pattern in k8s, but it's an old pattern that really ought to be replaced with Context.)

Show outdated Hide outdated pkg/scheduler/core/generic_scheduler.go
@@ -413,24 +419,23 @@ func (g *genericScheduler) findNodesThatFit(pod *v1.Pod, nodes []*v1.Node) ([]*v
}
if fits {

This comment has been minimized.

@bsalamat

bsalamat Aug 23, 2018

Contributor

If we re-write this if statement as below, we can change line 377 to filtered = make([]*v1.Node, numNodesToFind). This saves us some memory.

if fits {
	len := atomic.AddInt32(&filteredLen, 1)
	if len > numNodesToFind {
		cancel()
	} else {
		filtered[len - 1] = g.cachedNodeInfoMap[nodeName].Node()		
	}
}
@bsalamat

bsalamat Aug 23, 2018

Contributor

If we re-write this if statement as below, we can change line 377 to filtered = make([]*v1.Node, numNodesToFind). This saves us some memory.

if fits {
	len := atomic.AddInt32(&filteredLen, 1)
	if len > numNodesToFind {
		cancel()
	} else {
		filtered[len - 1] = g.cachedNodeInfoMap[nodeName].Node()		
	}
}

This comment has been minimized.

@wgliang

wgliang Aug 23, 2018

Member

DONE.

@wgliang

wgliang Aug 23, 2018

Member

DONE.

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Aug 24, 2018

Contributor

/retest

Contributor

bsalamat commented Aug 24, 2018

/retest

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Aug 27, 2018

Member

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

Member

wgliang commented Aug 27, 2018

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

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Aug 28, 2018

Member

/test pull-kubernetes-bazel-build

Member

wgliang commented Aug 28, 2018

/test pull-kubernetes-bazel-build

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Aug 28, 2018

Member

/ping @bsalamat @k82cn
for approve

Member

wgliang commented Aug 28, 2018

/ping @bsalamat @k82cn
for approve

@bsalamat

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 28, 2018

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat
Contributor

bsalamat commented Aug 28, 2018

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Aug 28, 2018

Member

LGTM, thanks :)

Member

k82cn commented Aug 28, 2018

LGTM, thanks :)

"sync"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
)
type DoWorkPieceFunc func(piece int)
// Parallelize is a very simple framework that allow for parallelizing
// Parallelize is a very simple framework that allows for parallelizing
// N independent pieces of work.
func Parallelize(workers, pieces int, doWorkPiece DoWorkPieceFunc) {

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

does it make sense to express this through the new one?

@sttts

sttts Aug 28, 2018

Contributor

does it make sense to express this through the new one?

This comment has been minimized.

@wgliang

wgliang Aug 28, 2018

Member

What do you think it can be? I have no idea.

@wgliang

wgliang Aug 28, 2018

Member

What do you think it can be? I have no idea.

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

Isn't Parallelize(...) = ParallelizeUntil(nil, ...)?

@sttts

sttts Aug 28, 2018

Contributor

Isn't Parallelize(...) = ParallelizeUntil(nil, ...)?

This comment has been minimized.

@wgliang

wgliang Aug 28, 2018

Member

Emmm, Parallelize(...) = ParallelizeUntil(nil, ...), there will be a separate PR that will replace all Parallelize with ParallelizeUntil.
just like misterikkit said:#67555 (comment)

@wgliang

wgliang Aug 28, 2018

Member

Emmm, Parallelize(...) = ParallelizeUntil(nil, ...), there will be a separate PR that will replace all Parallelize with ParallelizeUntil.
just like misterikkit said:#67555 (comment)

This comment has been minimized.

@sttts

sttts Sep 3, 2018

Contributor

Replacing the call is not what I mean. Why does this PR copy the implementation instead of calling the more flexible ParallelizeUntil?

@sttts

sttts Sep 3, 2018

Contributor

Replacing the call is not what I mean. Why does this PR copy the implementation instead of calling the more flexible ParallelizeUntil?

This comment has been minimized.

@wgliang

wgliang Sep 3, 2018

Member

@sttts The purpose of ParallelizeUntil is to allow the caller to control through the context. This PR is already using ParallelizeUntil.
The existence of Parallelize is temporary, as other packages are still using Parallelize. After this PR, I will submit a PR delete Parallelize separately.
I don't know if you understand what I mean. :)

@wgliang

wgliang Sep 3, 2018

Member

@sttts The purpose of ParallelizeUntil is to allow the caller to control through the context. This PR is already using ParallelizeUntil.
The existence of Parallelize is temporary, as other packages are still using Parallelize. After this PR, I will submit a PR delete Parallelize separately.
I don't know if you understand what I mean. :)

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

@Huang-Wei Huang-Wei referenced this pull request Aug 28, 2018

Closed

REQUEST: New membership for Huang-Wei #47

6 of 6 tasks complete
@misterikkit

As a side note, it would be nice to rewrite the commit description and PR description to more clearly explain the change.

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Aug 30, 2018

Member

@sttts @bsalamat @misterikkit
Ready for further review. :)

Member

wgliang commented Aug 30, 2018

@sttts @bsalamat @misterikkit
Ready for further review. :)

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Sep 1, 2018

Contributor

/assign @sttts

for another round of review and approval

Contributor

bsalamat commented Sep 1, 2018

/assign @sttts

for another round of review and approval

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Sep 3, 2018

Contributor

/lgtm
/approve

For client-go changes.

Contributor

sttts commented Sep 3, 2018

/lgtm
/approve

For client-go changes.

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 3, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 3, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Sep 3, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Sep 3, 2018

Contributor

/retest

Contributor

bsalamat commented Sep 3, 2018

/retest

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 4, 2018

Contributor

New changes are detected. LGTM label has been removed.

Contributor

k8s-ci-robot commented Sep 4, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 4, 2018

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Sep 4, 2018

Member

/retest

Member

wgliang commented Sep 4, 2018

/retest

@sttts sttts added the lgtm label Sep 4, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 4, 2018

Contributor

@wgliang: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

Contributor

k8s-ci-robot commented Sep 4, 2018

@wgliang: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Sep 4, 2018

Member

/ping @bsalamat
for milestone v1.12

Member

wgliang commented Sep 4, 2018

/ping @bsalamat
for milestone v1.12

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Sep 4, 2018

Contributor

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.

Contributor

k8s-merge-robot commented Sep 4, 2018

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.

@k8s-merge-robot k8s-merge-robot merged commit a0b457d into kubernetes:master Sep 4, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation wgliang 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment