Refactor computation of TAS assignments #4200

mimowo · 2025-02-10T12:26:17Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #3761

Preparatory for #4171

Special notes for your reviewer:

The main change in this PR is to refactor how the TAS assignment is computed when there are multiple PodSets. Previously the usage from previous podsets was already accounted regardless of the final scheduling result, see here. This works well only assuming the FindTopologyAssignment function is called once.

However, to support preemptions I will need to run this function multiple times for a different set of target workloads. Similarly, this assumption is also invalid for cohorts.

So I introduce a concept of assumedUsage which is tracked within one FindTopologyAssignment called across all PodSets targeting a flavor, see here.

This change also improves the error message, which I extracted to yet another preparatory PR: #4204, so that we can decide about cherry-picking independently.

Does this PR introduce a user-facing change?

NONE

netlify · 2025-02-10T12:26:41Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`70517e2`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67ac961458d6c600087338b3

mimowo · 2025-02-10T12:26:45Z

/cc @gabesaba @PBundyra PTAL

k8s-ci-robot · 2025-02-10T12:26:49Z

@mimowo: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @gabesaba @PBundyra PTAL

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pkg/scheduler/flavorassigner/tas_flavorassigner.go

mimowo · 2025-02-11T08:48:31Z

Rebased after #4204 was merged

pkg/scheduler/flavorassigner/tas_flavorassigner.go

pkg/scheduler/flavorassigner/flavorassigner.go

pkg/cache/tas_flavor_snapshot.go

pkg/scheduler/flavorassigner/tas_flavorassigner.go

PBundyra · 2025-02-11T12:44:26Z

pkg/cache/tas_flavor_snapshot.go

@@ -223,6 +223,56 @@ func (s *TASFlavorSnapshot) addTASUsage(domainID utiltas.TopologyDomainID, usage
 	s.leaves[domainID].tasUsage.Add(usage)
 }

+type TASPodSetRequests struct {


Is there a need to introduce a new type if we already have a similar one PodSetResources

Good point, there is no strict need, we could think about consolidating them, but there are subtle differences:

PodSetResources.Requests is the aggregated requests while TASPodSetRequests.SinglePodRequests is per pod

PodSetResources.Flavors is a resource -> Flavor map while for TAS we can have only one flavor per PodSet

So the question, how would we reconcile that? Should PodSetResources, some ideas for (1.):

make the PodSetResources to internally keep SinglePodRequests and have a function with TotalRequests

keep both SinglePodRequests and TotalRequets redudantly

make PodSetResources keep total requests internally and derive SinglePodRequests in a function

I think for (2.) it is quite straighforward that we would fit into the "Flavors" field, and just have a helper function OnlyFlavor which would be called in the TAS context.

WDYT? I'm ok to make the changes in this PR or in a follow up.

keep both SinglePodRequests and TotalRequets redudantly

Do we really need this? I guess single pod requests can be represented as TotalRequest[idx].Requests / TotalRequest[idx].Count?

Is there a reason why we prepare just the helper function? I can imagine the number of TotalRequest is smaller since it indicates the number of PodSet. At most, it is 4 like (pre-processing, driver, worker, post-processing).

Do we really need this?

I don't think so, it is one of the options in my previous comment. Other options I mentioned is to keep only one field and derive the other (by dividing or multiplication) in a function. I'm ok with either approach.

I can give it a try today in this PR, but if I encounter issues I would prefer to leave it for follow up to unblock the functional PRs (which is still preemption within ClusterQueue and cohort support).

I can give it a try today in this PR, but if I encounter issues I would prefer to leave it for follow up to unblock the functional PRs (which is still preemption within ClusterQueue and cohort support).

Separate PR sounds good to me.

I just realized there is one more complication: PodSetResources does not reference kueue.PodSet, I add it in TASPodSetRequests as ptr. This is needed for example as input with tolerations for the algorithm. So, I think we would need to also extend PodSetResources with *kueue.PodSet.

I'm for it, but indeed prefer a follow up,

Added the refactoring to the tracking spreadsheet as "commonize the structures for PodSetResources and TASPodSetRequests"

pkg/scheduler/flavorassigner/flavorassigner.go

PBundyra · 2025-02-11T12:53:38Z

pkg/scheduler/flavorassigner/flavorassigner.go

+	}
+
+	if features.Enabled(features.TopologyAwareScheduling) {
+		tasRequests := assignment.TopologyRequestsFor(a.wl, requests, a.cq)


WDYT about calling this function inside a.cq.FindTopologyAssignments

not so simple, because it needs access to the assignment to update it which is the receiver. Unless we refactor stuff somehow, but I'm not sure how.

pkg/scheduler/flavorassigner/flavorassigner.go

pkg/cache/clusterqueue_snapshot.go

tenzen-y

Thank you!
/approve
/lgtm

/hold for @PBundyra

pkg/scheduler/flavorassigner/tas_flavorassigner.go

tenzen-y · 2025-02-12T13:10:23Z

pkg/scheduler/flavorassigner/tas_flavorassigner.go

+		return nil, err
+	}
+	if cq.TASFlavors[*tasFlvr] == nil {
+		return nil, errors.New("workload requires Topology, but there is no TAS cache information for the assigned flavor")


Here, let us implement UTs similar to above. I will add this followup list issue.

Added to the tracking spreadsheet

tenzen-y · 2025-02-12T13:11:44Z

pkg/scheduler/flavorassigner/flavorassigner.go

+			// clear flavors to make the PodSet assignment as NoFit
+			psAssignment.Flavors = nil
+			// update the representative mode accordingly
+			assignment.representativeMode = ptr.To(NoFit)


Oh, you already have addressed, thanks!

k8s-ci-robot · 2025-02-12T13:12:46Z

LGTM label has been added.

Git tree hash: dbe54bd4c15a267e6666b7a591c9f1eb48a7a9ea

k8s-ci-robot · 2025-02-12T13:12:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, tenzen-y

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,tenzen-y]

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

mimowo · 2025-02-12T13:54:36Z

Thank you! /approve /lgtm

/hold for @PBundyra

I believe all comments are either addressed on added to the tracking spreadsheet. I will work on them in follow ups unless other contributors want to help.

tenzen-y · 2025-02-12T14:00:03Z

Thank you! /approve /lgtm
/hold for @PBundyra

I believe all comments are either addressed on added to the tracking spreadsheet. I will work on them in follow ups unless other contributors want to help.

In case of having an agreement with @PBundyra, I'm ok with upholding it. Idk the relationship between you and Patryk in your team, though

mimowo · 2025-02-12T14:02:44Z

In case of having an agreement with @PBundyra, I'm ok with upholding it.

Thanks, let's wait for Patryk :)

Idk the relationship between you and Patryk in your team, though

We are team mates, desk to desk :)

tenzen-y · 2025-02-12T14:03:25Z

In case of having an agreement with @PBundyra, I'm ok with upholding it. Idk the relationship between you and Patryk in your team, though

Sounds great :)

PBundyra · 2025-02-12T15:57:34Z

/unhold

Thanks for waiting for me :)

* Refactor computation of TAS assignments * review remarks

tenzen-y · 2025-02-19T16:11:39Z

/cherry-pick release-0.10

Preparing for #4322

k8s-infra-cherrypick-robot · 2025-02-19T16:12:16Z

@tenzen-y: #4200 failed to apply on top of branch "release-0.10":

Applying: Refactor computation of TAS assignments
Using index info to reconstruct a base tree...
M	pkg/cache/clusterqueue_snapshot.go
M	pkg/cache/tas_flavor_snapshot.go
M	pkg/scheduler/flavorassigner/tas_flavorassigner.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/scheduler/flavorassigner/tas_flavorassigner.go
CONFLICT (content): Merge conflict in pkg/scheduler/flavorassigner/tas_flavorassigner.go
Auto-merging pkg/cache/tas_flavor_snapshot.go
Auto-merging pkg/cache/clusterqueue_snapshot.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Refactor computation of TAS assignments

In response to this:

/cherry-pick release-0.10

Preparing for #4322

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tenzen-y · 2025-02-19T16:58:30Z

/cherry-pick release-0.10

k8s-infra-cherrypick-robot · 2025-02-19T16:59:10Z

@tenzen-y: new pull request created: #4327

In response to this:

/cherry-pick release-0.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 10, 2025

k8s-ci-robot requested review from mbobrovskyi and PBundyra February 10, 2025 12:26

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 Feb 10, 2025

k8s-ci-robot requested a review from gabesaba February 10, 2025 12:26

mimowo mentioned this pull request Feb 10, 2025

TAS: Improve error message for TAS workloads when no flavor assigned #4204

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2025