Repository navigation
Conversation
addScheduleInfoToNode skips nodes matching FLUID_SCHEDULE_INFO_EXCLUDE_NODE_SELECTOR, but removeScheduleInfoFromNode still unlabelled them when a worker left the node. Skip them there too. Signed-off-by: Srinivas Chippagiri <cvas22@gmail.com>
|
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the change making schedule-info removal honor the exclude selector.
No open issues 🎉
What changed in this PR
Addresses #6246 by making schedule-info removal honor FLUID_SCHEDULE_INFO_EXCLUDE_NODE_SELECTOR, matching the existing addition behavior.
Changes:
- Skip label removal for nodes matching the exclude selector, preserving any existing labels.
- Add unit tests covering normal runtime-label removal and unchanged labels on excluded nodes.
| File | Description |
|---|---|
pkg/utils/dataset/lifecycle/node.go |
Adds the exclude-selector guard before removing schedule information. |
pkg/utils/dataset/lifecycle/node_test.go |
Tests removal behavior for excluded and non-excluded nodes. |
Review effort: Balanced
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Both reviewers reproduced the reported bug independently: on the base branch, removeScheduleInfoFromNode ignores FLUID_SCHEDULE_INFO_EXCLUDE_NODE_SELECTOR, so an excluded node carrying a runtime label gets its Fluid labels stripped when its worker leaves (issue #6246). Two separate harnesses — one per reviewer — show the entry-point contract red on base and green on this head, with a non-excluded control node still cleaned up in the same sync, so the guard mirrors the add-path check without over-skipping. The full pkg/utils/dataset/lifecycle suite passes (26/26) including the PR's two new specs, and CI is green on this head. Nothing here blocks merge; three smaller items are worth a look, folded below. The one substantive disagreement between the reviewers — whether the stale labels on an excluded node persist forever — was settled during cross-critique and re-verified on this head: TearDownWorkers (pkg/ctrl/worker.go) strips the same label family at engine teardown with no selector check, so the window is bounded to the runtime's lifetime, not permanent. Reviewer C (Copilot) reviewed this head and raised no findings.
Next round
[MUST] None — merge is not gated on any finding from this review.
[SHOULD] Hoist an entry-point spec (excluded-skip driven through SyncScheduleInfoToCacheNodes) into the PR; both verify branches carry one to lift (F-MT-ENTRY-01).
[SHOULD] After maintainer confirmation of the semantic, add one operator-facing line next to runtime.syncScheduleInfoNodeExcludeSelector: excluding a previously-labelled node does not clean its Fluid labels until the runtime is deleted (F-QR-STALE-LABELS-01).
[SHOULD] Pin nodeExcludeSelector in the new Describe's BeforeEach and add a non-nil non-matching-selector case to the removal specs (F-NIT-SELECTOR-STATE-01).
[SHOULD] Follow-up issue (optional): envtest/e2e coverage of the selector plumbing — pre-existing gap, not this PR's (codex-pr6249-no-e2e-exclude-selector).
[Filed] None.
Non-blocking observations (4) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/utils/dataset/lifecycle/node.go:190 [minor] — F-QR-STALE-LABELS-01: excluded nodes keep pre-existing Fluid labels until the runtime is deleted — maintainer to confirm this semantic
F-QR-STALE-LABELS-01 (consensus; Claude F2 + Codex F1 merged, dispute on the mechanism adjudicated): a node labelled before the operator set FLUID_SCHEDULE_INFO_EXCLUDE_NODE_SELECTOR keeps its runtime/common/capacity/fluid_exclusive labels while the runtime exists — getActualNodesWithScheduleInfo re-diffs it into the removal set every sync and the new guard skips it each time, so during that window the NodeAffinityWithCache webhook keeps treating the node as cache-holding and each sync logs one V(1) skip line. Disputed sub-claim, settled in cross-critique and re-verified on head: the persistence is NOT permanent — Helper.TearDownWorkers (pkg/ctrl/worker.go:178-247) lists nodes by =true, strips the same label family (runtime/common/memory/disk/total/exclusive + DecreaseDatasetNum) with no selector check, and is reached from every engine's Shutdown(), so dataset/runtime deletion cleans an excluded node's stale labels; the window is bounded to the runtime's lifetime, not 'until the selector is changed'. This is the trade-off the author disclosed in PR body §V and it matches the issue's stated expectation, so both reviewers classify it as a design decision, not a defect — please confirm the semantic as maintainer and consider one operator-facing sentence next to runtime.syncScheduleInfoNodeExcludeSelector ("excluding a previously-labelled node does not clean its Fluid labels until the runtime is deleted"). Note while confirming: TearDownWorkers already label-patches excluded nodes unconditionally, so 'Fluid never touches those nodes' was never literally true of #5285's feature. Not gating. - pkg/utils/dataset/lifecycle/node_test.go:507 [minor] — F-MT-ENTRY-01: no entry-point coverage of the skip
F-MT-ENTRY-01 (consensus; Claude F1, confirmed by Codex who re-ran the spec green on head): the new specs cover the removeScheduleInfoFromNode helper only. The user-visible contract — an excluded node is not label-patched when the runtime scales off it — lives one level up in SyncScheduleInfoToCacheNodes (desired-from-worker-pods → actual-by-runtime-label → diff → apply), the single entry point all nine engines call; a helper-level test stays green if a future change re-introduces the patch elsewhere in the chain (e.g. the capacity-refresh path once #6244 merges). Recommend hoisting a variant of the harness spec (label both nodes → set the selector → delete the worker pods → sync → excluded node keeps its label, control node cleaned) into the PR — both verify branches carry one to lift, and an existing 'Test SyncScheduleInfoToCacheNodes' Describe in the same file proves the entry point is testable there. - test:1 [minor] — pre-existing, not introduced by this PR — codex-pr6249-no-e2e-exclude-selector: no e2e coverage for selector-gated schedule-info sync (pre-existing gap)
codex-pr6249-no-e2e-exclude-selector (consensus; Codex F2, confirmed by Claude with one factual correction): neither the add-side check (#5285 — which shipped with parse-level unit tests only, not 'no tests at all'; the parseNodeExcludeSelectorFromEnv specs are still at node_test.go:697-733 on base) nor this remove-side fix has e2e coverage; the feature is only exercised at unit level. This PR's unit tests cover both polarities of the changed function, which is the layer where the selector-match decision lives, so this does not block — but an envtest/e2e case (operator started with FLUID_SCHEDULE_INFO_EXCLUDE_NODE_SELECTOR, worker scaled off an excluded node, labels preserved) would close the loop on the env-var → helm-values plumbing (charts/fluid/fluid/templates/_helpers.tpl:165-169). Pre-existing gap, not introduced by this PR. - pkg/utils/dataset/lifecycle/node_test.go:528 [nit] — F-NIT-SELECTOR-STATE-01: pin nodeExcludeSelector in BeforeEach and cover a non-matching selector
F-NIT-SELECTOR-STATE-01 (consensus; Claude F3 + Codex F3 merged): the new Describe resets the package-global nodeExcludeSelector only in AfterEach (to nil, rather than restoring its pre-spec value), while the pre-existing 'Test parseNodeExcludeSelectorFromEnv / valid selector' spec leaves the var non-nil (type=valid-value) after its AfterEach only unsets the env var. No flake today — the leaked selector never matches the new fixtures' type=virtual-kubelet and declaration order runs the new Describe first — but pinning nodeExcludeSelector = nil in the new Describe's BeforeEach (or save/restore) makes each spec self-sufficient against future leaks. Complementary sub-point (Claude, credited by Codex): the PR's 'not excluded' spec only covers a nil selector, so the guard's Matches == false branch (non-nil, non-matching selector) is untested in the PR's own specs; a second 'not excluded' case with a non-matching selector would close it.
Verified review delta
- F-QR-STALE-LABELS-01: open (initial-pr)
- F-MT-ENTRY-01: open (initial-pr)
- codex-pr6249-no-e2e-exclude-selector: open (preexisting)
- F-NIT-SELECTOR-STATE-01: open (initial-pr)
Code: clear. CI: success; attribution: unknown. Required CI checks still apply.
Ⅰ. Describe what this PR does
removeScheduleInfoFromNodenow skips nodes that matchFLUID_SCHEDULE_INFO_EXCLUDE_NODE_SELECTOR, the same wayaddScheduleInfoToNodedoes, so excluded nodes like virtual-kubelet ones don't get label patches when a worker leaves them.Ⅱ. Does this pull request fix one issue?
fixes #6246
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
removeScheduleInfoFromNodetests for a node that isn't excluded (runtime label removed) and one that matches the selector (labels unchanged).Ⅳ. Describe how to verify it
Set the exclude selector so it matches a node that has a runtime's labels, scale the runtime so its worker leaves that node, and the node's labels stay as they are.
Ⅴ. Special notes for reviews
One trade-off: a node that got labelled before the operator excluded it now keeps those labels after its worker leaves, instead of getting them cleaned up. That seemed closer to what the selector promises (Fluid doesn't touch those nodes), but if you'd rather still clean them up on removal, I can drop the check and document that removal still cleans up excluded nodes instead.