fix(drain-plan): follow the Eviction API's budget rules; review follow-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.

Author: alexeymoskalev-devops

internal/server/node_handlers.go

@@ -97,7 +97,7 @@ func (s *Server) handleUncordonNode(w http.ResponseWriter, r *http.Request) {
 
 // drainOptionsFromRequest turns the JSON body shared by the drain and drain-plan
 // endpoints into DrainOptions. The two endpoints differ only in what an omitted
-// deleteEmptyDirData means: the drain keeps its historical default (true, matching
+// deleteEmptyDirData means: the drain defaults to true (matching
 // kubectl drain --delete-emptydir-data), the plan defaults to false so a preview
 // never silently includes emptyDir data. The plan echoes the options it used.
 func drainOptionsFromRequest(req DrainRequest, deleteEmptyDirDefault bool) k8s.DrainOptions {
@@ -170,7 +170,7 @@ func (s *Server) writeDrainPlan(w http.ResponseWriter, r *http.Request, client k
 			s.writeError(w, http.StatusForbidden, err.Error())
 			return
 		}
-		log.Printf("[node-ops] Failed to plan drain for node %s: %v", nodeName, err)
+		log.Printf("[node-ops] Failed to plan drain for node/%s: %v", nodeName, err)
 		s.writeError(w, http.StatusInternalServerError, err.Error())
 		return
 	}

packages/k8s-ui/src/components/shared/DrainPlanDialog.test.tsx

@@ -49,6 +49,12 @@ describe('canConfirmDrain', () => {
     expect(canConfirmDrain({ plan: plan([]), nodeName: 'other', options: off, loading: false, acknowledgedEmptyDir: false, planSupported: true })).toBe(false)
   })
 
+  it('keeps the drain disabled after a plan request error, even if an older plan is still around', () => {
+    const p = plan([pod('web', 'evict')])
+    expect(canConfirmDrain({ plan: p, nodeName: 'worker-1', options: off, loading: false, error: 'boom', acknowledgedEmptyDir: false, planSupported: true })).toBe(false)
+    expect(canConfirmDrain({ plan: null, nodeName: 'worker-1', options: off, loading: false, error: 'boom', acknowledgedEmptyDir: false, planSupported: false })).toBe(true)
+  })
+
   it('enables the drain once a matching plan is shown and no emptyDir data is at risk', () => {
     expect(canConfirmDrain({ plan: plan([pod('web', 'evict')]), nodeName: 'worker-1', options: off, loading: false, acknowledgedEmptyDir: false, planSupported: true })).toBe(true)
   })

packages/k8s-ui/src/components/shared/DrainPlanDialog.tsx

@@ -1,5 +1,4 @@
 import { useEffect, useState } from 'react'
-import { clsx } from 'clsx'
 import { Loader2 } from 'lucide-react'
 import { ConfirmDialog } from '../ui/ConfirmDialog'
 import { Badge, type BadgeSeverity } from '../ui/Badge'
@@ -59,6 +58,8 @@ export interface CanConfirmDrainInput {
   nodeName: string
   options: DrainDialogOptions
   loading: boolean
+  /** the last plan request failed; with plan support the drain stays disabled until a plan is shown again */
+  error?: string | null
   acknowledgedEmptyDir: boolean
   /** false when the host cannot compute plans (no onPlanDrain); the dialog then only gates on the acknowledgement */
   planSupported: boolean
@@ -70,8 +71,9 @@ export interface CanConfirmDrainInput {
  * requires an explicit acknowledgement: the plan is an estimate, and a pod that
  * starts using emptyDir between the estimate and the drain would still lose its data.
  */
-export function canConfirmDrain({ plan, nodeName, options, loading, acknowledgedEmptyDir, planSupported }: CanConfirmDrainInput): boolean {
+export function canConfirmDrain({ plan, nodeName, options, loading, error, acknowledgedEmptyDir, planSupported }: CanConfirmDrainInput): boolean {
   if (loading) return false
+  if (planSupported && error) return false
   const current = planMatches(plan, nodeName, options) ? plan : null
   if (planSupported && !current) return false
   if (options.deleteEmptyDirData && !acknowledgedEmptyDir) return false
@@ -191,27 +193,24 @@ export function DrainPlanContent({
       )}
 
       {needsAck && (
-        <label
-          className={clsx(
-            'flex items-start gap-2 cursor-pointer rounded p-2 border',
-            'border-red-500/40 bg-red-500/10 text-theme-text-primary',
-          )}
-        >
-          <input
-            type="checkbox"
-            checked={acknowledgedEmptyDir}
-            onChange={(e) => onAcknowledgeEmptyDir(e.target.checked)}
-            className="mt-0.5 rounded border-theme-border"
-          />
-          <span>
-            {current && atRisk.length > 0
-              ? `Discard the emptyDir data of ${pluralize(atRisk.length, 'pod')}: ${atRisk.map((p) => `${p.namespace}/${p.name}`).join(', ')}.`
-              : current
-                ? 'No pod on this node uses emptyDir right now; any that does when the drain runs will lose that data.'
-                : 'Discard the emptyDir data of every evicted pod that uses emptyDir volumes.'}{' '}
-            I understand this data cannot be recovered.
-          </span>
-        </label>
+        <AlertBanner variant="error" title="emptyDir data will be discarded">
+          <label className="flex items-start gap-2 cursor-pointer">
+            <input
+              type="checkbox"
+              checked={acknowledgedEmptyDir}
+              onChange={(e) => onAcknowledgeEmptyDir(e.target.checked)}
+              className="mt-0.5 rounded border-theme-border"
+            />
+            <span>
+              {current && atRisk.length > 0
+                ? `Discard the emptyDir data of ${pluralize(atRisk.length, 'pod')}: ${atRisk.map((p) => `${p.namespace}/${p.name}`).join(', ')}.`
+                : current
+                  ? 'No pod on this node uses emptyDir right now; any that does when the drain runs will lose that data.'
+                  : 'Discard the emptyDir data of every evicted pod that uses emptyDir volumes.'}{' '}
+              I understand this data cannot be recovered.
+            </span>
+          </label>
+        </AlertBanner>
       )}
     </div>
   )
@@ -243,7 +242,7 @@ export function DrainPlanDialog({
     setAcknowledgedEmptyDir(false)
   }, [options.force, options.deleteEmptyDirData, open, plan?.generatedAt])
 
-  const confirmEnabled = canConfirmDrain({ plan, nodeName, options, loading, acknowledgedEmptyDir, planSupported })
+  const confirmEnabled = canConfirmDrain({ plan, nodeName, options, loading, error, acknowledgedEmptyDir, planSupported })
 
   return (
     <ConfirmDialog

pkg/k8score/drain_plan.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strings"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -17,13 +18,13 @@ import (
 type DrainOutcome string
 
 const (
-	// DrainOutcomeEvict: the pod would be evicted.
 	DrainOutcomeEvict DrainOutcome = "evict"
 	// DrainOutcomeSkip: the pod would be left alone (DaemonSet, mirror, unmanaged, emptyDir, terminal).
 	DrainOutcomeSkip DrainOutcome = "skip"
-	// DrainOutcomeMayBlock: the pod would be evicted, but a PodDisruptionBudget currently allows no
-	// disruptions, so the eviction may be refused until the budget recovers. Evidence, not a verdict:
-	// only the Eviction API and live state decide.
+	// DrainOutcomeMayBlock: the pod would be evicted, but its PodDisruptionBudget would make the
+	// Eviction API refuse it right now (no disruptions allowed, status not yet reconciled, or more
+	// than one budget covering the pod). Evidence, not a verdict: only the Eviction API and live
+	// state decide.
 	DrainOutcomeMayBlock DrainOutcome = "may-block"
 )
 
@@ -35,22 +36,22 @@ type PodDrainDecision struct {
 	Reason    string       `json:"reason"`
 	EmptyDir  bool         `json:"emptyDir"`      // the pod uses emptyDir volumes; evicting it discards that data
 	PDB       string       `json:"pdb,omitempty"` // namespace/name of the PDB behind a may-block outcome
-	// PDBChecked is true when the PodDisruptionBudgets of the pod's namespace were available to
-	// this decision (a skip decided earlier in the filter chain never needs them). It is false
-	// during drain execution (the Eviction API decides) and when they could not be listed for the
-	// pod's namespace (see DrainPlan.PDBError).
+	// PDBChecked is true when PodDisruptionBudgets were consulted for this decision. It is false
+	// for decisions that never reach the budget check (skips, Pending or terminating pods), during
+	// drain execution (the Eviction API decides), and when the budgets of the pod's namespace could
+	// not be listed (see DrainPlan.PDBError).
 	PDBChecked bool `json:"pdbChecked"`
 }
 
 // ClassifyPodForDrain decides, without touching the cluster, what DrainNode would do with pod
 // under opts. The rules mirror kubectl drain's filters: terminal and mirror pods are skipped,
 // DaemonSet pods are never evicted (kubectl refuses the drain without --ignore-daemonsets;
 // DrainNode always ignores them), pods without a controller owner need Force, pods using
-// emptyDir need DeleteEmptyDirData. A pod that would be evicted but is covered by a PDB with
-// no disruptions allowed is reported as may-block. A pod that is already terminating is
-// reported as evict without consulting PDBs: the Eviction API admits it regardless of budgets.
+// emptyDir need DeleteEmptyDirData. Budget evaluation follows the Eviction API: pods that are
+// Pending or already terminating bypass PodDisruptionBudgets entirely; otherwise see
+// pdbBlocking for the cases reported as may-block.
 func ClassifyPodForDrain(pod corev1.Pod, opts DrainOptions, pdbs []policyv1.PodDisruptionBudget) PodDrainDecision {
-	d := PodDrainDecision{Namespace: pod.Namespace, Name: pod.Name, PDBChecked: pdbs != nil, EmptyDir: hasLocalStorage(pod)}
+	d := PodDrainDecision{Namespace: pod.Namespace, Name: pod.Name, EmptyDir: hasLocalStorage(pod)}
 	skip := func(reason string) PodDrainDecision {
 		d.Outcome, d.Reason = DrainOutcomeSkip, reason
 		return d
@@ -76,16 +77,21 @@ func ClassifyPodForDrain(pod corev1.Pod, opts DrainOptions, pdbs []policyv1.PodD
 		return skip("uses emptyDir volumes; their data is lost on eviction, enable deleteEmptyDirData to evict")
 	}
 
-	if pod.DeletionTimestamp != nil {
+	if bypassesPDB(pod) {
 		d.Outcome = DrainOutcomeEvict
-		d.Reason = "already terminating; the eviction is admitted regardless of PodDisruptionBudgets"
+		if pod.DeletionTimestamp != nil {
+			d.Reason = "already terminating; the eviction is admitted regardless of PodDisruptionBudgets"
+		} else {
+			d.Reason = "still pending; the eviction is admitted regardless of PodDisruptionBudgets"
+		}
 		return d
 	}
 
-	if name, blocked := pdbBlocking(pod, pdbs); blocked {
+	d.PDBChecked = pdbs != nil
+	if name, reason, blocked := pdbBlocking(pod, pdbs); blocked {
 		d.Outcome = DrainOutcomeMayBlock
 		d.PDB = name
-		d.Reason = fmt.Sprintf("PodDisruptionBudget %s currently allows no disruptions; the eviction may be refused until the budget recovers", name)
+		d.Reason = reason
 		return d
 	}
 
@@ -99,23 +105,83 @@ func ClassifyPodForDrain(pod corev1.Pod, opts DrainOptions, pdbs []policyv1.PodD
 	return d
 }
 
-// pdbBlocking returns the first PDB in the pod's namespace that selects the pod and has
-// no disruptions allowed. A nil selector matches nothing, an empty selector matches everything,
-// matching the policy/v1 semantics.
-func pdbBlocking(pod corev1.Pod, pdbs []policyv1.PodDisruptionBudget) (string, bool) {
+// bypassesPDB reports whether the Eviction API ignores PodDisruptionBudgets for the pod
+// (Succeeded, Failed or Pending phase, or a deletion already in progress), as in
+// canIgnorePDB of the API server's eviction handler.
+func bypassesPDB(pod corev1.Pod) bool {
+	return pod.DeletionTimestamp != nil ||
+		pod.Status.Phase == corev1.PodSucceeded ||
+		pod.Status.Phase == corev1.PodFailed ||
+		pod.Status.Phase == corev1.PodPending
+}
+
+// isPodReady mirrors k8s.io/kubernetes/pkg/api/v1/pod.IsPodReady: the Ready condition is True.
+func isPodReady(pod corev1.Pod) bool {
+	for _, c := range pod.Status.Conditions {
+		if c.Type == corev1.PodReady {
+			return c.Status == corev1.ConditionTrue
+		}
+	}
+	return false
+}
+
+// matchingPDBs returns the PDBs in the pod's namespace whose selector selects the pod.
+// A nil selector matches nothing, an empty selector matches everything (policy/v1 semantics).
+func matchingPDBs(pod corev1.Pod, pdbs []policyv1.PodDisruptionBudget) []*policyv1.PodDisruptionBudget {
 	podLabels := labels.Set(pod.Labels)
+	var out []*policyv1.PodDisruptionBudget
 	for i := range pdbs {
 		p := &pdbs[i]
-		if p.Namespace != pod.Namespace || p.Spec.Selector == nil || p.Status.DisruptionsAllowed > 0 {
+		if p.Namespace != pod.Namespace || p.Spec.Selector == nil {
 			continue
 		}
 		selector, err := metav1.LabelSelectorAsSelector(p.Spec.Selector)
 		if err != nil || !selector.Matches(podLabels) {
 			continue
 		}
-		return p.Namespace + "/" + p.Name, true
+		out = append(out, p)
 	}
-	return "", false
+	return out
+}
+
+// pdbBlocking reproduces the decisions of the API server's eviction handler on the given
+// snapshot and returns the budget name and an operator-facing reason when the eviction would
+// be refused right now:
+//   - more than one budget selects the pod: the eviction subresource refuses such pods;
+//   - the pod is not Ready: allowed under unhealthyPodEvictionPolicy AlwaysAllow, or under the
+//     default IfHealthyBudget when currentHealthy >= desiredHealthy > 0; otherwise the budget applies;
+//   - the budget's status is not reconciled yet (observedGeneration < generation): refused (429);
+//   - disruptionsAllowed is 0: refused (429).
+func pdbBlocking(pod corev1.Pod, pdbs []policyv1.PodDisruptionBudget) (name, reason string, blocked bool) {
+	matched := matchingPDBs(pod, pdbs)
+	if len(matched) == 0 {
+		return "", "", false
+	}
+	if len(matched) > 1 {
+		names := make([]string, 0, len(matched))
+		for _, p := range matched {
+			names = append(names, p.Namespace+"/"+p.Name)
+		}
+		return names[0], fmt.Sprintf("covered by %d PodDisruptionBudgets (%s); the Eviction API refuses pods covered by more than one budget", len(matched), strings.Join(names, ", ")), true
+	}
+	p := matched[0]
+	name = p.Namespace + "/" + p.Name
+
+	if !isPodReady(pod) {
+		if p.Spec.UnhealthyPodEvictionPolicy != nil && *p.Spec.UnhealthyPodEvictionPolicy == policyv1.AlwaysAllow {
+			return "", "", false
+		}
+		if p.Status.CurrentHealthy >= p.Status.DesiredHealthy && p.Status.DesiredHealthy > 0 {
+			return "", "", false
+		}
+	}
+	if p.Status.ObservedGeneration < p.Generation {
+		return name, fmt.Sprintf("PodDisruptionBudget %s has not been reconciled yet (observedGeneration %d < generation %d); evictions are refused until its status is up to date", name, p.Status.ObservedGeneration, p.Generation), true
+	}
+	if p.Status.DisruptionsAllowed <= 0 {
+		return name, fmt.Sprintf("PodDisruptionBudget %s currently allows no disruptions; the eviction may be refused until the budget recovers", name), true
+	}
+	return "", "", false
 }
 
 // DrainPlanOptions echoes the options a plan was evaluated with, so the caller never has to
@@ -150,8 +216,9 @@ type DrainPlan struct {
 
 // PlanNodeDrain lists the pods on nodeName and classifies each one with ClassifyPodForDrain.
 // It performs only reads with the given client, so it runs under the caller's RBAC identity and
-// never cordons or evicts. PDBs are listed per namespace; a namespace whose PDBs cannot be read
-// is classified without PDB knowledge and reported as such instead of pretending no PDB exists.
+// never cordons or evicts. PDBs are listed per namespace and only for namespaces holding pods
+// whose decision depends on them; a namespace whose PDBs cannot be read is classified without
+// PDB knowledge and reported as such instead of pretending no PDB exists.
 func PlanNodeDrain(ctx context.Context, client kubernetes.Interface, nodeName string, opts DrainOptions) (*DrainPlan, error) {
 	if _, err := client.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}); err != nil {
 		return nil, err
@@ -177,10 +244,21 @@ func PlanNodeDrain(ctx context.Context, client kubernetes.Interface, nodeName st
 		PDBsEvaluated: true,
 	}
 
+	// First pass without budgets: skip decisions and PDB bypasses are final and never need a
+	// PDB, so budgets are listed only for namespaces where a pod could reach the budget check.
+	firstPass := make([]PodDrainDecision, len(podList.Items))
+	needsPDB := map[string]bool{}
+	for i, pod := range podList.Items {
+		firstPass[i] = ClassifyPodForDrain(pod, opts, nil)
+		if firstPass[i].Outcome != DrainOutcomeSkip && !bypassesPDB(pod) {
+			needsPDB[pod.Namespace] = true
+		}
+	}
+
 	// PDBs per namespace: nil marks "could not list", an empty slice marks "listed, none".
 	pdbsByNamespace := map[string][]policyv1.PodDisruptionBudget{}
 	for _, pod := range podList.Items {
-		if _, seen := pdbsByNamespace[pod.Namespace]; seen {
+		if _, seen := pdbsByNamespace[pod.Namespace]; seen || !needsPDB[pod.Namespace] {
 			continue
 		}
 		list, err := client.PolicyV1().PodDisruptionBudgets(pod.Namespace).List(ctx, metav1.ListOptions{})
@@ -199,8 +277,11 @@ func PlanNodeDrain(ctx context.Context, client kubernetes.Interface, nodeName st
 		pdbsByNamespace[pod.Namespace] = pdbs
 	}
 
-	for _, pod := range podList.Items {
-		d := ClassifyPodForDrain(pod, opts, pdbsByNamespace[pod.Namespace])
+	for i, pod := range podList.Items {
+		d := firstPass[i]
+		if needsPDB[pod.Namespace] && pdbsByNamespace[pod.Namespace] != nil {
+			d = ClassifyPodForDrain(pod, opts, pdbsByNamespace[pod.Namespace])
+		}
 		plan.Pods = append(plan.Pods, d)
 		switch d.Outcome {
 		case DrainOutcomeEvict: