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 users of `factory.NewConfigFactory` to `scheduler.New` #71875

Merged
merged 1 commit into from Jan 9, 2019

Conversation

@wgliang
Copy link
Member

wgliang commented Dec 8, 2018

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #71859

Special notes for your reviewer:
/sig scheduling

Does this PR introduce a user-facing change?:

Move users of `factory.NewConfigFactory` to `scheduler.New`.
@mysunshine92

This comment has been minimized.

Copy link
Contributor

mysunshine92 commented Dec 8, 2018

/sig scheduling

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 8, 2018

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup and removed needs-kind labels Dec 8, 2018

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 8, 2018

/retest

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch 2 times, most recently from c0c3267 to aed0d7f Dec 8, 2018

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 8, 2018

/retest

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 9, 2018

@misterikkit
note:I am afraid that scheduler.NewFromConfig needs to be kept public, and there are still scenarios for external use, such as integration testing.

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Dec 10, 2018

there are still scenarios for external use

How much work do you think it will be to move integration tests to use New?

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 11, 2018

@misterikkit There are some integration like TestSchedulerCreationFromConfigMap tests for create scheduler form config. If we private NewFromConfig that these code maybe useless.

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Dec 11, 2018

Hmm, I see your point. Can we merge this PR but keep the issue open? I'd like us to explore what it would take to get rid of 'NewFromConfig'

@@ -36,7 +36,6 @@ import (
restclient "k8s.io/client-go/rest"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"
schedulerapp "k8s.io/kubernetes/cmd/kube-scheduler/app"
schedulerappconfig "k8s.io/kubernetes/cmd/kube-scheduler/app/config"

This comment has been minimized.

@misterikkit

misterikkit Dec 11, 2018

Contributor

Any chance we can refactor this test to not import cmd/kube-scheduler? It looks like it's only used for constructing the scheduler object.

This comment has been minimized.

@wgliang

wgliang Dec 11, 2018

Author Member

Done. You are right, thanks. :)

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch from aed0d7f to 7628045 Dec 11, 2018

@misterikkit
Copy link
Contributor

misterikkit left a comment

Could you fix the git commit message? Otherwise,
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 12, 2018

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch from 7628045 to 2dea9d6 Dec 12, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 12, 2018

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch from fe6852a to 5f493c7 Dec 20, 2018

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 20, 2018

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 20, 2018

@misterikkit
Copy link
Contributor

misterikkit left a comment

/lgtm

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Dec 21, 2018

/ping @ixdy PTAL, thanks.

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch from 5f493c7 to cf42d92 Dec 30, 2018

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch 2 times, most recently from 04ae6f7 to df05d17 Dec 30, 2018

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Jan 3, 2019

/ping @ixdy PTAL, thanks.

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Jan 8, 2019

@kubernetes/sig-testing-pr-reviews need someone to review & approve of integration test changes.


_, err := schedulerapp.NewSchedulerConfig(ss.Complete())
// _, err := schedulerapp.NewSchedulerConfig(ss.Complete())

This comment has been minimized.

@misterikkit

misterikkit Jan 8, 2019

Contributor

I just noticed this line. Should it be deleted?

This comment has been minimized.

@wgliang

wgliang Jan 8, 2019

Author Member

Done. thanks.

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Jan 8, 2019

/approve

but I agree with @misterikkit's comment

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, ixdy, 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

@wgliang wgliang force-pushed the wgliang:cleanup/remove-newfrom-functions branch from df05d17 to 3c24c99 Jan 8, 2019

@wgliang

This comment has been minimized.

Copy link
Member Author

wgliang commented Jan 8, 2019

@ixdy Thanks.

Name: policyConfigMap.Name,
},

sched, err := scheduler.New(clientSet,

This comment has been minimized.

@krmayankk

krmayankk Jan 9, 2019

Contributor

we need this call in extender_test.go as well

This comment has been minimized.

@misterikkit

misterikkit Jan 9, 2019

Contributor

@krmayankk Could you be more specific? I don't see where in that test we instantiate a scheduler at all. (And given that those are unit tests, I'm not sure we need to.)

This comment has been minimized.

@krmayankk

krmayankk Jan 9, 2019

Contributor

extender_test.go uses initTestScheduler, which in turn finally calls scheduler.NewFromConfig() in test/integration/scheduler/util.go

This comment has been minimized.

@misterikkit

misterikkit Jan 10, 2019

Contributor

Ah, that's a good point. Looks like we'll need a follow-up PR to get test/integration/util onto the new constructor.

@misterikkit
Copy link
Contributor

misterikkit left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 9, 2019

@k8s-ci-robot k8s-ci-robot merged commit 2c8b571 into kubernetes:master Jan 9, 2019

19 checks passed

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-godeps 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
tide In merge pool.
Details
@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Jan 9, 2019

❤️ 🍾 😃

@bsalamat

This comment has been minimized.

Copy link
Contributor

bsalamat commented Jan 9, 2019

Awesome! Thanks, @wgliang and @misterikkit! 🎉

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Jan 11, 2019

I suspect this PR broke the following test.

make test-integration WHAT="./test/integration/scheduler_perf" KUBE_TEST_ARGS="-run xx -bench Node/500/250 "

misterikkit added a commit to misterikkit/kubernetes that referenced this pull request Jan 18, 2019

Set scheduler event recorder in tests.
We accidentally removed the Recorder object from integration test
scheduler in kubernetes#71875. This
caused segfaults when running scheduler benchmarks.

    make test-integration WHAT="./test/integration/scheduler_perf" KUBE_TEST_ARGS="-run xx -bench ."

This change adds the recorder back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment