-
Notifications
You must be signed in to change notification settings - Fork 235
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
Add test case for TryNextFlavor but flavor is full #2482
Add test case for TryNextFlavor but flavor is full #2482
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/hold |
7742587
to
8a11c4f
Compare
/hold cancel However, it's still useful for coverage :) |
/assign @gabesaba |
/retest |
pkg/scheduler/scheduler_test.go
Outdated
}). | ||
ResourceGroup( | ||
*utiltesting.MakeFlavorQuotas("on-demand"). | ||
Resource(corev1.ResourceCPU, "0", "60").Obj(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider using ResourceQuotaWrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/scheduler/scheduler_test.go
Outdated
ResourceGroup( | ||
*utiltesting.MakeFlavorQuotas("on-demand"). | ||
Resource(corev1.ResourceCPU, "30", "0").Obj(), | ||
*utiltesting.MakeFlavorQuotas("spot"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/scheduler/scheduler_test.go
Outdated
Cohort("cohort"). | ||
ResourceGroup( | ||
*utiltesting.MakeFlavorQuotas("on-demand"). | ||
Resource(corev1.ResourceCPU, "30", "0").Obj(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete borrowing limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 is not the same as not defined, but it doesn't matter for this CQ, so removed.
Request(corev1.ResourceCPU, "22"). | ||
Obj(), | ||
}, | ||
deleteWorkloads: []kueue.Workload{*utiltesting.MakeWorkload("alpha2", "default").Obj()}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is deleteWorkloads testing anything useful in this case, or can we omit it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It marks the termination of alpha2, so that the incoming workload can be admitted.
Change-Id: I289e632d1d21a2fe5f49b524d191bb63ecf6824c
8a11c4f
to
bcb0a40
Compare
/lgtm |
LGTM label has been added. Git tree hash: 1fabedde51e32a6522c90c488933d265ccd4ee29
|
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adds test case for undertested feature of flavor fungibility when
whenCanBorrow
andwhenCanPreempt
are both set toTryNextFlavor
.In the test case, it's not possible to borrow or preempt in the second flavor, but it's possible to preempt in the first flavor.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?