TAS: Stop implicitly assuming ResrouceFlavor:Node = 1:N by tenzen-y · Pull Request #11210 · kubernetes-sigs/kueue

tenzen-y · 2026-05-15T05:53:37Z

What type of PR is this?

/kind bug
/area tas

What this PR does / why we need it:

I fixed a TAS over-subscription bug where multiple ResourceFlavor referencing the same Topology and overlapping on physical nodes each independently track usage, so sibling flavors believe a shared node is only partially used.

This PR refined the TAS cache so that sibling flavors aggregate usage.

Which issue(s) this PR fixes:

Fixes #10659

Special notes for your reviewer:

#10657 is problem reproduced PR.

Does this PR introduce a user-facing change?

TAS: fix over-subscription of nodes that belong to multiple ResourceFlavors sharing the same hostname-leaf Topology.

k8s-ci-robot · 2026-05-15T05:53:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2026-05-15T05:53:43Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`8bfa984`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/6a0b47b591dd030008c0210d

k8s-ci-robot · 2026-05-15T05:53:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

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

tenzen-y · 2026-05-15T05:53:47Z

/test all

tenzen-y · 2026-05-15T07:06:05Z

/test all

tenzen-y · 2026-05-15T11:58:10Z

/hold
for reviewers

mimowo · 2026-05-15T12:00:12Z

cc @pajakd @PBundyra ptal

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

pajakd · 2026-05-18T06:33:47Z

 	if features.Enabled(features.TopologyAwareScheduling) {
-		for flavor, cache := range c.tasCache.Clone() {
-			tasSnapshots[flavor] = cache.snapshot(log, c.tasCache.nodesCache.find(cache.flavor.NodeLabels, cache.topology.Levels))
+		flavorClone := c.tasCache.Clone()


Can we move it closer to its first usage?

I addressed it in 5290147

pajakd · 2026-05-18T07:31:04Z

+	// One shared assumedUsage map per TopologyName for this workload. PodSets
+	// landing on different sibling flavors (same Topology, hostname leaf)
+	// reserve against the same map, preventing intra-workload self-overlap on
+	// a shared physical node. Cache-write-time aggregation does not cover
+	// in-flight reservations because pending workloads have not hit addUsage.


This is tested in the test "multi-PodSet workload across sibling flavors must not self-overlap on shared node", right?

Yes, exactly, right.

pajakd · 2026-05-18T08:19:37Z

+				}
+			})
+
+			ginkgo.It("should admit the pending workload to tas-flavor-b on the free node when tas-flavor-a is full", func() {


How would this test fail without the fix?

This is repro PR: https://github.com/kubernetes-sigs/kueue/pull/10657/changes#r3118973170

pajakd · 2026-05-18T08:22:51Z

+			wantCalls:     3,
+			wantCollected: map[string]int{"a": 1, "b": 2, "c": 3},
+		},
+		"early termination: stops on first false": {


shouldn't we check the value of wantCollected in this test? This is the only test where it should differ from seed, no?

Oh, good point. You're right.

I fixed this in 70b8434

The seed is map. So, the expectation is not stable. Hence, I added a verification if the collected map key-value pairs match with seed's one.

pajakd · 2026-05-18T08:58:19Z

 	return old, existed
 }
+
+func (dwc *SyncMap[K, V]) Range(f func(key K, value V) bool) {


Since this function holds only rlock, if someone would try to call it with f(k,v) that modifies the underlying map, it could be dangerous (deadlock?). Shouldn't we add a comment warning about this?

Yes, good point.
I'm wondering if we should take RW lock instead of RLock here.

Any preference adding comments vs taking RW lock?

I just added a comment in 8bfa984

Because Writable Range will cause performance regressions for syncMap.

pajakd · 2026-05-18T09:26:15Z

+		for flavor, cache := range flavorClone {
+			nodes := c.tasCache.nodesCache.find(cache.flavor.NodeLabels, cache.topology.Levels)
+			var sharedUsage map[utiltas.TopologyDomainID]resources.Requests
+			if cache.topology.Usage != nil {


In the loop above we don't initialize the sharedSnapshotUsage[topologyName] if usageLength == 0. But here we don't check for that. But if a topology has 0 usage, it might still need the shared map (for example if the first workload has multiple podsets)?

Good catch, we should initialize that when sharedSnapshotUsage[topologyName] was not initialized in the previous loop block due to the usageLength.

I addressed this in 1be5bd2

The sharedSnapshotUsage[topologyName] is always initialized even when the length of usageLength is zero.

pajakd · 2026-05-18T10:33:52Z

 		tasFlavorCache := c.TASFlavors[tasFlavor]
-		flvResult := tasFlavorCache.FindTopologyAssignmentsForFlavor(flavorTASRequests, options...)
+		flvOpts := options
+		if tasFlavorCache != nil && tasFlavorCache.isLowestLevelNode {


if tasFlavorCache would be nil it would panic in the line below this if. The comment above says that "tasFlavor is already in the snapshot" so I think tasFlavorCache != nil should either be removed entirely or changed to a guard clause.

That totally makes sense. Let's remove tasFlavorCache != nil because we can expect that it has already been checked as described in the above code comment.

I address it in 3afdebb

…cMapRange UT Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y · 2026-05-18T17:11:51Z

@pajakd Thank you for your review, I addressed all your comments, PTAL, thank you 🙏

mimowo · 2026-05-18T18:20:28Z

@tenzen-y please rebase and I think it would be good to squeeze the commits in case we cherrypick

k8s-ci-robot · 2026-05-18T18:20:39Z

PR needs rebase.

Details

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.

mimowo · 2026-05-18T18:21:31Z

+		c.tasCache.RLock()
+		for topologyName, info := range c.tasCache.topologies {
+			if info.Usage == nil {
+				continue
+			}
+			clonedUsage := make(map[utiltas.TopologyDomainID]resources.Requests, info.Usage.Len())
+
+			info.Usage.Range(func(domainID utiltas.TopologyDomainID, req resources.Requests) bool {
+				clonedUsage[domainID] = req.Clone()
+				return true
+			})
+			sharedSnapshotUsage[topologyName] = clonedUsage
+		}
+		c.tasCache.RUnlock()


Exract this code to a helper function so that we can use the Lock, defer UnLock pattern

mimowo

Given the amount of non-trivial changes I would consider a feature gate for the fix, like TASHandleOverlappingFlavors

mimowo · 2026-05-18T18:23:24Z

cc @Ladicle maybe could also give it a pass who already have pretty solid understanding of TAS

k8s-ci-robot requested a review from olekzabl May 15, 2026 05:53

k8s-ci-robot requested a review from pajakd May 15, 2026 05:53

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 15, 2026

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 15, 2026

tenzen-y force-pushed the allow-multiple-flavors-for-node branch from bcba589 to f5595ef Compare May 15, 2026 07:05

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 15, 2026

tenzen-y marked this pull request as ready for review May 15, 2026 07:35

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 May 15, 2026

tenzen-y force-pushed the allow-multiple-flavors-for-node branch from f5595ef to f1c6d1d Compare May 15, 2026 07:35

k8s-ci-robot requested review from mbobrovskyi and mimowo May 15, 2026 07:35

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

TAS: Stop implicitly assuming ResrouceFlavor:Node = 1:N

4ab4a93

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the allow-multiple-flavors-for-node branch from f1c6d1d to 4ab4a93 Compare May 15, 2026 15:24

pajakd reviewed May 18, 2026

View reviewed changes

tenzen-y added 4 commits May 19, 2026 01:19

Verify wantCollected in 'early termination: stops on first false' Syn…

70b8434

…cMapRange UT Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

Move flavorClone to closed to used place in scheduler cache snapshot

5290147

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

sharedSnapshotUsage always shoud be initialized with clonsedUsage

1be5bd2

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

Remove unnecessary nil check for TASFlavors

3afdebb

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the allow-multiple-flavors-for-node branch from bb9dd0a to 3afdebb Compare May 18, 2026 17:05

SyncMap: Add a comment to describe the Write limitation

8bfa984

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

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

mimowo reviewed May 18, 2026

View reviewed changes

Conversation

tenzen-y commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

k8s-ci-robot commented May 15, 2026

Uh oh!

netlify Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

k8s-ci-robot commented May 15, 2026

Uh oh!

tenzen-y commented May 15, 2026

Uh oh!

tenzen-y commented May 15, 2026

Uh oh!

tenzen-y commented May 15, 2026

Uh oh!

mimowo commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pajakd May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y commented May 18, 2026

Uh oh!

mimowo commented May 18, 2026

Uh oh!

k8s-ci-robot commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo left a comment

Choose a reason for hiding this comment

Uh oh!

mimowo commented May 18, 2026

Uh oh!

tenzen-y commented May 15, 2026 •

edited

Loading

netlify Bot commented May 15, 2026 •

edited

Loading

tenzen-y May 18, 2026 •

edited

Loading

pajakd May 18, 2026 •

edited

Loading