Skip to content

fix(nvca): apply primary-PV selector in model cache cleanup#441

Open
mesutoezdil wants to merge 1 commit into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/fix/modelcache-pv-selector
Open

fix(nvca): apply primary-PV selector in model cache cleanup#441
mesutoezdil wants to merge 1 commit into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/fix/modelcache-pv-selector

Conversation

@mesutoezdil

@mesutoezdil mesutoezdil commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

TL;DR

The model cache cleanup selector was left empty because the result of
labels.Selector.Add was discarded. An empty selector matches every object, so
idle cleanup listed every PV in the cluster instead of only model-cache primary
PVs. This skewed the model cache backend count metric and made the cleanup loop
scan all PVs each tick. Reassign the Add result so the requirement applies, plus
a regression test.

Issues

Closes #440

Testing

go test ./pkg/storage/... with envtest passes. Added TestPrimaryPVSelector,
which fails before the fix (empty selector matches an empty label set) and passes
after. No QA needed.

Checklist

  • I am familiar with the Contributing Guidelines.
  • I have signed off my commits for Developer Certificate of Origin (DCO) compliance.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected persistent volume label selector construction so only volumes with the required primary-volume label are considered during model cache cleanup.
    • Ensures unlabeled volumes are no longer matched incorrectly.
  • Tests

    • Added coverage validating selector behavior for both unlabeled volumes and volumes carrying the expected primary-volume label.

@mesutoezdil
mesutoezdil requested a review from a team as a code owner July 25, 2026 09:53
@mesutoezdil
mesutoezdil requested a review from balajinvda July 25, 2026 09:53
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9052d21b-44ea-4aa7-80ed-96457875397f

📥 Commits

Reviewing files that changed from the base of the PR and between c9a42d1 and 2647a51.

📒 Files selected for processing (2)
  • src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go
  • src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go

📝 Walkthrough

Walkthrough

The primary PV selector now retains its label requirement, and a unit test verifies that it rejects unlabeled objects and matches PVs with the primary label.

Changes

Primary PV selector

Layer / File(s) Summary
Selector construction and matching validation
src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup.go, src/compute-plane-services/nvca/pkg/storage/modelcache_cleanup_test.go
primaryPVSel is reassigned after adding the primary PV label requirement, with tests covering matching and non-matching label sets.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: balajinvda

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and accurately describes the bug fix in model cache cleanup.
Linked Issues check ✅ Passed The PR satisfies #440 by retaining the selector result and adding the required regression test.
Out of Scope Changes check ✅ Passed The changes stay focused on the selector fix and its test, with no unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands.

labels.Selector.Add returns a new selector and does not mutate the
receiver, so the discarded return left primaryPVSel empty. An empty
selector matches every object, so the idle model cache cleanup listed
every PV in the cluster instead of only model-cache primary PVs. This
skewed the model cache backend count metric and made the cleanup loop
scan all PVs each tick.

Reassign the Add result so the primary-PV requirement is applied, and
add a regression test on the selector.

Closes NVIDIA#440

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
@mesutoezdil
mesutoezdil force-pushed the mesutoezdil/fix/modelcache-pv-selector branch from bec4165 to 2647a51 Compare July 25, 2026 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nvca model cache cleanup lists every PV due to discarded selector Add() result

1 participant