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

[WIP] support rescheduling based on realtime performance #2092

Closed
wants to merge 6 commits into from
Closed

[WIP] support rescheduling based on realtime performance #2092

wants to merge 6 commits into from

Conversation

Thor-wl
Copy link
Contributor

@Thor-wl Thor-wl commented Mar 16, 2022

@volcano-sh-bot volcano-sh-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2022
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign thor-wl
You can assign the PR to them by writing /assign @thor-wl in a comment when ready.

The full list of commands accepted by this bot can be found 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

@volcano-sh-bot volcano-sh-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 16, 2022
@Thor-wl Thor-wl changed the title [WIP] support rescheduling based on real time load [WIP] support rescheduling based on realtime performance Mar 16, 2022
@Thor-wl Thor-wl requested review from william-wang and removed request for huone1 March 16, 2022 02:48
k82cn
k82cn reviewed Mar 16, 2022
View reviewed changes
pkg/scheduler/actions/shuffle/shuffle.go
}
}
}
for _, task := range tasks {
Copy link
Member

@k82cn k82cn Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

let's check log level before go through all tasks to improve the performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Let me change the level to v5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

k82cn
k82cn reviewed Mar 16, 2022
View reviewed changes
pkg/scheduler/actions/shuffle/shuffle.go
victims := ssn.Victims(tasks)
for victim, _ := range victims {
klog.V(4).Infof("Victim %s: [ns: %s, job: %s]\n", victim.Name, victim.Namespace, victim.Job)
if err := ssn.Evict(victim, "shuffle"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

let's have a const value for "shuffle".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

k82cn
k82cn reviewed Mar 16, 2022
View reviewed changes
pkg/scheduler/api/node_info.go
@@ -34,6 +34,11 @@ func (o *AllocateFailError) Error() string {
return o.Reason
}

type NodeUsage struct {
Copy link
Member

Choose a reason for hiding this comment

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

prefer to seperate this one into two PRs: shuffle action & rescheduling plugin.

Copy link
Contributor Author

@Thor-wl Thor-wl Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In order to make the implementation looks complete and work as a whole feature, how about separate the PR into more commits and each commit foucs a relative smaller and independent functions?

Copy link
Member

Choose a reason for hiding this comment

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

In order to make the implementation looks complete and work as a whole feature, how about separate the PR into 2 commits?

shuffle action is a common feature which is not related to rescheduling. TDM plugin also use this action :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. That's reasonable. It's ready now.

Copy link
Member

Choose a reason for hiding this comment

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

let's have a seperate PR for shuffle action, not a commit.

@Thor-wl Thor-wl closed this Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k82cn k82cn k82cn left review comments

@shinytang6 shinytang6 Awaiting requested review from shinytang6

@william-wang william-wang Awaiting requested review from william-wang

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Thor-wl @volcano-sh-bot @k82cn