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/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 diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index c7a9f9bb..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. @@ -69,6 +72,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. @@ -95,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 @@ -505,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 3d5389b0..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,10 +35,22 @@ 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, +// 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 @@ -153,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, @@ -360,48 +389,106 @@ 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() - var stalledNodes []*bootcv1alpha1.BootcNode + threshold := r.effectiveDrainStallThreshold() + 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 := drainStallThreshold - 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, corev1.EventTypeWarning, eventReasonDrainTakingTooLong, eventActionDrain, - drainStalledNote{Threshold: drainStallThreshold}, + drainStalledNote{Threshold: threshold}, ) } + 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 0c69cddd..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,26 +656,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) + 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 +672,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 +977,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 { 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) 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") +}