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 3ff697d..f96e7c8 100644 --- a/api/v1alpha1/understudy_types.go +++ b/api/v1alpha1/understudy_types.go @@ -45,7 +45,6 @@ const ( ConditionProtected = "Protected" ConditionSurgeReady = "SurgeReady" ConditionZeroDowntimeStrategy = "ZeroDowntimeUpdateStrategy" - ConditionNodePinned = "NodePinned" ) type TargetReference struct { 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 0160413..5eea01f 100644 --- a/internal/controller/evictionwebhook_test.go +++ b/internal/controller/evictionwebhook_test.go @@ -17,16 +17,22 @@ 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" + "github.com/kylan11/understudy/internal/signal" ) func evict(pod *corev1.Pod, dryRun bool) error { @@ -45,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 { @@ -70,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) @@ -85,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) @@ -94,9 +94,53 @@ 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", func() { + f := newFixture(nil) + + By("owning no budget at all") + Consistently(func() bool { + return apierrors.IsNotFound(getPDB(f)) + }).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", func() { + f := newFixture(func(cr *appsv1alpha1.Understudy) { + 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()) By("triggering a surge via an eviction attempt") Eventually(func() bool { return evict(f.pod, false) != nil }).Should(BeTrue()) @@ -106,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 7e02fca..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,8 +44,6 @@ 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" @@ -55,9 +52,7 @@ const ( relaxReasonDeadline = "deadline" relaxReasonMode = "mode" - activeRequeue = 5 * time.Second - heartbeatPeriod = 60 * time.Second - protectedRequeue = 60 * time.Second + activeRequeue = 5 * time.Second ) type UnderstudyReconciler struct { @@ -81,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 @@ -141,11 +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) - protected, err := r.ensurePDB(ctx, &cr, &dep, base, mode, relaxReason, len(assessment.doomed) > 0) - if err != nil { - return ctrl.Result{}, err - } - setProtectedCondition(&status, &cr, protected, mode, relaxReason) + setProtectedCondition(&status, &cr, mode, relaxReason) switch { case len(assessment.doomed) == 0 && !surgeActive: @@ -153,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) @@ -281,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 @@ -296,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, @@ -356,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: "true", - "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 @@ -485,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 { @@ -536,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))). @@ -636,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 aba1b8a..de5c9e2 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -31,18 +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"}) - 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.", + 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.", }) blockedMu sync.Mutex @@ -78,5 +78,5 @@ func UntrackBlocked(key string) { } func init() { - ctrlmetrics.Registry.MustRegister(SurgesTotal, RelaxedTotal, EvictionAttemptsObserved, 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 3f62c71..71ed1b0 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 { @@ -50,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 { @@ -84,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 + } - protected := a.podProtected(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 } @@ -100,23 +112,95 @@ 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) + + 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) 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) 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 } - for i := range pdbs.Items { - selector, err := metav1.LabelSelectorAsSelector(pdbs.Items[i].Spec.Selector) + 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 + } + 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 + protectors = append(protectors, protector{cr: cr, dep: &dep}) + } + } + return protectors +} + +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 + } + 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 + } + 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++ + break + } } } - return false + 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()