Use Kueue API types in FlavorAssignerTest #2486

gabesaba · 2024-06-27T10:31:05Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We update the FlavorAssignerTest to use Kueue API types, rather than construct CQ/Cohort snapshot manually. This will allow us to change CQ/Cohort Snapshot logic without updating the tests each time.

Preparation for main changes in #79

Does this PR introduce a user-facing change?

NONE

netlify · 2024-06-27T10:31:28Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`005c37f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6683e70109167b00070f0c83

gabesaba · 2024-06-27T11:23:26Z

@macsko

gabesaba · 2024-06-27T11:41:23Z

pkg/scheduler/flavorassigner/flavorassigner_test.go

-				}.Unflatten(),
+				},
+				requestableResources: resources.FlavorResourceQuantitiesFlat{
+					{Flavor: "one", Resource: corev1.ResourceCPU}:  2000,


note to reviewers: This test started failing once borrowing limit was added (limitation of the Resource constructor - which requires you to specify borrowing limit to set lending limit)

I think the original test didn't match its intention: since previously there was no borrowing limit, the cq wasn't looking for capacity in its cohort, only looking in cq. I changed this value to 2000m, and specified 10000m borrowing limit.

I'm not convinced, if there is no borrowing limit, then the cq is free to borrow within the entire cq.
Would it be feasible to have a new dedicated constructor to let using borrowingLimit =0, so that we are sure we don't drop some coverage?

+1. nil borrowingLimit means "borrow any amount". Please revert numbers.

Sorry, my explaination was wrong, but the test was still broken. See explaination here #2486 (comment)

macsko

Looks good, two small comments

pkg/scheduler/flavorassigner/flavorassigner_test.go

gabesaba · 2024-06-27T15:13:12Z

Looks good, two small comments

thanks for the review

mimowo · 2024-06-28T13:29:37Z

pkg/scheduler/flavorassigner/flavorassigner_test.go

@@ -2373,6 +1993,19 @@ func TestAssignFlavors(t *testing.T) {
 	}
 }

+func updateResources(dest resources.FlavorResourceQuantities, src resources.FlavorResourceQuantitiesFlat) {


Can we use the pre-existing Unflatten function for this, like clusterQueue.Usage = tc.clusterQueueUsage.Unflatten()?

I could do this in all but one case - a test breaks unfortunately. But I don't like this approach either. We should clean this up in a future PR - by calling some public method of cache or snapshot to get the values we want, rather than manipulating the internal accounting of the CQs/Cohorts

Created #2502

Actually, was able to get rid of guaranteedQuota, as this value is now updated in cache logic.

pkg/scheduler/flavorassigner/flavorassigner_test.go

mimowo · 2024-06-28T13:51:04Z

pkg/scheduler/flavorassigner/flavorassigner_test.go

+				ResourceGroup(
+					utiltesting.MakeFlavorQuotas("one").
+						Resource(corev1.ResourcePods, "10").
+						Resource(corev1.ResourceCPU, "10000m", "1000", "1000m").


This borrowing limit "1000" looks suspicious, IIUC it is unbound in the deleted code, which is slightly different semantically, Would there be a way to express "unbound" now? Otherwise maybe Max int 32, but this may mean that we no longer cover cases when borrowingLimit==nil. (same question for some test cases below)

updated the tests to match original semantics

mimowo

Thanks, nice preparatory pr which will make the following PRs easier to review!

mimowo · 2024-06-28T16:15:46Z

/lgtm
/approve
/hold
in case @alculquicondor wants to give a pass

k8s-ci-robot · 2024-06-28T16:15:52Z

LGTM label has been added.

Git tree hash: d4dd3ceb413f316b130f7d4714b22633bd9853a0

k8s-ci-robot · 2024-06-28T16:15:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabesaba, mimowo

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:

~~OWNERS~~ [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/scheduler/flavorassigner/flavorassigner_test.go

pkg/util/testing/wrappers.go

alculquicondor · 2024-06-28T16:10:07Z

pkg/scheduler/flavorassigner/flavorassigner_test.go

-				}.Unflatten(),
+				},
+				requestableResources: resources.FlavorResourceQuantitiesFlat{
+					{Flavor: "one", Resource: corev1.ResourceCPU}:  2000,


+1. nil borrowingLimit means "borrow any amount". Please revert numbers.

pkg/scheduler/flavorassigner/flavorassigner_test.go

alculquicondor · 2024-06-28T16:17:08Z

pkg/scheduler/flavorassigner/flavorassigner_test.go

+		wlReclaimablePods           []kueue.ReclaimablePod
+		clusterQueue                kueue.ClusterQueue
+		clusterQueueUsage           resources.FlavorResourceQuantitiesFlat
+		clusterQueueGuaranteedQuota resources.FlavorResourceQuantitiesFlat


this should not be necessary. It should be calculated by the cache logic based on the lendingLimit

after removal, a bunch of tests fail. we can clean up in #2502

Maybe some tests didn't put the right LendingLimit.
Ok.

I was able to remove guaranteedQuota; I'm not sure what was happening last week (unless I thought you were referring to a different field?)

alculquicondor · 2024-06-28T17:10:56Z

pkg/scheduler/flavorassigner/flavorassigner_test.go

+					{Flavor: "one", Resource: corev1.ResourceCPU}: 2_000,
+				},
+				requestableResources: resources.FlavorResourceQuantitiesFlat{
+					{Flavor: "one", Resource: corev1.ResourceCPU}:  2_000,


It looks like this was 10_000 before

This test was previously broken/didn't make sense, as it set up the internal state incorrectly. With a lending limit of 0, it should have also set guaranteed quota to 10. I made the minimal update to make it work again. After my change to use the cache, it was failing as the workload actually fit.

I am going to rename, by s/lendinglimit/quota//

I'm pretty sure this test was added when the lendingLimit feature was added. So we are probably missing some coverage on it now. @kerthcet do you recall what was the intention of the test and can you send a PR to fix it?

I'll take a look later. Thanks for reminding, almost missed it.

I couldn't find the case, can anyone point that out？

pkg/scheduler/flavorassigner/flavorassigner_test.go

mimowo · 2024-07-02T11:02:18Z

LGTM, just one nit to rename the test

gabesaba · 2024-07-02T11:39:59Z

LGTM, just one nit to rename the test

Done. Thanks for the review!

mimowo · 2024-07-02T12:10:17Z

/lgtm

k8s-ci-robot · 2024-07-02T12:10:22Z

LGTM label has been added.

Git tree hash: cc59312abc3a5ed7512193dcc10f8fc4bbc5ca0e

mimowo · 2024-07-02T12:12:31Z

/hold cancel
I think we are good to merge as all comments are addressed. We can follow up in case @alculquicondor has another comment.

* Define ResourceQuotaWrapper; improve documentation * Update FlavorAssignerTest to use ClusterQueue API types * Add tests for deleted flavor * Improve readability of test quantities

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 27, 2024

k8s-ci-robot requested review from denkensk and mimowo June 27, 2024 10:31

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 27, 2024

gabesaba commented Jun 27, 2024

View reviewed changes

gabesaba mentioned this pull request Jun 27, 2024

Delete RGByResource map #2489

Closed

macsko reviewed Jun 27, 2024

View reviewed changes

pkg/scheduler/flavorassigner/flavorassigner_test.go Outdated Show resolved Hide resolved

pkg/scheduler/flavorassigner/flavorassigner_test.go Outdated Show resolved Hide resolved

gabesaba force-pushed the refactor_flavor_resource_test branch from 9ecf0a4 to ba89f13 Compare June 27, 2024 15:12

mimowo reviewed Jun 28, 2024

View reviewed changes

pkg/scheduler/flavorassigner/flavorassigner_test.go Show resolved Hide resolved

mimowo reviewed Jun 28, 2024

View reviewed changes

pkg/scheduler/flavorassigner/flavorassigner_test.go Show resolved Hide resolved

mimowo reviewed Jun 28, 2024

View reviewed changes

gabesaba force-pushed the refactor_flavor_resource_test branch from ba89f13 to 2e5eeeb Compare June 28, 2024 15:36

gabesaba mentioned this pull request Jun 28, 2024

cleanup manipulation of CQ/Cohort resource accounting in tests #2502

Closed

gabesaba force-pushed the refactor_flavor_resource_test branch from 2e5eeeb to 2917889 Compare June 28, 2024 15:53

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2024

k8s-ci-robot assigned mimowo Jun 28, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2024

alculquicondor reviewed Jun 28, 2024

View reviewed changes

Define ResourceQuotaWrapper; improve documentation

d7f71b3

gabesaba force-pushed the refactor_flavor_resource_test branch from 2917889 to 9f0b496 Compare June 28, 2024 16:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2024

k8s-ci-robot requested a review from mimowo June 28, 2024 16:57

alculquicondor reviewed Jun 28, 2024

View reviewed changes

pkg/scheduler/flavorassigner/flavorassigner_test.go Outdated Show resolved Hide resolved

gabesaba force-pushed the refactor_flavor_resource_test branch from 9f0b496 to d9a5e72 Compare July 2, 2024 10:36

mimowo reviewed Jul 2, 2024

View reviewed changes

pkg/scheduler/flavorassigner/flavorassigner_test.go Outdated Show resolved Hide resolved

gabesaba added 3 commits July 2, 2024 11:37

Update FlavorAssignerTest to use ClusterQueue API types

8c94d7e

Add tests for deleted flavor

2c55601

Improve readability of test quantities

005c37f

gabesaba force-pushed the refactor_flavor_resource_test branch from d9a5e72 to 005c37f Compare July 2, 2024 11:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024

k8s-ci-robot merged commit 8f3bfcb into kubernetes-sigs:main Jul 2, 2024
16 checks passed

k8s-ci-robot added this to the v0.8 milestone Jul 2, 2024

gabesaba deleted the refactor_flavor_resource_test branch July 9, 2024 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Kueue API types in FlavorAssignerTest #2486

Use Kueue API types in FlavorAssignerTest #2486

gabesaba commented Jun 27, 2024 •

edited

Loading

netlify bot commented Jun 27, 2024 •

edited

Loading

gabesaba commented Jun 27, 2024

gabesaba Jun 27, 2024

mimowo Jun 28, 2024

alculquicondor Jun 28, 2024

gabesaba Jul 2, 2024

macsko left a comment

gabesaba commented Jun 27, 2024

mimowo Jun 28, 2024

gabesaba Jun 28, 2024

gabesaba Jul 2, 2024

mimowo Jun 28, 2024 •

edited

Loading

gabesaba Jun 28, 2024

mimowo left a comment

mimowo commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

alculquicondor Jun 28, 2024

alculquicondor Jun 28, 2024

gabesaba Jun 28, 2024

alculquicondor Jun 28, 2024

gabesaba Jul 2, 2024

alculquicondor Jun 28, 2024

gabesaba Jul 2, 2024

alculquicondor Jul 2, 2024

kerthcet Jul 3, 2024

kerthcet Jul 8, 2024

alculquicondor Jul 8, 2024

mimowo commented Jul 2, 2024

gabesaba commented Jul 2, 2024

mimowo commented Jul 2, 2024

k8s-ci-robot commented Jul 2, 2024

mimowo commented Jul 2, 2024

Use Kueue API types in FlavorAssignerTest #2486

Use Kueue API types in FlavorAssignerTest #2486

Conversation

gabesaba commented Jun 27, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

netlify bot commented Jun 27, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

gabesaba commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macsko left a comment

Choose a reason for hiding this comment

gabesaba commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

mimowo commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo commented Jul 2, 2024

gabesaba commented Jul 2, 2024

mimowo commented Jul 2, 2024

k8s-ci-robot commented Jul 2, 2024

mimowo commented Jul 2, 2024

gabesaba commented Jun 27, 2024 •

edited

Loading

netlify bot commented Jun 27, 2024 •

edited

Loading

mimowo Jun 28, 2024 •

edited

Loading