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-2131] Add ready-to-use example for peemption #723

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

chenyulin0719
Copy link
Contributor

@chenyulin0719 chenyulin0719 commented Nov 9, 2023

What is this PR for?

Add an immediately usable example to reduce the learning curve for users.

The example shows how to config priority queue and demonstrates below behaviors of preemption:

  1. A task cannot preempt tasks outside its preemption fence.
  2. Preemption is aiming to ensure that the queue's resource usage reaches at least the guaranteed amount of resources.
  3. Preemption can never leave a queue lower than its guaranteed capacity.

What type of PR is it?

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

Todos

NA

What is the Jira issue?

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

How should this be tested?

Follow the example steps in yunikorn-k8shim/deployments/examples/preemption/README.md

Screenshots (if appropriate)

NA

Questions:

NA

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d2b8ebd) 71.89% compared to head (37a6886) 71.89%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #723   +/-   ##
=======================================
  Coverage   71.89%   71.89%           
=======================================
  Files          49       49           
  Lines        7956     7956           
=======================================
  Hits         5720     5720           
  Misses       2039     2039           
  Partials      197      197           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

LGTM. It is a good example to show how to perform preemption. Thanks for the contribution.

Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@chenyulin0719 this is a great patch. one small question left.

- name: sandbox
resources:
max:
{memory: 1000Mi, vcore: 1000m}
Copy link
Member

Choose a reason for hiding this comment

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

Is this max required for the demo? If not, it seems we can remove this queue sandbox

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is. Without max resources setting, the available resource depends on users' nodes and it will be hard to determine pod resource requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @chia7712 and @FrankYang0529 's review.
Yes, the max resource is required, otherwise the preemption won't be triggered if the queues have enough resources.

Copy link
Member

Choose a reason for hiding this comment

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

Got it and thanks for sharing. I will test it manually later.

Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

@chia7712 chia7712 merged commit 05bc647 into apache:master Dec 29, 2023
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.

3 participants