feat(nodes): drain plan — read-only estimate before draining (#1584) - #1718
Conversation
…log (skyhook-io#1584) Replaces the generic Drain Node confirmation with a server-backed plan. Backend (pkg/k8score, internal/server): - Pure classifier ClassifyPodForDrain returning a per-pod outcome (evict / skip / may-block) with an operator-facing reason. Rules follow kubectl drain's filters: terminal and mirror pods skipped, DaemonSet pods never evicted, no controller owner needs force, emptyDir needs deleteEmptyDirData. A pod counts as managed only with a controller owner reference (metav1.GetControllerOf); hasManagedOwner accepted any owner reference before. A pod that is already terminating is reported as evict without consulting PDBs, as the Eviction API admits it regardless. - POST /api/nodes/{name}/drain-plan: reads only, requesting user's client, same body as drain, returns evaluated options, generation time, summary, per-pod outcomes with reasons and an emptyDir flag; PDBs the caller cannot list are reported as not evaluated instead of counted as zero. - DrainNode runs on the same classifier and returns skippedPods with reasons (additive); the MCP manage_node tool is unchanged. Frontend (packages/k8s-ui, web): - DrainPlanDialog fetches the plan before enabling Drain, labels it an estimate re-evaluated on execution, lists every pod with its outcome and reason, starts with deleteEmptyDirData off and requires an explicit acknowledgement naming the affected pods, keeps force explicit. Both options are always sent explicitly; nothing relies on server defaults. - Post-drain toast lists evictions, skipped pods with reasons and individual failures. Tests: classifier table, PlanNodeDrain and the handler on a fake clientset asserting only get/list actions, status mapping, DrainNode skip reporting; frontend: confirm gating (plan required, emptyDir acknowledgement), dialog content rendering, plan request shape, drain result summary. Docs: endpoint list and RBAC note.
PR Summary by QodoAdd read-only node drain plans and safety-gated confirmation
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…w-ups Budget evaluation now reproduces the API server's eviction handler: Pending and terminating pods bypass PodDisruptionBudgets; a pod covered by more than one budget is reported as may-block (the eviction subresource refuses it); an unready pod is evictable under unhealthyPodEvictionPolicy AlwaysAllow, or under IfHealthyBudget when currentHealthy >= desiredHealthy; a budget whose status is not reconciled yet (observedGeneration < generation) is reported as may-block, because the API server answers 429 until it is. PlanNodeDrain lists budgets only for namespaces holding a pod whose decision depends on them, so a forbidden namespace with nothing but skipped pods no longer marks the plan as unevaluated; pdbChecked is set only when a decision reached the budget check. Frontend: the drain stays disabled after a plan request error; the plan request goes through apiUrl(); the plan mutation carries the toast metadata the app's mutation handler expects; the emptyDir acknowledgement uses the themed AlertBanner instead of a hardcoded colour. Comment and log-format nits from review.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ec1e2c7. Configure here.
The drain dialog is the only caller and shows a failed plan inline while keeping Drain disabled; the global mutation toast reported the same failure a second time on every unsuccessful refetch.
|
Thanks for this one, and sorry for the slow answers. Your PDB question needed real digging. One small thing first: in risk item (a), a pod whose owner references are all non-controller, with no DaemonSet among them, was evicted by an ordinary drain before your change, not skipped. The DaemonSet case is exactly as you wrote it. The code does the right thing either way. On the Same on the cluster-wide PDB listing, let's leave that one too. A single list pulls every budget in the cluster to use the handful on one node, so it's fewer big calls instead of more small ones rather than a clear win. It only runs on dialog open. If a slow dialog does get reported on a wide node, your version is the fix. Last thing, and it's a question. You flagged that the plan doesn't model a budget this drain would exhaust: two replicas on the node under |
|
Yes — I'd like to take this on. Please write it out when you have a moment and I'll work from that. Thanks for the detailed answers, and for the correction on risk item (a) — you're right, a pod whose owners are all non-controller was being evicted before, not skipped. |
|
@hisco no rush on this. I saw #1758 lists it as P3 with open decisions on your internal follow-up issue. If it saves you time, I can draft the spec from what you described: count the pods on the node that would actually consume a disruption from each budget, compare with |
2 participants
Description
Implements #1584 end to end: a pure drain classifier, a read-only drain-plan endpoint, and a plan dialog that replaces the generic Drain Node confirmation.
Backend
pkg/k8score:ClassifyPodForDrain(pod, opts, pdbs)returnsevict/skip/may-blockwith an operator-facing reason. Rules followkubectl drain's filters: terminal and mirror pods skipped, DaemonSet pods never evicted, no controller owner needs force,emptyDirneedsdeleteEmptyDirData. A pod counts as managed only with a controller owner reference (metav1.GetControllerOf), the way the rest of the codebase already does it;hasManagedOwneraccepted any owner reference before. A pod that is already terminating is reported asevictwithout consulting PDBs, because the Eviction API admits it regardless of budgets.PlanNodeDrainlists the node's pods and per-namespace PDBs with the given client and classifies them. Reads only. A namespace whose PDBs cannot be listed is reported as not evaluated (pdbsEvaluated,pdbError, per-podpdbChecked) rather than as "no PDB".may-blockis evidence, not a verdict: a matching PDB withdisruptionsAllowed == 0. The plan saysestimate: true; the drain endpoint re-lists live state as before.POST /api/nodes/{name}/drain-plannext to cordon/uncordon (regular timeout group), same JSON body as drain, evaluated with the requesting user's client.deleteEmptyDirDatadefaults tofalsehere and the response echoes the options used.DrainNoderuns on the same classifier and returnsskippedPodswith reasons (additive field). The MCPmanage_nodetool builds its own response and is unchanged.Frontend
DrainPlanDialog(packages/k8s-ui) replaces the confirmation: it fetches the plan when opened and again whenever an option changes, keeps Drain disabled until a plan for this node and these options is shown, labels the result an estimate that the drain re-evaluates, and lists every pod with its outcome and reason.deleteEmptyDirDatastarts off; enabling it shows an acknowledgement that names the pods whose data would be discarded and must be ticked before Drain enables. Force stays an explicit, explained checkbox. Both options are always sent explicitly to the drain endpoint.onPlanDrainkeep working: the dialog then gatesdeleteEmptyDirDataon the acknowledgement alone.CLAUDE.md, RBAC needs (getnodes,listpods,listpoddisruptionbudgets) indocs/in-cluster.md.Things I want to flag rather than have you find
deleteEmptyDirData: true; the new one starts with it off, so pods usingemptyDirare skipped unless the operator enables the option and ticks the acknowledgement. This is what the issue asks for, but it is a visible change for anyone who relied on the old default; worth a line in release notes.force, evicted with it). (b) DaemonSet pods are never evicted, even withIgnoreDaemonSets=false(previously they were); Radar always passestrue, so nothing observable changes, but the option is now only a reason string. (c) A terminating pod is evicted without a PDB pre-check. All three matchkubectl/ the Eviction API and also apply to the MCPmanage_nodedrain, whose code is untouched.deleteEmptyDirDatatotruewhen the body omits it. The dialog no longer relies on that (it sends both options explicitly), so the acceptance criterion holds for the UI; API callers that omit the field keep today's behaviour. Flipping the endpoint default is a one-line change if you prefer it.may-blockscope. Only a currently exhausted budget is reported, against the first matching PDB. A budget that this very drain would exhaust (disruptionsAllowed: 1, several evictable pods) is not modelled; those pods show asevict, and the reasons say "may be refused". That is the literal reading of "currently PDB-constrained".Type of change
How has this been tested?
PlanNodeDrainand the handler on a fake clientset asserting onlyget/listactions; 404 / 403 mapping; forbidden PDB list degrading to an unevaluated plan;DrainNodereporting skipped pods; request option parsing. Frontend:canConfirmDrain(no drain without a current plan, emptyDir acknowledgement required only when emptyDir pods would be evicted, hosts without plan support),DrainPlanContentrendering (outcomes and reasons, estimate label, stale plan ignored, acknowledgement rendered unchecked and naming the pods, PDB-not-evaluated warning), plan request path and body (targets/drain-plan, both options explicit), drain result summary.minAvailable: 2PDB, one of its pods stuck terminating, a Deployment withemptyDir, a DaemonSet and a bare Pod. In the built app: opening Drain fetched the plan (2 evict, 2 may-block, 4 skip, terminating podevict), the node stayed schedulable; enablingdeleteEmptyDirDatarefetched the plan and showed the acknowledgement naming the two emptyDir pods with Drain disabled until ticked; the drain then evicted 5, timed out on exactly the two may-block pods and skipped the DaemonSet and bare pods; the toast listed the skips with reasons. Same flow without the PDB: "Node drained: 5 evicted, 4 skipped" with reasons.go vet ./internal/server/andtscin packages/k8s-ui report pre-existing issues in files this PR does not touch (exec_origin_test.go,localterm_disable_test.go,trace/*.test.ts).Checklist
Related issues
Fixes #1584
Note
Medium Risk
Changes live drain pod-selection semantics via a shared classifier (controller refs, DaemonSet handling, terminating pods) and alters the UI default for emptyDir eviction; the plan path is read-only but mistaken reliance on estimates could still surprise operators during drain.
Overview
Adds a read-only drain plan so operators can preview node drain impact before cordoning or evicting anything.
API & core: New
POST /api/nodes/{name}/drain-plan(same optional body as drain, butdeleteEmptyDirDatadefaults to false for previews).pkg/k8scoregainsClassifyPodForDrain/PlanNodeDrain, classifying each pod as evict, skip, or may-block (PDB evidence) with reasons; PDB list failures degrade to “not evaluated” instead of implying no budgets.DrainNodenow uses the same classifier and returns structuredskippedPods(additive); execution still relies on the Eviction API for PDBs.UI: The node Drain flow is replaced by
DrainPlanDialog: fetches/refetches the plan when options change, keeps Drain disabled until a matching plan is shown (when the host supports planning), and requires an explicit acknowledgement when delete emptyDir data is enabled (UI default is now off, unlike the old dialog that always sentdeleteEmptyDirData: true). Post-drain toasts summarize evicted, skipped (with reasons), and failures.Docs:
CLAUDE.mdanddocs/in-cluster.mddocument the endpoint and RBAC (getnodes,listpods, optionallistpoddisruptionbudgets).Reviewed by Cursor Bugbot for commit 1d5122f. Bugbot is set up for automated code reviews on this repo. Configure here.