[YUNIKORN-2795] Handle Preemption cases between two siblings without … #944
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #944 +/- ##
==========================================
+ Coverage 80.41% 80.43% +0.02%
==========================================
Files 97 97
Lines 12431 12437 +6
==========================================
+ Hits 9996 10004 +8
+ Misses 2164 2163 -1
+ Partials 271 270 -1 ☔ View full report in Codecov by Sentry. |
pbacsko
reviewed
Aug 14, 2024
Comment on lines
+27
to
+41
| var smallestRes = resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5}) | ||
| var childRes = resources.NewResourceFromMap(map[string]resources.Quantity{"second": 5}) | ||
| var smallestResPlusChildRes = resources.Add(smallestRes, childRes) | ||
| var smallestResDouble = resources.Multiply(smallestRes, 2) | ||
| var smallestResDoublePlusChildRes = resources.Add(resources.Multiply(smallestRes, 2), childRes) | ||
| var smallestResMultiplyByZero = resources.NewResourceFromMap(map[string]resources.Quantity{"first": 0}) | ||
| var smallestResMultiplyByMinusOne = resources.Multiply(smallestRes, -1) | ||
| var smallestResMultiplyByMinusOnePlusChildRes = resources.Add(resources.Multiply(smallestRes, -1), childRes) | ||
| var childResMultiplyByZero = resources.NewResourceFromMap(map[string]resources.Quantity{"second": 0}) | ||
| var childResMultiplyByMinusOne = resources.Multiply(childRes, -1) | ||
| var childResDouble = resources.Multiply(childRes, 2) | ||
| var smallestResDoublePlusChildResDouble = resources.Add(resources.Multiply(smallestRes, 2), childResDouble) | ||
| var smallestResPlusChildResDouble = resources.Add(smallestRes, childResDouble) | ||
| var smallestResMultiplyByZeroPluschildResMultiplyByZero = resources.Add(smallestResMultiplyByZero, childResMultiplyByZero) | ||
| var smallestResMultiplyByZeroPluschildResMultiplyByMinusOne = resources.Add(smallestResMultiplyByZero, childResMultiplyByMinusOne) |
There was a problem hiding this comment.
Wouldn't it be better to just create the resource map in-place wherever it's needed? These variables barely reveal what they contain, the names are way too long sometimes.
There was a problem hiding this comment.
Agree with this, please simplify.
There was a problem hiding this comment.
Simplified the variable names like usual test variables. Couple of jiras YUNIKORN-2848 & YUNIKORN-2824 has been already raised which would re-use these variables eventually.
|
This needs a rebase on master now that SubOnlyExisting() has been updated. |
manirajv06
force-pushed
the
YUNIKORN-2795-1
branch
from
9d8607a to
3704db9
Compare
craigcondit
requested changes
Aug 21, 2024
Comment on lines
+27
to
+41
| var smallestRes = resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5}) | ||
| var childRes = resources.NewResourceFromMap(map[string]resources.Quantity{"second": 5}) | ||
| var smallestResPlusChildRes = resources.Add(smallestRes, childRes) | ||
| var smallestResDouble = resources.Multiply(smallestRes, 2) | ||
| var smallestResDoublePlusChildRes = resources.Add(resources.Multiply(smallestRes, 2), childRes) | ||
| var smallestResMultiplyByZero = resources.NewResourceFromMap(map[string]resources.Quantity{"first": 0}) | ||
| var smallestResMultiplyByMinusOne = resources.Multiply(smallestRes, -1) | ||
| var smallestResMultiplyByMinusOnePlusChildRes = resources.Add(resources.Multiply(smallestRes, -1), childRes) | ||
| var childResMultiplyByZero = resources.NewResourceFromMap(map[string]resources.Quantity{"second": 0}) | ||
| var childResMultiplyByMinusOne = resources.Multiply(childRes, -1) | ||
| var childResDouble = resources.Multiply(childRes, 2) | ||
| var smallestResDoublePlusChildResDouble = resources.Add(resources.Multiply(smallestRes, 2), childResDouble) | ||
| var smallestResPlusChildResDouble = resources.Add(smallestRes, childResDouble) | ||
| var smallestResMultiplyByZeroPluschildResMultiplyByZero = resources.Add(smallestResMultiplyByZero, childResMultiplyByZero) | ||
| var smallestResMultiplyByZeroPluschildResMultiplyByMinusOne = resources.Add(smallestResMultiplyByZero, childResMultiplyByMinusOne) |
There was a problem hiding this comment.
Agree with this, please simplify.
manirajv06
force-pushed
the
YUNIKORN-2795-1
branch
from
b972092 to
ad11714
Compare
|
This failure should go away with apache/yunikorn-k8shim#897. Triggering the workflows to get a clean build. |
apache/yunikorn-k8shim#897 merged; I've kicked off the build on the PR again to verify. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…causing preemption loop
What is this PR for?
Different preemption cases between siblings under common under guaranteed parent needs to be handled properly without causing preemption storm or loop. Please refer https://docs.google.com/document/d/1vfv8XJJsIqlZN3ecyhP2EFeiUSQnY-9GHEi_UJFB-BU/edit for more details.
A fixable case without causing storm or loop is freeing up resources from victim child queue (not guaranteed set) to under guaranteed preemptor who is starving for resources located under common under guaranteed parent. This case has been discussed in the doc under section 2a)
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2795
How should this be tested?
Screenshots (if appropriate)
Questions: