fix(server): allow disabling periodic idle vNPU cleanup to avoid premature reclamation - #120
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dartagnanli The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe CLI adds a flag for periodic idle vNPU cleanup. ChangesServer cleanup lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant PluginServer
participant Cleanup
participant GRPCServer
CLI->>PluginServer: pass cleanup flag to NewPluginServer
PluginServer->>PluginServer: Start with ps.wg.Go
PluginServer->>Cleanup: start periodic cleanup when enabled
PluginServer->>GRPCServer: start serving through ps.wg.Go
GRPCServer-->>PluginServer: stop or restart serving
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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 |
34bc10b to
0243445
Compare
DSFans2014
left a comment
DSFans2014
left a comment
There was a problem hiding this comment.
@dartagnanli please fix the lint and dco error
…ature reclamation The periodic idle vNPU cleanup goroutine (startPeriodicCheckIdleVNPUs) destroys vNPUs whose IsContainerUsed == 0. As discussed in Project-HAMi#86, a vNPU allocated to a Running Pod that has not yet started using the NPU also reports IsContainerUsed == 0, so the cleanup can reclaim it too early and break workloads (e.g. npu-smi info fails inside the container). Add the --enable_periodic_idle_vnpu_cleanup flag (default false) so operators can disable the periodic cleanup goroutine while the stricter policy is being revisited. The one-shot cleanup on plugin restart (cmd/main.go) is intentionally left unconditional and unchanged. Also apply Go modernizations in the touched files: interface{} -> any, wg.Add(1)+go -> wg.Go, range over int, slices.Backward. Refs Project-HAMi#86 Signed-off-by: libin18 <libin18@kingsoft.com>
0243445 to
084b97b
Compare
![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
🤖 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 `@internal/server/server_test.go`:
- Line 538: Update TestNewPluginServer to include the periodic-cleanup flag in
each test case, pass that case value to NewPluginServer, and assert
ps.enablePeriodicIdleVNPUCleanup matches it. Add at least one test case with the
flag set to true while preserving coverage for false.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5e18f1c-a9c7-4d4b-9218-0a9cdf33590b
📒 Files selected for processing (4)
cmd/main.gointernal/server/register.gointernal/server/server.gointernal/server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/main.go
- internal/server/register.go
- internal/server/server.go
| t.Parallel() | ||
| mgr := &FakeManager{CommonWordFunc: func() string { return tc.args.commonWord }} | ||
| ps, err := NewPluginServer(mgr, tc.args.nodeName, 60) | ||
| ps, err := NewPluginServer(mgr, tc.args.nodeName, 60, false) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the new periodic-cleanup flag in TestNewPluginServer.
Passing false only updates the call-site signature; this test never verifies that NewPluginServer stores the argument. Add the flag to the test cases and assert ps.enablePeriodicIdleVNPUCleanup, including a true case.
🤖 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 `@internal/server/server_test.go` at line 538, Update TestNewPluginServer to
include the periodic-cleanup flag in each test case, pass that case value to
NewPluginServer, and assert ps.enablePeriodicIdleVNPUCleanup matches it. Add at
least one test case with the flag set to true while preserving coverage for
false.
2 participants
What & Why
Issue #86 reports that the periodic idle vNPU cleanup goroutine (
startPeriodicCheckIdleVNPUs) destroys vNPUs based solely onIsContainerUsed == 0. Because a vNPU allocated to a Pod that is alreadyRunningbut whose container has not yet started using the NPU also reportsIsContainerUsed == 0, the cleanup can reclaim the vNPU too early — breaking workloads (e.g.npu-smi infofails inside the container shortly after the Pod starts).#86 is framed as a discussion and lists several more thorough directions (combining
IsContainerUsedwith Pod allocation state, adding a protection window, or a Pod-referenced rule likemind-cluster'sDestroyNotUsedVNPU). Those require deeper changes to the cleanup policy.This PR takes a smaller, complementary step: give operators a switch to disable the periodic cleanup goroutine so the premature-reclamation symptom stops on affected nodes without waiting for the larger redesign.
Changes
--enable_periodic_idle_vnpu_cleanup(defaultfalse). Whenfalse,Start()does not launch the periodicstartPeriodicCheckIdleVNPUsgoroutine.CleanupIdleVNPUs()on plugin restart (cmd/main.go) is intentionally left unconditional and unchanged — it is a separate entry point. The flag name (enable_periodic_...) and its help text make this boundary explicit to avoid the "total cleanup switch" ambiguity.NewPluginServer.Scope note
This is a mitigation, not a full resolution of #86's cleanup-policy discussion. The flag defaults to
false(periodic cleanup disabled) to stop the premature-reclamation symptom by default; setting it totruerestores the previous behavior. Reviewers may prefer a different default — easy to adjust.Testing
go build ./internal/server/... ./cmd/...✅go vet ./internal/server/... ./cmd/...✅go test ./internal/server/— 109 passed, consistent with baseline (ran 3×)Modernizations (in the touched files)
While editing these files I also applied the Go modernization lints the editor flagged. All behavior-preserving; happy to split into a separate PR if preferred:
interface{}→anywg.Add(1)+go func(){ defer Done() }→wg.Go(...)for i := 0; i < n; i++→for i := range nslices.BackwardRefs #86
Summary by CodeRabbit
--enable_periodic_idle_vnpu_cleanupto enable periodic cleanup of idle vNPU resources.