From 2ca1604f49caab1de4b4cdffe9970b46132b5021 Mon Sep 17 00:00:00 2001 From: kylan11 Date: Fri, 7 Aug 2026 14:05:44 +0200 Subject: [PATCH 1/2] Experiment: hold evictions at admission instead of owning a budget A PodDisruptionBudget is pre-checked: every disrupter that dry-runs budgets refuses to start while one blocks, which pins the node. The same not-yet answer given at admission time is invisible until the moment of the attempt, which is the one moment delay is expected and retried by every drainer in existence. Behind an understudy.sh/experimental-eviction-hold annotation on the CR: no budget is created, and the observe-only eviction webhook gains a hold: evictions of protected pods are answered with the same 429 a budget would return until a stand-in is ready, the phase says Relaxed, or hostage mode is off. Protection detection moves from owned-budget lookup to resolving the Understudy that covers the pod, which both modes need. The held attempt itself is the doom signal, so signal and hold are one mechanism and the state machine is untouched. --- api/v1alpha1/understudy_types.go | 6 ++ internal/controller/evictionwebhook_test.go | 56 ++++++++++ internal/controller/understudy_controller.go | 17 +++- internal/metrics/metrics.go | 7 +- internal/signal/evictionwebhook/adapter.go | 102 ++++++++++++++++--- 5 files changed, 168 insertions(+), 20 deletions(-) diff --git a/api/v1alpha1/understudy_types.go b/api/v1alpha1/understudy_types.go index 3ff697d..38135b2 100644 --- a/api/v1alpha1/understudy_types.go +++ b/api/v1alpha1/understudy_types.go @@ -46,6 +46,8 @@ const ( ConditionSurgeReady = "SurgeReady" ConditionZeroDowntimeStrategy = "ZeroDowntimeUpdateStrategy" ConditionNodePinned = "NodePinned" + + AnnotationExperimentalEvictionHold = "understudy.sh/experimental-eviction-hold" ) type TargetReference struct { @@ -137,6 +139,10 @@ func init() { }) } +func (u *Understudy) ExperimentalEvictionHold() bool { + return u.Annotations[AnnotationExperimentalEvictionHold] == "true" +} + func (u *Understudy) SurgeReplicasOrDefault() int32 { if u.Spec.SurgeReplicas != nil { return *u.Spec.SurgeReplicas diff --git a/internal/controller/evictionwebhook_test.go b/internal/controller/evictionwebhook_test.go index 0160413..64d1b08 100644 --- a/internal/controller/evictionwebhook_test.go +++ b/internal/controller/evictionwebhook_test.go @@ -17,18 +17,25 @@ limitations under the License. package controller import ( + "fmt" + "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" appsv1alpha1 "github.com/kylan11/understudy/api/v1alpha1" ) +const holdEnabled = "true" + func evict(pod *corev1.Pod, dryRun bool) error { eviction := &policyv1.Eviction{ ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, @@ -94,6 +101,55 @@ var _ = Describe("Eviction webhook adapter", func() { Consistently(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(1))) }) + It("holds evictions at admission instead of owning a budget (experimental)", func() { + f := newFixture(func(cr *appsv1alpha1.Understudy) { + cr.Annotations = map[string]string{appsv1alpha1.AnnotationExperimentalEvictionHold: holdEnabled} + }) + + By("owning no budget at all") + Consistently(func() bool { + _, err := getPDB(f) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + + By("answering the eviction with 429, exactly as a budget would") + var evictErr error + Eventually(func() bool { + evictErr = evict(f.pod, false) + return evictErr != nil + }).Should(BeTrue()) + Expect(apierrors.IsTooManyRequests(evictErr)).To(BeTrue(), + fmt.Sprintf("the hold must be indistinguishable from a budget block; got: %v", evictErr)) + + By("surging on the held attempt") + Eventually(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(2))) + + By("admitting the eviction once the stand-in is ready") + clearNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: f.ns + "-clear"}} + Expect(k8sClient.Create(ctx, clearNode)).To(Succeed()) + makePod(f.ns, "lead-understudy", f.dep.Name, clearNode.Name, true) + Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) + + By("scaling back once the old pod is gone") + Eventually(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(1))) + Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) + }) + + It("never wedges in hold mode: the readiness TTL admits the eviction (experimental)", func() { + f := newFixture(func(cr *appsv1alpha1.Understudy) { + cr.Annotations = map[string]string{appsv1alpha1.AnnotationExperimentalEvictionHold: holdEnabled} + cr.Spec.ReadinessDeadlineSeconds = ptr.To(int32(3)) + }) + + By("holding the first attempts") + Eventually(func() bool { return evict(f.pod, false) != nil }).Should(BeTrue()) + Eventually(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(2))) + makePod(f.ns, "lead-understudy", f.dep.Name, f.node.Name, false) + + By("admitting after the deadline even with no ready stand-in") + Eventually(func() error { return evict(f.pod, false) }, 20*time.Second).Should(Succeed()) + }) + It("un-surges once eviction attempts stop and the signal expires", func() { f := newFixture(nil) Eventually(func() error { _, err := getPDB(f); return err }).Should(Succeed()) diff --git a/internal/controller/understudy_controller.go b/internal/controller/understudy_controller.go index 7e02fca..be63034 100644 --- a/internal/controller/understudy_controller.go +++ b/internal/controller/understudy_controller.go @@ -51,6 +51,7 @@ const ( PodDeletionCostAnnotation = "controller.kubernetes.io/pod-deletion-cost" doomedPodDeletionCost = "-1000" + ownedLabelValue = "true" relaxReasonTTL = "ttl" relaxReasonDeadline = "deadline" relaxReasonMode = "mode" @@ -141,11 +142,21 @@ func (r *UnderstudyReconciler) Reconcile(ctx context.Context, req ctrl.Request) mode := cr.HostageModeOrDefault() relaxReason, ttlExpired := r.relaxDecision(&cr, mode, assessment, surgeReady, status.BlockedSince) - protected, err := r.ensurePDB(ctx, &cr, &dep, base, mode, relaxReason, len(assessment.doomed) > 0) + holdMode := cr.ExperimentalEvictionHold() + pdbMode := mode + if holdMode { + pdbMode = appsv1alpha1.HostageOff + } + protected, err := r.ensurePDB(ctx, &cr, &dep, base, pdbMode, relaxReason, len(assessment.doomed) > 0) if err != nil { return ctrl.Result{}, err } - setProtectedCondition(&status, &cr, protected, mode, relaxReason) + if holdMode && mode != appsv1alpha1.HostageOff { + setCond(&status, &cr, appsv1alpha1.ConditionProtected, boolToStatus(relaxReason == ""), "EvictionHold", + "experimental: evictions are held at admission while a stand-in is prepared; no budget is owned") + } else { + setProtectedCondition(&status, &cr, protected, mode, relaxReason) + } switch { case len(assessment.doomed) == 0 && !surgeActive: @@ -386,7 +397,7 @@ func (r *UnderstudyReconciler) ensurePDB(ctx context.Context, cr *appsv1alpha1.U Name: name, Namespace: cr.Namespace, Labels: map[string]string{ - LabelOwned: "true", + LabelOwned: ownedLabelValue, "app.kubernetes.io/managed-by": "understudy", }, Annotations: map[string]string{ diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index aba1b8a..a59d984 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -40,6 +40,11 @@ var ( Help: "Eviction attempts seen by the observe-only webhook, by whether the pod is protected by an Understudy-owned PDB.", }, []string{"protected"}) + EvictionsHeld = prometheus.NewCounter(prometheus.CounterOpts{ + Name: "understudy_evictions_held_total", + Help: "Evictions of protected pods answered with 429 at admission because no stand-in was ready yet (experimental eviction-hold mode).", + }) + NodePinnedTotal = prometheus.NewCounter(prometheus.CounterOpts{ Name: "understudy_node_pinned_total", Help: "Times a protected workload was detected pinning its node: a disrupter wanted the node and the hostage budget blocked it. Understudy reports this and deliberately does not act; see Limits and the tradeoff in the README.", @@ -78,5 +83,5 @@ func UntrackBlocked(key string) { } func init() { - ctrlmetrics.Registry.MustRegister(SurgesTotal, RelaxedTotal, EvictionAttemptsObserved, NodePinnedTotal, OldestBlockedEvictionSeconds) + ctrlmetrics.Registry.MustRegister(SurgesTotal, RelaxedTotal, EvictionAttemptsObserved, EvictionsHeld, NodePinnedTotal, OldestBlockedEvictionSeconds) } diff --git a/internal/signal/evictionwebhook/adapter.go b/internal/signal/evictionwebhook/adapter.go index 3f62c71..ec1e74d 100644 --- a/internal/signal/evictionwebhook/adapter.go +++ b/internal/signal/evictionwebhook/adapter.go @@ -18,11 +18,14 @@ package evictionwebhook import ( "context" + "fmt" + "net/http" "strconv" "time" + admissionv1 "k8s.io/api/admission/v1" + 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/labels" "k8s.io/apimachinery/pkg/types" @@ -32,15 +35,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + appsv1alpha1 "github.com/kylan11/understudy/api/v1alpha1" "github.com/kylan11/understudy/internal/metrics" "github.com/kylan11/understudy/internal/signal" ) const ( - Path = "/observe-eviction" - SourcePrefix = "eviction:" - ownedLabel = "understudy.sh/owned" - doomLifetime = 90 * time.Second + Path = "/observe-eviction" + SourcePrefix = "eviction:" + baseReplicasAnnotation = "understudy.sh/base-replicas" + doomLifetime = 90 * time.Second ) type Adapter struct { @@ -85,7 +89,7 @@ func (a *Adapter) Handle(ctx context.Context, req admission.Request) admission.R return allowed } - protected := a.podProtected(ctx, &pod) + cr, dep, protected := a.resolveProtector(ctx, &pod) metrics.EvictionAttemptsObserved.WithLabelValues(strconv.FormatBool(protected)).Inc() if !protected { return allowed @@ -100,23 +104,89 @@ func (a *Adapter) Handle(ctx context.Context, req admission.Request) admission.R }) logf.FromContext(ctx).V(1).Info("eviction attempt observed", "pod", req.Namespace+"/"+req.Name, "node", pod.Spec.NodeName, "evictor", req.UserInfo.Username) - return allowed + + if !cr.ExperimentalEvictionHold() { + return allowed + } + if cr.HostageModeOrDefault() == appsv1alpha1.HostageOff { + return allowed + } + if cr.Status.Phase == appsv1alpha1.PhaseRelaxed { + return allowed + } + if a.standInReady(ctx, dep) { + return allowed + } + metrics.EvictionsHeld.Inc() + logf.FromContext(ctx).Info("eviction held; stand-in not ready", + "pod", req.Namespace+"/"+req.Name, "evictor", req.UserInfo.Username) + return admission.Response{AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: false, + Result: &metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusTooManyRequests, + Reason: metav1.StatusReasonTooManyRequests, + Message: fmt.Sprintf("understudy is preparing a stand-in for %s/%s; retry shortly", req.Namespace, req.Name), + }, + }} } -func (a *Adapter) podProtected(ctx context.Context, pod *corev1.Pod) bool { - var pdbs policyv1.PodDisruptionBudgetList - if err := a.reader.List(ctx, &pdbs, client.InNamespace(pod.Namespace), - client.MatchingLabels{ownedLabel: "true"}); err != nil { - return false +func (a *Adapter) resolveProtector(ctx context.Context, pod *corev1.Pod) (*appsv1alpha1.Understudy, *appsv1.Deployment, bool) { + var crs appsv1alpha1.UnderstudyList + if err := a.reader.List(ctx, &crs, client.InNamespace(pod.Namespace)); err != nil { + return nil, nil, false } - for i := range pdbs.Items { - selector, err := metav1.LabelSelectorAsSelector(pdbs.Items[i].Spec.Selector) + for i := range crs.Items { + cr := &crs.Items[i] + if kind := cr.Spec.TargetRef.Kind; kind != "" && kind != "Deployment" { + continue + } + var dep appsv1.Deployment + if err := a.reader.Get(ctx, types.NamespacedName{Namespace: pod.Namespace, Name: cr.Spec.TargetRef.Name}, &dep); err != nil { + continue + } + selector, err := metav1.LabelSelectorAsSelector(dep.Spec.Selector) if err != nil { continue } if selector.Matches(labels.Set(pod.Labels)) { - return true + return cr, &dep, true + } + } + return nil, nil, false +} + +func (a *Adapter) standInReady(ctx context.Context, dep *appsv1.Deployment) bool { + base := int32(1) + if dep.Spec.Replicas != nil { + base = *dep.Spec.Replicas + } + if v, ok := dep.Annotations[baseReplicasAnnotation]; ok { + if parsed, err := strconv.ParseInt(v, 10, 32); err == nil { + base = int32(parsed) + } + } + selector, err := metav1.LabelSelectorAsSelector(dep.Spec.Selector) + if err != nil { + return false + } + var pods corev1.PodList + if err := a.reader.List(ctx, &pods, client.InNamespace(dep.Namespace), + client.MatchingLabelsSelector{Selector: selector}); err != nil { + return false + } + healthy := 0 + for i := range pods.Items { + p := &pods.Items[i] + if p.DeletionTimestamp != nil { + continue + } + for _, c := range p.Status.Conditions { + if c.Type == corev1.PodReady && c.Status == corev1.ConditionTrue { + healthy++ + break + } } } - return false + return healthy > int(base) } From 0c8b3430dadbc0180448bd4943ae9c495031c4c4 Mon Sep 17 00:00:00 2001 From: kylan11 Date: Fri, 7 Aug 2026 15:33:52 +0200 Subject: [PATCH 2/2] Replace the hostage budget with an admission-time eviction hold --- README.md | 77 ++--- api/v1alpha1/understudy_types.go | 7 - charts/understudy/Chart.yaml | 6 +- charts/understudy/templates/NOTES.txt | 8 +- charts/understudy/templates/deployment.yaml | 10 - charts/understudy/templates/failsafe.yaml | 87 ------ charts/understudy/templates/rbac.yaml | 2 +- charts/understudy/templates/webhook.yaml | 37 ++- charts/understudy/values.yaml | 19 +- cmd/main.go | 41 +-- cmd/sentinel/main.go | 84 +---- config/rbac/role.yaml | 4 - docs/how-it-works.md | 287 +++++------------- docs/roadmap.md | 107 +++---- internal/controller/evictionwebhook_test.go | 81 +++-- internal/controller/suite_test.go | 8 +- internal/controller/sweep.go | 55 ++++ internal/controller/understudy_controller.go | 157 +--------- .../controller/understudy_controller_test.go | 127 +++----- internal/metrics/metrics.go | 15 +- internal/signal/evictionwebhook/adapter.go | 80 +++-- internal/signal/pinned/pinned.go | 227 -------------- internal/signal/pinned/pinned_test.go | 210 ------------- internal/signal/signal.go | 2 +- 24 files changed, 431 insertions(+), 1307 deletions(-) delete mode 100644 charts/understudy/templates/failsafe.yaml create mode 100644 internal/controller/sweep.go delete mode 100644 internal/signal/pinned/pinned.go delete mode 100644 internal/signal/pinned/pinned_test.go diff --git a/README.md b/README.md index 9f00ab7..9ed84f3 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,11 @@ then exists only while a disruption is happening. Measured on EKS with Karpenter on arm64 spot nodes, one replica behind an application load balancer, with no spare capacity so every replacement waited for a new EC2 instance. The availability probe ran at roughly three requests -per second. +per second. The disruption results below are from the v0.3.x PDB mechanism; +v0.4.0 separately validated the admission path on the same staging cluster: +same-node stand-ins remained held, a clear-node stand-in released immediately, +the 10-second TTL released in 12 seconds, and operator-down fail-open admitted +in 423 ms. | Event | Failed requests | |---|---| @@ -95,8 +99,7 @@ understudies` shows the target, the mode, the phase, and how long anything has been blocked. The footprint of the default install is one Deployment with one replica. The -webhook is served from that same pod, and a small CronJob acts as a dead man's -switch for the operator's own budgets. There is no DaemonSet and no per-node +webhook is served from that same pod. There is no DaemonSet and no per-node agent. Drains, Karpenter disruption, autoscaler scale-downs and node upgrades are all detected from the API server by the operator pod alone, and on Karpenter clusters that includes spot interruptions, because Karpenter reacts @@ -152,10 +155,11 @@ against a deadline it cannot see. ## How it works -Briefly: a PodDisruptionBudget sized to block every eviction, a surge when a -node is reported doomed, a release once the replacement passes its readiness -gates, and a scale-back that removes the right pod. The full explanation, with -diagrams and the reasoning behind each decision, is in +Briefly: answer the eviction at the door with HTTP 429, surge when its node is +reported doomed, admit a retry once the replacement passes its readiness +gates, and scale back by removing the right pod. No PodDisruptionBudget is +created. The full explanation, with diagrams and the reasoning behind each +decision, is in [docs/how-it-works.md](docs/how-it-works.md). Signals reach the operator through four adapters, and the core contains no @@ -173,60 +177,29 @@ provider-specific code: Alert on `understudy_oldest_blocked_eviction_seconds`. It exists so that a held eviction is always visible and bounded. -If a surge cannot make progress, the budget is relaxed rather than stalling the -drain forever. If the operator itself disappears, a CronJob removes the budgets -it left behind and the cluster returns to plain Kubernetes behaviour. There is -a manual version of the same thing: +If a surge cannot make progress, the hold is relaxed rather than stalling the +drain forever. The webhook is fail-open: if every operator replica is +unavailable, Kubernetes admits evictions normally within the webhook timeout. +To disable holds immediately: ```sh -kubectl delete pdb -A -l understudy.sh/owned=true +kubectl delete validatingwebhookconfiguration understudy-eviction-hold ``` The operator refuses to manage itself, skips targets it cannot help, and stands down while a rollout is in progress. +For production, use `replicaCount: 2` and place the replicas on different +nodes. The webhook serves from every replica even though reconciliation is +leader-gated. If the node hosting the only operator pod is drained, its own +eviction is admitted and protection for pods later in that drain can disappear. + ## Limits and the tradeoff -Understudy reacts to disruptions; it never starts one. That line is the -design, and most of what follows falls out of it. - -**A protected workload can pin its node.** This is the one to understand -before protecting anything. Modern node orchestrators check budgets before -they act: Karpenter dry-runs PodDisruptionBudgets for consolidation, drift -and expiry alike, and simply never begins while the hostage budget allows no -disruptions. No cordon, no taint, no eviction attempt, so there is nothing -for any fail-safe to catch and nothing for Understudy to react to. -Bin-packing skips the node, and on clusters where nothing forces the issue, -so does node recycling. - -Understudy makes that visible rather than resolving it behind your back. -When a blocker reports that an owned budget stopped it, the affected -Understudy gets a `NodePinned` condition, a Warning event, and the -`understudy_node_pinned_total` counter moves. What to do about it is your -call, in rough order of preference: - -- Let deploys do it: every rollout reschedules the workload, and the pin - ends wherever the new pod lands. On anything that ships regularly this is - the whole answer. -- `kubectl rollout restart` is the manual version of the same thing when a - `NodePinned` condition bothers you. -- Delete the node claim itself. Deleting it starts a drain instead of - asking permission for one, so the taint appears, Understudy surges, and - the handover happens normally with no downtime. Pair it with a NodePool - `terminationGracePeriod` so that drain is bounded: budgets are still - honoured, but a replacement that never becomes ready cannot hold the node - open forever. -- `hostageMode: voluntary-only` or `off` trades protection for mobility on - workloads where bin-packing matters more than the last second of uptime. - -`terminationGracePeriod` on its own does not lift the pin, which is worth -saying plainly because it reads like it should. Measured on Karpenter 1.12: -a node whose claim carries the field is still refused with `Pdb prevents -pod evictions` while it is merely a disruption candidate. The field bounds -a drain that has started. It does not start one. - -What Understudy will not do is move a healthy pod on its own initiative. A -tool that guards availability should never be the reason a pod died. +Understudy reacts to disruptions; it never starts one. Because there is no +budget for a disrupter to pre-check, consolidation, drift and expiry can select +the node normally. The disrupter begins, retries the admission hold while the +stand-in starts, and proceeds as soon as the stand-in is ready. **In-flight requests are not saved.** Understudy guarantees a ready replacement, not the requests already travelling to the departing pod. Give diff --git a/api/v1alpha1/understudy_types.go b/api/v1alpha1/understudy_types.go index 38135b2..f96e7c8 100644 --- a/api/v1alpha1/understudy_types.go +++ b/api/v1alpha1/understudy_types.go @@ -45,9 +45,6 @@ const ( ConditionProtected = "Protected" ConditionSurgeReady = "SurgeReady" ConditionZeroDowntimeStrategy = "ZeroDowntimeUpdateStrategy" - ConditionNodePinned = "NodePinned" - - AnnotationExperimentalEvictionHold = "understudy.sh/experimental-eviction-hold" ) type TargetReference struct { @@ -139,10 +136,6 @@ func init() { }) } -func (u *Understudy) ExperimentalEvictionHold() bool { - return u.Annotations[AnnotationExperimentalEvictionHold] == "true" -} - func (u *Understudy) SurgeReplicasOrDefault() int32 { if u.Spec.SurgeReplicas != nil { return *u.Spec.SurgeReplicas diff --git a/charts/understudy/Chart.yaml b/charts/understudy/Chart.yaml index 723aeae..da4bdaf 100644 --- a/charts/understudy/Chart.yaml +++ b/charts/understudy/Chart.yaml @@ -5,14 +5,14 @@ description: >- downtime from announced node disruptions (drains, upgrades, consolidation, spot reclaims) without a standing second replica. type: application -version: 0.3.1 -appVersion: "0.3.1" +version: 0.4.0 +appVersion: "0.4.0" kubeVersion: ">=1.34.0-0" keywords: - availability - spot - eviction - - pdb + - admission-webhook - node-drain - karpenter - cluster-autoscaler diff --git a/charts/understudy/templates/NOTES.txt b/charts/understudy/templates/NOTES.txt index c6cd2c3..a15b4b7 100644 --- a/charts/understudy/templates/NOTES.txt +++ b/charts/understudy/templates/NOTES.txt @@ -18,7 +18,9 @@ Give a Deployment an understudy: Alert on the metric understudy_oldest_blocked_eviction_seconds: a blocked drain must page a human, never wedge a node. -ESCAPE HATCH - if the operator is ever gone while its PDBs still block -drains, every PDB it owns is labeled for one-command removal: +ESCAPE HATCH - delete the fail-open webhook configuration to disable holds +immediately: - kubectl delete pdb -A -l understudy.sh/owned=true + kubectl delete validatingwebhookconfiguration {{ include "understudy.fullname" . }}-eviction-hold + +If every operator replica is unavailable, Kubernetes admits evictions normally. diff --git a/charts/understudy/templates/deployment.yaml b/charts/understudy/templates/deployment.yaml index 2ccf3c9..7351262 100644 --- a/charts/understudy/templates/deployment.yaml +++ b/charts/understudy/templates/deployment.yaml @@ -37,11 +37,7 @@ spec: - --enable-cordon-adapter={{ .Values.signals.cordonAdapter }} - --enable-taint-adapter={{ .Values.signals.taintAdapter }} - --doom-taint-keys={{ join "," .Values.signals.doomTaintKeys }} - - --enable-eviction-webhook={{ .Values.signals.evictionWebhook }} - - --enable-pin-detection={{ .Values.signals.pinDetection }} - {{- if .Values.signals.evictionWebhook }} - --webhook-cert-path=/tmp/k8s-webhook-server/serving-certs - {{- end }} env: - name: OPERATOR_NAMESPACE valueFrom: @@ -54,16 +50,12 @@ spec: containerPort: 8081 - name: metrics containerPort: {{ regexReplaceAll "^.*:" .Values.metrics.bindAddress "" }} - {{- if .Values.signals.evictionWebhook }} - name: webhook containerPort: 9443 - {{- end }} - {{- if .Values.signals.evictionWebhook }} volumeMounts: - name: webhook-cert mountPath: /tmp/k8s-webhook-server/serving-certs readOnly: true - {{- end }} livenessProbe: httpGet: path: /healthz @@ -80,12 +72,10 @@ spec: drop: ["ALL"] resources: {{- toYaml .Values.resources | nindent 12 }} - {{- if .Values.signals.evictionWebhook }} volumes: - name: webhook-cert secret: secretName: {{ include "understudy.fullname" . }}-webhook-cert - {{- end }} {{- with .Values.nodeSelector }} nodeSelector: {{- toYaml . | nindent 8 }} diff --git a/charts/understudy/templates/failsafe.yaml b/charts/understudy/templates/failsafe.yaml deleted file mode 100644 index 2d568d8..0000000 --- a/charts/understudy/templates/failsafe.yaml +++ /dev/null @@ -1,87 +0,0 @@ -{{- if .Values.failsafe.enabled }} -apiVersion: v1 -kind: ServiceAccount -metadata: - name: {{ include "understudy.fullname" . }}-failsafe - labels: - {{- include "understudy.labels" . | nindent 4 }} ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: {{ include "understudy.fullname" . }}-failsafe - labels: - {{- include "understudy.labels" . | nindent 4 }} -rules: - - apiGroups: ["policy"] - resources: ["poddisruptionbudgets"] - verbs: ["get", "list", "watch", "delete"] ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding -metadata: - name: {{ include "understudy.fullname" . }}-failsafe - labels: - {{- include "understudy.labels" . | nindent 4 }} -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: {{ include "understudy.fullname" . }}-failsafe -subjects: - - kind: ServiceAccount - name: {{ include "understudy.fullname" . }}-failsafe - namespace: {{ .Release.Namespace }} ---- -apiVersion: batch/v1 -kind: CronJob -metadata: - name: {{ include "understudy.fullname" . }}-failsafe - labels: - {{- include "understudy.labels" . | nindent 4 }} -spec: - schedule: {{ .Values.failsafe.schedule | quote }} - concurrencyPolicy: Forbid - successfulJobsHistoryLimit: 1 - failedJobsHistoryLimit: 3 - jobTemplate: - spec: - backoffLimit: 1 - template: - metadata: - labels: - {{- include "understudy.selectorLabels" . | nindent 12 }} - app.kubernetes.io/component: failsafe - spec: - restartPolicy: Never - serviceAccountName: {{ include "understudy.fullname" . }}-failsafe - {{- with .Values.imagePullSecrets }} - imagePullSecrets: - {{- toYaml . | nindent 12 }} - {{- end }} - securityContext: - runAsNonRoot: true - seccompProfile: - type: RuntimeDefault - containers: - - name: janitor - image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}" - imagePullPolicy: {{ .Values.image.pullPolicy }} - command: ["/sentinel"] - args: - - --mode=janitor - - --stale-after={{ .Values.failsafe.staleAfter }} - securityContext: - allowPrivilegeEscalation: false - capabilities: - drop: ["ALL"] - resources: - {{- toYaml .Values.failsafe.resources | nindent 16 }} - {{- with .Values.nodeSelector }} - nodeSelector: - {{- toYaml . | nindent 12 }} - {{- end }} - {{- with .Values.tolerations }} - tolerations: - {{- toYaml . | nindent 12 }} - {{- end }} -{{- end }} diff --git a/charts/understudy/templates/rbac.yaml b/charts/understudy/templates/rbac.yaml index aa1239e..4921f3b 100644 --- a/charts/understudy/templates/rbac.yaml +++ b/charts/understudy/templates/rbac.yaml @@ -20,7 +20,7 @@ rules: verbs: ["get", "list", "watch", "update", "patch"] - apiGroups: ["policy"] resources: ["poddisruptionbudgets"] - verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] + verbs: ["get", "list", "delete"] - apiGroups: [""] resources: ["pods"] verbs: ["get", "list", "watch", "update", "patch"] diff --git a/charts/understudy/templates/webhook.yaml b/charts/understudy/templates/webhook.yaml index 2edb782..e377ef0 100644 --- a/charts/understudy/templates/webhook.yaml +++ b/charts/understudy/templates/webhook.yaml @@ -1,21 +1,39 @@ -{{- if .Values.signals.evictionWebhook }} {{- $fullname := include "understudy.fullname" . }} {{- $svc := printf "%s-webhook" $fullname }} +{{- $secretName := printf "%s-webhook-cert" $fullname }} {{- $altNames := list (printf "%s.%s.svc" $svc .Release.Namespace) (printf "%s.%s.svc.cluster.local" $svc .Release.Namespace) }} {{- $ca := genCA (printf "%s-ca" $fullname) 3650 }} {{- $cert := genSignedCert $svc nil $altNames 3650 $ca }} +{{- $caCert := $ca.Cert }} +{{- $tlsCert := $cert.Cert }} +{{- $tlsKey := $cert.Key }} +{{- $existingSecret := lookup "v1" "Secret" .Release.Namespace $secretName }} +{{- $existingWebhook := lookup "admissionregistration.k8s.io/v1" "ValidatingWebhookConfiguration" "" (printf "%s-eviction-hold" $fullname) }} +{{- if not $existingWebhook }} +{{- $existingWebhook = lookup "admissionregistration.k8s.io/v1" "ValidatingWebhookConfiguration" "" (printf "%s-eviction-observer" $fullname) }} +{{- end }} +{{- if and $existingSecret (get $existingSecret.data "tls.crt") (get $existingSecret.data "tls.key") }} +{{- $tlsCert = get $existingSecret.data "tls.crt" | b64dec }} +{{- $tlsKey = get $existingSecret.data "tls.key" | b64dec }} +{{- if get $existingSecret.data "ca.crt" }} +{{- $caCert = get $existingSecret.data "ca.crt" | b64dec }} +{{- else if and $existingWebhook $existingWebhook.webhooks }} +{{- $caCert = (index $existingWebhook.webhooks 0).clientConfig.caBundle | b64dec }} +{{- end }} +{{- end }} apiVersion: v1 kind: Secret metadata: - name: {{ $fullname }}-webhook-cert + name: {{ $secretName }} labels: {{- include "understudy.labels" . | nindent 4 }} annotations: helm.sh/resource-policy: keep type: kubernetes.io/tls data: - tls.crt: {{ $cert.Cert | b64enc }} - tls.key: {{ $cert.Key | b64enc }} + ca.crt: {{ $caCert | b64enc }} + tls.crt: {{ $tlsCert | b64enc }} + tls.key: {{ $tlsKey | b64enc }} --- apiVersion: v1 kind: Service @@ -34,31 +52,30 @@ spec: apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - name: {{ $fullname }}-eviction-observer + name: {{ $fullname }}-eviction-hold labels: {{- include "understudy.labels" . | nindent 4 }} webhooks: - - name: eviction-observer.understudy.sh + - name: eviction-hold.understudy.sh admissionReviewVersions: ["v1"] sideEffects: NoneOnDryRun matchPolicy: Equivalent failurePolicy: Ignore - timeoutSeconds: {{ .Values.signals.evictionWebhookTimeoutSeconds }} + timeoutSeconds: {{ .Values.webhook.timeoutSeconds }} clientConfig: service: name: {{ $svc }} namespace: {{ .Release.Namespace }} path: /observe-eviction port: 443 - caBundle: {{ $ca.Cert | b64enc }} + caBundle: {{ $caCert | b64enc }} rules: - apiGroups: [""] apiVersions: ["v1"] operations: ["CREATE"] resources: ["pods/eviction"] scope: Namespaced - {{- with .Values.signals.evictionWebhookNamespaceSelector }} + {{- with .Values.webhook.namespaceSelector }} namespaceSelector: {{- toYaml . | nindent 6 }} {{- end }} -{{- end }} diff --git a/charts/understudy/values.yaml b/charts/understudy/values.yaml index 920c406..87fcea3 100644 --- a/charts/understudy/values.yaml +++ b/charts/understudy/values.yaml @@ -14,10 +14,10 @@ signals: - karpenter.sh/disrupted - ToBeDeletedByClusterAutoscaler - DeletionCandidateOfClusterAutoscaler - evictionWebhook: true - evictionWebhookTimeoutSeconds: 2 - evictionWebhookNamespaceSelector: {} - pinDetection: true + +webhook: + timeoutSeconds: 2 + namespaceSelector: {} sentinel: enabled: false @@ -34,17 +34,6 @@ sentinel: limits: memory: 64Mi -failsafe: - enabled: true - schedule: "*/5 * * * *" - staleAfter: 15m - resources: - requests: - cpu: 10m - memory: 32Mi - limits: - memory: 64Mi - leaderElection: enabled: true diff --git a/cmd/main.go b/cmd/main.go index f3fc7da..b14972c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -25,14 +25,10 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -44,7 +40,6 @@ import ( "github.com/kylan11/understudy/internal/signal" "github.com/kylan11/understudy/internal/signal/cordon" "github.com/kylan11/understudy/internal/signal/evictionwebhook" - "github.com/kylan11/understudy/internal/signal/pinned" "github.com/kylan11/understudy/internal/signal/taint" // +kubebuilder:scaffold:imports ) @@ -70,7 +65,7 @@ func main() { var probeAddr string var secureMetrics bool var enableHTTP2 bool - var enableCordonAdapter, enableTaintAdapter, enableEvictionWebhook, enablePinDetection bool + var enableCordonAdapter, enableTaintAdapter bool var doomTaintKeys string var tlsOpts []func(*tls.Config) flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ @@ -96,13 +91,6 @@ func main() { flag.BoolVar(&enableTaintAdapter, "enable-taint-adapter", true, "Watch drainer-owned node taints as a doom signal (see --doom-taint-keys). Covers Karpenter "+ "and cluster-autoscaler dialects by default.") - flag.BoolVar(&enableEvictionWebhook, "enable-eviction-webhook", false, - "Serve an observe-only ValidatingWebhook on pods/eviction CREATE. It always admits and never denies; "+ - "it exists to see eviction attempts from drainers that neither cordon nor taint.") - flag.BoolVar(&enablePinDetection, "enable-pin-detection", true, - "Watch DisruptionBlocked events that name an operator-owned PDB and surface a NodePinned "+ - "condition, event and metric on the affected Understudy. Detection only: a pinned node is "+ - "reported, never resolved. See The tradeoff in the README.") flag.StringVar(&doomTaintKeys, "doom-taint-keys", strings.Join(taint.DefaultKeys, ","), "Comma-separated node taint KEYS treated as doom signals (matched by key only; values are "+ "ignored because drainers put timestamps there). A new drainer dialect is a new key here, "+ @@ -166,14 +154,6 @@ func main() { HealthProbeBindAddress: probeAddr, LeaderElection: enableLeaderElection, LeaderElectionID: "e9abbdca.understudy.sh", - Cache: cache.Options{ - ByObject: map[client.Object]cache.ByObject{ - &corev1.Event{}: {Field: fields.AndSelectors( - fields.OneTermEqualSelector("reason", pinned.BlockedReason), - fields.OneTermEqualSelector("involvedObject.kind", "Node"), - )}, - }, - }, }) if err != nil { setupLog.Error(err, "Failed to start manager") @@ -182,7 +162,7 @@ func main() { ctx := ctrl.SetupSignalHandler() registry := signal.NewRegistry() - registry.StartJanitor(ctx, 10*time.Second) + registry.StartExpiryLoop(ctx, 10*time.Second) adapters := []signal.Adapter{} if enableCordonAdapter { @@ -197,9 +177,7 @@ func main() { } adapters = append(adapters, taint.New(keys)) } - if enableEvictionWebhook { - adapters = append(adapters, evictionwebhook.New()) - } + adapters = append(adapters, evictionwebhook.New()) for _, a := range adapters { if err := a.SetupWithManager(mgr, registry); err != nil { setupLog.Error(err, "Failed to set up signal adapter", "adapter", a.Name()) @@ -207,15 +185,6 @@ func main() { } setupLog.Info("signal adapter enabled", "adapter", a.Name()) } - if enablePinDetection { - watcher := pinned.New() - if err := watcher.SetupWithManager(mgr); err != nil { - setupLog.Error(err, "Failed to set up pin detection") - os.Exit(1) - } - setupLog.Info("pin detection enabled") - } - if err := (&controller.UnderstudyReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), @@ -227,6 +196,10 @@ func main() { setupLog.Error(err, "Failed to create controller", "controller", "understudy") os.Exit(1) } + if err := mgr.Add(&controller.PDBSweep{Reader: mgr.GetAPIReader(), Client: mgr.GetClient()}); err != nil { + setupLog.Error(err, "Failed to add legacy PodDisruptionBudget sweep") + os.Exit(1) + } // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/cmd/sentinel/main.go b/cmd/sentinel/main.go index 2e0bc0d..6b933bf 100644 --- a/cmd/sentinel/main.go +++ b/cmd/sentinel/main.go @@ -20,16 +20,13 @@ import ( "context" "flag" "os" - "strconv" "time" - policyv1 "k8s.io/api/policy/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" "github.com/kylan11/understudy/internal/signal/sentinel" @@ -42,23 +39,20 @@ func init() { } func main() { - var mode, cloud, nodeName, staleAfter string + var cloud, nodeName string var surgeOnRebalance bool var interval time.Duration - flag.StringVar(&mode, "mode", "sentinel", "Operating mode: sentinel or janitor.") flag.StringVar(&cloud, "cloud", "auto", "Cloud metadata dialect to probe: aws, gcp, azure or auto.") flag.StringVar(&nodeName, "node-name", os.Getenv("NODE_NAME"), "Node this sentinel is responsible for.") flag.BoolVar(&surgeOnRebalance, "surge-on-rebalance", false, "Treat AWS rebalance recommendations as doom signals. They indicate elevated risk, not a committed reclaim.") flag.DurationVar(&interval, "interval", 2*time.Second, "Metadata poll interval.") - flag.StringVar(&staleAfter, "stale-after", "15m", - "Janitor mode: delete owned PDBs whose heartbeat is older than this.") opts := zap.Options{Development: false} opts.BindFlags(flag.CommandLine) flag.Parse() ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) - log := ctrl.Log.WithName(mode) + log := ctrl.Log.WithName("sentinel") ctx := ctrl.SetupSignalHandler() cfg, err := ctrl.GetConfig() @@ -71,35 +65,18 @@ func main() { log.Error(err, "failed to build client") os.Exit(1) } - - switch mode { - case "janitor": - d, err := time.ParseDuration(staleAfter) - if err != nil { - log.Error(err, "invalid stale-after") - os.Exit(1) - } - if err := runJanitor(logf.IntoContext(ctx, log), c, d); err != nil { - log.Error(err, "janitor failed") - os.Exit(1) - } - case "sentinel": - if nodeName == "" { - log.Error(nil, "node-name is required (set NODE_NAME via the downward API)") - os.Exit(1) - } - runner := &sentinel.Runner{ - Client: c, - Probe: selectProbe(ctx, cloud, surgeOnRebalance), - NodeName: nodeName, - Interval: interval, - } - if err := runner.Run(ctx); err != nil { - log.Error(err, "sentinel failed") - os.Exit(1) - } - default: - log.Error(nil, "unknown mode", "mode", mode) + if nodeName == "" { + log.Error(nil, "node-name is required (set NODE_NAME via the downward API)") + os.Exit(1) + } + runner := &sentinel.Runner{ + Client: c, + Probe: selectProbe(ctx, cloud, surgeOnRebalance), + NodeName: nodeName, + Interval: interval, + } + if err := runner.Run(ctx); err != nil { + log.Error(err, "sentinel failed") os.Exit(1) } } @@ -120,36 +97,3 @@ func selectProbe(ctx context.Context, cloud string, surgeOnRebalance bool) senti return p } } - -func runJanitor(ctx context.Context, c client.Client, staleAfter time.Duration) error { - log := logf.FromContext(ctx) - var pdbs policyv1.PodDisruptionBudgetList - if err := c.List(ctx, &pdbs, client.MatchingLabels{"understudy.sh/owned": "true"}); err != nil { - return err - } - now := time.Now() - deleted := 0 - for i := range pdbs.Items { - pdb := &pdbs.Items[i] - raw := pdb.Annotations["understudy.sh/heartbeat"] - secs, err := strconv.ParseInt(raw, 10, 64) - if err != nil { - log.Info("owned PDB has no usable heartbeat; leaving it alone", - "pdb", pdb.Namespace+"/"+pdb.Name) - continue - } - age := now.Sub(time.Unix(secs, 0)) - if age < staleAfter { - continue - } - if err := c.Delete(ctx, pdb); err != nil { - log.Error(err, "failed to delete stale PDB", "pdb", pdb.Namespace+"/"+pdb.Name) - continue - } - deleted++ - log.Info("deleted stale hostage PDB; operator appears to be gone", - "pdb", pdb.Namespace+"/"+pdb.Name, "heartbeatAge", age.String()) - } - log.Info("janitor finished", "ownedPDBs", len(pdbs.Items), "deleted", deleted) - return nil -} diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 278b404..55e89e2 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -80,10 +80,6 @@ rules: resources: - poddisruptionbudgets verbs: - - create - delete - get - list - - patch - - update - - watch diff --git a/docs/how-it-works.md b/docs/how-it-works.md index c76d2e0..a83e950 100644 --- a/docs/how-it-works.md +++ b/docs/how-it-works.md @@ -1,59 +1,32 @@ # How Understudy works -This document explains the mechanism in enough detail to reason about what -happens to your workload during a disruption, and to debug it when something -looks wrong. - -## The problem - -A Deployment with one replica has no redundancy, so any event that removes its -pod removes the service. Kubernetes offers PodDisruptionBudgets to protect -against this, but a budget cannot create capacity. With one replica, a budget -either blocks the disruption entirely (and stalls the drain) or permits it -(and you lose the service until the pod is rescheduled). - -The gap between those two options is where Understudy sits. It notices that a -node is going away, brings up a second pod somewhere healthy, waits until that -pod is actually serving traffic, and only then lets the original be evicted. +Understudy protects a single-replica Deployment during announced node +disruptions without keeping a second replica running all the time. ## The mechanism -Five steps, in order. +Five steps happen in order. -**1. Take the eviction hostage.** For each `Understudy` resource, the operator -maintains a PodDisruptionBudget whose `minAvailable` equals the target's -current replica count. With one replica that means zero disruptions are -allowed, so every graceful eviction is rejected with HTTP 429. This is not a -denial of service: every drainer that matters retries. `kubectl drain` retries -every five seconds, Karpenter retries continuously, cluster-autoscaler retries -for two minutes. Blocking is how the operator buys time to arrange a -replacement. +**1. Answer the eviction at the door.** The always-on validating webhook +handles `pods/eviction` CREATE. For a protected pod with no ready stand-in it +returns HTTP 429 and `TooManyRequests`. Drainers already retry that response. +No PodDisruptionBudget is created, so Karpenter and cluster-autoscaler can +select a node without being blocked by a budget pre-check. -**2. Notice the node is doomed.** Signal adapters watch for the various ways a -cluster announces a departing node and normalise all of them into one internal -value. See [Signals](#signals). +**2. Record the node as doomed.** Cordon, taint, eviction and optional cloud +sentinel adapters normalize their observations into a `NodeDoom`. -**3. Surge.** The operator raises the target's replica count by one and records -the original count in an annotation on the Deployment, so the state survives an -operator restart and is visible to a human mid-incident. It also stamps the -doomed pod with a negative `controller.kubernetes.io/pod-deletion-cost`, which -tells the ReplicaSet controller to prefer deleting that pod when scaling back -down. +**3. Surge.** The controller increases the Deployment replica count and marks +the old pod with a negative `controller.kubernetes.io/pod-deletion-cost`. -**4. Wait for traffic readiness, then release.** The new pod counts only once -its `Ready` condition is true, which includes every pod readiness gate. This -matters more than it first appears: with a load balancer in front of the -service, pod-Ready alone does not mean the load balancer has registered the -pod. The AWS Load Balancer Controller, GKE NEGs and Azure AGIC all express -registration through readiness gates, so waiting for `Ready` gives correct -behaviour on all of them without a line of provider-specific code. +**4. Wait for traffic readiness.** A stand-in counts only when it is Ready, is +not deleting, and runs on neither the victim node nor another doomed node. +Readiness gates therefore include load balancer registration. Once enough +healthy stand-ins exist, the webhook admits the drainer's next retry. -Once the replacement is ready there are two pods, the budget now permits one -disruption, and the drainer's next retry succeeds on its own. The operator does -not evict anything itself. - -**5. Scale back.** When the doomed pod is gone, replicas return to the original -count. The deletion-cost stamp ensures the pod that goes is the old one. +**5. Scale back.** After the old pod leaves, the Deployment returns to its +original replica count. The deletion cost makes the old pod the preferred +scale-down target. ```mermaid sequenceDiagram @@ -63,21 +36,22 @@ sequenceDiagram participant W as Workload D->>K: evict pod - K-->>D: 429, budget does not allow it - Note over U: adapter reports the node as doomed + K->>U: admission review + U-->>K: 429, stand-in not ready + K-->>D: retry later + Note over U: node recorded as doomed U->>W: replicas 1 to 2 - U->>K: stamp old pod with negative deletion cost - W-->>U: new pod Ready, including readiness gates - Note over U: budget now allows one disruption + W-->>U: stand-in Ready, including readiness gates D->>K: evict pod (retry) + K->>U: admission review + U-->>K: admit K-->>D: accepted U->>W: replicas back to 1 ``` ## Signals -The core of the operator contains no cloud-specific or drainer-specific code. -It consumes exactly one value: +The core consumes one provider-neutral value: ```go NodeDoom{ @@ -89,177 +63,80 @@ NodeDoom{ } ``` -Adapters produce it. Adding support for a new drainer means adding a -configuration entry or a small adapter, never touching the state machine. - -```mermaid -flowchart LR - subgraph adapters [Adapters] - C[cordon watch] - T[taint watch] - W[eviction webhook] - S[cloud sentinel] - end - N["NodeDoom"] - subgraph core [Core, no provider code] - SM[surge state machine] - end - C --> N - T --> N - W --> N - S --> N - N --> SM -``` - -**Cordon watch** looks for `spec.unschedulable`. This covers `kubectl drain`, -GKE and AKS and EKS upgrade drains, cluster-autoscaler, kured, and most -in-house tooling. - -**Taint watch** looks for a configurable list of taint keys. Matching is by key -only, because the values carry no useful information: cluster-autoscaler stores -the timestamp at which it applied the taint, and Karpenter's taint has no value -at all. Defaults cover Karpenter (`karpenter.sh/disrupted`) and -cluster-autoscaler (`ToBeDeletedByClusterAutoscaler` and, as an early hint, -`DeletionCandidateOfClusterAutoscaler`). Karpenter never cordons, so without -this adapter a Karpenter-managed cluster produces no signal at all. - -**Eviction webhook** is the catch-all. It watches `pods/eviction` CREATE and -always admits; the budget does the blocking. Because admission runs before the -budget is evaluated, the webhook sees attempts that are subsequently rejected -with 429, which means it sees every drainer including ones that neither cordon -nor taint. It only reports a signal when the evicted pod is covered by a budget -this operator owns, so an unrelated eviction on the same node does not disturb -your workload. The webhook is fail-open: if the operator is down, evictions are -admitted normally. - -**Cloud sentinels** are the only optional component, and they are off by -default. Nothing else depends on them: the three adapters above run inside the -operator pod, and on Karpenter clusters even spot interruptions arrive without -a sentinel, as the taint Karpenter applies when it consumes its interruption -queue. What a sentinel adds is the deadline. It is a DaemonSet that reads the -cloud's termination notice, which is only served on the instance's own -link-local metadata endpoint, and taints its own node with the time remaining. -Reporting through a taint rather than a network call means the signal survives -an operator restart, is visible in `kubectl describe node`, and, because the -taint carries `NoSchedule`, keeps the replacement pod off the dying node for -free. Scope it to preemptible node pools; if you skip it entirely, set -`readinessDeadlineSeconds` below the platform notice window for spot -workloads. - -| Cloud | Source | Notice | -|---|---|---| -| AWS | IMDS spot interruption notice, optionally rebalance recommendations | about 2 minutes | -| GCP | metadata server preemption flag | about 30 seconds, and the shutdown has already begun | -| Azure | Scheduled Events, `Preempt` and `Terminate` and `Redeploy` | 30 seconds for preemption, 5 to 15 minutes otherwise | - -Only the AWS probe has been validated against real infrastructure. The GCP and -Azure probes are covered by unit tests against fake metadata servers and should -be treated as experimental. - -## Deciding whether to hold or release +| Adapter | Signal | +|---|---| +| Cordon watch | `spec.unschedulable`, including `kubectl drain` and many managed upgrades | +| Taint watch | Karpenter, cluster-autoscaler and configured drainer taints | +| Eviction webhook | Every matching eviction attempt, including eviction-only drainers | +| Cloud sentinel | Optional cloud termination notice with its deadline | -Holding an eviction is only useful when time is unbounded. If the node will be -destroyed at a fixed moment regardless of what any budget says, then blocking -the graceful eviction path does not save the pod. It only wastes the window in -which the pod could have been draining its connections. +The webhook is both a signal source and the enforcement point. Dry-run +requests, deleting pods, unsupported targets and unprotected pods are admitted. +If multiple Understudy resources match a pod, every one must be ready or +relaxed before the eviction is admitted. -So an involuntary signal carries a deadline, and the operator compares it to -`minSurgeTimeSeconds`, your estimate of how long this workload takes to go from -signal to serving traffic. If the deadline is nearer than that, the budget is -relaxed immediately, the surge still happens in parallel, and the old pod -spends its remaining seconds shutting down cleanly. +## Hold and release decisions -This is why the deadline travels with the signal instead of being a constant. -Two minutes on AWS is thirty seconds on GCP and Azure. +Voluntary disruption is held until the stand-in is ready. An involuntary +deadline is compared with `minSurgeTimeSeconds`; if too little time remains, +the hold is relaxed immediately while the surge continues. A hold is also +relaxed after `readinessDeadlineSeconds`, ensuring an unschedulable stand-in +cannot wedge a drain forever. ```mermaid flowchart TD - A[node reported doomed] --> B{class} - B -->|voluntary| C[hold the eviction until ready] - B -->|involuntary| D{deadline nearer than minSurgeTimeSeconds} - D -->|no| C - D -->|yes| E[relax now, surge in parallel, drain connections] - C --> F{ready before readinessDeadlineSeconds} - F -->|yes| G[release, drainer retry succeeds] - F -->|no| H[relax anyway, emit event and metric] + A[eviction attempt] --> B{protected target} + B -->|no| C[admit] + B -->|yes| D{mode off or relaxed} + D -->|yes| C + D -->|no| E{healthy stand-in ready} + E -->|yes| C + E -->|no| F[return 429 and surge] + F --> G{ready before deadline} + G -->|yes| C + G -->|no| H[relax hold and admit] ``` ## Phases -`kubectl get understudies` shows a phase. - | Phase | Meaning | |---|---| -| `Idle` | Nothing is happening. The budget is in place and blocking. | -| `Surging` | A doomed node was seen. The replacement is not ready yet. | -| `Releasing` | The replacement is serving traffic. The eviction can proceed. | -| `Relaxed` | The budget was opened without a ready replacement, either because the deadline was too near or because the readiness deadline expired. | -| `Unsupported` | The target is not something Understudy can protect. Nothing will happen. | +| `Idle` | No disruption is active; matching evictions are protected | +| `Surging` | A doomed node was seen and the stand-in is not ready | +| `Releasing` | A healthy stand-in is ready and eviction retries are admitted | +| `Relaxed` | The hold was released without a ready stand-in | +| `Unsupported` | The target cannot be protected and is never held | ## Failure behaviour -Every one of these exists because a production incident showed what happens -without it. - -**A blocked drain must never be permanent.** If a surge makes no progress for -`readinessDeadlineSeconds`, the budget is relaxed, an event is emitted and -`understudy_pdb_relaxed_total` increments. The workload takes a brief outage -instead of the node being stuck forever. This matters most on clusters where -the drainer waits indefinitely, which is Karpenter's default. - -**An operator outage must not leave the cluster wedged.** The operator -heartbeats an annotation on the budgets it owns, and a CronJob deletes budgets -whose heartbeat has gone stale. Within a few minutes of the operator -disappearing, the cluster is back to plain Kubernetes behaviour. There is also -a manual escape hatch, since every budget it creates is labelled: - -```sh -kubectl delete pdb -A -l understudy.sh/owned=true -``` - -**The operator never manages itself.** It is stateless, so its own availability -story is a fast reschedule, not a surge. - -**Unsupported targets are skipped quietly.** A StatefulSet or a missing -Deployment produces a condition and an event, then silence. No error loop. - -**A rollout is left alone.** A Deployment that is already rolling is already -surging, so Understudy stands down rather than fighting it. - -## What to alert on +The webhook uses `failurePolicy: Ignore`. If all operator replicas are +unavailable, requests fail open after `webhook.timeoutSeconds` and Kubernetes +returns to its normal eviction behavior. Deleting the +`-understudy-eviction-hold` ValidatingWebhookConfiguration disables +holds immediately. -`understudy_oldest_blocked_eviction_seconds` is the one that matters. It is the -age of the oldest currently-blocked eviction, and it exists so that "Understudy -is holding something" is always a visible, bounded state. Alert if it stays -high. +An operator restart therefore creates a protection gap rather than a stuck +cluster. Run two replicas on different nodes for production. Reconciliation +uses leader election, but every replica serves webhook traffic. -The others: `understudy_surges_total` by signal class, -`understudy_pdb_relaxed_total` by reason, and -`understudy_eviction_attempts_observed_total` split by whether the pod was -protected. +The upgrade from 0.3.x runs a leader-gated startup sweep that deletes legacy +PodDisruptionBudgets labeled `understudy.sh/owned=true`. PDB read/delete RBAC +exists only for that migration and is scheduled for removal in v0.5.0. -## Limits worth knowing before you deploy +## Metrics -Understudy guarantees a ready replacement. It does not change what happens to -requests already in flight to the departing pod. Measured on a workload that -exits immediately on SIGTERM, a node drain lost one request in 350, which is -the same loss as an ordinary rolling update of the same workload. If you care -about those requests, give the pod a `preStop` hook that outlives the load -balancer's deregistration delay. That is a workload concern and no surge -mechanism can fix it. +- `understudy_oldest_blocked_eviction_seconds`: age of the oldest active hold +- `understudy_evictions_held_total`: admission requests answered with 429 +- `understudy_eviction_attempts_observed_total`: attempts split by protection +- `understudy_surges_total`: surges by signal class +- `understudy_hold_relaxed_total`: holds relaxed by reason -On a cold cluster with no spare capacity the replacement has to wait for a new -node. For voluntary disruption this costs nothing but time, because the budget -holds the eviction while the node is provisioned. For an involuntary deadline -it is a race: a fast-booting workload can complete the handover inside AWS's -two minutes, but a slow one cannot, and thirty seconds on GCP or Azure is not -winnable at all. In those cases the value is an orderly release and a -replacement started as early as possible, not zero downtime. +## Limits -Karpenter checks budgets before it begins consolidating a node, so a protected -workload can prevent its node from being consolidated at all. You keep -availability and lose some bin-packing. +Understudy guarantees a ready replacement, not preservation of requests +already in flight. Use a `preStop` hook long enough for load balancer +deregistration. -Workloads that cannot tolerate two simultaneous instances are out of scope, and -that is permanent. If two pods cannot overlap briefly, no surge-based approach -works, including ordinary rolling updates. +Cold capacity still takes time. Voluntary disruptions can wait; involuntary +ones race the platform deadline. Workloads that cannot briefly run two +instances, including single-replica StatefulSets, are out of scope. diff --git a/docs/roadmap.md b/docs/roadmap.md index 04756cd..174fa7c 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -1,76 +1,37 @@ # Roadmap -Milestone 1 (shipped in this repo) is the core surge state machine plus the -two universal signal adapters. Everything below plugs into the same -`signal.Adapter` / `signal.NodeDoom` contract - none of it touches core. - -## Milestone 2 - eviction webhook (shipped in v0.2.0) - -An observe-only ValidatingWebhook on `pods/eviction` CREATE: the universal -fallback that sees every eviction attempt from every drainer past, present -and future - including attempts the PDB then rejects with 429 (empirically -verified during the project's analysis phase; admission runs before the -eviction registry's PDB check). Design points: - -- **Fail-open, always-allow.** `failurePolicy: Ignore`, admits everything; - the PDB does the blocking. Operator down -> plain-Kubernetes behavior. -- The AdmissionReview carries pod name/namespace and the evictor's - `userInfo` - free `NodeDoom.Source` attribution (which service account is - draining). -- Signal shape: `NodeDoom{Class: VoluntaryUnbounded}` for the pod's node - - an evictor that retries is by definition waiting on us. -- Needs cert management (cert-manager or built-in rotation) - the reason - it's not in the bootstrap milestone. - -## Milestone 3 - cloud sentinels (shipped in v0.2.0; AWS live-validated, GCP and Azure experimental) - -Per-node probes (DaemonSet) that watch each cloud's metadata channel and -normalize to `NodeDoom{Involuntary, deadline}`. Deadlines are data measured -from the cloud's own notice - never constants. Per-cloud notes (full signal -matrix in the README): - -- **AWS**: IMDS spot ITN (~120s) + rebalance recommendation (early, - non-guaranteed - surging on it is a churn/cost knob, off by default). -- **GCP**: metadata `preempted` hanging-GET (~30s total, ACPI fires - immediately by default) - always below `minSurgeTimeSeconds`, so the value - is fast *release*, not surge. -- **Azure**: Scheduled Events (Preempt 30s; Redeploy/Terminate 5-15 min) - - uniquely, events can be **held and approved early** once the surge - completes; the sentinel should exploit this. - -This is the only place cloud SDKs/endpoints will ever live. - -## Post-M3 / open design items - -- **Budget-pinned nodes are reported, never resolved.** Karpenter dry-runs - PDBs and refuses to start consolidation, drift or expiry while - `disruptionsAllowed=0`; cluster-autoscaler pre-checks similarly. The - hostage PDB can therefore pin a node with no drain signal ever appearing. - Settled position: understudy detects the refusal and surfaces it (the - `NodePinned` condition, a Warning event, `understudy_node_pinned_total`) - and the README documents the mitigations, but the operator does not act. - Measured on Karpenter 1.12, a NodePool `terminationGracePeriod` does NOT - lift the pin: a node claim carrying it is still refused with `Pdb - prevents pod evictions` during candidate selection. It bounds a drain - that has started, so it belongs with the node-claim-deletion path, not on - its own. What actually moves a pinned pod is rescheduling it. - Acting would mean initiating pod movement on another controller's - behalf, which is out of scope by design: understudy reacts to - disruptions, it never starts one. -- **Azure event approval.** Azure Scheduled Events can be held and approved - early; the sentinel currently only reads them. Approving once the surge is - traffic-ready would make Azure the only cloud where an involuntary - disruption is fully mediated. -- **GCP and Azure sentinels are untested against real infrastructure.** Both - are covered by fake-metadata-server unit tests only. They must be labelled - experimental until someone runs them on GKE and AKS. -- **GitOps coexistence.** Argo CD / Flux will fight the replica bump. - Mitigations to document/implement: `ignoreDifferences` recipes, or driving - the surge through a separate standby Deployment instead of `spec.replicas`. -- **EvictionRequest API migration.** KEP-4563 (alpha targeted v1.37) gives - controllers a first-class hook on eviction intent. When it lands and - drainers adopt it, Understudy becomes an EvictionRequest *responder* that - surges before acking - same state machine, better signal, no hostage PDB. - The `signal.Adapter` contract is where it plugs in. -- **kubectl plugin** (`kubectl understudy status`) for incident-time +## v0.4.0 - admission-time eviction hold + +The validating webhook is now the enforcement point. It returns the same HTTP +429 as a PodDisruptionBudget until a healthy stand-in is ready, then admits the +drainer's retry. Understudy owns no budgets. + +This resolves budget-pinned nodes structurally. Karpenter was live-observed +retrying the webhook 21 times over 43 seconds and completing when admission +opened, with no `DisruptionBlocked` event. `kubectl drain` followed the same +retry path. A startup migration sweep removes 0.3.x budgets. + +The failure posture is intentionally fail-open: operator unavailability creates +a protection gap, not a stuck cluster. PDB migration RBAC remains through +v0.4.x and will be removed in v0.5.0. + +## Shipped foundations + +- Core surge state machine with cordon and taint adapters +- Universal eviction webhook and admission hold +- AWS cloud sentinel, with GCP and Azure probes covered by unit tests +- Readiness gates, deletion-cost steering and bounded hold relaxation + +## Open design items + +- **Azure event approval.** Scheduled Events can be approved after traffic + readiness instead of merely observed. +- **GCP and Azure live validation.** Both sentinels remain experimental until + exercised against their real metadata services. +- **GitOps coexistence.** Document Argo CD and Flux `ignoreDifferences` + recipes, or move surge state to a separate standby Deployment. +- **EvictionRequest migration.** KEP-4563 is the future interception point. + When drainers adopt it, the same state machine can respond before acking + without changing the product boundary. +- **kubectl plugin.** Add `kubectl understudy status` for incident-time visibility. diff --git a/internal/controller/evictionwebhook_test.go b/internal/controller/evictionwebhook_test.go index 64d1b08..5eea01f 100644 --- a/internal/controller/evictionwebhook_test.go +++ b/internal/controller/evictionwebhook_test.go @@ -32,10 +32,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" appsv1alpha1 "github.com/kylan11/understudy/api/v1alpha1" + "github.com/kylan11/understudy/internal/signal" ) -const holdEnabled = "true" - func evict(pod *corev1.Pod, dryRun bool) error { eviction := &policyv1.Eviction{ ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, @@ -52,17 +51,11 @@ var _ = Describe("Eviction webhook adapter", func() { It("surges when a protected pod's eviction is attempted, with no node signal at all", func() { f := newFixture(nil) - By("waiting for the hostage PDB so the pod counts as protected") - Eventually(func() error { - _, err := getPDB(f) - return err - }).Should(Succeed()) - - By("attempting an eviction the PDB will reject") + By("attempting an eviction the webhook will reject") Eventually(func() bool { err := evict(f.pod, false) return err != nil - }).Should(BeTrue(), "the hostage PDB should reject the eviction with 429") + }).Should(BeTrue(), "the eviction hold should reject the eviction with 429") By("surging purely on the observed eviction attempt") Eventually(func() int32 { @@ -77,7 +70,7 @@ var _ = Describe("Eviction webhook adapter", func() { It("ignores evictions of pods that no Understudy protects", func() { f := newFixture(nil) - Eventually(func() error { _, err := getPDB(f); return err }).Should(Succeed()) + Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) By("creating an unrelated pod on the same node") other := makePod(f.ns, "bystander", "bystander", f.node.Name, true) @@ -92,7 +85,7 @@ var _ = Describe("Eviction webhook adapter", func() { It("ignores dry-run eviction attempts", func() { f := newFixture(nil) - Eventually(func() error { _, err := getPDB(f); return err }).Should(Succeed()) + Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) By("issuing a dry-run eviction") _ = evict(f.pod, true) @@ -101,15 +94,12 @@ var _ = Describe("Eviction webhook adapter", func() { Consistently(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(1))) }) - It("holds evictions at admission instead of owning a budget (experimental)", func() { - f := newFixture(func(cr *appsv1alpha1.Understudy) { - cr.Annotations = map[string]string{appsv1alpha1.AnnotationExperimentalEvictionHold: holdEnabled} - }) + It("holds evictions at admission instead of owning a budget", func() { + f := newFixture(nil) By("owning no budget at all") Consistently(func() bool { - _, err := getPDB(f) - return apierrors.IsNotFound(err) + return apierrors.IsNotFound(getPDB(f)) }).Should(BeTrue()) By("answering the eviction with 429, exactly as a budget would") @@ -135,9 +125,8 @@ var _ = Describe("Eviction webhook adapter", func() { Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) }) - It("never wedges in hold mode: the readiness TTL admits the eviction (experimental)", func() { + It("never wedges in hold mode: the readiness TTL admits the eviction", func() { f := newFixture(func(cr *appsv1alpha1.Understudy) { - cr.Annotations = map[string]string{appsv1alpha1.AnnotationExperimentalEvictionHold: holdEnabled} cr.Spec.ReadinessDeadlineSeconds = ptr.To(int32(3)) }) @@ -152,7 +141,6 @@ var _ = Describe("Eviction webhook adapter", func() { It("un-surges once eviction attempts stop and the signal expires", func() { f := newFixture(nil) - Eventually(func() error { _, err := getPDB(f); return err }).Should(Succeed()) By("triggering a surge via an eviction attempt") Eventually(func() bool { return evict(f.pod, false) != nil }).Should(BeTrue()) @@ -162,4 +150,55 @@ var _ = Describe("Eviction webhook adapter", func() { Eventually(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(1))) Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) }) + + It("keeps holding when a ready stand-in is on the victim or another doomed node", func() { + f := newFixture(nil) + Eventually(func() bool { return evict(f.pod, false) != nil }).Should(BeTrue()) + makePod(f.ns, "same-node-understudy", f.dep.Name, f.node.Name, true) + Consistently(func() bool { return apierrors.IsTooManyRequests(evict(f.pod, false)) }).Should(BeTrue()) + + doomedNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: f.ns + "-doomed"}} + Expect(k8sClient.Create(ctx, doomedNode)).To(Succeed()) + registry.Set(signal.NodeDoom{Node: doomedNode.Name, Class: signal.VoluntaryUnbounded, Source: "test"}) + DeferCleanup(func() { registry.Clear(doomedNode.Name, "test") }) + makePod(f.ns, "doomed-node-understudy", f.dep.Name, doomedNode.Name, true) + Consistently(func() bool { return apierrors.IsTooManyRequests(evict(f.pod, false)) }).Should(BeTrue()) + + clearNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: f.ns + "-clear"}} + Expect(k8sClient.Create(ctx, clearNode)).To(Succeed()) + makePod(f.ns, "clear-node-understudy", f.dep.Name, clearNode.Name, true) + Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) + }) + + It("never holds an unsupported target", func() { + f := newFixture(func(cr *appsv1alpha1.Understudy) { + cr.Spec.TargetRef.Kind = "StatefulSet" + }) + Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseUnsupported)) + Expect(evict(f.pod, false)).To(Succeed()) + }) + + It("requires every matching protector to admit", func() { + f := newFixture(nil) + second := makeDeployment(f.ns, "second", 2) + second.Spec.Selector = f.dep.Spec.Selector.DeepCopy() + second.Spec.Template.Labels = map[string]string{appLabel: f.dep.Name} + Expect(k8sClient.Create(ctx, second)).To(Succeed()) + settleDeployment(second) + secondCR := &appsv1alpha1.Understudy{ + ObjectMeta: metav1.ObjectMeta{Name: "second", Namespace: f.ns}, + Spec: appsv1alpha1.UnderstudySpec{ + TargetRef: appsv1alpha1.TargetReference{Name: second.Name}, + }, + } + Expect(k8sClient.Create(ctx, secondCR)).To(Succeed()) + + Eventually(func() bool { return evict(f.pod, false) != nil }).Should(BeTrue()) + clearNode := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: f.ns + "-clear-multi"}} + Expect(k8sClient.Create(ctx, clearNode)).To(Succeed()) + makePod(f.ns, "first-clear", f.dep.Name, clearNode.Name, true) + Consistently(func() bool { return apierrors.IsTooManyRequests(evict(f.pod, false)) }).Should(BeTrue()) + makePod(f.ns, "second-clear", f.dep.Name, clearNode.Name, true) + Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) + }) }) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 02bf7bd..5ea43b5 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -42,7 +42,6 @@ import ( "github.com/kylan11/understudy/internal/signal" "github.com/kylan11/understudy/internal/signal/cordon" "github.com/kylan11/understudy/internal/signal/evictionwebhook" - "github.com/kylan11/understudy/internal/signal/pinned" "github.com/kylan11/understudy/internal/signal/taint" // +kubebuilder:scaffold:imports ) @@ -113,11 +112,10 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) registry = signal.NewRegistry() - registry.StartJanitor(ctx, time.Second) + registry.StartExpiryLoop(ctx, time.Second) Expect(cordon.New().SetupWithManager(mgr, registry)).To(Succeed()) Expect(taint.New(nil).SetupWithManager(mgr, registry)).To(Succeed()) Expect((&evictionwebhook.Adapter{DoomLifetime: 5 * time.Second}).SetupWithManager(mgr, registry)).To(Succeed()) - Expect((&pinned.Watcher{StaleAfter: 5 * time.Second}).SetupWithManager(mgr)).To(Succeed()) Expect((&UnderstudyReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), @@ -152,9 +150,9 @@ func evictionWebhookConfig() *admissionv1.ValidatingWebhookConfiguration { scope := admissionv1.NamespacedScope path := evictionwebhook.Path return &admissionv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "understudy-eviction-observer"}, + ObjectMeta: metav1.ObjectMeta{Name: "understudy-eviction-hold"}, Webhooks: []admissionv1.ValidatingWebhook{{ - Name: "eviction-observer.understudy.sh", + Name: "eviction-hold.understudy.sh", FailurePolicy: &failurePolicy, SideEffects: &sideEffects, AdmissionReviewVersions: []string{"v1"}, diff --git a/internal/controller/sweep.go b/internal/controller/sweep.go new file mode 100644 index 0000000..806d077 --- /dev/null +++ b/internal/controller/sweep.go @@ -0,0 +1,55 @@ +/* +Copyright 2026 The Understudy Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + + policyv1 "k8s.io/api/policy/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +type PDBSweep struct { + Reader client.Reader + Client client.Client +} + +func (s *PDBSweep) NeedLeaderElection() bool { + return true +} + +func (s *PDBSweep) Start(ctx context.Context) error { + return sweepOwnedPDBs(ctx, s.Reader, s.Client) +} + +func sweepOwnedPDBs(ctx context.Context, reader client.Reader, c client.Client) error { + var pdbs policyv1.PodDisruptionBudgetList + if err := reader.List(ctx, &pdbs, client.MatchingLabels{LabelOwned: "true"}); err != nil { + return err + } + log := logf.FromContext(ctx) + for i := range pdbs.Items { + pdb := &pdbs.Items[i] + if err := c.Delete(ctx, pdb); err != nil && !apierrors.IsNotFound(err) { + return err + } + log.Info("Deleted legacy PodDisruptionBudget", "namespace", pdb.Namespace, "name", pdb.Name) + } + return nil +} diff --git a/internal/controller/understudy_controller.go b/internal/controller/understudy_controller.go index be63034..ac48403 100644 --- a/internal/controller/understudy_controller.go +++ b/internal/controller/understudy_controller.go @@ -24,7 +24,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - policyv1 "k8s.io/api/policy/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -45,20 +44,15 @@ import ( const ( AnnotationBaseReplicas = "understudy.sh/base-replicas" - AnnotationRelaxed = "understudy.sh/relaxed" - AnnotationHeartbeat = "understudy.sh/heartbeat" LabelOwned = "understudy.sh/owned" PodDeletionCostAnnotation = "controller.kubernetes.io/pod-deletion-cost" doomedPodDeletionCost = "-1000" - ownedLabelValue = "true" relaxReasonTTL = "ttl" relaxReasonDeadline = "deadline" relaxReasonMode = "mode" - activeRequeue = 5 * time.Second - heartbeatPeriod = 60 * time.Second - protectedRequeue = 60 * time.Second + activeRequeue = 5 * time.Second ) type UnderstudyReconciler struct { @@ -82,7 +76,7 @@ type podAssessment struct { // +kubebuilder:rbac:groups=apps.understudy.sh,resources=understudies/status,verbs=get;update;patch // +kubebuilder:rbac:groups=apps.understudy.sh,resources=understudies/finalizers,verbs=update // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups=policy,resources=poddisruptionbudgets,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=policy,resources=poddisruptionbudgets,verbs=get;list;delete // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=events,verbs=get;list;watch;create;patch @@ -142,21 +136,7 @@ func (r *UnderstudyReconciler) Reconcile(ctx context.Context, req ctrl.Request) mode := cr.HostageModeOrDefault() relaxReason, ttlExpired := r.relaxDecision(&cr, mode, assessment, surgeReady, status.BlockedSince) - holdMode := cr.ExperimentalEvictionHold() - pdbMode := mode - if holdMode { - pdbMode = appsv1alpha1.HostageOff - } - protected, err := r.ensurePDB(ctx, &cr, &dep, base, pdbMode, relaxReason, len(assessment.doomed) > 0) - if err != nil { - return ctrl.Result{}, err - } - if holdMode && mode != appsv1alpha1.HostageOff { - setCond(&status, &cr, appsv1alpha1.ConditionProtected, boolToStatus(relaxReason == ""), "EvictionHold", - "experimental: evictions are held at admission while a stand-in is prepared; no budget is owned") - } else { - setProtectedCondition(&status, &cr, protected, mode, relaxReason) - } + setProtectedCondition(&status, &cr, mode, relaxReason) switch { case len(assessment.doomed) == 0 && !surgeActive: @@ -164,11 +144,7 @@ func (r *UnderstudyReconciler) Reconcile(ctx context.Context, req ctrl.Request) status.BlockedSince = nil metrics.UntrackBlocked(req.String()) setCond(&status, &cr, appsv1alpha1.ConditionSurgeReady, boolToStatus(surgeReady), "Steady", "no disruption in sight") - result := ctrl.Result{} - if mode != appsv1alpha1.HostageOff { - result.RequeueAfter = protectedRequeue - } - return result, r.patchStatus(ctx, &cr, status) + return ctrl.Result{}, r.patchStatus(ctx, &cr, status) case len(assessment.doomed) == 0 && surgeActive: return r.reconcileScaleBack(ctx, &cr, &dep, status, assessment, base, req) @@ -292,6 +268,7 @@ func (r *UnderstudyReconciler) reconcileDoomed(ctx context.Context, cr *appsv1al return ctrl.Result{}, err } + previousPhase := status.Phase switch { case surgeReady: status.Phase = appsv1alpha1.PhaseReleasing @@ -307,8 +284,11 @@ func (r *UnderstudyReconciler) reconcileDoomed(ctx context.Context, cr *appsv1al now := metav1.Now() status.LastReleaseTime = &now } - r.eventOnce(cr, &status, corev1.EventTypeWarning, "PDBRelaxed", "Relax", - fmt.Sprintf("hostage PDB for %s relaxed (%s); eviction can proceed without a ready understudy", dep.Name, relaxReason)) + if previousPhase != appsv1alpha1.PhaseRelaxed { + metrics.RelaxedTotal.WithLabelValues(relaxReason).Inc() + } + r.eventOnce(cr, &status, corev1.EventTypeWarning, "HoldRelaxed", "Relax", + fmt.Sprintf("eviction hold for %s relaxed (%s); eviction can proceed without a ready understudy", dep.Name, relaxReason)) default: status.Phase = appsv1alpha1.PhaseSurging setCond(&status, cr, appsv1alpha1.ConditionSurgeReady, metav1.ConditionFalse, @@ -367,96 +347,6 @@ func (r *UnderstudyReconciler) clearDeletionCosts(ctx context.Context, pods []co return nil } -func (r *UnderstudyReconciler) ensurePDB(ctx context.Context, cr *appsv1alpha1.Understudy, dep *appsv1.Deployment, - base int32, mode appsv1alpha1.HostageMode, relaxReason string, predicamentActive bool) (bool, error) { - - name := "understudy-" + cr.Name - key := types.NamespacedName{Namespace: cr.Namespace, Name: name} - var pdb policyv1.PodDisruptionBudget - getErr := r.Get(ctx, key, &pdb) - - if mode == appsv1alpha1.HostageOff { - if getErr == nil { - return false, r.Delete(ctx, &pdb) - } - if apierrors.IsNotFound(getErr) { - return false, nil - } - return false, getErr - } - - desiredMin := intstr.FromInt32(base) - relaxed := relaxReason != "" - if relaxed { - desiredMin = intstr.FromInt32(0) - } - - if apierrors.IsNotFound(getErr) { - pdb = policyv1.PodDisruptionBudget{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: cr.Namespace, - Labels: map[string]string{ - LabelOwned: ownedLabelValue, - "app.kubernetes.io/managed-by": "understudy", - }, - Annotations: map[string]string{ - AnnotationHeartbeat: strconv.FormatInt(time.Now().Unix(), 10), - }, - }, - Spec: policyv1.PodDisruptionBudgetSpec{ - MinAvailable: &desiredMin, - Selector: dep.Spec.Selector, - }, - } - if relaxed { - pdb.Annotations[AnnotationRelaxed] = relaxReason - } - if err := ctrl.SetControllerReference(cr, &pdb, r.Scheme); err != nil { - return false, err - } - if err := r.Create(ctx, &pdb); err != nil { - return false, err - } - return !relaxed, nil - } - if getErr != nil { - return false, getErr - } - - if prev, wasRelaxed := pdb.Annotations[AnnotationRelaxed]; wasRelaxed && predicamentActive && !relaxed { - relaxed = true - relaxReason = prev - desiredMin = intstr.FromInt32(0) - } - - patched := pdb.DeepCopy() - patched.Spec.MinAvailable = &desiredMin - patched.Spec.Selector = dep.Spec.Selector - if patched.Annotations == nil { - patched.Annotations = map[string]string{} - } - now := time.Now() - if prev, err := strconv.ParseInt(patched.Annotations[AnnotationHeartbeat], 10, 64); err != nil || - now.Sub(time.Unix(prev, 0)) >= heartbeatPeriod { - patched.Annotations[AnnotationHeartbeat] = strconv.FormatInt(now.Unix(), 10) - } - if relaxed { - if _, ok := patched.Annotations[AnnotationRelaxed]; !ok { - patched.Annotations[AnnotationRelaxed] = relaxReason - metrics.RelaxedTotal.WithLabelValues(relaxReason).Inc() - } - } else { - delete(patched.Annotations, AnnotationRelaxed) - } - if !equalPDB(&pdb, patched) { - if err := r.Patch(ctx, patched, client.MergeFrom(&pdb)); err != nil { - return false, err - } - } - return !relaxed, nil -} - func (r *UnderstudyReconciler) markUnsupported(ctx context.Context, cr *appsv1alpha1.Understudy, status appsv1alpha1.UnderstudyStatus, msg string) (ctrl.Result, error) { alreadyMarked := status.Phase == appsv1alpha1.PhaseUnsupported @@ -496,16 +386,15 @@ func (r *UnderstudyReconciler) setStrategyCondition(status *appsv1alpha1.Underst } func setProtectedCondition(status *appsv1alpha1.UnderstudyStatus, cr *appsv1alpha1.Understudy, - protected bool, mode appsv1alpha1.HostageMode, relaxReason string) { - if protected { - setCond(status, cr, appsv1alpha1.ConditionProtected, metav1.ConditionTrue, "HostagePDB", "blocking PDB in place") - return - } + mode appsv1alpha1.HostageMode, relaxReason string) { reason, msg := "HostageOff", "hostageMode is off; evictions are not blocked" if mode != appsv1alpha1.HostageOff { - reason, msg = "Relaxed", "hostage PDB relaxed ("+relaxReason+"); evictions may proceed" + reason, msg = "EvictionHold", "evictions are held at admission while a stand-in is prepared" + if relaxReason != "" { + reason, msg = "Relaxed", "eviction hold relaxed ("+relaxReason+"); evictions may proceed" + } } - setCond(status, cr, appsv1alpha1.ConditionProtected, metav1.ConditionFalse, reason, msg) + setCond(status, cr, appsv1alpha1.ConditionProtected, boolToStatus(mode != appsv1alpha1.HostageOff && relaxReason == ""), reason, msg) } func (r *UnderstudyReconciler) patchStatus(ctx context.Context, cr *appsv1alpha1.Understudy, status appsv1alpha1.UnderstudyStatus) error { @@ -547,7 +436,6 @@ func (r *UnderstudyReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&appsv1alpha1.Understudy{}). - Owns(&policyv1.PodDisruptionBudget{}). Watches(&appsv1.Deployment{}, handler.EnqueueRequestsFromMapFunc(mapDeployment)). Watches(&corev1.Pod{}, handler.EnqueueRequestsFromMapFunc(mapPod)). WatchesRawSource(source.Channel(r.Registry.Subscribe(64), handler.EnqueueRequestsFromMapFunc(mapDoom))). @@ -647,19 +535,6 @@ func timePtrEqual(a, b *metav1.Time) bool { return a.Equal(b) } -func equalPDB(a, b *policyv1.PodDisruptionBudget) bool { - switch { - case a.Spec.MinAvailable == nil || b.Spec.MinAvailable == nil: - if a.Spec.MinAvailable != b.Spec.MinAvailable { - return false - } - case *a.Spec.MinAvailable != *b.Spec.MinAvailable: - return false - } - return a.Annotations[AnnotationRelaxed] == b.Annotations[AnnotationRelaxed] && - a.Annotations[AnnotationHeartbeat] == b.Annotations[AnnotationHeartbeat] -} - func doomSource(d *signal.NodeDoom) string { if d == nil { return "unknown" diff --git a/internal/controller/understudy_controller_test.go b/internal/controller/understudy_controller_test.go index 04f02f0..3564748 100644 --- a/internal/controller/understudy_controller_test.go +++ b/internal/controller/understudy_controller_test.go @@ -184,22 +184,6 @@ func taintNode(node *corev1.Node, key string) { }).Should(Succeed()) } -func emitBlockedEvent(node, pdbRef string) { - ev := &corev1.Event{ - ObjectMeta: metav1.ObjectMeta{ - Name: node + ".consolidation-blocked", - Namespace: metav1.NamespaceDefault, - }, - InvolvedObject: corev1.ObjectReference{Kind: "Node", Name: node}, - Reason: "DisruptionBlocked", - Type: corev1.EventTypeNormal, - Message: fmt.Sprintf("Cannot disrupt Node: pdb %q prevents pod evictions", pdbRef), - Source: corev1.EventSource{Component: "karpenter"}, - LastTimestamp: metav1.Now(), - } - ExpectWithOffset(1, k8sClient.Create(ctx, ev)).To(Succeed()) -} - func getDeployment(f *fixture) *appsv1.Deployment { dep := &appsv1.Deployment{} ExpectWithOffset(1, k8sClient.Get(ctx, client.ObjectKeyFromObject(f.dep), dep)).To(Succeed()) @@ -212,10 +196,9 @@ func getCR(f *fixture) *appsv1alpha1.Understudy { return cr } -func getPDB(f *fixture) (*policyv1.PodDisruptionBudget, error) { +func getPDB(f *fixture) error { pdb := &policyv1.PodDisruptionBudget{} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: f.ns, Name: "understudy-" + f.cr.Name}, pdb) - return pdb, err + return k8sClient.Get(ctx, types.NamespacedName{Namespace: f.ns, Name: "understudy-" + f.cr.Name}, pdb) } func expectSurged(f *fixture) { @@ -238,20 +221,43 @@ func expectSurged(f *fixture) { var _ = Describe("Understudy controller", func() { - It("creates the hostage PDB and stays Idle on a healthy node", func() { + It("deletes legacy owned budgets and preserves unrelated budgets", func() { f := newFixture(nil) + owned := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "legacy-owned", + Namespace: f.ns, + Labels: map[string]string{LabelOwned: "true"}, + }, + } + unowned := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "unowned", Namespace: f.ns}, + } + Expect(k8sClient.Create(ctx, owned)).To(Succeed()) + Expect(k8sClient.Create(ctx, unowned)).To(Succeed()) - By("owning a blocking PDB with minAvailable = base replicas") - Eventually(func() (int, error) { - pdb, err := getPDB(f) - if err != nil { - return -1, err - } - return pdb.Spec.MinAvailable.IntValue(), nil - }).Should(Equal(1)) + Expect(sweepOwnedPDBs(ctx, k8sClient, k8sClient)).To(Succeed()) + Eventually(func() bool { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(owned), &policyv1.PodDisruptionBudget{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(unowned), &policyv1.PodDisruptionBudget{})).To(Succeed()) + }) + + It("owns no objects and stays Idle on a healthy node", func() { + f := newFixture(nil) + + By("owning no PodDisruptionBudget") + Consistently(func() bool { + return apierrors.IsNotFound(getPDB(f)) + }).Should(BeTrue()) By("reporting Idle and never touching replicas") Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) + Expect(findCond(getCR(f), appsv1alpha1.ConditionProtected)).To(And( + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", "EvictionHold"), + )) Consistently(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(1))) Expect(getDeployment(f).Annotations).NotTo(HaveKey(AnnotationBaseReplicas)) }) @@ -277,39 +283,6 @@ var _ = Describe("Understudy controller", func() { }) } - It("reports a pinned node without acting on it", func() { - f := newFixture(nil) - Eventually(func() error { - _, err := getPDB(f) - return err - }).Should(Succeed()) - - emitBlockedEvent(f.node.Name, f.ns+"/understudy-"+f.cr.Name) - - By("raising the NodePinned condition") - Eventually(func() metav1.ConditionStatus { - if c := findCond(getCR(f), appsv1alpha1.ConditionNodePinned); c != nil { - return c.Status - } - return metav1.ConditionUnknown - }).Should(Equal(metav1.ConditionTrue)) - - By("touching nothing: no surge, no annotations, the budget keeps blocking") - Consistently(func() int32 { return *getDeployment(f).Spec.Replicas }).Should(Equal(int32(1))) - Expect(getDeployment(f).Annotations).NotTo(HaveKey(AnnotationBaseReplicas)) - pdb, err := getPDB(f) - Expect(err).NotTo(HaveOccurred()) - Expect(pdb.Spec.MinAvailable.IntValue()).To(Equal(1)) - - By("clearing the condition once the refusals go stale") - Eventually(func() metav1.ConditionStatus { - if c := findCond(getCR(f), appsv1alpha1.ConditionNodePinned); c != nil { - return c.Status - } - return metav1.ConditionUnknown - }, 30*time.Second).Should(Equal(metav1.ConditionFalse)) - }) - It("does not surge on unrelated taints or healthy nodes", func() { f := newFixture(nil) taintNode(f.node, "example.com/some-unrelated-taint") @@ -390,25 +363,15 @@ var _ = Describe("Understudy controller", func() { Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseIdle)) }) - It("fires the never-wedge TTL: a stuck surge relaxes the PDB instead of wedging the drain", func() { + It("fires the never-wedge TTL and admits the eviction", func() { f := newFixture(func(cr *appsv1alpha1.Understudy) { cr.Spec.ReadinessDeadlineSeconds = ptr.To(int32(1)) }) cordonNode(f.node) expectSurged(f) - By("relaxing the PDB after the readiness deadline") - Eventually(func() (int, error) { - pdb, err := getPDB(f) - if err != nil { - return -1, err - } - return pdb.Spec.MinAvailable.IntValue(), nil - }).Should(Equal(0), "PDB should be relaxed to minAvailable=0") - pdb, err := getPDB(f) - Expect(err).NotTo(HaveOccurred()) - Expect(pdb.Annotations).To(HaveKeyWithValue(AnnotationRelaxed, relaxReasonTTL)) Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseRelaxed)) + Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) }) It("releases immediately when an involuntary deadline is shorter than minSurgeTimeSeconds", func() { @@ -426,19 +389,14 @@ var _ = Describe("Understudy controller", func() { }) DeferCleanup(func() { registry.Clear(f.node.Name, "test:injected-itn") }) - By("relaxing the PDB at once; holding a hostage cannot help below minSurgeTime") - Eventually(func() (int, error) { - pdb, err := getPDB(f) - if err != nil { - return -1, err - } - return pdb.Spec.MinAvailable.IntValue(), nil - }).Should(Equal(0)) - pdb, _ := getPDB(f) - Expect(pdb.Annotations).To(HaveKeyWithValue(AnnotationRelaxed, relaxReasonDeadline)) + By("relaxing the hold at once because waiting cannot help below minSurgeTime") + Eventually(func() appsv1alpha1.Phase { return getCR(f).Status.Phase }).Should(Equal(appsv1alpha1.PhaseRelaxed)) By("still surging in parallel to minimize the gap") expectSurged(f) + + By("admitting the eviction") + Eventually(func() error { return evict(f.pod, false) }).Should(Succeed()) }) It("skips unsupported target kinds with a condition, never error-looping", func() { @@ -457,8 +415,7 @@ var _ = Describe("Understudy controller", func() { cr.Spec.HostageMode = appsv1alpha1.HostageOff }) Consistently(func() bool { - _, err := getPDB(f) - return apierrors.IsNotFound(err) + return apierrors.IsNotFound(getPDB(f)) }).Should(BeTrue(), "no PDB may exist in hostageMode=off") cordonNode(f.node) diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index a59d984..de5c9e2 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -31,23 +31,18 @@ var ( }, []string{"class"}) RelaxedTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ - Name: "understudy_pdb_relaxed_total", - Help: "Hostage PDBs relaxed without a ready understudy, by reason.", + Name: "understudy_hold_relaxed_total", + Help: "Eviction holds relaxed without a ready understudy, by reason.", }, []string{"reason"}) EvictionAttemptsObserved = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "understudy_eviction_attempts_observed_total", - Help: "Eviction attempts seen by the observe-only webhook, by whether the pod is protected by an Understudy-owned PDB.", + Help: "Eviction attempts seen by the webhook, by whether the pod is protected by an Understudy.", }, []string{"protected"}) EvictionsHeld = prometheus.NewCounter(prometheus.CounterOpts{ Name: "understudy_evictions_held_total", - Help: "Evictions of protected pods answered with 429 at admission because no stand-in was ready yet (experimental eviction-hold mode).", - }) - - NodePinnedTotal = prometheus.NewCounter(prometheus.CounterOpts{ - Name: "understudy_node_pinned_total", - Help: "Times a protected workload was detected pinning its node: a disrupter wanted the node and the hostage budget blocked it. Understudy reports this and deliberately does not act; see Limits and the tradeoff in the README.", + Help: "Evictions of protected pods answered with 429 at admission because no stand-in was ready yet.", }) blockedMu sync.Mutex @@ -83,5 +78,5 @@ func UntrackBlocked(key string) { } func init() { - ctrlmetrics.Registry.MustRegister(SurgesTotal, RelaxedTotal, EvictionAttemptsObserved, EvictionsHeld, NodePinnedTotal, OldestBlockedEvictionSeconds) + ctrlmetrics.Registry.MustRegister(SurgesTotal, RelaxedTotal, EvictionAttemptsObserved, EvictionsHeld, OldestBlockedEvictionSeconds) } diff --git a/internal/signal/evictionwebhook/adapter.go b/internal/signal/evictionwebhook/adapter.go index ec1e74d..71ed1b0 100644 --- a/internal/signal/evictionwebhook/adapter.go +++ b/internal/signal/evictionwebhook/adapter.go @@ -54,6 +54,11 @@ type Adapter struct { registry *signal.Registry } +type protector struct { + cr *appsv1alpha1.Understudy + dep *appsv1.Deployment +} + func New() *Adapter { return &Adapter{} } func (a *Adapter) lifetime() time.Duration { @@ -88,10 +93,13 @@ func (a *Adapter) Handle(ctx context.Context, req admission.Request) admission.R if pod.Spec.NodeName == "" { return allowed } + if pod.DeletionTimestamp != nil { + return allowed + } - cr, dep, protected := a.resolveProtector(ctx, &pod) - metrics.EvictionAttemptsObserved.WithLabelValues(strconv.FormatBool(protected)).Inc() - if !protected { + protectors := a.resolveProtectors(ctx, &pod) + metrics.EvictionAttemptsObserved.WithLabelValues(strconv.FormatBool(len(protectors) > 0)).Inc() + if len(protectors) == 0 { return allowed } @@ -105,39 +113,39 @@ func (a *Adapter) Handle(ctx context.Context, req admission.Request) admission.R logf.FromContext(ctx).V(1).Info("eviction attempt observed", "pod", req.Namespace+"/"+req.Name, "node", pod.Spec.NodeName, "evictor", req.UserInfo.Username) - if !cr.ExperimentalEvictionHold() { - return allowed - } - if cr.HostageModeOrDefault() == appsv1alpha1.HostageOff { - return allowed - } - if cr.Status.Phase == appsv1alpha1.PhaseRelaxed { - return allowed - } - if a.standInReady(ctx, dep) { - return allowed - } - metrics.EvictionsHeld.Inc() - logf.FromContext(ctx).Info("eviction held; stand-in not ready", - "pod", req.Namespace+"/"+req.Name, "evictor", req.UserInfo.Username) - return admission.Response{AdmissionResponse: admissionv1.AdmissionResponse{ - Allowed: false, - Result: &metav1.Status{ - Status: metav1.StatusFailure, - Code: http.StatusTooManyRequests, - Reason: metav1.StatusReasonTooManyRequests, - Message: fmt.Sprintf("understudy is preparing a stand-in for %s/%s; retry shortly", req.Namespace, req.Name), - }, - }} + for _, p := range protectors { + if p.cr.HostageModeOrDefault() == appsv1alpha1.HostageOff || + p.cr.Status.Phase == appsv1alpha1.PhaseRelaxed || + a.standInReady(ctx, p.dep, pod.Spec.NodeName) { + continue + } + metrics.EvictionsHeld.Inc() + logf.FromContext(ctx).Info("Eviction held; stand-in not ready", + "pod", req.Namespace+"/"+req.Name, "evictor", req.UserInfo.Username) + return admission.Response{AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: false, + Result: &metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusTooManyRequests, + Reason: metav1.StatusReasonTooManyRequests, + Message: fmt.Sprintf("understudy is preparing a stand-in for %s/%s; retry shortly", req.Namespace, req.Name), + }, + }} + } + return allowed } -func (a *Adapter) resolveProtector(ctx context.Context, pod *corev1.Pod) (*appsv1alpha1.Understudy, *appsv1.Deployment, bool) { +func (a *Adapter) resolveProtectors(ctx context.Context, pod *corev1.Pod) []protector { var crs appsv1alpha1.UnderstudyList if err := a.reader.List(ctx, &crs, client.InNamespace(pod.Namespace)); err != nil { - return nil, nil, false + return nil } + var protectors []protector for i := range crs.Items { cr := &crs.Items[i] + if cr.Status.Phase == appsv1alpha1.PhaseUnsupported { + continue + } if kind := cr.Spec.TargetRef.Kind; kind != "" && kind != "Deployment" { continue } @@ -150,13 +158,13 @@ func (a *Adapter) resolveProtector(ctx context.Context, pod *corev1.Pod) (*appsv continue } if selector.Matches(labels.Set(pod.Labels)) { - return cr, &dep, true + protectors = append(protectors, protector{cr: cr, dep: &dep}) } } - return nil, nil, false + return protectors } -func (a *Adapter) standInReady(ctx context.Context, dep *appsv1.Deployment) bool { +func (a *Adapter) standInReady(ctx context.Context, dep *appsv1.Deployment, victimNode string) bool { base := int32(1) if dep.Spec.Replicas != nil { base = *dep.Spec.Replicas @@ -181,6 +189,12 @@ func (a *Adapter) standInReady(ctx context.Context, dep *appsv1.Deployment) bool if p.DeletionTimestamp != nil { continue } + if p.Spec.NodeName == victimNode { + continue + } + if _, doomed := a.registry.Lookup(p.Spec.NodeName); doomed { + continue + } for _, c := range p.Status.Conditions { if c.Type == corev1.PodReady && c.Status == corev1.ConditionTrue { healthy++ @@ -188,5 +202,5 @@ func (a *Adapter) standInReady(ctx context.Context, dep *appsv1.Deployment) bool } } } - return healthy > int(base) + return healthy >= int(base) } diff --git a/internal/signal/pinned/pinned.go b/internal/signal/pinned/pinned.go deleted file mode 100644 index 628173f..0000000 --- a/internal/signal/pinned/pinned.go +++ /dev/null @@ -1,227 +0,0 @@ -/* -Copyright 2026 The Understudy Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package pinned - -import ( - "context" - "fmt" - "regexp" - "strings" - "sync" - "time" - - corev1 "k8s.io/api/core/v1" - policyv1 "k8s.io/api/policy/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/events" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/predicate" - - appsv1alpha1 "github.com/kylan11/understudy/api/v1alpha1" - "github.com/kylan11/understudy/internal/metrics" -) - -const ( - BlockedReason = "DisruptionBlocked" - nodeKind = "Node" - ownedLabel = "understudy.sh/owned" - ownedPDBPrefix = "understudy-" - defaultStaleAfter = 15 * time.Minute -) - -var ( - pdbWord = regexp.MustCompile(`(?i)\bpdbs?\b`) - objRef = regexp.MustCompile(`\b[a-z0-9][a-z0-9.-]*/[a-z0-9][a-z0-9.-]*\b`) -) - -type Watcher struct { - StaleAfter time.Duration - - client client.Client - recorder events.EventRecorder - mu sync.Mutex - pinnedBy map[types.NamespacedName]types.NamespacedName -} - -func New() *Watcher { return &Watcher{} } - -func (w *Watcher) Name() string { return "pin-detection" } - -func (w *Watcher) staleAfter() time.Duration { - if w.StaleAfter > 0 { - return w.StaleAfter - } - return defaultStaleAfter -} - -func (w *Watcher) SetupWithManager(mgr ctrl.Manager) error { - w.client = mgr.GetClient() - w.recorder = mgr.GetEventRecorder("understudy") - w.pinnedBy = map[types.NamespacedName]types.NamespacedName{} - return ctrl.NewControllerManagedBy(mgr). - For(&corev1.Event{}). - Named("pin-detection"). - WithEventFilter(predicate.NewPredicateFuncs(func(obj client.Object) bool { - ev, ok := obj.(*corev1.Event) - return ok && ev.Reason == BlockedReason && ev.InvolvedObject.Kind == nodeKind - })). - Complete(w) -} - -func (w *Watcher) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - var ev corev1.Event - if err := w.client.Get(ctx, req.NamespacedName, &ev); err != nil { - if apierrors.IsNotFound(err) { - if cr, ok := w.forget(req.NamespacedName); ok { - return ctrl.Result{}, w.setPinned(ctx, cr, false, "") - } - return ctrl.Result{}, nil - } - return ctrl.Result{}, err - } - if ev.Reason != BlockedReason || ev.InvolvedObject.Kind != nodeKind { - return ctrl.Result{}, nil - } - - seen := lastSeen(&ev) - if seen.IsZero() { - seen = time.Now() - } - fresh := time.Since(seen) < w.staleAfter() - - for _, ref := range pdbRefs(ev.Message) { - cr, owned, err := w.resolveOwned(ctx, ref) - if err != nil { - return ctrl.Result{}, err - } - if !owned { - continue - } - if !fresh { - w.forget(req.NamespacedName) - return ctrl.Result{}, w.setPinned(ctx, cr, false, "") - } - w.remember(req.NamespacedName, cr) - if err := w.setPinned(ctx, cr, true, ev.InvolvedObject.Name); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: w.staleAfter() * 3 / 2}, nil - } - return ctrl.Result{}, nil -} - -func (w *Watcher) resolveOwned(ctx context.Context, ref types.NamespacedName) (types.NamespacedName, bool, error) { - var pdb policyv1.PodDisruptionBudget - if err := w.client.Get(ctx, ref, &pdb); err != nil { - if apierrors.IsNotFound(err) { - return types.NamespacedName{}, false, nil - } - return types.NamespacedName{}, false, err - } - if pdb.Labels[ownedLabel] != "true" || !strings.HasPrefix(pdb.Name, ownedPDBPrefix) { - return types.NamespacedName{}, false, nil - } - return types.NamespacedName{ - Namespace: pdb.Namespace, - Name: strings.TrimPrefix(pdb.Name, ownedPDBPrefix), - }, true, nil -} - -func (w *Watcher) setPinned(ctx context.Context, key types.NamespacedName, pinned bool, node string) error { - var cr appsv1alpha1.Understudy - if err := w.client.Get(ctx, key, &cr); err != nil { - return client.IgnoreNotFound(err) - } - desired := metav1.ConditionFalse - reason := "NoRecentRefusals" - message := "no disruption refusals observed recently" - if pinned { - desired = metav1.ConditionTrue - reason = "BudgetBlocksDisruption" - message = fmt.Sprintf( - "a disrupter wants node %s but the hostage budget blocks it; graceful consolidation, drift and expiry cannot proceed (see Limits and the tradeoff in the README)", - node) - } - current := meta.FindStatusCondition(cr.Status.Conditions, appsv1alpha1.ConditionNodePinned) - if current != nil && current.Status == desired { - return nil - } - patched := cr.DeepCopy() - meta.SetStatusCondition(&patched.Status.Conditions, metav1.Condition{ - Type: appsv1alpha1.ConditionNodePinned, - Status: desired, - Reason: reason, - Message: message, - ObservedGeneration: cr.Generation, - }) - if err := w.client.Status().Patch(ctx, patched, client.MergeFrom(&cr)); err != nil { - return err - } - if pinned { - metrics.NodePinnedTotal.Inc() - if w.recorder != nil { - w.recorder.Eventf(&cr, nil, corev1.EventTypeWarning, "NodePinned", "Detect", "%s", message) - } - logf.FromContext(ctx).Info("protected workload is pinning its node", - "understudy", key.String(), "node", node) - } - return nil -} - -func (w *Watcher) remember(event, cr types.NamespacedName) { - w.mu.Lock() - defer w.mu.Unlock() - w.pinnedBy[event] = cr -} - -func (w *Watcher) forget(event types.NamespacedName) (types.NamespacedName, bool) { - w.mu.Lock() - defer w.mu.Unlock() - cr, ok := w.pinnedBy[event] - delete(w.pinnedBy, event) - return cr, ok -} - -func pdbRefs(message string) []types.NamespacedName { - if !pdbWord.MatchString(message) { - return nil - } - var refs []types.NamespacedName - for _, token := range objRef.FindAllString(message, 8) { - parts := strings.SplitN(token, "/", 2) - refs = append(refs, types.NamespacedName{Namespace: parts[0], Name: parts[1]}) - } - return refs -} - -func lastSeen(ev *corev1.Event) time.Time { - t := ev.FirstTimestamp.Time - for _, c := range []time.Time{ev.LastTimestamp.Time, ev.EventTime.Time} { - if c.After(t) { - t = c - } - } - if ev.Series != nil && ev.Series.LastObservedTime.After(t) { - t = ev.Series.LastObservedTime.Time - } - return t -} diff --git a/internal/signal/pinned/pinned_test.go b/internal/signal/pinned/pinned_test.go deleted file mode 100644 index f0c511d..0000000 --- a/internal/signal/pinned/pinned_test.go +++ /dev/null @@ -1,210 +0,0 @@ -/* -Copyright 2026 The Understudy Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package pinned - -import ( - "context" - "testing" - "time" - - corev1 "k8s.io/api/core/v1" - policyv1 "k8s.io/api/policy/v1" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - - appsv1alpha1 "github.com/kylan11/understudy/api/v1alpha1" -) - -const ( - testNS = "team-a" - testCR = "lead" - testPDB = ownedPDBPrefix + testCR - testNode = "n1" -) - -func TestPDBRefsParsesKarpenterQuotedMessage(t *testing.T) { - refs := pdbRefs(`Cannot disrupt Node: pdb "team-a/understudy-lead" prevents pod evictions`) - if len(refs) != 1 { - t.Fatalf("expected 1 ref, got %v", refs) - } - want := types.NamespacedName{Namespace: testNS, Name: testPDB} - if refs[0] != want { - t.Fatalf("expected %v, got %v", want, refs[0]) - } -} - -func TestPDBRefsParsesTheLiveObservedKarpenterFormat(t *testing.T) { - for _, msg := range []string{ - "Pdb prevents pod evictions (PodDisruptionBudget=[team-a/understudy-lead])", - "(combined from similar events): Pdb prevents pod evictions (PodDisruptionBudget=[team-a/understudy-lead])", - "Cannot disrupt NodeClaim: PDB team-a/understudy-lead prevents pod evictions", - } { - refs := pdbRefs(msg) - if len(refs) != 1 || refs[0].Name != testPDB || refs[0].Namespace != testNS { - t.Fatalf("expected the live-observed format to parse, got %v for %q", refs, msg) - } - } -} - -func TestPDBRefsIgnoresMessagesWithoutABudget(t *testing.T) { - for _, msg := range []string{ - "Cannot disrupt Node: state node is nominated for a pending pod", - `Cannot disrupt Node: pod "team-a/lead-0" has "karpenter.sh/do-not-disrupt" annotation`, - "Node isn't initialized", - } { - if refs := pdbRefs(msg); refs != nil { - t.Fatalf("expected no refs for %q, got %v", msg, refs) - } - } -} - -func testScheme(t *testing.T) *runtime.Scheme { - t.Helper() - s := runtime.NewScheme() - if err := clientgoscheme.AddToScheme(s); err != nil { - t.Fatal(err) - } - if err := appsv1alpha1.AddToScheme(s); err != nil { - t.Fatal(err) - } - return s -} - -func fixtures() (*policyv1.PodDisruptionBudget, *appsv1alpha1.Understudy) { - pdb := &policyv1.PodDisruptionBudget{ - ObjectMeta: metav1.ObjectMeta{ - Name: testPDB, - Namespace: testNS, - Labels: map[string]string{ownedLabel: "true"}, - }, - } - cr := &appsv1alpha1.Understudy{ - ObjectMeta: metav1.ObjectMeta{Name: testCR, Namespace: testNS}, - } - return pdb, cr -} - -func blockedEvent(message string, seen time.Time) *corev1.Event { - return &corev1.Event{ - ObjectMeta: metav1.ObjectMeta{Name: testNode + ".blocked", Namespace: "default"}, - InvolvedObject: corev1.ObjectReference{Kind: nodeKind, Name: testNode}, - Reason: BlockedReason, - Type: corev1.EventTypeNormal, - Message: message, - LastTimestamp: metav1.NewTime(seen), - } -} - -func newTestWatcher(t *testing.T, objs ...runtime.Object) *Watcher { - t.Helper() - c := fake.NewClientBuilder(). - WithScheme(testScheme(t)). - WithRuntimeObjects(objs...). - WithStatusSubresource(&appsv1alpha1.Understudy{}). - Build() - return &Watcher{client: c, pinnedBy: map[types.NamespacedName]types.NamespacedName{}} -} - -func reconcile(t *testing.T, w *Watcher, ev *corev1.Event) ctrl.Result { - t.Helper() - res, err := w.Reconcile(context.Background(), - ctrl.Request{NamespacedName: types.NamespacedName{Namespace: ev.Namespace, Name: ev.Name}}) - if err != nil { - t.Fatalf("reconcile failed: %v", err) - } - return res -} - -func pinnedCondition(t *testing.T, w *Watcher) *metav1.Condition { - t.Helper() - var cr appsv1alpha1.Understudy - if err := w.client.Get(context.Background(), - types.NamespacedName{Namespace: testNS, Name: testCR}, &cr); err != nil { - t.Fatalf("get cr: %v", err) - } - return meta.FindStatusCondition(cr.Status.Conditions, appsv1alpha1.ConditionNodePinned) -} - -func TestFreshRefusalSetsNodePinned(t *testing.T) { - pdb, cr := fixtures() - ev := blockedEvent(`Cannot disrupt Node: pdb "team-a/understudy-lead" prevents pod evictions`, time.Now()) - w := newTestWatcher(t, pdb, cr, ev) - - res := reconcile(t, w, ev) - cond := pinnedCondition(t, w) - if cond == nil || cond.Status != metav1.ConditionTrue { - t.Fatalf("expected NodePinned=True, got %+v", cond) - } - if res.RequeueAfter == 0 { - t.Fatal("expected a requeue to eventually clear a stale pin") - } -} - -func TestStaleRefusalClearsNodePinned(t *testing.T) { - pdb, cr := fixtures() - fresh := blockedEvent(`Cannot disrupt Node: pdb "team-a/understudy-lead" prevents pod evictions`, time.Now()) - w := newTestWatcher(t, pdb, cr, fresh) - reconcile(t, w, fresh) - - stale := fresh.DeepCopy() - stale.LastTimestamp = metav1.NewTime(time.Now().Add(-time.Hour)) - if err := w.client.Update(context.Background(), stale); err != nil { - t.Fatalf("age the event: %v", err) - } - reconcile(t, w, stale) - - cond := pinnedCondition(t, w) - if cond == nil || cond.Status != metav1.ConditionFalse { - t.Fatalf("expected NodePinned=False after the refusals went stale, got %+v", cond) - } -} - -func TestDeletedEventClearsNodePinned(t *testing.T) { - pdb, cr := fixtures() - ev := blockedEvent(`Cannot disrupt Node: pdb "team-a/understudy-lead" prevents pod evictions`, time.Now()) - w := newTestWatcher(t, pdb, cr, ev) - reconcile(t, w, ev) - - if err := w.client.Delete(context.Background(), ev); err != nil { - t.Fatalf("delete event: %v", err) - } - reconcile(t, w, ev) - - cond := pinnedCondition(t, w) - if cond == nil || cond.Status != metav1.ConditionFalse { - t.Fatalf("expected NodePinned=False after the event vanished, got %+v", cond) - } -} - -func TestForeignBudgetSetsNothing(t *testing.T) { - _, cr := fixtures() - foreign := &policyv1.PodDisruptionBudget{ - ObjectMeta: metav1.ObjectMeta{Name: "bystander", Namespace: testNS}, - } - ev := blockedEvent(`Cannot disrupt Node: pdb "team-a/bystander" prevents pod evictions`, time.Now()) - w := newTestWatcher(t, foreign, cr, ev) - reconcile(t, w, ev) - - if cond := pinnedCondition(t, w); cond != nil { - t.Fatalf("a budget this operator does not own must not pin anything, got %+v", cond) - } -} diff --git a/internal/signal/signal.go b/internal/signal/signal.go index 8d76131..caf9b18 100644 --- a/internal/signal/signal.go +++ b/internal/signal/signal.go @@ -119,7 +119,7 @@ func (r *Registry) Lookup(node string) (NodeDoom, bool) { return best, true } -func (r *Registry) StartJanitor(ctx context.Context, interval time.Duration) { +func (r *Registry) StartExpiryLoop(ctx context.Context, interval time.Duration) { go func() { ticker := time.NewTicker(interval) defer ticker.Stop()