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.

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

[YUNIKORN-2107] Allow preemption to be disabled globally #690

Closed
wants to merge 1 commit into from

Conversation

craigcondit
Copy link
Contributor

@craigcondit craigcondit commented Nov 2, 2023

What is this PR for?

Added configuration service.disablePreemption which can be used to disable the preemption subsystem entirely when set to true.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2107

How should this be tested?

Added unit tests.

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@wilfred-s
Copy link
Contributor

Why not use the existing parameter in the partition config for this? (See jira for details also)

@craigcondit craigcondit force-pushed the YUNIKORN-2107 branch 3 times, most recently from d7e4332 to 30f65a6 Compare November 3, 2023 17:27
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #690 (0642f8a) into master (47ca6fe) will increase coverage by 0.05%.
Report is 6 commits behind head on master.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #690      +/-   ##
==========================================
+ Coverage   77.57%   77.63%   +0.05%     
==========================================
  Files          80       80              
  Lines       13133    13271     +138     
==========================================
+ Hits        10188    10303     +115     
- Misses       2619     2648      +29     
+ Partials      326      320       -6     
Files Coverage Δ
pkg/common/configs/config.go 100.00% <ø> (ø)
pkg/scheduler/objects/application.go 68.29% <100.00%> (ø)
pkg/scheduler/partition.go 78.58% <100.00%> (+0.16%) ⬆️
pkg/scheduler/objects/queue.go 77.66% <0.00%> (ø)

... and 9 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@craigcondit
Copy link
Contributor Author

Why not use the existing parameter in the partition config for this? (See jira for details also)

Because then we end up with opt-in preemption rather than opt out as there's no distinction in the existing configuration between Enabled: false and not specified. That's backwards-incompatible.

Managed to get this working by changing the type of the Enabled field in the config to *bool, so we can detect unset values and default to true.

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

+1

@manirajv06 manirajv06 closed this in ca73344 Nov 6, 2023
@craigcondit craigcondit deleted the YUNIKORN-2107 branch November 6, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants