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 scheduler package to enable preemption for batch/service jobs #5545

Merged
merged 2 commits into from
Apr 12, 2019
Merged

Refactor scheduler package to enable preemption for batch/service jobs #5545

merged 2 commits into from
Apr 12, 2019

Conversation

preetapan
Copy link
Contributor

No functional changes, this moves some code around for batch/service job preemption

endocrimes
endocrimes approved these changes Apr 11, 2019
View reviewed changes
Copy link
Contributor

@endocrimes endocrimes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, one nit and a design question, but neither are blocking

scheduler/generic_sched.go Outdated
@@ -501,6 +501,7 @@ func (s *GenericScheduler) computePlacements(destructive, place []placementResul
AllocatedResources: resources,
DesiredStatus: structs.AllocDesiredStatusRun,
ClientStatus: structs.AllocClientStatusPending,

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rm extra newline?

scheduler/stack_oss.go
package scheduler

// NewGenericStack constructs a stack used for selecting service placements
func NewGenericStack(batch bool, ctx Context) *GenericStack {
Copy link
Contributor

@endocrimes endocrimes Apr 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do we potentially want to have a way to modify the stack, rather than duping the whole setup? (mostly worried about duplicate work when introducing other new scheduler things that could easily be forgotten to be also PR'd to ENT)

@preetapan preetapan merged commit 176b48c into master Apr 12, 2019
@endocrimes endocrimes deleted the f-preemption-scheduler-refactor branch April 15, 2019 08:34
@notnoop
Copy link
Contributor

notnoop commented Jun 5, 2019

This PR fixes a panic that result when preempter considers a new alloc that's part of the plan in https://github.com/hashicorp/nomad/pull/5545/files#diff-4b31728e0a7a88025f9d7c26ecce3e5eR152 . This is observed in #5777 .

notnoop pushed a commit that referenced this pull request Jun 5, 2019
update changelog for GH-5545
f1dc5a6
@notnoop notnoop mentioned this pull request Jun 5, 2019
Closed
@github-actions
Copy link

github-actions bot commented Feb 9, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@endocrimes endocrimes endocrimes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@preetapan @notnoop @endocrimes