Repository navigation
refactor: Reuse TAS topology trees across scheduler snapshots - #13819
Conversation
kubernetes-prow
Bot
added
kind/cleanup
|
Hi @akshay-pm. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
kubernetes-prow
Bot
added
cncf-cla: yes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTAS scheduling now reuses generation-aware topology trees and stores mutable placement state in individual snapshots. Balanced placement, capacity accounting, matching, cloning, pruning, tests, and benchmarks use snapshot-backed state. ChangesTAS snapshot reuse and placement state
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TASNodesCache
participant TASFlavorCache
participant topologyTree
participant TASFlavorSnapshot
participant BalancedPlacement
TASNodesCache->>TASFlavorCache: synchronize nodes and generation
TASFlavorCache->>topologyTree: reuse or rebuild topology tree
TASFlavorCache->>TASFlavorSnapshot: create isolated snapshot state
TASFlavorSnapshot->>BalancedPlacement: provide domain and capacity state
BalancedPlacement->>TASFlavorSnapshot: update assignment and pruning state
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
|
Cc @j-skiba |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/cache/scheduler/tas_flavor_snapshot_bench_test.go (2)
224-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the length-inequality proxy assertion.
The check fails when
len(snapshot.state)equalslen(snapshot.domains). It infers "state was cloned" from a length difference. That inference depends on an implementation detail of the cloning path, not on a stated invariant. A future change that clones state without changing its length would make this benchmark fail with a message that does not describe the real problem.Assert the property directly, or drop the check and rely on the placement result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cache/scheduler/tas_flavor_snapshot_bench_test.go` around lines 224 - 226, Remove the length-based clone assertion in the balanced placement preflight, since len(snapshot.state) versus len(snapshot.domains) does not directly verify cloning. Either assert the actual cloned-state property using an exposed identity/content invariant, or rely solely on the placement result; do not retain this proxy check or its misleading failure message.
228-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
b.ResetTimer()/b.StopTimer()calls aroundb.Loop().
b.Loop()already resets the timer before the first iteration and stops it when the loop ends, so these calls add no timing benefit.runBenchmarkTASFlavorSnapshotusesb.Loop()without them, so removing them keeps both benchmarks consistent.♻️ Proposed cleanup
- b.ResetTimer() for b.Loop() { result = snapshot.FindTopologyAssignmentsForFlavor(b.Context(), requests) } - b.StopTimer() if failure := result.Failure(); failure != nil { b.Fatalf("repeated balanced placement failed: %s", failure.Reason) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cache/scheduler/tas_flavor_snapshot_bench_test.go` around lines 228 - 232, Remove the redundant b.ResetTimer() and b.StopTimer() calls surrounding the b.Loop() in the benchmark using snapshot.FindTopologyAssignmentsForFlavor. Keep the b.Loop() iteration and result assignment unchanged, matching runBenchmarkTASFlavorSnapshot.pkg/cache/scheduler/tas_flavor_snapshot_test.go (1)
43-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the difference between
addDomainsWithStateandaddDomainWithState.This package now has two similarly named helpers with different aliasing behavior.
addDomainsWithStatecopiesspec.domaininto a new value, so a shared fixture can be registered in several snapshots safely.addDomainWithStateinpkg/cache/scheduler/tas_balanced_placement_test.gomutates the caller's*domainin place and returns the same pointer, so a shared fixture would be aliased across snapshots.Add a short doc comment on each helper that states the aliasing behavior. That prevents a future test from passing a shared
*domaintoaddDomainWithStateand getting cross-test state corruption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cache/scheduler/tas_flavor_snapshot_test.go` around lines 43 - 52, Add short doc comments to both addDomainsWithState and addDomainWithState describing their distinct aliasing behavior: addDomainsWithState copies each domain value before registering it, while addDomainWithState mutates and returns the caller’s existing pointer. Keep the comments focused on preventing shared-fixture aliasing across snapshots.pkg/cache/scheduler/tas_flavor_test.go (1)
142-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the equal-generation case in
storeTree.
storeTreereplaces the cached tree whentree.generation >= c.tree.generation, so a second tree at the same generation wins. No test pins that branch. If someone later changes>=to>, no test fails, and the tie behavior changes silently.♻️ Proposed additional assertion
if got := cache.cachedTree(); got != newer { t.Fatal("storeTree replaced the cached tree with an older tree") } + + sameGeneration := &topologyTree{generation: 2} + if got := cache.storeTree(sameGeneration); got != sameGeneration { + t.Fatal("storeTree did not adopt a tree at the same generation") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cache/scheduler/tas_flavor_test.go` around lines 142 - 156, Extend TestStoreTreeReturnsNewest to store a second topologyTree with the same generation as the cached tree and assert that storeTree returns and cachedTree expose this newer equal-generation tree, pinning the >= replacement behavior without changing the existing older-generation assertions.pkg/cache/scheduler/tas_flavor_snapshot.go (1)
1944-1949: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the lifetime rule for cloned domains.
The truncation makes every
*domainproduced byshallowCloneWithStateinvalid, because those clones carryidx >= len(s.domains). After this reslice,stateOf(staleClone)either panics or, once new clones are appended, aliases an unrelated clone's state.The invariant holds today:
fillInCountsruns at the start offindTopologyAssignment, all clones are created later in the same call, andbuildTopologyAssignmentForLevelscopies values out instead of returning*domain. The rule is enforced only by convention, so state it here.♻️ Proposed comment addition
// cleanup the state in case some remaining values are present from computing // assignments for previous PodSets. Truncating to discard the state // slots of domain copies made for the previous PodSet. + // + // This invalidates every *domain returned by shallowCloneWithState, whose + // idx points past len(s.domains). Cloned domains must not outlive the + // findTopologyAssignment call that created them. s.state = s.state[:len(s.domains)] clear(s.state)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cache/scheduler/tas_flavor_snapshot.go` around lines 1944 - 1949, Add a concise comment beside the state truncation in TASFlavorSnapshot.fillInCounts documenting that shallowCloneWithState results must not outlive this reset because their state indices are invalidated or may alias later clones. State that fillInCounts runs before creating clones and that downstream topology assignment copies values rather than returning clone pointers, preserving this lifetime convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cache/scheduler/tas_flavor_snapshot.go`:
- Around line 456-462: Update the comment above addNonTASUsage to state that
domainID comes from the topologyTree’s nodeToDomain map, while leaves is
populated from the same node set by newTopologyTree, guaranteeing the
corresponding leaf exists. Remove the outdated explanation tying the guarantee
to nodes having TAS pods; leave the dereference and surrounding logic unchanged.
---
Nitpick comments:
In `@pkg/cache/scheduler/tas_flavor_snapshot_bench_test.go`:
- Around line 224-226: Remove the length-based clone assertion in the balanced
placement preflight, since len(snapshot.state) versus len(snapshot.domains) does
not directly verify cloning. Either assert the actual cloned-state property
using an exposed identity/content invariant, or rely solely on the placement
result; do not retain this proxy check or its misleading failure message.
- Around line 228-232: Remove the redundant b.ResetTimer() and b.StopTimer()
calls surrounding the b.Loop() in the benchmark using
snapshot.FindTopologyAssignmentsForFlavor. Keep the b.Loop() iteration and
result assignment unchanged, matching runBenchmarkTASFlavorSnapshot.
In `@pkg/cache/scheduler/tas_flavor_snapshot_test.go`:
- Around line 43-52: Add short doc comments to both addDomainsWithState and
addDomainWithState describing their distinct aliasing behavior:
addDomainsWithState copies each domain value before registering it, while
addDomainWithState mutates and returns the caller’s existing pointer. Keep the
comments focused on preventing shared-fixture aliasing across snapshots.
In `@pkg/cache/scheduler/tas_flavor_snapshot.go`:
- Around line 1944-1949: Add a concise comment beside the state truncation in
TASFlavorSnapshot.fillInCounts documenting that shallowCloneWithState results
must not outlive this reset because their state indices are invalidated or may
alias later clones. State that fillInCounts runs before creating clones and that
downstream topology assignment copies values rather than returning clone
pointers, preserving this lifetime convention.
In `@pkg/cache/scheduler/tas_flavor_test.go`:
- Around line 142-156: Extend TestStoreTreeReturnsNewest to store a second
topologyTree with the same generation as the cached tree and assert that
storeTree returns and cachedTree expose this newer equal-generation tree,
pinning the >= replacement behavior without changing the existing
older-generation assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eb30ae1-dba1-4982-ae63-b7f585ad13f2
📒 Files selected for processing (12)
pkg/cache/scheduler/snapshot.gopkg/cache/scheduler/tas_balanced_placement.gopkg/cache/scheduler/tas_balanced_placement_test.gopkg/cache/scheduler/tas_cache_test.gopkg/cache/scheduler/tas_flavor.gopkg/cache/scheduler/tas_flavor_snapshot.gopkg/cache/scheduler/tas_flavor_snapshot_bench_test.gopkg/cache/scheduler/tas_flavor_snapshot_test.gopkg/cache/scheduler/tas_flavor_snapshot_tree_test.gopkg/cache/scheduler/tas_flavor_test.gopkg/cache/scheduler/tas_nodes_cache.gopkg/cache/scheduler/tas_nodes_cache_test.go
|
It looks unrelated because this is non TAS, but I haven't seen this before, not sure |
|
@coderabbitai please open a flake issue remembering to add the kind/flake label and the link to the failed log |
|
/test pull-kueue-test-integration-shard-2-main |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kubernetes-sigs/kueue"
log_url="https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/13819/pull-kueue-test-integration-shard-2-main/2084671642113413120"
printf '%s\n' '--- Prow result ---'
curl -fsSL "$log_url" | sed -n '1,220p'
printf '%s\n' '--- Required label ---'
gh label list --repo "$repo" --search "kind/flake" --limit 20Length of output: 8291 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kubernetes-sigs/kueue"
log_url="https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/13819/pull-kueue-test-integration-shard-2-main/2084671642113413120"
artifact_url="https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/13819/pull-kueue-test-integration-shard-2-main/2084671642113413120/build-log.txt"
printf '%s\n' '--- Build-log failure context ---'
curl -fsSL "$artifact_url" |
grep -nE -i '(^FAIL|--- FAIL:|error:|panic:|timed out|timeout|failed)' |
tail -80 || true
printf '%s\n' '--- Existing issues with this log ---'
gh issue list \
--repo "$repo" \
--state all \
--search "\"$log_url\" in:body" \
--limit 20 \
--json number,title,url,stateLength of output: 20224 🏁 Script executed: #!/bin/bash
set -euo pipefail
artifact_url="https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/13819/pull-kueue-test-integration-shard-2-main/2084671642113413120/build-log.txt"
curl -fsSL "$artifact_url" | sed -n '17480,17635p'Length of output: 23160 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kubernetes-sigs/kueue"
log_url="https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/13819/pull-kueue-test-integration-shard-2-main/2084671642113413120"
pr_url="https://github.com/kubernetes-sigs/kueue/pull/13819"
body=$(cat <<EOF
## Summary
The \`pull-kueue-test-integration-shard-2-main\` Prow job failed with one failing integration test. The PR changes TAS snapshot caching, but the failed suite is the non-TAS core controller integration suite.
## Failed test
\`\`\`
Queue controller [It] Should update conditions when clusterQueues that its localQueue references are updated
\`\`\`
- Suite: \`core\` (\`test/integration/singlecluster/controller/core\`)
- Source location: \`test/integration/singlecluster/controller/core/localqueue_controller_test.go:183\`
- Result: 72 passed, 1 failed
## Failed log
${log_url}
## Context
- Reported from PR `#13819`: ${pr_url}
- Requested by: `@mimowo`
## Acceptance criteria
- Reproduce or determine the source of the intermittent failure.
- Stabilize the test or the tested controller behavior.
- Verify repeated execution of the affected integration suite does not reproduce the failure.
EOF
)
issue_url=$(gh issue create \
--repo "$repo" \
--title "[Flake] LocalQueue conditions update integration test" \
--body "$body" \
--label "kind/flake" \
--assignee "mimowo")
printf 'Created issue: %s\n' "$issue_url"Length of output: 1646
The issue has the You are interacting with an AI system. |
|
/test pull-kueue-test-integration-shard-2-main |
kubernetes-prow
Bot
added
the
needs-rebase
|
/assign |
|
|
||
| // domainState is the per-snapshot mutable state of a domain during the | ||
| // assignment algorithm, addressed by domain.idx. | ||
| type domainState struct { |
There was a problem hiding this comment.
maybe nit:
IMO it gets a little overwhelming to use "state" everywhere. I think the *.state*. fields in domainState could be named more expressively in the first place like podCount/sliceCount maybe. It feels more intuitive to me.
It could be a separate PR.
Because now we have a few "state" structs that refer to state in different aspects. domainState has possible pod count states while leafState has resource capacity.
Or maybe we could at least rename leafState to leafCapacity or something similar.
Also naming here is a little inconsistent:
type TASFlavorSnapshot struct {
log logr.Logger
// topologyName indicates the name of the topology specified in the
// ResourceFlavor spec.topologyName field.
topologyName kueue.TopologyReference
// topologyTree is the static topology structure, shared with the other
// snapshots of the flavor. It must not be mutated.
*topologyTree
// state holds the per-snapshot mutable domain state, indexed by
// domain.idx.
state []domainState
// leafStates holds the per-snapshot mutable leaf state, indexed by
// leafDomain.leafIdx.
leafStates []leafStatestate and leafStates
There was a problem hiding this comment.
Agreed. I'll raise a separate PR for this. The rename comes to roughly 500 lines of mechanical changes across 6 files, which could dominate the diff and make it easier to miss any core issues.
|
|
||
| // domain holds the static information about placement of a topology | ||
| // domain in the hierarchy of topology domains. | ||
| // Its per-snapshot mutable state lives in TASFlavorSnapshot.state, |
There was a problem hiding this comment.
I'm wondering if splitting this file would be good decision now.
We have 2 topics here:
- immutable, read-only structures
- per-cycle mutable strcutures
it looks like a candidate for splitting. Tree already has separate test file so it sounds natural to me.
There was a problem hiding this comment.
Done. Added a new file tas_topology_tree.go with all the static parts and renamed the test file to tas_topology_tree_test.go match.
| FreeCapacity map[corev1.ResourceName]int64 | ||
| TASUsage map[corev1.ResourceName]int64 |
There was a problem hiding this comment.
I think we could use Requests from pkg/resources/interface.go here and when calling cmp.Diff pass cmp.Comparer(resources.Equal).
There was a problem hiding this comment.
Thanks for pointing it out. MIssed resources.Equal. Switched to resources.Requests and dropped requestsToMap entirely.
| func requestsToMap(r resources.Requests) map[corev1.ResourceName]int64 { | ||
| if r == nil { | ||
| return nil | ||
| } | ||
| result := make(map[corev1.ResourceName]int64, r.Len()) | ||
| r.ForEach(func(name corev1.ResourceName, value int64) { | ||
| result[name] = value | ||
| }) | ||
| return result | ||
| } |
There was a problem hiding this comment.
if we do https://github.com/kubernetes-sigs/kueue/pull/13819/changes#r3720516229 then we won't need that function
| for b.Loop() { | ||
| result = snapshot.FindTopologyAssignmentsForFlavor(b.Context(), requests) | ||
| } | ||
| b.StopTimer() |
There was a problem hiding this comment.
I think we don't need to call that, b.Loop() should stop timer internally - https://cs.opensource.google/go/go/+/refs/tags/go1.26.5:src/testing/benchmark.go;drc=fb5156a0981c2b89a118695138194e1af162ac8e;l=482
| b.StopTimer() |
There was a problem hiding this comment.
Yes it was not required. Removed.
| domainState := s.stateOf(d) | ||
| domainState.stateWithLeader = int32(0) | ||
| domainState.sliceStateWithLeader = int32(0) | ||
| domainState.leaderState = int32(0) |
There was a problem hiding this comment.
do we need these int32 wraps?
There was a problem hiding this comment.
No I don't think its required. Removed.
| cycle := 0 | ||
| for b.Loop() { | ||
| switch { |
There was a problem hiding this comment.
We could reset timer here and not include setup procedures in the measurement, the same way we do in the new benchmark
| cycle := 0 | |
| for b.Loop() { | |
| switch { | |
| cycle := 0 | |
| b.ResetTimer() | |
| for b.Loop() { | |
| switch { |
There was a problem hiding this comment.
It seems like b.Loop() also resets the timer first time its called. https://cs.opensource.google/go/go/+/refs/tags/go1.26.5:src/testing/benchmark.go;drc=fb5156a0981c2b89a118695138194e1af162ac8e;l=480
|
cc @apullo777 would you like to give it a pass here? |
|
/assign |
|
LGTM label has been added. DetailsGit tree hash: 8586c429eb028695a9c83b1160be12ad613ad459 |
mimowo
left a comment
mimowo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thank you for this excellent effort 👍 It not only improves the performance but also the readability of code with the tas_topology_tree extracted.
Thank you also for @j-skiba raising the idea and @apullo777 for your review 🥇
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshay-pm, mimowo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
…cross scheduler snapshots (#14644) * Reuse TAS topology trees across scheduler snapshots * Add TASCacheTopologyTree Feature Gate Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> * Fix 2 formatting errors Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> --------- Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> Co-authored-by: Akshay PM <akshaypm.mail@gmail.com>
…cross scheduler snapshots (#14639) * Reuse TAS topology trees across scheduler snapshots * Add TASCacheTopologyTree Feature Gate Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> * TASCacheTopologyTree FG phase is changed Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> --------- Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> Co-authored-by: Akshay PM <akshaypm.mail@gmail.com>
|
/kind feature |
kubernetes-prow
Bot
added
kind/feature
|
/release-note-edit |
|
/release-note-edit |
What type of PR is this?
/kind cleanup
/area tas
What this PR does / why we need it:
TAS snapshots previously rebuilt the complete topology tree for every scheduling cycle, including the domain hierarchy, node-to-domain mapping, and leaf capacities. Most of this data remains unchanged between cycles.
This PR:
Benchmark configuration:
pprof flame graphs
Before:
After:
Which issue(s) this PR fixes:
Part of #12580
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Performance
Scheduling
Reliability
Testing