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.

Sign up for GitHub

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

Jump to bottom

refactor: remove configurator in scheduler #108790

Merged
merged 2 commits into from
Mar 20, 2022
Merged

refactor: remove configurator in scheduler #108790

merged 2 commits into from
Mar 20, 2022

Conversation

kerthcet
Copy link
Member

Signed-off-by: kerthcet kerthcet@gmail.com

What type of PR is this?

/kind cleanup
/sig scheduling

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part 1 of #108788

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 18, 2022
@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 18, 2022
@k8s-ci-robot
Copy link
Contributor

@kerthcet: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 18, 2022
@kerthcet
Copy link
Member Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 18, 2022
kerthcet
kerthcet commented Mar 18, 2022
View reviewed changes
pkg/scheduler/scheduler.go
frameworkruntime.WithClientSet(client),
frameworkruntime.WithKubeConfig(options.kubeConfig),
frameworkruntime.WithInformerFactory(informerFactory),
frameworkruntime.WithSnapshotSharedLister(snapshot),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove frameworkruntime.WithRunAllFilters(c.alwaysCheckAllPredicates) here as c.alwaysCheckAllPredicates is always false now. I also create a issue #108770 to discuss about whether we should remove this field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we can remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you cleanup WithRunAllFilters and runAllFilters from the framework too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still be discussed in issue #108770 , if we come to an agreement, I'll create another pr to follow this.

@kerthcet
Copy link
Member Author

/retest

2 similar comments
@kerthcet
Copy link
Member Author

/retest

@kerthcet
Copy link
Member Author

/retest

@kerthcet kerthcet marked this pull request as ready for review March 18, 2022 07:51
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2022
@kerthcet
Copy link
Member Author

/cc @ahg-g

ahg-g
ahg-g reviewed Mar 19, 2022
View reviewed changes
pkg/scheduler/factory.go Outdated
c.nodeInfoSnapshot,
c.percentageOfNodesToScore)
return sched, nil
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally would like to keep it as more clearly to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the confusion that you try to clear? it is not typical to explicitly return at the end of a function that returns nothing.

Copy link
Member

@ahg-g ahg-g Mar 20, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, you have named return variables. So the best practice is not to use named variables on long functions, can you please explicitly define them in the function and in the return statements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to follow the practice: The names are not mandatory but they can make code shorter and clearer: they're documentation in https://go.dev/doc/effective_go#named-results.
Updated then.

pkg/scheduler/scheduler.go
frameworkruntime.WithClientSet(client),
frameworkruntime.WithKubeConfig(options.kubeConfig),
frameworkruntime.WithInformerFactory(informerFactory),
frameworkruntime.WithSnapshotSharedLister(snapshot),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we can remove it

pkg/scheduler/factory.go Outdated Show resolved Hide resolved
@kerthcet
Copy link
Member Author

/retest

@ahg-g
Copy link
Member

ahg-g commented Mar 20, 2022

thanks, can you please squash.

@kerthcet
refactor: remove configurator in scheduler
5ecaeb3 
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet
remove unused Binder struct
1cf535f 
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet kerthcet force-pushed the refactor/remove-configurator-in-scheduler branch from 76ff537 to 1cf535f Compare March 20, 2022 15:58
@kerthcet
Copy link
Member Author

squashed.

@ahg-g
Copy link
Member

ahg-g commented Mar 20, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, kerthcet

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2022
@ahg-g
Copy link
Member

ahg-g commented Mar 20, 2022

another followup is to re-organize the code as follows:

  1. remove factory.go file and move the remaining functions to scheduler.go.
  2. add a new file named schedule_one.go, which would include the scheduleOne func and all other local functions it invokes.

With this, scheduler.go would be the file the includes the instantiation logic, while schedule_one.go includes the scheduling logic.

@k8s-ci-robot k8s-ci-robot merged commit 3439589 into kubernetes:master Mar 20, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 20, 2022
@kerthcet kerthcet deleted the refactor/remove-configurator-in-scheduler branch March 21, 2022 03:02
@kerthcet kerthcet mentioned this pull request Mar 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denkensk denkensk denkensk left review comments

@ahg-g ahg-g ahg-g left review comments

@chendave chendave Awaiting requested review from chendave

@Huang-Wei Huang-Wei Awaiting requested review from Huang-Wei

Assignees

@ahg-g ahg-g

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kerthcet @k8s-ci-robot @ahg-g @denkensk