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-1601] Shim: Implement preemption predicate handling #542

Closed
wants to merge 1 commit into from

Conversation

craigcondit
Copy link
Contributor

@craigcondit craigcondit commented Feb 24, 2023

What is this PR for?

Add PreemptionPredicates() RM callback implementation to support preemption in core.

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-1601

How should this be tested?

Manually verified that algorithm functions when combined with apache/yunikorn-core#511.
Basic unit tests.
Will add e2e tests once the core implementation is finalized.

Screenshots (if appropriate)

Questions:

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

@craigcondit craigcondit self-assigned this Feb 24, 2023
@craigcondit craigcondit changed the title [YUNIKORN-1601] [WIP] Shim: Implement preemption predicate handling [YUNIKORN-1601] Shim: Implement preemption predicate handling Feb 27, 2023
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #542 (1e8e4d2) into master (9389241) will decrease coverage by 0.32%.
The diff coverage is 34.28%.

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   69.56%   69.24%   -0.32%     
==========================================
  Files          45       45              
  Lines        7731     7801      +70     
==========================================
+ Hits         5378     5402      +24     
- Misses       2156     2199      +43     
- Partials      197      200       +3     
Impacted Files Coverage Δ
pkg/cache/context.go 41.06% <0.00%> (-1.36%) ⬇️
pkg/plugin/predicates/predicate_manager.go 51.42% <55.81%> (+0.69%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@craigcondit craigcondit marked this pull request as ready for review February 28, 2023 20:50
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.

Need unit test for IsPodFitNodeViaPreemption().

@craigcondit
Copy link
Contributor Author

Need unit test for IsPodFitNodeViaPreemption().

Again, same as above, this would require mocking out a substantial amount of the underlying code, which doesn't really verify that it works properly. As with IsPodFitNode() (which this mirrors) we will test this primarily with e2e tests.

@craigcondit craigcondit requested a review from pbacsko March 3, 2023 15:04
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.

Questioms, concerns have been addressed.

+1 LGTM

@craigcondit craigcondit deleted the YUNIKORN-1601 branch March 3, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants