TAS: introduce scheduler-level tests #3361

mimowo · 2024-10-29T15:17:21Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

To do not work-around accounting for TAS pods and workloads.

Which issue(s) this PR fixes:

Part of #2724

Special notes for your reviewer:

This was ticked as "Account for usage from TAS workloads in the unit tests for cache" in the spreadsheet.
I'm going to expand the tests in follow up tasks.
I moved already the most "workaround"-ish test, but I could move tests in the future, let me know.

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2024-10-29T15:17:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

netlify · 2024-10-29T15:17:38Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`76b817c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6721ce1b14dff7000882bbf2

mimowo · 2024-10-29T15:17:51Z

/cc @PBundyra @tenzen-y

mimowo · 2024-10-29T17:10:53Z

pkg/cache/clusterqueue.go

 	if in.Spec.Preemption != nil {
 		c.Preemption = *in.Spec.Preemption
 	} else {
 		c.Preemption = defaultPreemption
 	}

+	c.UpdateWithFlavors(resourceFlavors)


FYI: The new unit test discovered this subtle bug - the UpdateWithFlavors requires the Preemption to be set, so that the check for Preemption.withinClusterQueue != "Never" is correct (otherwise it is "", so considered not equal, and the CQ is inactivated).

I checked this bug in the flow (c.UpdateWithFlavors() -> c.updateQueueStatus() -> c.isTASViolated()) and (c.updateWithAdmissionChecks() -> c.updateQueueStatus() -> c.isTASViolated()).

mimowo · 2024-10-30T09:12:17Z

pkg/cache/tas_cache_test.go

@@ -805,51 +801,6 @@ func TestFindTopologyAssignment(t *testing.T) {
 				},
 			},
 		},
-		"don't double-count TAS pods when computing the capacity": {


FYI this scenario is moved to the new suite as "workload gets scheduled as the usage of TAS pods and workloads is not double-counted", since it was tricky to account for the Workloads' usage in this setup (see comment below). So the test used to create a "TAS" Pod, but it didn't belong to any TAS workload. The new suite allows to account for Workloads and Pods.

mimowo · 2024-10-30T14:58:01Z

/cc @mbobrovskyi

mbobrovskyi · 2024-10-30T15:23:20Z

/lgtm

Thanks!

k8s-ci-robot · 2024-10-30T15:23:28Z

LGTM label has been added.

Git tree hash: 05b191c81bebb6a951f3d178648642045d9b82eb

tenzen-y · 2024-10-30T15:41:01Z

pkg/cache/clusterqueue.go

 	if in.Spec.Preemption != nil {
 		c.Preemption = *in.Spec.Preemption
 	} else {
 		c.Preemption = defaultPreemption
 	}

+	c.UpdateWithFlavors(resourceFlavors)


I checked this bug in the flow (c.UpdateWithFlavors() -> c.updateQueueStatus() -> c.isTASViolated()) and (c.updateWithAdmissionChecks() -> c.updateQueueStatus() -> c.isTASViolated()).

tenzen-y · 2024-10-30T15:54:30Z

pkg/scheduler/scheduler_test.go

+
+			ctx, cancel := context.WithTimeout(ctx, queueingTimeout)
+			go qManager.CleanUpOnContext(ctx)
+			defer cancel()


Suggested change

defer cancel()

t.Cleanup(cancel())

yeah, it was copy-pasted from here. I think it warrants a cleanup PR which fixes all tests, otherwise people will continue to copy-paste the old pattern. Opened: #3382

tenzen-y · 2024-10-30T16:01:07Z

pkg/scheduler/scheduler_test.go

+					EventType: corev1.EventTypeNormal,
+				},
+			},
+		},


What if more than one flavor was assigned to a single resource like

kueue/pkg/scheduler/flavorassigner/tas_flavorassigner.go

Line 84 in 5ce02ea

} else if *result != v.Name {

?

This check was added proactively. I'm not sure how to trigger it e2e (or if this is possible). I will investigate separately, added a point in the spreadsheet as "Investigate if possible to have two flavors for the same resource"

Thank you for adding this to the sheet.

* TAS: introduce scheduled-level tests * some test cleanups

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 Oct 29, 2024

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 29, 2024

k8s-ci-robot requested review from denkensk and mbobrovskyi October 29, 2024 15:17

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2024

k8s-ci-robot requested review from PBundyra and tenzen-y October 29, 2024 15:17

mimowo changed the title ~~TAS: introduce scheduled-level tests~~ WIP: TAS: introduce scheduled-level tests Oct 29, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024

mimowo changed the title ~~WIP: TAS: introduce scheduled-level tests~~ WIP: TAS: introduce scheduler-level tests Oct 29, 2024

mimowo force-pushed the tas-scheduler-level-tests branch from 0660f4e to 39bb53e Compare October 29, 2024 16:39

mimowo changed the title ~~WIP: TAS: introduce scheduler-level tests~~ TAS: introduce scheduler-level tests Oct 29, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024

mimowo changed the title ~~TAS: introduce scheduler-level tests~~ WIP: TAS: introduce scheduler-level tests Oct 29, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024

TAS: introduce scheduled-level tests

441cecd

mimowo force-pushed the tas-scheduler-level-tests branch from 39bb53e to 441cecd Compare October 29, 2024 17:09

mimowo changed the title ~~WIP: TAS: introduce scheduler-level tests~~ TAS: introduce scheduler-level tests Oct 29, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024

mimowo commented Oct 29, 2024

View reviewed changes

some test cleanups

76b817c

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2024

mimowo mentioned this pull request Oct 30, 2024

TAS: Skip RFs which don't match PodSet TAS requirements #3373

Merged

mimowo commented Oct 30, 2024

View reviewed changes

k8s-ci-robot assigned mbobrovskyi Oct 30, 2024

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

k8s-ci-robot merged commit 5ce02ea into kubernetes-sigs:main Oct 30, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Oct 30, 2024

tenzen-y reviewed Oct 30, 2024

View reviewed changes

mimowo mentioned this pull request Nov 4, 2024

Cleanup use of deref cancel in tests #3382

Closed

kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024

TAS: introduce scheduler-level tests (kubernetes-sigs#3361)

cc6dbef

* TAS: introduce scheduled-level tests * some test cleanups

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TAS: introduce scheduler-level tests #3361

TAS: introduce scheduler-level tests #3361

mimowo commented Oct 29, 2024 •

edited

Loading

k8s-ci-robot commented Oct 29, 2024

netlify bot commented Oct 29, 2024 •

edited

Loading

mimowo commented Oct 29, 2024

mimowo Oct 29, 2024

tenzen-y Oct 30, 2024

mimowo Oct 30, 2024 •

edited

Loading

mimowo commented Oct 30, 2024

mbobrovskyi commented Oct 30, 2024

k8s-ci-robot commented Oct 30, 2024

tenzen-y Oct 30, 2024

tenzen-y Oct 30, 2024

mimowo Oct 30, 2024

tenzen-y Oct 30, 2024

mimowo Oct 31, 2024

tenzen-y Oct 31, 2024

TAS: introduce scheduler-level tests #3361

TAS: introduce scheduler-level tests #3361

Conversation

mimowo commented Oct 29, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 29, 2024

netlify bot commented Oct 29, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mimowo commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

mimowo commented Oct 30, 2024

mbobrovskyi commented Oct 30, 2024

k8s-ci-robot commented Oct 30, 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

mimowo commented Oct 29, 2024 •

edited

Loading

netlify bot commented Oct 29, 2024 •

edited

Loading

mimowo Oct 30, 2024 •

edited

Loading