From fa75c097966d4c04b34038f57ef9e35c1acaa349 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 12:53:21 +0000 Subject: [PATCH 1/4] controller: Refactor drain helper construction Extract the drain.Helper setup into a shared constructor so drain diagnostics can inspect the same pod set the drainer uses. Also make the drain stall threshold configurable on the reconciler so tests can use a short threshold; the default remains five minutes. Assisted-by: AI Signed-off-by: Alice Frosi --- .../controller/bootcnodepool_controller.go | 4 ++ internal/controller/events.go | 14 ++++- internal/controller/rollout.go | 52 ++++++++++++------- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index c7a9f9bb..e69a35d3 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -69,6 +69,10 @@ type BootcNodePoolReconciler struct { TagResolver TagResolver TagResolutionInterval time.Duration + // DrainStallThreshold is how long a drain may run before the + // controller reports it as stalled. Zero uses the default. + DrainStallThreshold time.Duration + // PullSecretNamespaces is the allowlist of namespaces pools may // reference pull secrets from. The manager's Secret cache must be // restricted to the same namespaces. diff --git a/internal/controller/events.go b/internal/controller/events.go index 3d5389b0..a7802571 100644 --- a/internal/controller/events.go +++ b/internal/controller/events.go @@ -37,6 +37,15 @@ const ( eventNoteSuffix = "..." ) +// effectiveDrainStallThreshold returns the configured drain stall threshold, +// falling back to the default when the reconciler field is unset. +func (r *BootcNodePoolReconciler) effectiveDrainStallThreshold() time.Duration { + if r.DrainStallThreshold > 0 { + return r.DrainStallThreshold + } + return drainStallThreshold +} + // EventNote renders the human-readable message of a Kubernetes Event. Each // implementation owns a small set of well-defined fields and is responsible for // producing a message that fits the events.k8s.io/v1 1 KiB note limit. Notes @@ -369,6 +378,7 @@ func (r *BootcNodePoolReconciler) recordDrainStalls( nodes map[string]*bootcv1alpha1.BootcNode, ) time.Duration { now := time.Now() + threshold := r.effectiveDrainStallThreshold() var stalledNodes []*bootcv1alpha1.BootcNode var nextCheck time.Duration @@ -378,7 +388,7 @@ func (r *BootcNodePoolReconciler) recordDrainStalls( continue } - remaining := drainStallThreshold - now.Sub(status.startTime) + remaining := threshold - now.Sub(status.startTime) if remaining > 0 { nextCheck = earlierRequeue(nextCheck, remaining) continue @@ -398,7 +408,7 @@ func (r *BootcNodePoolReconciler) recordDrainStalls( corev1.EventTypeWarning, eventReasonDrainTakingTooLong, eventActionDrain, - drainStalledNote{Threshold: drainStallThreshold}, + drainStalledNote{Threshold: threshold}, ) } diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index 0c69cddd..b1d4aee9 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -648,26 +648,10 @@ func (r *BootcNodePoolReconciler) ensureDrain( } r.drains[bn.Name] = status - var drainTimeout time.Duration // 0 means no timeout - if pool.Spec.Rollout != nil && pool.Spec.Rollout.DrainTimeoutSeconds != nil { - drainTimeout = time.Duration(*pool.Spec.Rollout.DrainTimeoutSeconds) * time.Second - } - - drainLog := log.WithValues("node", bn.Name) + var drainTimeout = resolveDrainTimeout(pool) go func() { - drainer := &drain.Helper{ - Client: r.KubeClient, - Ctx: drainCtx, - Timeout: drainTimeout, - EvictErrorRetryDelay: 5 * time.Second, - Force: true, - IgnoreAllDaemonSets: true, - DeleteEmptyDirData: true, - GracePeriodSeconds: -1, - Out: newDrainOutWriter(drainLog, bn.Name), - ErrOut: newDrainErrWriter(drainLog, bn.Name), - } + drainer := r.newDrainHelper(drainCtx, bn.Name, drainTimeout) status.result <- drain.RunNodeDrain(drainer, bn.Name) // Re-enqueue the owning pool so the reconciler picks up the @@ -680,6 +664,29 @@ func (r *BootcNodePoolReconciler) ensureDrain( }() } +// newDrainHelper builds the drain.Helper configuration used for a node. It is +// shared by the drain goroutine and drain-stall diagnostics so both observe the +// same pod set. +func (r *BootcNodePoolReconciler) newDrainHelper( + ctx context.Context, + nodeName string, + timeout time.Duration, +) *drain.Helper { + log := logf.FromContext(ctx).WithValues("node", nodeName) + return &drain.Helper{ + Client: r.KubeClient, + Ctx: ctx, + Timeout: timeout, + EvictErrorRetryDelay: 5 * time.Second, + Force: true, + IgnoreAllDaemonSets: true, + DeleteEmptyDirData: true, + GracePeriodSeconds: -1, + Out: newDrainOutWriter(log, nodeName), + ErrOut: newDrainErrWriter(log, nodeName), + } +} + // collectDrainResults checks all in-progress drains for completed // results. On success, it sets desiredImageState to Booted on the // BootcNode. @@ -962,6 +969,15 @@ func (s nodeState) String() string { } } +// resolveDrainTimeout returns the configured drain timeout, or 0 when drains +// are allowed to run without a timeout. +func resolveDrainTimeout(pool *bootcv1alpha1.BootcNodePool) time.Duration { + if pool.Spec.Rollout == nil || pool.Spec.Rollout.DrainTimeoutSeconds == nil { + return 0 + } + return time.Duration(*pool.Spec.Rollout.DrainTimeoutSeconds) * time.Second +} + // resolveRebootTimeout returns the configured reboot timeout, or 0 if the // timeout is disabled (unset). func resolveRebootTimeout(pool *bootcv1alpha1.BootcNodePool) time.Duration { From f3b8f056cc5e4a48588bd04ffb9c91de50495e29 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 13:03:40 +0000 Subject: [PATCH 2/4] controller: Report drains blocked by PodDisruptionBudgets When a drain stalls past the stall threshold, inspect the pods the drain would evict and report the PodDisruptionBudgets that prevent eviction. Emit a DrainBlockedByPDB event, re-emitting only when the set of blocking budgets changes, and surface blocked nodes in the pool status message. When every occupied reboot slot is held by a blocked drain and the rollout has consumed all of its disruption budget, report UpToDate=False with reason PDBBlocked. Partial blocks keep the RolloutInProgress reason so existing behavior is unchanged. Stalled drains are rechecked every 30 seconds, and the controller now has permission to get and list PodDisruptionBudgets. Assisted-by: AI Signed-off-by: Alice Frosi --- api/v1alpha1/bootcnodepool_types.go | 5 + config/rbac/role.yaml | 7 + .../controller/bootcnodepool_controller.go | 6 +- internal/controller/events.go | 111 ++++++- internal/controller/events_test.go | 12 +- internal/controller/pdb.go | 292 ++++++++++++++++ internal/controller/pdb_test.go | 312 ++++++++++++++++++ internal/controller/rollout.go | 10 +- internal/controller/rollout_envtest_test.go | 188 +++++++++++ internal/controller/status.go | 44 +++ internal/controller/status_test.go | 149 +++++++++ internal/controller/suite_test.go | 4 + 12 files changed, 1118 insertions(+), 22 deletions(-) create mode 100644 internal/controller/pdb.go create mode 100644 internal/controller/pdb_test.go diff --git a/api/v1alpha1/bootcnodepool_types.go b/api/v1alpha1/bootcnodepool_types.go index 559c691a..26983a00 100644 --- a/api/v1alpha1/bootcnodepool_types.go +++ b/api/v1alpha1/bootcnodepool_types.go @@ -28,6 +28,11 @@ const ( // PoolPaused means updates are pending but the pool is paused. PoolPaused string = "Paused" + + // PoolPDBBlocked means stalled drains are blocked by + // PodDisruptionBudgets and that is keeping the rollout from making + // progress. + PoolPDBBlocked string = "PDBBlocked" ) // PoolDegraded condition reasons. Precedence ordering (which reason diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 50e4ed03..98ede860 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -83,6 +83,13 @@ rules: - bootcnodes/status verbs: - get +- apiGroups: + - policy + resources: + - poddisruptionbudgets + verbs: + - get + - list --- apiVersion: rbac.authorization.k8s.io/v1 kind: Role diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index e69a35d3..64d82105 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -48,6 +48,9 @@ type drainStatus struct { cancel context.CancelFunc // to abort on targetDigest change or node removal startTime time.Time // for stall detection isStalled bool // set after the one-shot drain stall event is emitted + // lastPDBBlockKey tracks the blocking PDB set already reported for this + // stall so a changed set re-emits the event and repeats stay quiet. + lastPDBBlockKey string } // TagResolver resolves a container image reference to a digest. @@ -99,6 +102,7 @@ type BootcNodePoolReconciler struct { // +kubebuilder:rbac:groups="",namespace=system,resources=secrets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list // +kubebuilder:rbac:groups="",resources=pods/eviction,verbs=create +// +kubebuilder:rbac:groups=policy,resources=poddisruptionbudgets,verbs=get;list // +kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=get // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch @@ -509,7 +513,7 @@ func (r *BootcNodePoolReconciler) Reconcile( } resolveResult.RequeueAfter = earlierRequeue( resolveResult.RequeueAfter, - r.recordDrainStalls(&pool, ownedBootcNodes), + r.recordDrainStalls(ctx, &pool, rs, ownedBootcNodes), ) // Early-return paths above (TargetDigest empty, InvalidSpec) skip diff --git a/internal/controller/events.go b/internal/controller/events.go index a7802571..0c809e25 100644 --- a/internal/controller/events.go +++ b/internal/controller/events.go @@ -5,6 +5,7 @@ package controller import ( "context" "fmt" + "slices" "strings" "time" "unicode/utf8" @@ -25,6 +26,7 @@ const ( eventReasonRolloutCompleted = "RolloutCompleted" eventReasonDrainFailed = "DrainFailed" eventReasonDrainTakingTooLong = "DrainTakingTooLong" + eventReasonDrainBlockedByPDB = "DrainBlockedByPDB" eventActionResolveImage = "ResolveImage" eventActionRollout = "Rollout" @@ -33,8 +35,11 @@ const ( eventActionDrain = "Drain" drainStallThreshold = 5 * time.Minute - eventNoteLimit = 1024 - eventNoteSuffix = "..." + // drainStallRequeueInterval is short enough to notice a PDB starting to + // allow evictions without generating excessive API traffic. + drainStallRequeueInterval = 30 * time.Second + eventNoteLimit = 1024 + eventNoteSuffix = "..." ) // effectiveDrainStallThreshold returns the configured drain stall threshold, @@ -162,6 +167,21 @@ func (n drainStalledNote) Note() string { ) } +// drainBlockedByPDBNote lists the PDBs preventing a drain from making +// progress. PDB names and hints are bounded by their own fields, but the note +// caps itself defensively because it aggregates several of them. +type drainBlockedByPDBNote struct { + Blocks []pdbBlock +} + +func (n drainBlockedByPDBNote) Note() string { + parts := make([]string, 0, len(n.Blocks)) + for _, block := range n.Blocks { + parts = append(parts, block.describe()) + } + return capNote("Drain blocked by " + strings.Join(parts, "; ")) +} + func (r *BootcNodePoolReconciler) recordPoolEvents( pool *bootcv1alpha1.BootcNodePool, previous *bootcv1alpha1.BootcNodePoolStatus, @@ -369,39 +389,55 @@ func (r *BootcNodePoolReconciler) recordDrainFailedEvent( ) } -// recordDrainStalls emits one warning per drain that crosses the stall -// threshold and returns when the next active drain should be checked. The -// existing in-memory drain state is sufficient because drains are restarted -// after a controller restart. +// recordDrainStalls emits a warning for each drain that crosses the stall +// threshold, then inspects stalled drains for PodDisruptionBudget blocks. While +// a drain remains stalled it returns a short requeue interval so PDB state is +// re-evaluated. The existing in-memory drain state is sufficient because drains +// are restarted after a controller restart. func (r *BootcNodePoolReconciler) recordDrainStalls( + ctx context.Context, pool *bootcv1alpha1.BootcNodePool, + rs *rolloutState, nodes map[string]*bootcv1alpha1.BootcNode, ) time.Duration { + log := logf.FromContext(ctx) now := time.Now() threshold := r.effectiveDrainStallThreshold() - var stalledNodes []*bootcv1alpha1.BootcNode + var newlyStalled []*bootcv1alpha1.BootcNode + var stalledNodes []string var nextCheck time.Duration r.drainsMu.Lock() for nodeName, status := range r.drains { - if status.isStalled { + // r.drains is shared by every pool, so only consider drains for + // nodes owned by the pool being reconciled. + if _, ok := nodes[nodeName]; !ok { continue } - remaining := threshold - now.Sub(status.startTime) - if remaining > 0 { - nextCheck = earlierRequeue(nextCheck, remaining) - continue + if !status.isStalled { + remaining := threshold - now.Sub(status.startTime) + if remaining > 0 { + nextCheck = earlierRequeue(nextCheck, remaining) + continue + } + + status.isStalled = true + if node, ok := nodes[nodeName]; ok { + newlyStalled = append(newlyStalled, node) + } } - status.isStalled = true - if node, ok := nodes[nodeName]; ok { - stalledNodes = append(stalledNodes, node) - } + stalledNodes = append(stalledNodes, nodeName) + nextCheck = earlierRequeue(nextCheck, drainStallRequeueInterval) } r.drainsMu.Unlock() - for _, node := range stalledNodes { + slices.Sort(stalledNodes) + slices.SortFunc(newlyStalled, func(a, b *bootcv1alpha1.BootcNode) int { + return strings.Compare(a.Name, b.Name) + }) + for _, node := range newlyStalled { r.recordEvent( node, pool, @@ -412,6 +448,47 @@ func (r *BootcNodePoolReconciler) recordDrainStalls( ) } + blocked := map[string][]pdbBlock{} + for _, nodeName := range stalledNodes { + node := nodes[nodeName] + + blocks, err := r.findPDBBlocksForNode(ctx, pool, nodeName) + if err != nil { + log.Error( + err, + "Failed to check PodDisruptionBudgets for stalled drain", + "node", nodeName, + ) + continue + } + + key := pdbBlocksKey(blocks) + r.drainsMu.Lock() + status := r.drains[nodeName] + emit := status != nil && key != "" && status.lastPDBBlockKey != key + if status != nil { + status.lastPDBBlockKey = key + } + r.drainsMu.Unlock() + + if len(blocks) == 0 { + continue + } + + blocked[nodeName] = blocks + if emit { + r.recordEvent( + node, + pool, + corev1.EventTypeWarning, + eventReasonDrainBlockedByPDB, + eventActionDrain, + drainBlockedByPDBNote{Blocks: blocks}, + ) + } + } + + rs.pdbBlocked = blocked return nextCheck } diff --git a/internal/controller/events_test.go b/internal/controller/events_test.go index 467c0cac..658334cb 100644 --- a/internal/controller/events_test.go +++ b/internal/controller/events_test.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" "sigs.k8s.io/controller-runtime/pkg/client" bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" @@ -686,15 +687,19 @@ func TestRecordDrainStalls(t *testing.T) { stalledStatus := &drainStatus{startTime: time.Now().Add(-drainStallThreshold)} freshStatus := &drainStatus{startTime: time.Now()} reconciler := &BootcNodePoolReconciler{ - Recorder: recorder, + Recorder: recorder, + KubeClient: fake.NewSimpleClientset(), drains: map[string]*drainStatus{ stalledNode.Name: stalledStatus, freshNode.Name: freshStatus, }, } + rs := &rolloutState{} nextCheck := reconciler.recordDrainStalls( + context.Background(), pool, + rs, map[string]*bootcv1alpha1.BootcNode{ stalledNode.Name: stalledNode, freshNode.Name: freshNode, @@ -703,8 +708,7 @@ func TestRecordDrainStalls(t *testing.T) { g.Expect(stalledStatus.isStalled).To(BeTrue()) g.Expect(freshStatus.isStalled).To(BeFalse()) - g.Expect(nextCheck).To(BeNumerically(">", drainStallThreshold-time.Second)) - g.Expect(nextCheck).To(BeNumerically("<=", drainStallThreshold)) + g.Expect(nextCheck).To(Equal(drainStallRequeueInterval)) g.Expect(recorder.events).To(Equal([]capturedEvent{{ regarding: stalledNode.Name, related: pool.Name, @@ -719,7 +723,9 @@ func TestRecordDrainStalls(t *testing.T) { // The isStalled marker suppresses duplicate events on later reconciles. reconciler.recordDrainStalls( + context.Background(), pool, + rs, map[string]*bootcv1alpha1.BootcNode{stalledNode.Name: stalledNode}, ) g.Expect(recorder.events).To(HaveLen(1)) diff --git a/internal/controller/pdb.go b/internal/controller/pdb.go new file mode 100644 index 00000000..064fe86d --- /dev/null +++ b/internal/controller/pdb.go @@ -0,0 +1,292 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "fmt" + "slices" + "strings" + + corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + logf "sigs.k8s.io/controller-runtime/pkg/log" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" +) + +const ( + // pdbMultipleMatchHint explains that a pod selected by more than one + // PodDisruptionBudget is a configuration error. + pdbMultipleMatchHint = "misconfiguration: pod is selected by multiple PodDisruptionBudgets" + + // pdbNeverAllowsEvictionHint explains a budget that cannot allow an + // eviction even when every pod is healthy. + pdbNeverAllowsEvictionHint = "never allows eviction (desiredHealthy >= expectedPods)" +) + +// pdbRef identifies a PodDisruptionBudget. +type pdbRef struct { + Namespace string + Name string +} + +func (r pdbRef) String() string { + return r.Namespace + "/" + r.Name +} + +// pdbBlock describes why a pod cannot be evicted through a +// PodDisruptionBudget. PDBs holds one budget normally, or every matching +// budget when a pod is selected by more than one. +type pdbBlock struct { + PDBs []pdbRef + Hint string +} + +// key returns a stable identifier for the blocking PDB set. +func (b pdbBlock) key() string { + refs := make([]string, len(b.PDBs)) + for i, ref := range b.PDBs { + refs[i] = ref.String() + } + slices.Sort(refs) + return strings.Join(refs, ",") +} + +// describe renders the block for an event note. +func (b pdbBlock) describe() string { + refs := make([]string, len(b.PDBs)) + for i, ref := range b.PDBs { + refs[i] = ref.String() + } + slices.Sort(refs) + + noun := "PodDisruptionBudget" + if len(refs) > 1 { + noun = "PodDisruptionBudgets" + } + return fmt.Sprintf("%s %s: %s", noun, strings.Join(refs, ", "), b.Hint) +} + +// pdbCanIgnorePod reports whether eviction bypasses PodDisruptionBudgets for +// this pod. The eviction API ignores budgets for pending, terminal, and +// already-terminating pods. +func pdbCanIgnorePod(pod *corev1.Pod) bool { + switch pod.Status.Phase { + case corev1.PodPending, corev1.PodSucceeded, corev1.PodFailed: + return true + } + return pod.DeletionTimestamp != nil +} + +// podReady reports whether the pod's Ready condition is true. +func podReady(pod *corev1.Pod) bool { + for _, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady { + return condition.Status == corev1.ConditionTrue + } + } + return false +} + +// unhealthyPodEvictableByPDB reports whether the pod may be evicted despite a +// zero disruption budget because it is unhealthy and the budget's unhealthy-pod +// eviction policy allows it. +func unhealthyPodEvictableByPDB(pod *corev1.Pod, pdb *policyv1.PodDisruptionBudget) bool { + if podReady(pod) { + return false + } + + policy := policyv1.IfHealthyBudget + if pdb.Spec.UnhealthyPodEvictionPolicy != nil { + policy = *pdb.Spec.UnhealthyPodEvictionPolicy + } + + switch policy { + case policyv1.AlwaysAllow: + return true + case policyv1.IfHealthyBudget: + return pdb.Status.DesiredHealthy > 0 && + pdb.Status.CurrentHealthy >= pdb.Status.DesiredHealthy + default: + return false + } +} + +// pdbBlockHint explains why a zero-budget PDB cannot make progress. +func pdbBlockHint(pdb *policyv1.PodDisruptionBudget) string { + if pdb.Status.CurrentHealthy < pdb.Status.ExpectedPods { + notReady := pdb.Status.ExpectedPods - pdb.Status.CurrentHealthy + return fmt.Sprintf("%d pod(s) not ready", notReady) + } + return pdbNeverAllowsEvictionHint +} + +// pdbBlockForPod returns why pod cannot be evicted because of pdbs, or nil if +// no PDB blocks it. +func pdbBlockForPod(pod *corev1.Pod, pdbs []policyv1.PodDisruptionBudget) (*pdbBlock, error) { + if pdbCanIgnorePod(pod) { + return nil, nil + } + + var matched []policyv1.PodDisruptionBudget + for i := range pdbs { + selector, err := metav1.LabelSelectorAsSelector(pdbs[i].Spec.Selector) + if err != nil { + return nil, fmt.Errorf( + "invalid selector on PodDisruptionBudget %s/%s: %w", + pdbs[i].Namespace, + pdbs[i].Name, + err, + ) + } + if selector.Matches(labels.Set(pod.Labels)) { + matched = append(matched, pdbs[i]) + } + } + + switch { + case len(matched) >= 2: + refs := make([]pdbRef, len(matched)) + for i := range matched { + refs[i] = pdbRef{Namespace: matched[i].Namespace, Name: matched[i].Name} + } + return &pdbBlock{PDBs: refs, Hint: pdbMultipleMatchHint}, nil + case len(matched) == 1: + pdb := &matched[0] + if pdb.Status.DisruptionsAllowed != 0 { + return nil, nil + } + if unhealthyPodEvictableByPDB(pod, pdb) { + return nil, nil + } + return &pdbBlock{ + PDBs: []pdbRef{{Namespace: pdb.Namespace, Name: pdb.Name}}, + Hint: pdbBlockHint(pdb), + }, nil + default: + return nil, nil + } +} + +// pdbBlocksForPods returns the unique PDB blocks for all pods. +func pdbBlocksForPods( + pods []corev1.Pod, + pdbsByNamespace map[string][]policyv1.PodDisruptionBudget, +) ([]pdbBlock, error) { + var blocks []pdbBlock + seen := map[string]bool{} + + for i := range pods { + block, err := pdbBlockForPod(&pods[i], pdbsByNamespace[pods[i].Namespace]) + if err != nil { + return nil, err + } + if block == nil || seen[block.key()] { + continue + } + seen[block.key()] = true + blocks = append(blocks, *block) + } + + slices.SortFunc(blocks, func(a, b pdbBlock) int { + return strings.Compare(a.key(), b.key()) + }) + return blocks, nil +} + +// pdbBlocksKey returns a stable key for a set of blocks so callers can detect +// when the blocking PDBs change. +func pdbBlocksKey(blocks []pdbBlock) string { + keys := make([]string, len(blocks)) + for i := range blocks { + keys[i] = blocks[i].key() + } + slices.Sort(keys) + return strings.Join(keys, ";") +} + +// pdbBlocksDescription names the PDBs in the blocks for status messages. +func pdbBlocksDescription(blocks []pdbBlock) string { + seen := map[string]bool{} + var refs []string + for _, block := range blocks { + for _, ref := range block.PDBs { + key := ref.String() + if seen[key] { + continue + } + seen[key] = true + refs = append(refs, key) + } + } + slices.Sort(refs) + + noun := "PDB" + if len(refs) > 1 { + noun = "PDBs" + } + return noun + " " + strings.Join(refs, ", ") +} + +// findPDBBlocksForNode lists the pods the drainer would evict from nodeName and +// reports which PodDisruptionBudgets block them. +func (r *BootcNodePoolReconciler) findPDBBlocksForNode( + ctx context.Context, + pool *bootcv1alpha1.BootcNodePool, + nodeName string, +) ([]pdbBlock, error) { + log := logf.FromContext(ctx) + + helper := r.newDrainHelper(ctx, nodeName, resolveDrainTimeout(pool)) + podList, errs := helper.GetPodsForDeletion(nodeName) + for _, err := range errs { + log.V(1).Info( + "Ignoring pod-list warning while checking PodDisruptionBudgets", + "node", nodeName, + "error", err, + ) + } + if podList == nil { + return nil, fmt.Errorf("listing pods to drain from node %s failed", nodeName) + } + + var pods []corev1.Pod + namespaces := map[string]bool{} + for _, pod := range podList.Pods() { + if pod.DeletionTimestamp != nil { + continue + } + pods = append(pods, pod) + namespaces[pod.Namespace] = true + } + if len(pods) == 0 { + return nil, nil + } + + namespaceNames := make([]string, 0, len(namespaces)) + for namespace := range namespaces { + namespaceNames = append(namespaceNames, namespace) + } + slices.Sort(namespaceNames) + + pdbsByNamespace := make(map[string][]policyv1.PodDisruptionBudget, len(namespaceNames)) + for _, namespace := range namespaceNames { + pdbList, err := r.KubeClient.PolicyV1().PodDisruptionBudgets(namespace).List( + ctx, + metav1.ListOptions{}, + ) + if err != nil { + return nil, fmt.Errorf( + "listing PodDisruptionBudgets in namespace %s: %w", + namespace, + err, + ) + } + pdbsByNamespace[namespace] = pdbList.Items + } + + return pdbBlocksForPods(pods, pdbsByNamespace) +} diff --git a/internal/controller/pdb_test.go b/internal/controller/pdb_test.go new file mode 100644 index 00000000..9a3d1639 --- /dev/null +++ b/internal/controller/pdb_test.go @@ -0,0 +1,312 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "testing" + "time" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +func testPDBPod(labels map[string]string, ready, terminating bool) *corev1.Pod { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Namespace: "default", + Labels: labels, + }, + } + if terminating { + now := metav1.Now() + pod.DeletionTimestamp = &now + } + pod.Status.Phase = corev1.PodRunning + status := corev1.ConditionFalse + if ready { + status = corev1.ConditionTrue + } + pod.Status.Conditions = []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: status, + }} + return pod +} + +func testPDB( + name string, + selector *metav1.LabelSelector, + allowed, current, expected, desired int32, + policy *policyv1.UnhealthyPodEvictionPolicyType, +) policyv1.PodDisruptionBudget { + return policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: selector, + UnhealthyPodEvictionPolicy: policy, + }, + Status: policyv1.PodDisruptionBudgetStatus{ + DisruptionsAllowed: allowed, + CurrentHealthy: current, + ExpectedPods: expected, + DesiredHealthy: desired, + }, + } +} + +func TestPDBBlockForPod(t *testing.T) { + matching := &metav1.LabelSelector{MatchLabels: map[string]string{"app": "demo"}} + empty := &metav1.LabelSelector{} + other := &metav1.LabelSelector{MatchLabels: map[string]string{"app": "other"}} + alwaysAllow := policyv1.AlwaysAllow + ifHealthyBudget := policyv1.IfHealthyBudget + + tests := []struct { + name string + pod *corev1.Pod + pdbs []policyv1.PodDisruptionBudget + wantPDBs []string + wantHint string + }{ + { + name: "zero budget blocks a ready pod", + pod: testPDBPod(map[string]string{"app": "demo"}, true, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 1, 1, 1, nil), + }, + wantPDBs: []string{"default/demo"}, + wantHint: pdbNeverAllowsEvictionHint, + }, + { + name: "available disruption does not block", + pod: testPDBPod(map[string]string{"app": "demo"}, true, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 1, 1, 1, 1, nil), + }, + }, + { + name: "terminating pod is ignored", + pod: testPDBPod(map[string]string{"app": "demo"}, true, true), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 1, 1, 1, nil), + }, + }, + { + name: "pending pod is ignored", + pod: func() *corev1.Pod { + pod := testPDBPod(map[string]string{"app": "demo"}, false, false) + pod.Status.Phase = corev1.PodPending + return pod + }(), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 0, 1, 1, nil), + }, + }, + { + name: "succeeded pod is ignored", + pod: func() *corev1.Pod { + pod := testPDBPod(map[string]string{"app": "demo"}, false, false) + pod.Status.Phase = corev1.PodSucceeded + return pod + }(), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 0, 1, 1, nil), + }, + }, + { + name: "empty selector matches every pod in the namespace", + pod: testPDBPod(map[string]string{"app": "demo"}, true, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("everything", empty, 0, 1, 1, 1, nil), + }, + wantPDBs: []string{"default/everything"}, + wantHint: pdbNeverAllowsEvictionHint, + }, + { + name: "non-matching selector does not block", + pod: testPDBPod(map[string]string{"app": "demo"}, true, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("other", other, 0, 1, 1, 1, nil), + }, + }, + { + name: "AlwaysAllow evicts an unhealthy pod", + pod: testPDBPod(map[string]string{"app": "demo"}, false, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 0, 2, 2, &alwaysAllow), + }, + }, + { + name: "AlwaysAllow does not evict a healthy pod", + pod: testPDBPod(map[string]string{"app": "demo"}, true, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 1, 1, 1, &alwaysAllow), + }, + wantPDBs: []string{"default/demo"}, + wantHint: pdbNeverAllowsEvictionHint, + }, + { + name: "IfHealthyBudget evicts an unhealthy pod when healthy", + pod: testPDBPod(map[string]string{"app": "demo"}, false, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 1, 3, 1, &ifHealthyBudget), + }, + }, + { + name: "IfHealthyBudget blocks an unhealthy pod without budget", + pod: testPDBPod(map[string]string{"app": "demo"}, false, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 1, 3, 2, &ifHealthyBudget), + }, + wantPDBs: []string{"default/demo"}, + wantHint: "2 pod(s) not ready", + }, + { + name: "IfHealthyBudget requires a nonzero desired healthy", + pod: testPDBPod(map[string]string{"app": "demo"}, false, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 0, 1, 0, &ifHealthyBudget), + }, + wantPDBs: []string{"default/demo"}, + wantHint: "1 pod(s) not ready", + }, + { + name: "default policy behaves as IfHealthyBudget", + pod: testPDBPod(map[string]string{"app": "demo"}, false, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("demo", matching, 0, 1, 3, 2, nil), + }, + wantPDBs: []string{"default/demo"}, + wantHint: "2 pod(s) not ready", + }, + { + name: "multiple matching PDBs are a misconfiguration", + pod: testPDBPod(map[string]string{"app": "demo"}, true, false), + pdbs: []policyv1.PodDisruptionBudget{ + testPDB("first", matching, 1, 1, 1, 1, nil), + testPDB("second", matching, 1, 1, 1, 1, nil), + }, + wantPDBs: []string{"default/first", "default/second"}, + wantHint: pdbMultipleMatchHint, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + block, err := pdbBlockForPod(tt.pod, tt.pdbs) + g.Expect(err).NotTo(HaveOccurred()) + + if tt.wantPDBs == nil { + g.Expect(block).To(BeNil()) + return + } + + g.Expect(block).NotTo(BeNil()) + g.Expect(block.Hint).To(Equal(tt.wantHint)) + names := make([]string, len(block.PDBs)) + for i, ref := range block.PDBs { + names[i] = ref.String() + } + g.Expect(names).To(ConsistOf(tt.wantPDBs)) + }) + } +} + +func TestDrainBlockedByPDBNote(t *testing.T) { + tests := []struct { + name string + blocks []pdbBlock + want string + }{ + { + name: "single PDB", + blocks: []pdbBlock{{ + PDBs: []pdbRef{{Namespace: "default", Name: "web"}}, + Hint: "2 pod(s) not ready", + }}, + want: "Drain blocked by PodDisruptionBudget default/web: 2 pod(s) not ready", + }, + { + name: "multiple PDBs", + blocks: []pdbBlock{{ + PDBs: []pdbRef{ + {Namespace: "default", Name: "web"}, + {Namespace: "default", Name: "api"}, + }, + Hint: pdbMultipleMatchHint, + }}, + want: "Drain blocked by PodDisruptionBudgets default/api, default/web: " + + pdbMultipleMatchHint, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(drainBlockedByPDBNote{Blocks: tt.blocks}.Note()).To(Equal(tt.want)) + }) + } +} + +func TestRecordDrainStallsPDBBlocked(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + pod := testPDBPod(map[string]string{"app": "demo"}, true, false) + pod.Name = "blocked-pod" + pod.Spec.NodeName = "pdb-worker" + + selector := &metav1.LabelSelector{MatchLabels: map[string]string{"app": "demo"}} + pdb := testPDB("budget-one", selector, 0, 1, 1, 1, nil) + + kubeClient := fake.NewSimpleClientset(pod, &pdb) + recorder := &capturingEventRecorder{} + status := &drainStatus{startTime: time.Now().Add(-time.Hour)} + reconciler := &BootcNodePoolReconciler{ + Recorder: recorder, + KubeClient: kubeClient, + DrainStallThreshold: time.Millisecond, + drains: map[string]*drainStatus{pod.Spec.NodeName: status}, + } + pool := testutil.NewPool("pdb-pool", testImageDigestRefA, testutil.WithWorkerSelector()) + node := testutil.NewBootcNode(pod.Spec.NodeName, testImageDigestRefA) + rs := &rolloutState{} + nodes := map[string]*bootcv1alpha1.BootcNode{node.Name: node} + + nextCheck := reconciler.recordDrainStalls(ctx, pool, rs, nodes) + g.Expect(nextCheck).To(Equal(drainStallRequeueInterval)) + g.Expect(rs.pdbBlocked).To(HaveKey(node.Name)) + g.Expect(rs.pdbBlocked[node.Name]).To(HaveLen(1)) + g.Expect(recorder.events).To(HaveLen(2)) + g.Expect(recorder.events[0].reason).To(Equal(eventReasonDrainTakingTooLong)) + g.Expect(recorder.events[1].reason).To(Equal(eventReasonDrainBlockedByPDB)) + g.Expect(recorder.events[1].note).To(ContainSubstring("default/budget-one")) + + // The same blocking PDB set does not re-emit the event. + reconciler.recordDrainStalls(ctx, pool, rs, nodes) + g.Expect(recorder.events).To(HaveLen(2)) + + // Changing the blocking PDB set re-emits the event. + g.Expect(kubeClient.PolicyV1().PodDisruptionBudgets("default").Delete( + ctx, pdb.Name, metav1.DeleteOptions{}, + )).To(Succeed()) + replacement := testPDB("budget-two", selector, 0, 1, 1, 1, nil) + _, err := kubeClient.PolicyV1().PodDisruptionBudgets("default").Create( + ctx, &replacement, metav1.CreateOptions{}, + ) + g.Expect(err).NotTo(HaveOccurred()) + + reconciler.recordDrainStalls(ctx, pool, rs, nodes) + g.Expect(recorder.events).To(HaveLen(3)) + g.Expect(recorder.events[2].reason).To(Equal(eventReasonDrainBlockedByPDB)) + g.Expect(recorder.events[2].note).To(ContainSubstring("default/budget-two")) +} diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go index b1d4aee9..ab16c043 100644 --- a/internal/controller/rollout.go +++ b/internal/controller/rollout.go @@ -56,6 +56,13 @@ type rolloutState struct { // BootcNodes with the in-reboot-slot annotation occupiedSlots int + // maxUnavailable is the effective rollout concurrency for this pass. + maxUnavailable int + + // pdbBlocked maps node names to the PodDisruptionBudgets blocking their + // stalled drains. + pdbBlocked map[string][]pdbBlock + // requeueAfter, when non-zero, asks the reconciler to requeue the // pool after this duration. Used for time-based transitions like the // reboot timeout, which fire without any incoming event. @@ -151,6 +158,7 @@ func (r *BootcNodePoolReconciler) driveRollout( if err != nil { return nil, err } + rs.maxUnavailable = maxUnavail availableSlots := max(0, maxUnavail-rs.occupiedSlots) slotted, unslotted := partitionSlottedNodes(rs.staged) @@ -648,7 +656,7 @@ func (r *BootcNodePoolReconciler) ensureDrain( } r.drains[bn.Name] = status - var drainTimeout = resolveDrainTimeout(pool) + drainTimeout := resolveDrainTimeout(pool) go func() { drainer := r.newDrainHelper(drainCtx, bn.Name, drainTimeout) diff --git a/internal/controller/rollout_envtest_test.go b/internal/controller/rollout_envtest_test.go index 62b92722..e7659e3b 100644 --- a/internal/controller/rollout_envtest_test.go +++ b/internal/controller/rollout_envtest_test.go @@ -5,15 +5,18 @@ package controller import ( "context" "testing" + "time" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + eventsv1 "k8s.io/api/events/v1" policyv1 "k8s.io/api/policy/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/util/retry" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" @@ -558,6 +561,188 @@ func setNodeReady(g Gomega, ctx context.Context, nodeName string) { g.Expect(k8sClient.Status().Patch(ctx, modified, client.MergeFrom(&node))).To(Succeed()) } +// TestDrainBlockedByPDB verifies that a stalled drain reports the blocking +// PodDisruptionBudget through an event and the pool's PDBBlocked condition, and +// that both clear when the budget starts allowing evictions. +func TestDrainBlockedByPDB(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + const ( + poolName = "pdb-blocked-pool" + nodeName = "pdb-blocked-w1" + namespace = "default" + ) + + node := testutil.NewK8sNode(nodeName, testutil.WorkerLabels()) + g.Expect(k8sClient.Create(ctx, node)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, node) + }) + + podLabels := map[string]string{"app": "pdb-block-" + nodeName} + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pdb-blocker-" + nodeName, + Namespace: namespace, + Labels: podLabels, + }, + Spec: corev1.PodSpec{ + NodeName: nodeName, + // envtest has no kubelet to finalize pod deletion, so make + // eviction remove the pod immediately. + TerminationGracePeriodSeconds: ptr.To[int64](0), + Containers: []corev1.Container{{ + Name: "pause", + Image: "registry.k8s.io/pause:3.9", + }}, + }, + } + g.Expect(k8sClient.Create(ctx, pod)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, pod) + }) + + pod.Status.Phase = corev1.PodRunning + pod.Status.Conditions = []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }} + g.Expect(k8sClient.Status().Update(ctx, pod)).To(Succeed()) + + minAvail := intstr.FromInt32(1) + pdb := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pdb-block-" + nodeName, + Namespace: namespace, + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + MinAvailable: &minAvail, + Selector: &metav1.LabelSelector{ + MatchLabels: podLabels, + }, + }, + } + g.Expect(k8sClient.Create(ctx, pdb)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, pdb) + }) + + // envtest does not run the disruption controller, so populate the PDB + // status as if one healthy pod filled the entire budget. + g.Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + var current policyv1.PodDisruptionBudget + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pdb), ¤t); err != nil { + return err + } + current.Status.DisruptionsAllowed = 0 + current.Status.CurrentHealthy = 1 + current.Status.DesiredHealthy = 1 + current.Status.ExpectedPods = 1 + return k8sClient.Status().Update(ctx, ¤t) + })).To(Succeed()) + + pool := testutil.NewPool(poolName, testImageDigestRefB, + testutil.WithWorkerSelector(), + testutil.WithMaxUnavailable(intstr.FromInt32(1)), + ) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, pool) + }) + + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bootcv1alpha1.BootcNode{}) + }).Should(Succeed()) + + simulateDaemonStatus(g, ctx, nodeName, testDigestA, bootcv1alpha1.NodeReasonStaged) + + g.Eventually(func() (map[string]string, error) { + var bn bootcv1alpha1.BootcNode + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bn) + return bn.Annotations, err + }).Should(HaveKey(bootcv1alpha1.AnnotationInRebootSlot)) + + var bootcNode bootcv1alpha1.BootcNode + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bootcNode)).To(Succeed()) + + g.Eventually(func() ([]eventsv1.Event, error) { + return eventsForObject(ctx, "BootcNode", nodeName, bootcNode.UID) + }).Should(ContainElement(And( + HaveField("Type", "Warning"), + HaveField("Reason", eventReasonDrainBlockedByPDB), + HaveField("Note", ContainSubstring("default/"+pdb.Name)), + HaveField("Note", ContainSubstring("never allows eviction")), + ))) + + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKey{Name: poolName}, &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", bootcv1alpha1.PoolPDBBlocked), + HaveField("Message", ContainSubstring(nodeName+" blocked by PDB")), + ))) + + // Relax the budget. envtest does not run the disruption controller, so + // also write the status it would produce for minAvailable: 0. + g.Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + var current policyv1.PodDisruptionBudget + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pdb), ¤t); err != nil { + return err + } + zero := intstr.FromInt32(0) + current.Spec.MinAvailable = &zero + return k8sClient.Update(ctx, ¤t) + })).To(Succeed()) + g.Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + var current policyv1.PodDisruptionBudget + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pdb), ¤t); err != nil { + return err + } + current.Status.ObservedGeneration = current.Generation + current.Status.DisruptionsAllowed = 1 + current.Status.CurrentHealthy = 1 + current.Status.DesiredHealthy = 0 + current.Status.ExpectedPods = 1 + return k8sClient.Status().Update(ctx, ¤t) + })).To(Succeed()) + + // The drain retries eviction after its configured retry delay, so allow + // enough time for it to observe the relaxed budget and finish. + g.Eventually(func() (bootcv1alpha1.DesiredImageState, error) { + var bn bootcv1alpha1.BootcNode + err := k8sClient.Get(ctx, client.ObjectKey{Name: nodeName}, &bn) + return bn.Spec.DesiredImageState, err + }).WithTimeout(20 * time.Second).Should(Equal(bootcv1alpha1.DesiredImageStateBooted)) + + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKey{Name: poolName}, &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Reason", bootcv1alpha1.PoolRolloutInProgress), + ))) + + simulateDaemonStatus(g, ctx, nodeName, testDigestB, bootcv1alpha1.NodeReasonIdle) + setNodeReady(g, ctx, nodeName) + + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKey{Name: poolName}, &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolAllUpdated), + ))) +} + // TestNodeLeavesPoolCancelsDrain verifies that when a node leaves the pool // while a drain is in progress, the drain is cancelled and the node is // cleaned up: BootcNode deleted, managed label removed, and node uncordoned. @@ -591,6 +776,9 @@ func TestNodeLeavesPoolCancelsDrain(t *testing.T) { }, Spec: corev1.PodSpec{ NodeName: nodeName, + // envtest has no kubelet to finalize pod deletion, so make + // eviction remove the pod immediately. + TerminationGracePeriodSeconds: ptr.To[int64](0), Containers: []corev1.Container{{ Name: "pause", Image: "registry.k8s.io/pause:3.9", diff --git a/internal/controller/status.go b/internal/controller/status.go index 266d0ac4..3d40ee13 100644 --- a/internal/controller/status.go +++ b/internal/controller/status.go @@ -5,6 +5,7 @@ package controller import ( "fmt" "math" + "slices" "strings" apimeta "k8s.io/apimachinery/pkg/api/meta" @@ -59,6 +60,13 @@ func syncUpToDateCondition(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) Reason: bootcv1alpha1.PoolPaused, Message: rolloutBreakdown(pool, rs), }) + case pool.Status.NodeCount != pool.Status.UpdatedCount && allSlotsPDBBlocked(rs): + apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ + Type: bootcv1alpha1.PoolUpToDate, + Status: metav1.ConditionFalse, + Reason: bootcv1alpha1.PoolPDBBlocked, + Message: rolloutBreakdown(pool, rs), + }) case pool.Status.NodeCount != pool.Status.UpdatedCount: apimeta.SetStatusCondition(&pool.Status.Conditions, metav1.Condition{ Type: bootcv1alpha1.PoolUpToDate, @@ -75,6 +83,30 @@ func syncUpToDateCondition(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) } } +// allSlotsPDBBlocked reports whether every occupied reboot slot is held by a +// stalled drain blocked by a PodDisruptionBudget, and the rollout has consumed +// all of its disruption budget. +func allSlotsPDBBlocked(rs *rolloutState) bool { + if rs.maxUnavailable <= 0 || + rs.occupiedSlots == 0 || + rs.occupiedSlots < rs.maxUnavailable || + len(rs.pdbBlocked) == 0 { + return false + } + + slotted := 0 + for _, bn := range rs.staged { + if !metav1.HasAnnotation(bn.ObjectMeta, bootcv1alpha1.AnnotationInRebootSlot) { + continue + } + slotted++ + if _, ok := rs.pdbBlocked[bn.Name]; !ok { + return false + } + } + return slotted > 0 && slotted == rs.occupiedSlots +} + func rolloutBreakdown(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) string { var b strings.Builder fmt.Fprintf(&b, "%d/%d updated", len(rs.upToDate), rs.nodeCount()) @@ -98,5 +130,17 @@ func rolloutBreakdown(pool *bootcv1alpha1.BootcNodePool, rs *rolloutState) strin } fmt.Fprintf(&b, "%d %s", bk.count, bk.name) } + + if len(rs.pdbBlocked) > 0 { + nodeNames := make([]string, 0, len(rs.pdbBlocked)) + for nodeName := range rs.pdbBlocked { + nodeNames = append(nodeNames, nodeName) + } + slices.Sort(nodeNames) + for _, nodeName := range nodeNames { + fmt.Fprintf(&b, "; %s blocked by %s", + nodeName, pdbBlocksDescription(rs.pdbBlocked[nodeName])) + } + } return b.String() } diff --git a/internal/controller/status_test.go b/internal/controller/status_test.go index 296a11a0..fdfb639b 100644 --- a/internal/controller/status_test.go +++ b/internal/controller/status_test.go @@ -4,6 +4,7 @@ package controller import ( "math" + "slices" "testing" . "github.com/onsi/gomega" @@ -195,6 +196,154 @@ func TestSyncPoolStatusEmptyPool(t *testing.T) { ))) } +// pdbStagedNode builds a staged node, optionally holding a reboot slot. +func pdbStagedNode(name string, slotted bool) *bootcv1alpha1.BootcNode { + node := testutil.NewBootcNode(name, testImageDigestRefA) + if slotted { + node.Annotations = map[string]string{ + bootcv1alpha1.AnnotationInRebootSlot: "", + } + } + return node +} + +// Verifies that a fully consumed rollout blocked by PDBs reports +// UpToDate=False/PDBBlocked, while partial blocks keep RolloutInProgress with a +// per-node hint. Paused keeps priority over PDBBlocked. +func TestSyncPoolStatusPDBBlocked(t *testing.T) { + tests := []struct { + name string + paused bool + occupiedSlots int + maxUnavailable int + slottedNodes []string + blockedNodes []string + wantReason string + wantMessage string + }{ + { + name: "total block", + occupiedSlots: 2, + maxUnavailable: 2, + slottedNodes: []string{"n1", "n2"}, + blockedNodes: []string{"n1", "n2"}, + wantReason: bootcv1alpha1.PoolPDBBlocked, + wantMessage: "0/2 updated; 0 pending, 0 staging, 2 staged, 0 rebooting, " + + "0 degraded; n1 blocked by PDB default/a; " + + "n2 blocked by PDB default/b", + }, + { + name: "partial block", + occupiedSlots: 2, + maxUnavailable: 2, + slottedNodes: []string{"n1", "n2"}, + blockedNodes: []string{"n1"}, + wantReason: bootcv1alpha1.PoolRolloutInProgress, + wantMessage: "0/2 updated; 0 pending, 0 staging, 2 staged, 0 rebooting, " + + "0 degraded; n1 blocked by PDB default/a", + }, + { + name: "slots not fully consumed", + occupiedSlots: 1, + maxUnavailable: 2, + slottedNodes: []string{"n1"}, + blockedNodes: []string{"n1"}, + wantReason: bootcv1alpha1.PoolRolloutInProgress, + wantMessage: "0/2 updated; 0 pending, 0 staging, 2 staged, 0 rebooting, " + + "0 degraded; n1 blocked by PDB default/a", + }, + { + name: "paused has priority", + paused: true, + occupiedSlots: 2, + maxUnavailable: 2, + slottedNodes: []string{"n1", "n2"}, + blockedNodes: []string{"n1", "n2"}, + wantReason: bootcv1alpha1.PoolPaused, + wantMessage: "0/2 updated; 0 pending, 0 staging, 2 staged, 0 rebooting, " + + "0 degraded; n1 blocked by PDB default/a; " + + "n2 blocked by PDB default/b", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + pool := testutil.NewPool( + "test", + testImageDigestRefA, + testutil.WithWorkerSelector(), + testutil.WithPaused(tt.paused), + ) + pool.Status.TargetDigest = testDigestA + + rs := &rolloutState{ + staged: []*bootcv1alpha1.BootcNode{ + pdbStagedNode("n1", slices.Contains(tt.slottedNodes, "n1")), + pdbStagedNode("n2", slices.Contains(tt.slottedNodes, "n2")), + }, + occupiedSlots: tt.occupiedSlots, + maxUnavailable: tt.maxUnavailable, + pdbBlocked: map[string][]pdbBlock{}, + } + for _, nodeName := range tt.blockedNodes { + pdbName := "a" + if nodeName == "n2" { + pdbName = "b" + } + rs.pdbBlocked[nodeName] = []pdbBlock{{ + PDBs: []pdbRef{{Namespace: "default", Name: pdbName}}, + Hint: pdbNeverAllowsEvictionHint, + }} + } + + syncPoolStatus(pool, rs) + + g.Expect(pool.Status.Conditions).To(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", tt.wantReason), + HaveField("Message", tt.wantMessage), + ))) + }) + } +} + +// Verifies that the PDBBlocked reason clears when the blocking PDBs no longer +// block the drains. +func TestSyncPoolStatusPDBBlockedClears(t *testing.T) { + g := NewWithT(t) + + pool := testutil.NewPool("test", testImageDigestRefA, testutil.WithWorkerSelector()) + pool.Status.TargetDigest = testDigestA + + rs := &rolloutState{ + staged: []*bootcv1alpha1.BootcNode{pdbStagedNode("n1", true)}, + occupiedSlots: 1, + maxUnavailable: 1, + pdbBlocked: map[string][]pdbBlock{ + "n1": {{ + PDBs: []pdbRef{{Namespace: "default", Name: "a"}}, + Hint: pdbNeverAllowsEvictionHint, + }}, + }, + } + syncPoolStatus(pool, rs) + g.Expect(pool.Status.Conditions).To(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Reason", bootcv1alpha1.PoolPDBBlocked), + ))) + + rs.pdbBlocked = nil + syncPoolStatus(pool, rs) + g.Expect(pool.Status.Conditions).To(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Reason", bootcv1alpha1.PoolRolloutInProgress), + HaveField("Message", Not(ContainSubstring("blocked by PDB"))), + ))) +} + // Verifies that deployedDigest retains its previous value while a rollout // is in progress, and updateAvailable is true. func TestSyncPoolStatusDeployedDigestPreserved(t *testing.T) { diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index c6d2c194..58e9923e 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "testing" + "time" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -96,6 +97,9 @@ func TestMain(m *testing.M) { KubeClient: kubeClient, EventNamespace: testutil.OperatorNamespaceName, PullSecretNamespaces: pullSecretNamespaces, + // Use a short threshold so envtest can observe stalled drains + // without waiting the production default of five minutes. + DrainStallThreshold: 500 * time.Millisecond, } if err := testReconciler.SetupWithManager(mgr); err != nil { fmt.Fprintf(os.Stderr, "Failed to setup reconciler: %v\n", err) From 2412e185b7cd7f1f8a8559c14b9c03f12515cd4c Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 14:28:31 +0000 Subject: [PATCH 3/4] docs: Document PDB-blocked rollouts Add an operations section covering the DrainBlockedByPDB event, the PDBBlocked UpToDate reason, and how to resolve each hint. Add the PDBBlocked reason to the architecture condition table. Assisted-by: AI Signed-off-by: Alice Frosi --- docs/design/ARCHITECTURE.md | 1 + docs/src/operations/pool.md | 55 +++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/docs/design/ARCHITECTURE.md b/docs/design/ARCHITECTURE.md index cf52b47d..0b5f12b4 100644 --- a/docs/design/ARCHITECTURE.md +++ b/docs/design/ARCHITECTURE.md @@ -135,6 +135,7 @@ Pool conditions and their reasons: | UpToDate | True | AllUpdated | All nodes are running targetDigest | | UpToDate | False | RolloutInProgress | Nodes are actively being updated | | UpToDate | False | Paused | Updates pending but pool is paused | +| UpToDate | False | PDBBlocked | Stalled drains blocked by PodDisruptionBudgets | | Degraded | True | NodeConflict | Node selector overlaps with another pool | | Degraded | True | NodeDegraded | At least one node has errors or isn't converging | | Degraded | True | RolloutHalted | 2+ unhealthy nodes in reboot slots; rollout stopped | diff --git a/docs/src/operations/pool.md b/docs/src/operations/pool.md index 1bd235fb..49d4221c 100644 --- a/docs/src/operations/pool.md +++ b/docs/src/operations/pool.md @@ -97,6 +97,61 @@ kubectl get bootcnodes Each `BootcNode` status reflects the output of `bootc status` on that node: the booted image, any staged image, and the rollback entry. +## Rollout stuck on PodDisruptionBudget + +A drain can stall when a `PodDisruptionBudget` (PDB) refuses to let the +workloads on a node be evicted. Once a drain has been running for five +minutes, the operator inspects the pods it is trying to evict and reports the +blocking budget. + +The `BootcNode` receives a `DrainBlockedByPDB` warning event: + +```shell +kubectl describe bootcnode workers-w1 +``` + +``` +Warning DrainBlockedByPDB Drain Drain blocked by PodDisruptionBudget +default/web: 2 pod(s) not ready +``` + +While a drain remains stalled, the event is re-emitted only when the set of +blocking budgets changes, so it does not hide other events. + +When every occupied reboot slot is held by a drain blocked this way, the pool's +`UpToDate` condition reports `False` with reason `PDBBlocked`: + +```shell +kubectl get bootcnodepool workers -o yaml +``` + +```yaml +conditions: +- type: UpToDate + status: "False" + reason: PDBBlocked + message: "9/10 updated; 0 pending, 0 staging, 1 staged, 0 rebooting, 0 degraded; workers-w1 blocked by PDB default/web" +``` + +If only some slots are blocked, the reason stays `RolloutInProgress`, but the +message still names each affected node and budget. + +The message hint explains why the budget has no disruptions available: + +- `N pod(s) not ready`: fewer pods are healthy than the budget expects. + Bring the workload back to a healthy state, for example by scaling it up or + fixing failing pods. +- `never allows eviction (desiredHealthy >= expectedPods)`: the budget cannot + permit an eviction even when every pod is healthy. Relax the budget's + `minAvailable` or `maxUnavailable` for the duration of the rollout. +- `misconfiguration: pod is selected by multiple PodDisruptionBudgets`: + more than one budget selects the pod. Make the selectors mutually exclusive + so each pod is covered by a single budget. + +The operator keeps retrying the drain while the rollout is blocked and +continues normally as soon as the budget allows an eviction. The rollout +concurrency and reboot behavior are otherwise unchanged. + ## Pausing and resuming To pause a rollout (nodes already staging will complete, but no new reboots From fcd484b8e5c8cc2c37b143d502d7ae7ade146315 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 14:30:47 +0000 Subject: [PATCH 4/4] e2e: Test rollout blocked by PodDisruptionBudget Run a single-replica application with a zero disruption budget on a one-node pool and start a rollout. Verify the controller reports the blocking budget through a DrainBlockedByPDB event and the PDBBlocked pool condition while the node stays cordoned and does not reboot. Then relax the budget and verify the drain and rollout complete. Assisted-by: AI Signed-off-by: Alice Frosi --- test/e2e/pdb_test.go | 201 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 test/e2e/pdb_test.go diff --git a/test/e2e/pdb_test.go b/test/e2e/pdb_test.go new file mode 100644 index 00000000..d259be5e --- /dev/null +++ b/test/e2e/pdb_test.go @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "context" + "os" + "testing" + "time" + + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + "github.com/bootc-dev/bootc-operator/test/e2e/e2eutil" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +const ( + // defaultWorkloadImage is a tiny image that every kubelet already has + // as its pod sandbox image. Override with E2E_WORKLOAD_IMAGE when the + // nodes cannot pull it. + defaultWorkloadImage = "registry.k8s.io/pause:3.10" + + // drainStallWait is measured from patching the pool. It covers staging + // the image, taking a reboot slot, the controller's five minute drain + // stall threshold, and reconciling to report the blocking PDB. It is + // deliberately generous to absorb slow image pulls. + drainStallWait = 8 * time.Minute +) + +// TestRolloutBlockedByPDB provisions a single node running an application +// whose PodDisruptionBudget allows no disruptions, so the node cannot be +// drained. It verifies that the controller reports the blocking budget through +// a DrainBlockedByPDB event and the UpToDate=False/PDBBlocked pool condition +// without rebooting the node, then relaxes the budget and verifies that the +// drain and the rollout finish. +func TestRolloutBlockedByPDB(t *testing.T) { + e2eutil.Providers(t, "bink", "eks") + t.Parallel() + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + + env := e2eutil.New(t) + nodeName := env.AddNode(t) + + ctx := context.Background() + + pool := env.NewPool("pdb", env.NodeImageDigestedPullSpec()) + g.Expect(env.Client.Create(ctx, pool)).To(Succeed()) + + testutil.WaitForNodeIdle(t, g, ctx, env.Client, nodeName, 3*time.Minute) + t.Logf("Node %q is Idle with original image", nodeName) + + var bootcNode bootcv1alpha1.BootcNode + g.Expect(env.Client.Get(ctx, client.ObjectKey{Name: nodeName}, &bootcNode)).To(Succeed()) + + // Phase 1: Run a single-replica application on the node and protect it + // with a PDB that requires that replica to stay available. + workloadImage := defaultWorkloadImage + if img := os.Getenv("E2E_WORKLOAD_IMAGE"); img != "" { + workloadImage = img + } + appName := "pdb-" + env.TestID() + appLabels := map[string]string{"app": appName} + + deploy := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: appName, + Namespace: metav1.NamespaceDefault, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](1), + Selector: &metav1.LabelSelector{MatchLabels: appLabels}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: appLabels}, + Spec: corev1.PodSpec{ + NodeSelector: env.TestLabels(), + Containers: []corev1.Container{{ + Name: "app", + Image: workloadImage, + ImagePullPolicy: corev1.PullIfNotPresent, + }}, + }, + }, + }, + } + g.Expect(env.Client.Create(ctx, deploy)).To(Succeed()) + t.Cleanup(func() { + _ = env.Client.Delete(ctx, deploy) + }) + + pdb := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: appName, + Namespace: metav1.NamespaceDefault, + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + MinAvailable: ptr.To(intstr.FromInt32(1)), + Selector: &metav1.LabelSelector{MatchLabels: appLabels}, + }, + } + g.Expect(env.Client.Create(ctx, pdb)).To(Succeed()) + t.Cleanup(func() { + _ = env.Client.Delete(ctx, pdb) + }) + + pdbKey := client.ObjectKeyFromObject(pdb) + + g.Eventually(func() (policyv1.PodDisruptionBudgetStatus, error) { + var current policyv1.PodDisruptionBudget + err := env.Client.Get(ctx, pdbKey, ¤t) + return current.Status, err + }).WithTimeout(3*time.Minute).Should(And( + HaveField("CurrentHealthy", BeEquivalentTo(1)), + HaveField("ExpectedPods", BeEquivalentTo(1)), + HaveField("DisruptionsAllowed", BeEquivalentTo(0)), + ), "expected the PDB to allow no disruptions") + + t.Logf("Application %q is running with a zero disruption budget", appName) + + // Phase 2: Roll out a new image. The node stages it, takes a reboot + // slot, and then gets stuck draining because the PDB refuses eviction. + updateRef := env.NodeImageUpdateDigestedPullSpec() + + modified := pool.DeepCopy() + modified.Spec.Image.Ref = updateRef + g.Expect(env.Client.Patch(ctx, modified, client.MergeFrom(pool))).To(Succeed()) + *pool = *modified + + t.Logf("Patched pool to update image %s", updateRef) + + // Phase 3: The stall is reported on the node and on the pool. + g.Eventually(fetchEvents(ctx, env.Client, "BootcNode", nodeName, bootcNode.UID)). + WithTimeout(drainStallWait). + Should(ContainElement(And( + HaveField("Type", corev1.EventTypeWarning), + HaveField("Reason", "DrainBlockedByPDB"), + HaveField("Action", "Drain"), + HaveField("Note", ContainSubstring(metav1.NamespaceDefault+"/"+appName)), + )), "expected a DrainBlockedByPDB event naming the PDB") + + g.Eventually(fetchPoolStatus(ctx, env.Client, pool)).Should(HaveField( + "Conditions", ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolUpToDate), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", bootcv1alpha1.PoolPDBBlocked), + HaveField("Message", ContainSubstring(nodeName+" blocked by PDB")), + HaveField("Message", ContainSubstring(metav1.NamespaceDefault+"/"+appName)), + )), + ), "expected the pool to report PDBBlocked") + + // The node must stay cordoned, holding its slot, and must not have been + // told to reboot while the drain is blocked. + var blocked bootcv1alpha1.BootcNode + g.Expect(env.Client.Get(ctx, client.ObjectKey{Name: nodeName}, &blocked)).To(Succeed()) + g.Expect(blocked.Spec.DesiredImageState).To(Equal(bootcv1alpha1.DesiredImageStateStaged)) + g.Expect(blocked.Annotations).To(HaveKey(bootcv1alpha1.AnnotationInRebootSlot)) + + var node corev1.Node + g.Expect(env.Client.Get(ctx, client.ObjectKey{Name: nodeName}, &node)).To(Succeed()) + g.Expect(node.Spec.Unschedulable).To(BeTrue(), "expected the blocked node to stay cordoned") + + t.Logf("Rollout is reported as blocked by PDB %s/%s", metav1.NamespaceDefault, appName) + + // Phase 4: Allow disruptions and verify the drain and rollout resume. + g.Expect(env.Client.Get(ctx, pdbKey, pdb)).To(Succeed()) + relaxed := pdb.DeepCopy() + relaxed.Spec.MinAvailable = ptr.To(intstr.FromInt32(0)) + g.Expect(env.Client.Patch(ctx, relaxed, client.MergeFrom(pdb))).To(Succeed()) + + t.Logf("Relaxed PDB to minAvailable=0") + + testutil.WaitForNodeIdle(t, g, ctx, env.Client, nodeName, 8*time.Minute, + HaveField("Booted", imageMatchesDigest(env.NodeImageUpdateDigest())), + ) + + t.Logf("Node %q is Idle with update image", nodeName) + + g.Eventually(fetchPoolStatus(ctx, env.Client, pool)). + Should(poolAllUpdated(1, env.NodeImageUpdateDigest())) + g.Eventually(fetchEvents(ctx, env.Client, "BootcNodePool", pool.Name, pool.UID)). + Should(ContainElement(And( + HaveField("Type", corev1.EventTypeNormal), + HaveField("Reason", "RolloutCompleted"), + ))) + + g.Eventually(func() (bool, error) { + var n corev1.Node + err := env.Client.Get(ctx, client.ObjectKey{Name: nodeName}, &n) + return n.Spec.Unschedulable, err + }).WithTimeout(3*time.Minute).Should(BeFalse(), + "expected node to be schedulable after update") +}