diff --git a/README.md b/README.md index 4d33277..0580b7d 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ version tag in production. - **Talos Linux and Ubuntu images**, resolved per architecture. - **Placement groups** for spreading nodes across physical hosts. - **Cost controls** — opt out of the billed public IPv4 (and/or IPv6) per node class for private-network clusters. -- **Multi-cluster safe** — every managed server is tagged with the cluster name, so several clusters can share one Hetzner project without touching each other's nodes. +- **Multi-cluster safe** — every managed server is tagged with the cluster name and the cluster's `kube-system` UID, so several clusters can share one Hetzner project without touching each other's nodes. Servers created before the UID label existed are matched on name alone, so give each cluster a distinct `clusterName` until the fleet has rolled. ## How it works @@ -40,6 +40,7 @@ version tag in production. │ • instancetype — server types → priced InstanceTypes │ │ • imagefamily — resolve Talos/Ubuntu images per arch │ │ • nodeclass ctrl— validate HCloudNodeClass, set Ready │ +│ • instance GC — reclaim servers with no NodeClaim │ └───────────────┬────────────────────────────────────────────┘ │ hcloud API ┌───────▼────────┐ @@ -49,6 +50,83 @@ version tag in production. A `NodePool` references an `HCloudNodeClass`. When pods are unschedulable, Karpenter asks this provider for instance types, picks the cheapest compatible offering, creates the server, and the node joins the cluster. +### Reclaiming orphaned servers + +A server can outlive Karpenter's record of it. If the operator dies between the +hcloud create call and writing the provider ID to the NodeClaim — a lost leader +election, an evicted pod, an API-server timeout — the machine boots and runs with +nothing pointing at it. Karpenter core does not reclaim it: its garbage collector +deletes NodeClaims that have no server, never the reverse. + +Two mechanisms cover this: + +- **Adoption.** Hetzner rejects duplicate server names, so the next attempt for + the same NodeClaim collides. Rather than retrying into that collision forever, + the provider looks the server up and adopts it, provided it belongs to this + cluster and this NodeClaim and matches the requested type, location and image. +- **Garbage collection.** A sweep every two minutes reclaims servers Karpenter + has no NodeClaim for, along with the Node objects they left behind. A server + must be seen unowned on several consecutive sweeps, and one whose node is + registered and still `Ready` is never touched — a machine carrying workloads is + core's to drain, not this sweep's to destroy. + + Every path that declines to act resets the count, so the window always measures + an uninterrupted run of sweeps that found nothing in the way; a machine the + `Ready` guard protected never sits on a spent window waiting for its first + NotReady blip. The count is per-process, so a restart or leader handover starts + it again — and the operator must additionally have been sweeping for a full + window before it may reclaim anything, so instability delays reclamation rather + than authorising it on a short history. + +**`clusterName` must be unique per cluster within a Hetzner project.** Servers +are labelled with it, and the sweep uses that label to decide what it owns. Two +clusters sharing a name in one project would each see the other's servers as +unclaimed. The operator therefore also stamps the UID of the cluster's +`kube-system` namespace on every server it creates and refuses to touch a server +carrying a different one, logging the collision once and counting it as +`karpenter_hetzner_orphaned_server_gc_total{result="skipped_foreign_cluster"}` on +every sweep. + +Two things this does not cover. It protects servers created from this version +onward; servers predating it carry no UID and are still matched on name alone, so +until a fleet has fully rolled, distinct names remain the thing to get right. +And the UID identifies the *control plane*, not the servers: rebuilding a cluster +from scratch mints a new `kube-system` UID, after which the previous +incarnation's servers are refused forever — never reclaimed, still billing. The +`skipped_foreign_cluster` counter is the signal for both. Recovering from a +rebuild means relabelling those servers with the new UID +(`hcloud server add-label karpenter.sh/cluster-uid=`, where `` +is `kubectl get ns kube-system -o jsonpath='{.metadata.uid}'`) or deleting them +by hand. + +> **Upgrading an existing cluster.** This version adds a controller that +> **deletes Hetzner servers**. On first start it reclaims every server in the +> project that carries this cluster's labels and has no NodeClaim — which is the +> point, but on a fleet nobody has audited it is worth seeing first. +> +> Set `instanceGarbageCollection.mode: observe` to run every check and report +> what *would* be reclaimed without deleting anything. Watch +> `karpenter_hetzner_orphaned_server_gc_total{result="would_reap"}` and the +> `WouldGarbageCollect` events on the affected Nodes, satisfy yourself the list +> is right, then switch to `enabled`. Reclamations are recorded as +> `GarbageCollected` events on the Node, so `kubectl describe node` explains a +> server that disappeared. + +Set `instanceGarbageCollection.mode: disabled` to pause the sweep during +maintenance that removes NodeClaims wholesale (reinstalling the CRDs, restoring +etcd, clearing finalizers by hand), so it does not act on a cluster that only +looks empty. Provisioning and disruption keep working while it is off. + +Watch `karpenter_hetzner_orphaned_server_gc_total` and +`karpenter_hetzner_server_adopt_total`, both labelled by `result`. Sustained +`server_adopt_total{result="adopted"}` means creates are losing their results. +Sustained `result="declined"` is worse: a NodeClaim keeps colliding with a server +adoption refuses to take, so the machine bills with nothing able to claim it. On +the sweep, a rising `orphaned_server_gc_total{result="error"}` means a server +cannot be reclaimed and is still billing, and `result="sweep_failed"` means the +sweep itself cannot run — it swallows list failures to protect its cadence, so +this counter is the only place a permanently broken sweep shows up. + ## Installation You need a Hetzner Cloud API token with read/write access and a Kubernetes cluster on Hetzner (the [hcloud Cloud Controller Manager](https://github.com/hetznercloud/hcloud-cloud-controller-manager) should set node provider IDs as `hcloud://`). @@ -176,7 +254,8 @@ comments explaining every field. | Env var | Required | Description | |---------|----------|-------------| | `HCLOUD_TOKEN` | yes | Hetzner Cloud API token | -| `CLUSTER_NAME` | yes | Cluster identifier; scopes managed servers | +| `CLUSTER_NAME` | yes | Cluster identifier; scopes managed servers. Must be unique per Hetzner project — two clusters sharing a value will reclaim each other's servers | +| `INSTANCE_GARBAGE_COLLECTION_MODE` | no (`enabled`) | `enabled`, `observe` or `disabled`; an unrecognised value stops the operator starting (chart value: `instanceGarbageCollection.mode`) | | `METRICS_PORT` | no (8080) | Prometheus metrics port | | `HEALTH_PROBE_PORT` | no (8081) | Health/readiness probe port | diff --git a/charts/karpenter-provider-hetzner/Chart.yaml b/charts/karpenter-provider-hetzner/Chart.yaml index 4fccd6b..455381f 100644 --- a/charts/karpenter-provider-hetzner/Chart.yaml +++ b/charts/karpenter-provider-hetzner/Chart.yaml @@ -2,7 +2,7 @@ apiVersion: v2 name: karpenter-provider-hetzner description: Karpenter cloud provider for Hetzner Cloud type: application -version: 2.1.0 +version: 2.2.0 appVersion: "2.1.0" # home points at Paperclip.inc (the project's canonical page) so listing # "Homepage" links drive backlinks to the domain; GitHub stays as source. @@ -31,7 +31,5 @@ annotations: artifacthub.io/category: integration-delivery artifacthub.io/containsSecurityUpdates: "false" artifacthub.io/changes: | - - kind: fixed - description: "Pods bound to an hcloud CSI volume can now trigger provisioning: csi.hetzner.cloud/location is aliased to topology.kubernetes.io/zone" - kind: added - description: "Expose nodeSelector, affinity, tolerations, topologySpreadConstraints, imagePullSecrets, priorityClassName, command, args and an optional PodDisruptionBudget (minAvailable or maxUnavailable) on the Deployment via chart values" + description: "Reclaim Hetzner servers Karpenter no longer has a NodeClaim for, which would otherwise run and bill unowned; set instanceGarbageCollection.mode to observe to preview it, or disabled to pause it during maintenance that removes NodeClaims wholesale" diff --git a/charts/karpenter-provider-hetzner/README.md b/charts/karpenter-provider-hetzner/README.md index 6286a29..5185b9b 100644 --- a/charts/karpenter-provider-hetzner/README.md +++ b/charts/karpenter-provider-hetzner/README.md @@ -36,7 +36,8 @@ Existing `v1alpha1` objects are not migrated automatically; recreate them under | Key | Default | Description | |-----|---------|-------------| -| `clusterName` | `""` (required) | Scopes which servers the controller manages | +| `clusterName` | `""` (required) | Scopes which servers the controller manages; must be unique per Hetzner project | +| `instanceGarbageCollection.mode` | `enabled` | `enabled`, `observe` (report what would be reclaimed, delete nothing) or `disabled` (see values.yaml) | | `replicas` | `1` | Controller replicas | | `image.repository` | `ghcr.io/paperclipinc/karpenter-provider-hetzner` | Image | | `image.tag` | `""` | Empty tracks the chart appVersion; pin a tag in production | @@ -86,4 +87,12 @@ When `serviceMonitor.enabled=true` the chart creates: - a `Service` named `karpenter-provider-hetzner-metrics` exposing port `http-metrics` - a `ServiceMonitor` that selects that Service and scrapes `/metrics` at the configured interval -Requires the [Prometheus Operator](https://github.com/prometheus-operator/prometheus-operator) CRDs to be present. The controller exposes provider metrics under the `karpenter_hetzner_` prefix (server creates/deletes, durations, drift reasons, instance-type cache hits/misses, and raw hcloud API call counts). +Requires the [Prometheus Operator](https://github.com/prometheus-operator/prometheus-operator) CRDs to be present. The controller exposes provider metrics under the `karpenter_hetzner_` prefix (server creates/deletes, durations, drift reasons, instance-type cache hits/misses, orphaned-server garbage-collection outcomes, adopted servers, unpriceable nodes, and raw hcloud API call counts). + +Worth an alert: + +- `karpenter_hetzner_orphaned_server_gc_total{result="error"}` — a server cannot be reclaimed and is still billing. +- `karpenter_hetzner_server_adopt_total{result="declined"}` — a NodeClaim keeps colliding with a server adoption refuses to take. +- `karpenter_hetzner_orphaned_server_gc_total{result="skipped_foreign_cluster"}` — servers carry this cluster's `clusterName` but another cluster's UID. Either two clusters share a name in one Hetzner project, or this cluster's control plane was rebuilt and these servers predate it. Either way they will never be reclaimed. + +Note that the metrics endpoint is scraped on every replica, but the sweeps behind these metrics run only on the leader. Aggregate with `max()` rather than `avg()`/`min()`, or a standby's zero will read as a healthy cluster. diff --git a/charts/karpenter-provider-hetzner/templates/deployment.yaml b/charts/karpenter-provider-hetzner/templates/deployment.yaml index 146d2a5..2c93b2b 100644 --- a/charts/karpenter-provider-hetzner/templates/deployment.yaml +++ b/charts/karpenter-provider-hetzner/templates/deployment.yaml @@ -68,6 +68,17 @@ spec: key: {{ .Values.auth.secretRef.key }} - name: CLUSTER_NAME value: {{ .Values.clusterName | quote }} + {{- if hasKey .Values.instanceGarbageCollection "disabled" }} + {{- fail "instanceGarbageCollection.disabled has been replaced by instanceGarbageCollection.mode (enabled|observe|disabled). Your existing value is being ignored, which would leave the orphaned-server sweep RUNNING. Set mode explicitly and remove the disabled key." }} + {{- end }} + # Emitted unconditionally rather than through `with`, which skips + # falsy values: `mode: false` -- the natural typo when migrating from + # the boolean this replaced -- would omit the variable, the operator + # would default to enabled, and a value meant to stop the sweep would + # start it. Passed through so an invalid value reaches parseGCMode, + # which refuses to start. + - name: INSTANCE_GARBAGE_COLLECTION_MODE + value: {{ .Values.instanceGarbageCollection.mode | quote }} - name: METRICS_PORT value: {{ .Values.metrics.port | quote }} - name: HEALTH_PROBE_PORT diff --git a/charts/karpenter-provider-hetzner/templates/rbac.yaml b/charts/karpenter-provider-hetzner/templates/rbac.yaml index 01dc666..8ffcc64 100644 --- a/charts/karpenter-provider-hetzner/templates/rbac.yaml +++ b/charts/karpenter-provider-hetzner/templates/rbac.yaml @@ -15,14 +15,23 @@ rules: - apiGroups: [""] resources: ["nodes", "nodes/status"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - # Core resources Karpenter's scheduling simulation observes. + # Core resources Karpenter's scheduling simulation observes. "namespaces" is + # also load-bearing for startup: the operator reads the kube-system namespace's + # UID to identify this cluster and refuses to start without it, so narrowing + # this rule takes the whole controller down, not just scheduling. - apiGroups: [""] resources: ["pods", "persistentvolumes", "persistentvolumeclaims", "replicationcontrollers", "namespaces"] verbs: ["get", "list", "watch"] - apiGroups: [""] resources: ["pods/eviction"] verbs: ["create"] - - apiGroups: [""] + # Karpenter core and leader election use the deprecated core-group recorder; + # this provider's garbage collector uses the manager's recorder, which writes + # to events.k8s.io/v1. RBAC authorizes on (apiGroup, resource), so the core + # group alone would 403 every GarbageCollected event -- silently, since only a + # klog line reports it, while the README tells operators to validate observe + # mode by reading those very events. + - apiGroups: ["", "events.k8s.io"] resources: ["events"] verbs: ["create", "patch"] - apiGroups: [""] diff --git a/charts/karpenter-provider-hetzner/values.yaml b/charts/karpenter-provider-hetzner/values.yaml index 3e5cecf..5c802bd 100644 --- a/charts/karpenter-provider-hetzner/values.yaml +++ b/charts/karpenter-provider-hetzner/values.yaml @@ -52,6 +52,24 @@ args: [] # Required: scopes managed servers so multiple clusters can share one Hetzner project. clusterName: "" +# The operator reclaims servers Karpenter no longer has a NodeClaim for, which +# would otherwise run and bill with nothing pointing at them. +instanceGarbageCollection: + # enabled - reclaim orphaned servers. + # observe - run every check and report what WOULD be reclaimed, deleting + # nothing. Start here on an existing fleet: watch + # karpenter_hetzner_orphaned_server_gc_total{result="would_reap"}, + # satisfy yourself it names the right machines, then switch to + # enabled. A kill switch only helps after something has gone wrong. + # disabled - do not sweep at all. For maintenance that removes NodeClaims + # wholesale (reinstalling the CRDs, restoring etcd, clearing + # finalizers by hand), so the sweep does not act on a cluster that + # only looks empty. Provisioning and disruption keep working. + # + # An unrecognised value stops the operator starting rather than falling back + # to enabled: a typo here would reap the fleet it was meant to protect. + mode: enabled + metrics: port: 8080 healthProbe: diff --git a/cmd/controller/main.go b/cmd/controller/main.go index e442fe8..281d075 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -1,11 +1,16 @@ package main import ( + "context" + "time" + "sigs.k8s.io/controller-runtime/pkg/log" // Register karpenter core types into the default k8s scheme. _ "sigs.k8s.io/karpenter/pkg/apis/v1" + "github.com/awslabs/operatorpkg/controller" + "sigs.k8s.io/karpenter/pkg/cloudprovider/overlay" "sigs.k8s.io/karpenter/pkg/controllers" "sigs.k8s.io/karpenter/pkg/controllers/state" @@ -15,6 +20,7 @@ import ( _ "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" hetznercp "github.com/paperclipinc/karpenter-provider-hetzner/pkg/cloudprovider" + instancegc "github.com/paperclipinc/karpenter-provider-hetzner/pkg/controllers/instance/garbagecollection" "github.com/paperclipinc/karpenter-provider-hetzner/pkg/controllers/nodeclass" hetznerop "github.com/paperclipinc/karpenter-provider-hetzner/pkg/operator" "github.com/paperclipinc/karpenter-provider-hetzner/pkg/providers/imagefamily" @@ -22,6 +28,10 @@ import ( "github.com/paperclipinc/karpenter-provider-hetzner/pkg/providers/instancetype" ) +// clusterUIDTimeout bounds the one API-server read the operator makes before the +// manager -- and therefore the health probes -- are running. +const clusterUIDTimeout = 30 * time.Second + func main() { ctx, op := operator.NewOperator() @@ -38,8 +48,27 @@ func main() { return } + // Identify this cluster independently of its operator-chosen name. CLUSTER_NAME + // is not guaranteed unique, and two clusters sharing one in a single Hetzner + // project would otherwise each treat the other's servers as its own -- which + // now means deleting them. The kube-system UID is unique per cluster and + // stable for its lifetime. Read through the API reader because the manager's + // cache is not running yet. + // + // Bound it: this runs before the manager starts, so the health probes are not + // listening yet and an apiserver that accepts the connection but never answers + // would hang the process where nothing can observe it. The rest config sets no + // per-request timeout of its own. + uidCtx, cancelUID := context.WithTimeout(ctx, clusterUIDTimeout) + clusterUID, err := hetznerop.ClusterUID(uidCtx, op.GetAPIReader()) + cancelUID() + if err != nil { + log.FromContext(ctx).Error(err, "failed to read the cluster UID") + return + } + // Create the three providers. - instanceProvider := instance.NewProviderWithPlacementGroups(&hcloudClient.Server, &hcloudClient.PlacementGroup, cfg.ClusterName, &hcloudClient.Action) + instanceProvider := instance.NewProviderWithPlacementGroups(&hcloudClient.Server, &hcloudClient.PlacementGroup, cfg.ClusterName, clusterUID, &hcloudClient.Action) typeProvider := instancetype.NewProvider(&hcloudClient.ServerType) imageProvider := imagefamily.NewProvider(&hcloudClient.Image) @@ -60,6 +89,38 @@ func main() { // Our NodeClass status controller (network + image validation, Ready). nodeClassController := nodeclass.NewController(op.GetClient(), &hcloudClient.Network, &hcloudClient.Firewall, &hcloudClient.SSHKey, imageProvider) + providerControllers := []controller.Controller{nodeClassController} + // Reap servers whose NodeClaim is gone. Karpenter core only garbage collects + // the opposite direction (NodeClaims with no instance), so without this an + // orphaned server runs and bills indefinitely. + // + // Every mode is logged, not just the unusual ones: this controller deletes + // machines, so which mode took effect must be answerable from the operator's + // own startup logs rather than inferred from a values file. + // + // Every mode is named explicitly and `default` refuses to start. Routing the + // unknown case to the deleting branch would re-open, one layer down, exactly + // the hole parseGCMode exists to close: GCMode's zero value is "", not + // "enabled", so any Config built without LoadConfig -- or any mode added to + // the parser and forgotten here -- would silently select "delete servers". + switch cfg.InstanceGarbageCollectionMode { + case hetznerop.GCDisabled: + log.FromContext(ctx).Info("instance garbage collection is disabled; " + + "servers whose NodeClaim is gone will not be reclaimed") + case hetznerop.GCObserve, hetznerop.GCEnabled: + mode := instancegc.Mode(cfg.InstanceGarbageCollectionMode) + log.FromContext(ctx).Info("instance garbage collection is active", + "mode", string(mode), + "reclaims", mode == instancegc.ModeEnabled) + providerControllers = append(providerControllers, + instancegc.NewController(op.GetClient(), instanceProvider, + cfg.ClusterName, clusterUID, mode, op.Clock)) + default: + log.FromContext(ctx).Error(nil, "unhandled instance garbage collection mode; refusing to start", + "mode", string(cfg.InstanceGarbageCollectionMode)) + return + } + // Wire and start all controllers. op.WithControllers(ctx, append( controllers.NewControllers( @@ -73,6 +134,6 @@ func main() { clusterState, op.InstanceTypeStore, ), - nodeClassController, + providerControllers..., )...).Start(ctx) } diff --git a/pkg/apis/v1/labels.go b/pkg/apis/v1/labels.go index 0a6fd43..5f28f12 100644 --- a/pkg/apis/v1/labels.go +++ b/pkg/apis/v1/labels.go @@ -21,4 +21,41 @@ const ( ServerLabelNodeClaim = "karpenter.sh/nodeclaim" ServerLabelNodePool = "karpenter.sh/nodepool" ServerValueManagedBy = "karpenter" + + // ServerLabelClusterUID carries the UID of this cluster's kube-system + // namespace, which is unique per cluster and stable for its lifetime. + // + // ServerLabelCluster alone is not a safe ownership test: CLUSTER_NAME is + // operator-supplied and nothing enforces uniqueness, so two clusters sharing + // a name in one Hetzner project each see the other's servers as their own. + // That was harmless when the label only scoped listings; it is not now that + // unclaimed servers are deleted. A server whose UID is present and different + // belongs to someone else. A missing UID means the server predates this + // label, and is treated as ours so existing fleets stay managed. + ServerLabelClusterUID = "karpenter.sh/cluster-uid" ) + +// OwnedByCluster reports whether a server's labels mark it as belonging to the +// installation identified by clusterName and clusterUID. +// +// This is the single definition of ownership. It is consulted from two places +// that both act destructively on the answer -- the orphan sweep deletes, and +// adoption hands a live machine to Karpenter, which eventually terminates it -- +// so the rule they apply has to be one rule. The legacy exemption below is the +// part that must not drift: it is a migration affordance that will be tightened +// once fleets have rolled, and tightening it in one caller but not the other +// would either strand every pre-UID orphan or resume cross-cluster deletion. +// +// A UID that is present and different belongs to another cluster. A missing UID +// predates the label and is treated as ours, because refusing those would strand +// every server created before it existed. +func OwnedByCluster(labels map[string]string, clusterName, clusterUID string) bool { + if labels[ServerLabelManagedBy] != ServerValueManagedBy { + return false + } + if labels[ServerLabelCluster] != clusterName { + return false + } + uid := labels[ServerLabelClusterUID] + return uid == "" || uid == clusterUID +} diff --git a/pkg/cloudprovider/cloudprovider_test.go b/pkg/cloudprovider/cloudprovider_test.go index 0f94365..b809b94 100644 --- a/pkg/cloudprovider/cloudprovider_test.go +++ b/pkg/cloudprovider/cloudprovider_test.go @@ -101,6 +101,17 @@ func (f *fakeServerClient) AllWithOpts(_ context.Context, _ hcloud.ServerListOpt return out, nil } +func (f *fakeServerClient) Update(_ context.Context, server *hcloud.Server, opts hcloud.ServerUpdateOpts) (*hcloud.Server, *hcloud.Response, error) { + s, ok := f.servers[server.ID] + if !ok { + return nil, nil, hcloud.Error{Code: hcloud.ErrorCodeNotFound, Message: "not found"} + } + if opts.Labels != nil { + s.Labels = opts.Labels + } + return s, nil, nil +} + type fakeServerTypeClient struct{ types []*hcloud.ServerType } func (f *fakeServerTypeClient) All(_ context.Context) ([]*hcloud.ServerType, error) { diff --git a/pkg/controllers/instance/garbagecollection/controller.go b/pkg/controllers/instance/garbagecollection/controller.go new file mode 100644 index 0000000..8be45f3 --- /dev/null +++ b/pkg/controllers/instance/garbagecollection/controller.go @@ -0,0 +1,493 @@ +// Package garbagecollection terminates Hetzner servers that Karpenter has no +// NodeClaim for. +// +// Karpenter core's own garbage collector only runs in one direction: it deletes +// NodeClaims whose instance has disappeared. Nothing in core terminates an +// instance whose NodeClaim has disappeared, so that direction is the cloud +// provider's responsibility. +// +// Servers end up orphaned when the controller dies between the Hetzner create +// call and persisting the provider ID — a crash window no in-process error +// handling can close. The server keeps running and billing, and because its +// NodeClaim is gone nothing ever reaps it. +package garbagecollection + +import ( + "context" + "fmt" + "time" + + "github.com/awslabs/operatorpkg/reconciler" + "github.com/awslabs/operatorpkg/singleton" + "github.com/hetznercloud/hcloud-go/v2/hcloud" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/tools/events" + "k8s.io/utils/clock" + controllerruntime "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/manager" + karpv1 "sigs.k8s.io/karpenter/pkg/apis/v1" + karpcp "sigs.k8s.io/karpenter/pkg/cloudprovider" + "sigs.k8s.io/karpenter/pkg/operator/injection" + nodeutils "sigs.k8s.io/karpenter/pkg/utils/node" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" + "github.com/paperclipinc/karpenter-provider-hetzner/pkg/metrics" + "github.com/paperclipinc/karpenter-provider-hetzner/pkg/providers/instance" +) + +const ( + // requiredUnownedSweeps is how many consecutive sweeps a server must be seen + // with no NodeClaim before it is reaped. Grace is counted in observations + // rather than elapsed time so it measures how long the server has been an + // orphan, not how old the machine is, and so it depends on no clock at all. + // The counter resets the moment an owner reappears, so a transient gap in the + // NodeClaim list can never accumulate to a deletion. + requiredUnownedSweeps = 3 + + // resyncInterval is how often the orphan sweep runs. + resyncInterval = 2 * time.Minute +) + +// InstanceProvider is the narrow instance API this controller needs. +type InstanceProvider interface { + List(ctx context.Context) ([]*hcloud.Server, error) + Delete(ctx context.Context, providerID string) error +} + +// Controller sweeps orphaned servers and the Node objects left behind with them. +type Controller struct { + kubeClient client.Client + instances InstanceProvider + clusterName string + clusterUID string + clock clock.Clock + + // mode selects whether the sweep reclaims or only reports. + mode Mode + + // startedAt is when this process's sweeps began. Grace is counted in + // consecutive observations, which a restart or a leader handover resets to + // zero -- and the instability that strands servers is exactly what causes + // those. Without a floor, a fresh process could reap on its third sweep + // having seen the cluster for six minutes; with one, it must have been + // watching for a full grace window first. + // + // This is the piece that makes in-process counting safe rather than merely + // simple. It converts operator instability into DELAYED reaping instead of + // either never reaping (no floor, counters forever reset) or reaping on + // stale evidence (a durable marker written by a process that is gone). + startedAt time.Time + + // recorder publishes reclamations onto the Node. Nil outside the manager. + recorder events.EventRecorder + + // unownedSweeps counts, per provider ID, how many consecutive sweeps have + // seen the server without an owner. Entries vanish as soon as a server is + // owned again or stops being returned by List, so the map cannot grow beyond + // the current fleet. Only Reconcile touches it, and the controller is a + // singleton, so it needs no lock. + unownedSweeps map[string]int + + // reportedForeignUIDs remembers which colliding clusters have already been + // reported, so a shared CLUSTER_NAME is logged once rather than every sweep. + reportedForeignUIDs map[string]bool +} + +// Mode selects how the sweep behaves. It is the config's own type rather than a +// bool so that adding a mode cannot silently fall through to the deleting +// branch: the controller decides what each mode means, in one place. +type Mode string + +const ( + // ModeEnabled reclaims orphaned servers. + ModeEnabled Mode = "enabled" + // ModeObserve runs every check and reports what it would reclaim, deleting + // nothing and writing nothing outside the cluster. + ModeObserve Mode = "observe" +) + +func NewController( + kubeClient client.Client, + instances InstanceProvider, + clusterName, clusterUID string, + mode Mode, + clk clock.Clock, +) *Controller { + return &Controller{ + kubeClient: kubeClient, + instances: instances, + clusterName: clusterName, + clusterUID: clusterUID, + mode: mode, + clock: clk, + startedAt: clk.Now(), + unownedSweeps: map[string]int{}, + reportedForeignUIDs: map[string]bool{}, + } +} + +// watchedLongEnough reports whether this process has been sweeping for a full +// grace window. Until it has, its observation counts describe too short a +// history to justify deleting anything. +func (c *Controller) watchedLongEnough() bool { + return c.clock.Since(c.startedAt) >= requiredUnownedSweeps*resyncInterval +} + +// managedByThisCluster reports whether this installation may act on a server, +// reporting the one kind of refusal an operator needs to hear about. +// +// The ownership rule itself lives in apiv1.OwnedByCluster, shared with the +// adoption path so the two destructive callers cannot drift apart. +func (c *Controller) managedByThisCluster(ctx context.Context, s *hcloud.Server) bool { + if apiv1.OwnedByCluster(s.Labels, c.clusterName, c.clusterUID) { + return true + } + // A server that carries our management labels and our cluster name but a + // different UID was refused on the UID alone -- the one refusal that means + // something is misconfigured rather than simply not ours. + if s.Labels[apiv1.ServerLabelManagedBy] == apiv1.ServerValueManagedBy && + s.Labels[apiv1.ServerLabelCluster] == c.clusterName { + c.reportForeignCluster(ctx, s.Labels[apiv1.ServerLabelClusterUID]) + } + return false +} + +// reportForeignCluster records a server refused because its cluster UID is not +// ours. +// +// Declining is the safe half; saying so is the useful half. Left silent, this is +// indistinguishable from a cluster that simply has no orphans. The metric fires +// every sweep because it is the only alertable evidence -- the log deliberately +// does not repeat, so after a pod restart it is the sole remaining signal. The +// log fires once per foreign cluster, where it stays readable. +func (c *Controller) reportForeignCluster(ctx context.Context, uid string) { + metrics.RecordOrphanGC(metrics.OrphanSkippedForeignCluster) + if c.reportedForeignUIDs[uid] { + return + } + c.reportedForeignUIDs[uid] = true + logf.FromContext(ctx).Info( + "refusing servers labelled for this cluster's name but stamped with a different cluster UID; "+ + "either two clusters share a CLUSTER_NAME in one Hetzner project, or this cluster's "+ + "control plane was rebuilt and these servers predate it", + "clusterName", c.clusterName, "ourClusterUID", c.clusterUID, "theirClusterUID", uid) +} + +func (c *Controller) Name() string { return "instance.garbagecollection" } + +// claims indexes the NodeClaims Karpenter currently has by both of the signals a +// server carries: the provider ID Karpenter writes to the NodeClaim status, and +// the NodeClaim name the provider stamps on the server at create time. The label +// is set before the server exists, so it survives a crash mid-launch — the +// provider ID does not, because it is written only after Create returns. +type claims struct { + providerIDs sets.Set[string] + names sets.Set[string] +} + +// owns reports whether Karpenter still has a NodeClaim for this server. +func (cl claims) owns(s *hcloud.Server) bool { + if cl.providerIDs.Has(instance.FormatProviderID(s.ID)) { + return true + } + name := s.Labels[apiv1.ServerLabelNodeClaim] + return name != "" && cl.names.Has(name) +} + +func (c *Controller) Reconcile(ctx context.Context) (reconciler.Result, error) { + ctx = injection.WithControllerName(ctx, c.Name()) + log := logf.FromContext(ctx) + + // The provider scopes this by the managed-by and cluster labels, but every + // server is re-checked below rather than trusting that. + servers, err := c.instances.List(ctx) + if err != nil { + log.Error(err, "listing servers for the orphan sweep") + metrics.RecordOrphanGC(metrics.OrphanSweepFailed) + return reconciler.Result{RequeueAfter: resyncInterval}, nil + } + if len(servers) == 0 { + c.unownedSweeps = map[string]int{} + return reconciler.Result{RequeueAfter: resyncInterval}, nil + } + + claimed, err := c.currentClaims(ctx) + if err != nil { + // Treat an unreadable NodeClaim list as "everything is owned": losing sight + // of the claims must never be the reason a machine is destroyed. Leaving the + // counters untouched means nothing advances toward deletion this sweep. + log.Error(err, "listing nodeclaims for the orphan sweep") + metrics.RecordOrphanGC(metrics.OrphanSweepFailed) + return reconciler.Result{RequeueAfter: resyncInterval}, nil + } + + // Nodes are only needed once an orphan candidate turns up, which is rare, so + // the index is built at most once per sweep and only on demand. + var idx *nodeIndex + nodeIndexOnce := func() (*nodeIndex, error) { + if idx == nil { + loaded, err := c.buildNodeIndex(ctx) + if err != nil { + return nil, err + } + idx = &loaded + } + return idx, nil + } + + // unowned records which servers were seen without an owner on THIS sweep. + // Replacing the previous map at the end both advances the counters and drops + // servers that regained an owner or no longer exist. + unowned := make(map[string]int, len(c.unownedSweeps)) + + for _, s := range servers { + providerID := instance.FormatProviderID(s.ID) + + // Re-check ownership labels here rather than relying on the caller's list + // filter. The scoping that keeps this sweep inside its own cluster lives in + // the provider's label selector, which this package cannot see and its + // tests do not exercise; a widened List would silently turn a per-cluster + // sweep into a project-wide deleter. + if !c.managedByThisCluster(ctx, s) { + continue + } + if claimed.owns(s) { + continue + } + + nodes, err := nodeIndexOnce() + if err != nil { + // Keep the cadence: dropping RequeueAfter here ratchets controller-runtime + // backoff toward its cap and delays every orphan in the fleet. Leave the + // counters untouched as the NodeClaim path above does -- `unowned` holds + // only the servers visited so far, so committing it here would reset the + // grace of every server behind this one. + log.Error(err, "listing nodes for the orphan sweep") + metrics.RecordOrphanGC(metrics.OrphanSweepFailed) + return reconciler.Result{RequeueAfter: resyncInterval}, nil + } + node, ambiguous := nodes.find(providerID, s.Name) + if ambiguous { + // Two Nodes claiming one provider ID is an ambiguous state Karpenter core + // refuses to resolve by guessing, and neither should a sweep that deletes + // machines. + log.Info("skipping server whose provider ID maps to multiple nodes", + "name", s.Name, "id", s.ID) + metrics.RecordOrphanGC(metrics.OrphanSkippedAmbiguous) + continue + } + // A registered kubelet still reporting Ready means the machine is alive and + // carrying workloads, whatever the NodeClaims say. Karpenter core applies + // the same check before acting on cloud-provider truth; without it a lost + // NodeClaim (a CRD reinstall, an etcd restore, a forced finalizer removal) + // turns this sweep into a fleet-wide termination with no eviction, no drain + // and no volume detachment. + // + // Ready alone is the wrong signal though. A server that booted and joined + // but never finished registering carries no karpenter.sh/registered label, + // and those are exactly the orphans this package exists to reap. Sparing + // them because the kubelet answers would leave the leak in place. + if node != nil && + nodeutils.GetCondition(node, corev1.NodeReady).Status == corev1.ConditionTrue && + isRegistered(node) { + log.Info("skipping orphaned server whose registered node is still Ready", + "name", s.Name, "id", s.ID, "node", node.Name) + metrics.RecordOrphanGC(metrics.OrphanSkippedReady) + continue + } + + // Only a sweep that got this far counts toward grace: the server is + // unowned AND nothing above objected to reclaiming it. + // + // Counting earlier would be wrong in a way that is easy to miss. A machine + // spared by the guards above would still advance -- and, once it reached the + // threshold, sit there fully armed -- so the first sweep that happened to + // catch its kubelet briefly NotReady would destroy it outright, with no + // drain and no volume detachment. Dropping the entry when a guard fires is + // not enough on its own either: the count simply cycles back up, leaving the + // machine armed a predictable fraction of the time. + // + // Counting only unguarded sweeps means a machine that stops being spared + // must earn a complete fresh window before anything happens to it. + sweeps := c.unownedSweeps[providerID] + 1 + unowned[providerID] = sweeps + if sweeps < requiredUnownedSweeps { + continue + } + + // Counters reset when this process started, so a fresh leader could reach + // the threshold having watched the cluster for only a few minutes. Require + // that it has been sweeping for a full window first: after a restart or a + // handover the reap is delayed, never skipped and never premature. + if !c.watchedLongEnough() { + continue + } + + // Observe mode stops here, before anything is deleted and before anything + // outside the cluster is written. That boundary is the point: an operator + // evaluating this on an unaudited fleet is told it changes nothing, and + // nothing is what it must change -- including labels on their servers. + if c.mode == ModeObserve { + log.Info("would garbage collect orphaned server (observe mode, nothing deleted)", + "name", s.Name, "id", s.ID, "providerID", providerID) + metrics.RecordOrphanGC(metrics.OrphanWouldReap) + c.recordOnNode(node, corev1.EventTypeNormal, "WouldGarbageCollect", + "Hetzner server %s (%s) has no NodeClaim and would be reclaimed; "+ + "instance garbage collection is in observe mode, so nothing was deleted", + s.Name, providerID) + continue + } + + // A NodeClaimNotFoundError means the server is already gone, which is the + // outcome we wanted; fall through and clean up its Node object. + if err := karpcp.IgnoreNodeClaimNotFoundError(c.instances.Delete(ctx, providerID)); err != nil { + // One undeletable server (delete protection, a locked server) must not + // suppress the sweep cadence. Returning an error drops the RequeueAfter + // and ratchets controller-runtime's backoff toward its cap, delaying every + // other orphan behind the one that cannot be reaped. The metric is what + // makes it alertable -- otherwise a permanently undeletable server bills + // forever behind a log line nobody reads. + // + // The count is carried forward here, unlike the guards above: this + // server did look reapable, so the next sweep should retry rather than + // re-earn the whole grace window. + unowned[providerID] = sweeps + log.Error(err, "deleting orphaned server", "name", s.Name, "id", s.ID, "providerID", providerID) + metrics.RecordOrphanGC(metrics.OrphanError) + continue + } + // Dropping the entry here matters because Hetzner deletes asynchronously: + // the server keeps being listed while the delete runs, and a still-armed + // counter would delete it again on the next sweep, double-counting the + // reap or reporting a spurious error for a machine already reclaimed. + delete(unowned, providerID) + log.Info("garbage collected orphaned server", + "name", s.Name, "id", s.ID, "providerID", providerID, "unownedSweeps", sweeps) + metrics.RecordOrphanGC(metrics.OrphanReaped) + // Normal, not Warning: reclaiming an orphan is this controller working, not + // a fault. A cluster alerting on Warning events against Nodes would page + // every time the sweep does its job. + c.recordOnNode(node, corev1.EventTypeNormal, "GarbageCollected", + "Hetzner server %s (%s) was reclaimed after %d consecutive sweeps with no NodeClaim; "+ + "this Node is being removed with it", + s.Name, providerID, sweeps) + + if node != nil { + // The index was read before a run of blocking hcloud calls, so this Node + // may have been replaced by a same-named one since. Deleting by UID makes + // that a no-op instead of destroying the replacement. + if err := client.IgnoreNotFound(c.kubeClient.Delete(ctx, node, + client.Preconditions{UID: &node.UID})); err != nil { + log.Error(err, "deleting the node of a garbage collected server", "node", node.Name) + } + } + } + c.unownedSweeps = unowned + return reconciler.Result{RequeueAfter: resyncInterval}, nil +} + +// isRegistered reports whether Karpenter finished registering this node. +// Karpenter stamps karpenter.sh/registered=true as the last step of registration +// and treats the label as the definition of registered itself +// (state/statenode.go), so its absence means the node never completed the +// handshake no matter what its kubelet reports. +// +// The unregistered taint is deliberately not used for this. It reaches a node +// only when the NodePool declares it as a startupTaint, so a cluster that does +// not configure it would have every orphan look registered and nothing would ever +// be reaped. Karpenter applies this label on its own. +func isRegistered(node *corev1.Node) bool { + return node.Labels[karpv1.NodeRegisteredLabelKey] == "true" +} + +// currentClaims returns the NodeClaims Karpenter still has, indexed by provider +// ID and by name. A NodeClaim in any state counts as ownership: only servers +// with no NodeClaim at all are orphans. +func (c *Controller) currentClaims(ctx context.Context) (claims, error) { + list := &karpv1.NodeClaimList{} + if err := c.kubeClient.List(ctx, list); err != nil { + return claims{}, fmt.Errorf("listing nodeclaims: %w", err) + } + cl := claims{providerIDs: sets.New[string](), names: sets.New[string]()} + for i := range list.Items { + if id := list.Items[i].Status.ProviderID; id != "" { + cl.providerIDs.Insert(id) + } + cl.names.Insert(list.Items[i].Name) + } + return cl, nil +} + +// nodeIndex locates the Node backing a server. Provider ID is the reliable key, +// but the hcloud CCM stamps it only after the kubelet has already registered the +// Node, so during that window -- or whenever the CCM is down -- a live node has +// none. Indexing by name as well closes that gap: this provider names every +// server after its NodeClaim, and the node inherits that name as its hostname. +type nodeIndex struct { + byProviderID map[string][]*corev1.Node + byName map[string]*corev1.Node +} + +// find returns the Node for a server and whether the match was ambiguous. +func (idx nodeIndex) find(providerID, serverName string) (*corev1.Node, bool) { + if matching := idx.byProviderID[providerID]; len(matching) > 0 { + if len(matching) > 1 { + return nil, true + } + return matching[0], false + } + // Fall back to the name only for nodes carrying no provider ID at all. A node + // stamped with a different provider ID belongs to a different server. + if node, ok := idx.byName[serverName]; ok && node.Spec.ProviderID == "" { + return node, false + } + return nil, false +} + +// buildNodeIndex lists Nodes once and indexes them by provider ID and by name. +// Provider-ID collisions are kept rather than collapsed so the caller can refuse +// to act on an ambiguous provider ID. +func (c *Controller) buildNodeIndex(ctx context.Context) (nodeIndex, error) { + list := &corev1.NodeList{} + if err := c.kubeClient.List(ctx, list); err != nil { + return nodeIndex{}, fmt.Errorf("listing nodes: %w", err) + } + idx := nodeIndex{ + byProviderID: make(map[string][]*corev1.Node, len(list.Items)), + byName: make(map[string]*corev1.Node, len(list.Items)), + } + for i := range list.Items { + node := &list.Items[i] + idx.byName[node.Name] = node + if id := node.Spec.ProviderID; id != "" { + idx.byProviderID[id] = append(idx.byProviderID[id], node) + } + } + return idx, nil +} + +// recordOnNode publishes an event against the Node a server backed. +// +// Most orphans have no Node at all -- the crash-window case this package exists +// for never registered one -- so this is a best-effort supplement to the log and +// the metric, not the primary signal. The event also outlives the Node it hangs +// off only for the cluster's event TTL, and `kubectl describe node` cannot show +// it once the Node is deleted; `kubectl get events` can. +func (c *Controller) recordOnNode(node *corev1.Node, eventType, reason, note string, args ...any) { + if c.recorder == nil || node == nil { + return + } + c.recorder.Eventf(node, nil, eventType, reason, reason, note, args...) +} + +// Register wires the controller into the manager. +func (c *Controller) Register(_ context.Context, m manager.Manager) error { + c.recorder = m.GetEventRecorder(c.Name()) + return controllerruntime.NewControllerManagedBy(m). + Named(c.Name()). + WatchesRawSource(singleton.Source()). + Complete(singleton.AsReconciler(c)) +} diff --git a/pkg/controllers/instance/garbagecollection/controller_test.go b/pkg/controllers/instance/garbagecollection/controller_test.go new file mode 100644 index 0000000..a2893e4 --- /dev/null +++ b/pkg/controllers/instance/garbagecollection/controller_test.go @@ -0,0 +1,817 @@ +package garbagecollection + +import ( + "bytes" + "context" + "fmt" + "testing" + "time" + + "github.com/go-logr/zapr" + "github.com/hetznercloud/hcloud-go/v2/hcloud" + "github.com/prometheus/client_golang/prometheus" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + clocktesting "k8s.io/utils/clock/testing" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + logf "sigs.k8s.io/controller-runtime/pkg/log" + crmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" + karpv1 "sigs.k8s.io/karpenter/pkg/apis/v1" + karpcp "sigs.k8s.io/karpenter/pkg/cloudprovider" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" + "github.com/paperclipinc/karpenter-provider-hetzner/pkg/metrics" + "github.com/paperclipinc/karpenter-provider-hetzner/pkg/providers/instance" +) + +type fakeInstanceProvider struct { + servers []*hcloud.Server + deleted []string + listErr error + deleteErr error +} + +func (f *fakeInstanceProvider) List(_ context.Context) ([]*hcloud.Server, error) { + if f.listErr != nil { + return nil, f.listErr + } + return f.servers, nil +} + +func (f *fakeInstanceProvider) Delete(_ context.Context, providerID string) error { + if f.deleteErr != nil { + return f.deleteErr + } + f.deleted = append(f.deleted, providerID) + return nil +} + +const ( + testCluster = "test-cluster" + testClusterUID = "34f25cbf-c7b5-49d1-833b-103bff8a34ad" +) + +// server is a server this installation created: it carries the ownership labels +// the sweep re-checks before touching anything. It deliberately sets no Created +// timestamp -- the sweep never reads one, and a fixture that took an age would +// imply a young server is protected when nothing in the code protects it. +func server(id int64, name string) *hcloud.Server { + return &hcloud.Server{ + ID: id, Name: name, + Labels: map[string]string{ + apiv1.ServerLabelManagedBy: apiv1.ServerValueManagedBy, + apiv1.ServerLabelCluster: testCluster, + }, + } +} + +func newTestController(kubeClient client.Client, instances InstanceProvider) *Controller { + c := NewController(kubeClient, instances, testCluster, testClusterUID, ModeEnabled, clocktesting.NewFakeClock(time.Now())) + // Tests assert the observation counter, not the startup floor; treat this + // process as already having watched a full window. + c.startedAt = c.clock.Now().Add(-requiredUnownedSweeps * resyncInterval) + return c +} + +// sweep runs one reconcile and fails the test on error. +func sweep(t *testing.T, c *Controller) { + t.Helper() + if _, err := c.Reconcile(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// sweepPastGrace runs exactly enough sweeps for an orphan to become reapable. +func sweepPastGrace(t *testing.T, c *Controller) { + t.Helper() + for range requiredUnownedSweeps { + sweep(t, c) + } +} + +func nodeClaimFor(id int64, name string) *karpv1.NodeClaim { + return &karpv1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: karpv1.NodeClaimStatus{ProviderID: instance.FormatProviderID(id)}, + } +} + +func nodeFor(id int64, name string) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: corev1.NodeSpec{ProviderID: instance.FormatProviderID(id)}, + } +} + +// readyNodeFor is Ready but never completed registration -- no +// karpenter.sh/registered label. This is the shape of a server that booted and +// joined but whose NodeClaim was lost before Karpenter finished with it. +func readyNodeFor(id int64, name string) *corev1.Node { + n := nodeFor(id, name) + n.Status.Conditions = []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionTrue}} + return n +} + +// registeredReadyNodeFor is a fully registered, Ready node: Karpenter stamped +// karpenter.sh/registered=true on it, which is core's own definition of +// registered (state/statenode.go Registered()). +func registeredReadyNodeFor(id int64, name string) *corev1.Node { + n := readyNodeFor(id, name) + n.Labels = map[string]string{karpv1.NodeRegisteredLabelKey: "true"} + return n +} + +// newFakeClient registers NodeClaim's status subresource so the fake behaves like +// a real apiserver: status is stripped on CREATE and must be written through +// Status(). Without it a test could seed a provider ID in a single Create and +// prove a sequence karpenter never actually performs. +func newFakeClient(objs ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithStatusSubresource(&karpv1.NodeClaim{}). + WithObjects(objs...). + Build() +} + +// Observation counts reset when the process does, so a fresh leader could reach +// the threshold having watched the cluster for only a few minutes. The floor +// makes a restart delay the reap rather than authorise one on thin evidence -- +// the whole reason grace can safely stay in memory. +func TestReconcile_FreshProcessWaitsBeforeReaping(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + clk := clocktesting.NewFakeClock(time.Now()) + c := NewController(newFakeClient(), instances, testCluster, testClusterUID, ModeEnabled, clk) + + // Enough observations, but this process has only just started. + for range requiredUnownedSweeps + 2 { + sweep(t, c) + } + if len(instances.deleted) != 0 { + t.Fatalf("a just-started process reaped on its own short history: %v", instances.deleted) + } + + // Once it has been watching for a full window, the same evidence suffices. + clk.Step(requiredUnownedSweeps * resyncInterval) + sweep(t, c) + if len(instances.deleted) != 1 { + t.Errorf("failed to reap after watching a full window: %v", instances.deleted) + } +} + +// Observe mode must change nothing at all -- not the fleet, and not the servers' +// labels. An operator evaluating this on an unaudited fleet is told it is +// read-only, so read-only is what it has to be. +func TestReconcile_ObserveModeReportsAndChangesNothing(t *testing.T) { + srv := server(42, "worker-abc") + before := len(srv.Labels) + instances := &fakeInstanceProvider{servers: []*hcloud.Server{srv}} + kubeClient := newFakeClient(nodeFor(42, "worker-abc")) + + clk := clocktesting.NewFakeClock(time.Now()) + c := NewController(kubeClient, instances, testCluster, testClusterUID, ModeObserve, clk) + c.startedAt = clk.Now().Add(-requiredUnownedSweeps * resyncInterval) + + reported := orphanGCCount(t, metrics.OrphanWouldReap) + for range requiredUnownedSweeps + 1 { + sweep(t, c) + } + + if len(instances.deleted) != 0 { + t.Errorf("observe mode deleted servers: %v", instances.deleted) + } + if len(srv.Labels) != before { + t.Errorf("observe mode mutated server labels: %v", srv.Labels) + } + node := &corev1.Node{} + if err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, node); err != nil { + t.Errorf("observe mode deleted the Node object: %v", err) + } + if orphanGCCount(t, metrics.OrphanWouldReap) <= reported { + t.Error("observe mode reported nothing; the mode is only useful if it says what it would do") + } +} + +// Switching observe -> enabled must not reap on the first sweep. The counters +// are this process's, so a new controller starts from zero and re-earns the +// window; nothing observed during evaluation is banked against the fleet. +func TestReconcile_ObserveThenEnabledDoesNotReapInstantly(t *testing.T) { + srv := server(42, "worker-abc") + instances := &fakeInstanceProvider{servers: []*hcloud.Server{srv}} + kubeClient := newFakeClient() + + clk := clocktesting.NewFakeClock(time.Now()) + observe := NewController(kubeClient, instances, testCluster, testClusterUID, ModeObserve, clk) + observe.startedAt = clk.Now().Add(-72 * time.Hour) + for range 20 { + sweep(t, observe) + } + + // The operator flips the mode; the pod restarts with a new controller. + enabled := NewController(kubeClient, instances, testCluster, testClusterUID, ModeEnabled, clk) + sweep(t, enabled) + if len(instances.deleted) != 0 { + t.Errorf("switching observe->enabled reaped on the first sweep: %v", instances.deleted) + } +} + +// A server whose NodeClaim no longer exists is billing with no owner. Nothing in +// Karpenter core reaps it: core's garbage collector only deletes NodeClaims that +// have no instance, never the reverse. +func TestReconcile_DeletesOrphanedServerAndNode(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient(nodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 1 || instances.deleted[0] != instance.FormatProviderID(42) { + t.Errorf("expected server 42 deleted, got %v", instances.deleted) + } + node := &corev1.Node{} + err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, node) + if err == nil { + t.Error("expected the orphaned Node object to be deleted, it still exists") + } +} + +func TestReconcile_SparesServerWithNodeClaim(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient(nodeClaimFor(42, "worker-abc"), nodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("deleted a server that has a NodeClaim: %v", instances.deleted) + } + node := &corev1.Node{} + if err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, node); err != nil { + t.Errorf("deleted the Node of a live NodeClaim: %v", err) + } +} + +// A server whose provider ID has not reached the NodeClaim status yet looks +// unowned. Requiring several consecutive unowned observations keeps the sweep +// clear of nodes that are still being born. +func TestReconcile_SparesServerNotYetSeenUnownedEnough(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + c := newTestController(newFakeClient(), instances) + + for range requiredUnownedSweeps - 1 { + sweep(t, c) + if len(instances.deleted) != 0 { + t.Fatalf("deleted before the orphan was observed enough times: %v", instances.deleted) + } + } +} + +// The counter must measure how long the server has been an orphan, not how long +// the process has been running. An owner reappearing resets it, so a single bad +// snapshot of the NodeClaim list can never accumulate into a deletion. +func TestReconcile_OwnerReappearingResetsTheCounter(t *testing.T) { + srv := server(42, "worker-abc") + instances := &fakeInstanceProvider{servers: []*hcloud.Server{srv}} + kubeClient := newFakeClient() + c := newTestController(kubeClient, instances) + + // One sweep short of reapable. + for range requiredUnownedSweeps - 1 { + sweep(t, c) + } + // The NodeClaim comes back (an informer relist, an etcd blip resolving). Write + // it in the two steps karpenter's launch controller actually performs: a real + // apiserver strips status on CREATE for a resource with a status subresource, + // so seeding the provider ID through Create would only work against the fake. + nc := &karpv1.NodeClaim{ObjectMeta: metav1.ObjectMeta{Name: "worker-abc"}} + if err := kubeClient.Create(context.Background(), nc); err != nil { + t.Fatalf("seeding nodeclaim: %v", err) + } + nc.Status.ProviderID = instance.FormatProviderID(42) + if err := kubeClient.Status().Update(context.Background(), nc); err != nil { + t.Fatalf("writing the nodeclaim provider ID: %v", err) + } + sweep(t, c) + // It disappears again; the count must start over rather than resume. + if err := kubeClient.Delete(context.Background(), nodeClaimFor(42, "worker-abc")); err != nil { + t.Fatalf("removing nodeclaim: %v", err) + } + sweep(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("counter survived the owner reappearing: %v", instances.deleted) + } +} + +func TestReconcile_OrphanWithoutNodeObject(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient() + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 1 { + t.Errorf("expected the server deleted even with no Node object, got %v", instances.deleted) + } +} + +// A server that is not this installation's must never be touched, whatever the +// caller's List returned. The label selector that normally scopes List lives in +// another package and no test here exercises it, so the sweep re-checks. +func TestReconcile_IgnoresServerFromAnotherCluster(t *testing.T) { + foreign := server(42, "worker-abc") + foreign.Labels[apiv1.ServerLabelCluster] = "someone-elses-cluster" + instances := &fakeInstanceProvider{servers: []*hcloud.Server{foreign}} + + c := newTestController(newFakeClient(), instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("deleted another cluster's server: %v", instances.deleted) + } +} + +// CLUSTER_NAME is operator-supplied with no uniqueness guarantee. Two clusters +// sharing one in a single Hetzner project would otherwise each see the other's +// servers as unclaimed and delete them. The cluster UID settles it. +func TestReconcile_IgnoresServerOfAnotherClusterWithTheSameName(t *testing.T) { + foreign := server(42, "worker-abc") + foreign.Labels[apiv1.ServerLabelClusterUID] = "6e5f8dfb-e54b-41ee-8fb3-89a48a42231f" + instances := &fakeInstanceProvider{servers: []*hcloud.Server{foreign}} + + c := newTestController(newFakeClient(), instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("deleted a same-named other cluster's server: %v", instances.deleted) + } +} + +// Silently declining to touch the other cluster's servers is safe but useless: +// a name collision would look exactly like a cluster with no orphans. +// +// The log says so once per colliding cluster -- once per sweep would be noise +// nobody reads. That is also why it cannot be the only signal: after a pod +// restart the line has already scrolled away, so the metric has to fire every +// sweep. Assert the log and the counter, not the bookkeeping map, or removing +// either report would leave every test green. +func TestReconcile_ReportsAForeignClusterSharingOurName(t *testing.T) { + const foreignUID = "6e5f8dfb-e54b-41ee-8fb3-89a48a42231f" + const sweeps = 4 + foreign := server(42, "worker-abc") + foreign.Labels[apiv1.ServerLabelClusterUID] = foreignUID + instances := &fakeInstanceProvider{servers: []*hcloud.Server{foreign}} + + var buf bytes.Buffer + core := zapcore.NewCore( + zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig()), + zapcore.AddSync(&buf), zapcore.DebugLevel) + ctx := logf.IntoContext(context.Background(), zapr.NewLogger(zap.New(core))) + + c := newTestController(newFakeClient(), instances) + before := skippedForeignCount(t) + for range sweeps { + if _, err := c.Reconcile(ctx); err != nil { + t.Fatalf("unexpected error: %v", err) + } + } + + if got := bytes.Count(buf.Bytes(), []byte(foreignUID)); got != 1 { + t.Errorf("logged the colliding cluster %d times over %d sweeps, want 1; got %s", + got, sweeps, buf.String()) + } + if got := skippedForeignCount(t) - before; got != sweeps { + t.Errorf("counter rose by %v over %d sweeps, want one per sweep", got, sweeps) + } +} + +// skippedForeignCount reads the running total of servers refused for carrying +// another cluster's UID. +func skippedForeignCount(t *testing.T) float64 { + t.Helper() + return orphanGCCount(t, metrics.OrphanSkippedForeignCluster) +} + +// orphanGCCount reads the running total of one sweep outcome. +func orphanGCCount(t *testing.T, result string) float64 { + t.Helper() + mfs, err := crmetrics.Registry.(prometheus.Gatherer).Gather() + if err != nil { + t.Fatalf("gather: %v", err) + } + for _, mf := range mfs { + if mf.GetName() != "karpenter_hetzner_orphaned_server_gc_total" { + continue + } + for _, m := range mf.GetMetric() { + for _, l := range m.GetLabel() { + if l.GetName() == "result" && l.GetValue() == result { + return m.GetCounter().GetValue() + } + } + } + } + return 0 +} + +// A server carrying our own UID is ours, name collision or not. +func TestReconcile_ReapsServerWithMatchingClusterUID(t *testing.T) { + own := server(42, "worker-abc") + own.Labels[apiv1.ServerLabelClusterUID] = testClusterUID + instances := &fakeInstanceProvider{servers: []*hcloud.Server{own}} + + c := newTestController(newFakeClient(), instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 1 { + t.Errorf("failed to reap our own orphan: %v", instances.deleted) + } +} + +// Servers created before this label existed carry no UID. Treating them as +// foreign would strand every pre-existing orphan permanently. +func TestReconcile_ReapsLegacyServerWithoutClusterUID(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + + c := newTestController(newFakeClient(), instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 1 { + t.Errorf("failed to reap a legacy orphan with no cluster UID: %v", instances.deleted) + } +} + +func TestReconcile_IgnoresServerNotManagedByKarpenter(t *testing.T) { + unmanaged := server(42, "worker-abc") + delete(unmanaged.Labels, apiv1.ServerLabelManagedBy) + instances := &fakeInstanceProvider{servers: []*hcloud.Server{unmanaged}} + + c := newTestController(newFakeClient(), instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("deleted a server this provider does not manage: %v", instances.deleted) + } +} + +func TestReconcile_EmptyListIsNoOp(t *testing.T) { + instances := &fakeInstanceProvider{} + c := newTestController(newFakeClient(), instances) + if _, err := c.Reconcile(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(instances.deleted) != 0 { + t.Errorf("deleted something from an empty list: %v", instances.deleted) + } +} + +// If the server could not be terminated it is still running, so its Node object +// must stay too. Removing the Node while the server lives would hide a billing +// instance from the very sweep meant to catch it. +// +// A server that can never be deleted (Hetzner delete protection, a locked +// server) also must not poison the sweep: returning an error drops RequeueAfter +// and ratchets controller-runtime's backoff toward its cap, so one stuck server +// would delay reaping every other orphan. +func TestReconcile_DeleteFailureKeepsNodeAndCadence(t *testing.T) { + instances := &fakeInstanceProvider{ + servers: []*hcloud.Server{server(42, "worker-abc")}, + deleteErr: fmt.Errorf("api unavailable"), + } + kubeClient := newFakeClient(nodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + for range requiredUnownedSweeps - 1 { + sweep(t, c) + } + res, err := c.Reconcile(context.Background()) + if err != nil { + t.Fatalf("a single undeletable server must not fail the sweep: %v", err) + } + if res.RequeueAfter != resyncInterval { + t.Errorf("expected the sweep to keep its cadence, got RequeueAfter %v", res.RequeueAfter) + } + + node := &corev1.Node{} + if err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, node); err != nil { + t.Errorf("removed the Node object though the server delete failed: %v", err) + } +} + +// The provider ID is written to the NodeClaim only after Create returns, and +// Create blocks waiting on Hetzner's create actions. The nodeclaim label is +// stamped on the server before any of that, so it is the ownership signal that +// survives a crash — or a slow launch — mid-create. +func TestReconcile_SparesServerLabelledForALiveNodeClaim(t *testing.T) { + s := server(42, "worker-abc") + // Assign into the map rather than replacing it: dropping managed-by and + // cluster would make the sweep skip this server on the ownership re-check, + // and the name-based branch under test would never be reached. + s.Labels[apiv1.ServerLabelNodeClaim] = "worker-abc" + instances := &fakeInstanceProvider{servers: []*hcloud.Server{s}} + // The NodeClaim exists but its status has no provider ID yet. + kubeClient := newFakeClient(&karpv1.NodeClaim{ObjectMeta: metav1.ObjectMeta{Name: "worker-abc"}}) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("deleted a server whose NodeClaim is still alive: %v", instances.deleted) + } +} + +// A kubelet still reporting Ready means the machine is alive and running +// workloads, whatever the NodeClaims say. Karpenter core refuses to act on +// cloud-provider truth in this state, and so must this sweep: a lost NodeClaim +// (CRD reinstall, etcd restore, forced finalizer removal) would otherwise become +// a fleet-wide termination with no eviction and no drain. +func TestReconcile_SparesOrphanWithRegisteredReadyNode(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient(registeredReadyNodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("destroyed a server whose node is still Ready: %v", instances.deleted) + } + node := &corev1.Node{} + if err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, node); err != nil { + t.Errorf("deleted the Node of a Ready machine: %v", err) + } +} + +// The Ready check must not key on Ready alone. A server that booted and joined +// but never completed registration carries no karpenter.sh/registered label, and +// those are precisely the orphans this package exists to reap -- sparing them +// because the kubelet answers would leave them billing forever. +// +// Registration, not the unregistered taint, is the signal: that taint reaches a +// node only if the NodePool declares it as a startupTaint, so a cluster that does +// not would never reap anything. Karpenter applies the registered label itself. +func TestReconcile_ReapsReadyButUnregisteredNode(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient(readyNodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 1 { + t.Errorf("failed to reap an unregistered orphan whose kubelet still reports Ready: %v", instances.deleted) + } +} + +// A node the CCM has not stamped with a provider ID yet cannot be matched by +// provider ID, and failing to find it must not silently skip the safety guard. +// The server name equals the NodeClaim name equals the node name, so fall back to +// that rather than treating the node as absent. +func TestReconcile_SparesRegisteredReadyNodeWithoutProviderID(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + node := registeredReadyNodeFor(42, "worker-abc") + node.Spec.ProviderID = "" // hcloud CCM has not filled it in yet + kubeClient := newFakeClient(node) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("destroyed a live node the CCM had not yet stamped: %v", instances.deleted) + } +} + +// Two Nodes claiming one provider ID is an ambiguous state Karpenter core +// refuses to resolve by guessing. A sweep that destroys machines must not pick +// one arbitrarily by list order. +func TestReconcile_SparesServerWithAmbiguousNodes(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient(nodeFor(42, "worker-abc"), nodeFor(42, "worker-abc-stale")) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + if len(instances.deleted) != 0 { + t.Errorf("acted on an ambiguous provider ID: %v", instances.deleted) + } +} + +// A failed list must not become a returned error: operatorpkg discards +// RequeueAfter on error and lets controller-runtime's rate limiter ratchet the +// interval toward its cap, so a spell of API failures would slow the sweep for +// every orphan in the fleet. Log it and keep the cadence. +func TestReconcile_ListErrorKeepsCadence(t *testing.T) { + instances := &fakeInstanceProvider{listErr: fmt.Errorf("api unavailable")} + c := newTestController(newFakeClient(), instances) + + res, err := c.Reconcile(context.Background()) + if err != nil { + t.Fatalf("a list failure must not fail the sweep: %v", err) + } + if res.RequeueAfter != resyncInterval { + t.Errorf("expected the sweep to keep its cadence, got RequeueAfter %v", res.RequeueAfter) + } + if len(instances.deleted) != 0 { + t.Errorf("deleted something despite failing to list: %v", instances.deleted) + } +} + +// Losing sight of the NodeClaims must never be the reason a machine dies: an +// unreadable claim list makes every server look unowned. +func TestReconcile_NodeClaimListFailureDeletesNothing(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + c := newTestController(&failingClaimClient{Client: newFakeClient()}, instances) + + for range requiredUnownedSweeps + 2 { + res, err := c.Reconcile(context.Background()) + if err != nil { + t.Fatalf("a nodeclaim list failure must not fail the sweep: %v", err) + } + if res.RequeueAfter != resyncInterval { + t.Errorf("expected the sweep to keep its cadence, got %v", res.RequeueAfter) + } + } + if len(instances.deleted) != 0 { + t.Errorf("destroyed machines while unable to read NodeClaims: %v", instances.deleted) + } +} + +// failingClaimClient fails NodeClaim listing and passes everything else through. +type failingClaimClient struct { + client.Client +} + +func (f *failingClaimClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*karpv1.NodeClaimList); ok { + return fmt.Errorf("apiserver unavailable") + } + return f.Client.List(ctx, list, opts...) +} + +// failingNodeClient fails Node listing and passes everything else through. +type failingNodeClient struct { + client.Client +} + +func (f *failingNodeClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*corev1.NodeList); ok { + return fmt.Errorf("apiserver unavailable") + } + return f.Client.List(ctx, list, opts...) +} + +// Sparing a server must restart its grace, not merely postpone the deletion by +// one sweep. Otherwise a NodeClaim wipe (a CRD reinstall, an etcd restore) leaves +// every live machine sitting at a spent counter, and the first sweep that catches +// one with a briefly NotReady kubelet -- a reboot, a kubelet upgrade, a network +// blip -- destroys it with no drain and no volume detachment. +// The number of sweeps spent being spared is varied deliberately. If sparing +// merely drops the counter rather than never advancing it, the count cycles +// 1, 2, 3, reset, so whether the machine survives depends on where in that cycle +// the kubelet blips -- a test with a single fixed iteration count passes or fails +// by luck. +func TestReconcile_SparingAServerRestartsItsGrace(t *testing.T) { + for _, sparedSweeps := range []int{1, 2, 3, 4, 5, 7, 11, 200} { + t.Run(fmt.Sprintf("spared_%d", sparedSweeps), func(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + kubeClient := newFakeClient(registeredReadyNodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + for range sparedSweeps { + sweep(t, c) + } + if len(instances.deleted) != 0 { + t.Fatalf("destroyed a registered, Ready machine: %v", instances.deleted) + } + + // The kubelet stops reporting for a moment. + live := &corev1.Node{} + if err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, live); err != nil { + t.Fatalf("reading the node: %v", err) + } + live.Status.Conditions = []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionUnknown}} + if err := kubeClient.Status().Update(context.Background(), live); err != nil { + t.Fatalf("updating the node status: %v", err) + } + + sweep(t, c) + if len(instances.deleted) != 0 { + t.Errorf("one NotReady sweep destroyed a machine spared for %d sweeps: %v", + sparedSweeps, instances.deleted) + } + }) + } +} + +// A machine that stays unreachable is still an orphan. Once sparing stops, a +// full fresh grace window must elapse -- no more, no less. +func TestReconcile_ReapsAfterAFullFreshWindowOfNotReady(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + node := registeredReadyNodeFor(42, "worker-abc") + node.Status.Conditions = []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionUnknown}} + kubeClient := newFakeClient(node) + + c := newTestController(kubeClient, instances) + for range requiredUnownedSweeps - 1 { + sweep(t, c) + } + if len(instances.deleted) != 0 { + t.Fatalf("reaped before a full window elapsed: %v", instances.deleted) + } + sweep(t, c) + if len(instances.deleted) != 1 { + t.Errorf("failed to reap after a full window of NotReady: %v", instances.deleted) + } +} + +// Hetzner deletes asynchronously, so a reaped server keeps being listed while the +// delete runs. Re-deleting it double-counts the reap, or reports an error for a +// machine that was in fact reclaimed -- and result="error" is the signal the +// README tells operators to alert on. +func TestReconcile_DoesNotImmediatelyReReapADyingServer(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{server(42, "worker-abc")}} + + c := newTestController(newFakeClient(), instances) + sweepPastGrace(t, c) + if len(instances.deleted) != 1 { + t.Fatalf("expected exactly one delete, got %v", instances.deleted) + } + + // The server is still listed because Hetzner has not finished with it. + sweep(t, c) + if len(instances.deleted) != 1 { + t.Errorf("deleted a server already being deleted: %v", instances.deleted) + } +} + +// The grace counters of servers the sweep never reached must survive a mid-loop +// Node-list failure. Committing the partially built map would reset them, so a +// persistent Node-list failure plus one orphan early in hcloud's list order would +// keep every orphan behind it at zero forever -- the billing leak this package +// exists to close. +func TestReconcile_NodeListFailureKeepsCountersOfUnvisitedServers(t *testing.T) { + instances := &fakeInstanceProvider{servers: []*hcloud.Server{ + server(42, "worker-abc"), + server(43, "worker-def"), + }} + kubeClient := &failingNodeClient{Client: newFakeClient()} + + c := newTestController(kubeClient, instances) + // Both servers accumulate grace; the first to reach it trips the Node list. + for range requiredUnownedSweeps + 2 { + res, err := c.Reconcile(context.Background()) + if err != nil { + t.Fatalf("a node list failure must not fail the sweep: %v", err) + } + if res.RequeueAfter != resyncInterval { + t.Errorf("expected the sweep to keep its cadence, got %v", res.RequeueAfter) + } + } + if len(instances.deleted) != 0 { + t.Fatalf("deleted while unable to read Nodes: %v", instances.deleted) + } + // Nothing may advance toward deletion while the guards cannot be evaluated, + // and no server may be penalised for its position in the list. + first, second := instance.FormatProviderID(42), instance.FormatProviderID(43) + if c.unownedSweeps[first] != c.unownedSweeps[second] { + t.Errorf("servers treated asymmetrically by list position: counters=%v", c.unownedSweeps) + } + + // Once Nodes are readable again both progress together and are reaped in the + // same sweep -- the outage neither advanced nor permanently penalised them. + c.kubeClient = newFakeClient() + for range requiredUnownedSweeps - 1 { + sweep(t, c) + if len(instances.deleted) != 0 { + t.Fatalf("reaped before a full window after recovery: %v", instances.deleted) + } + } + sweep(t, c) + if len(instances.deleted) != 2 { + t.Errorf("expected both servers reaped after recovery, got %v", instances.deleted) + } +} + +// A NodeClaimNotFoundError from Delete means hcloud has already finished with the +// server -- the outcome the sweep wanted -- so it must fall through and clean up +// the Node object rather than treat it as a failure. Without the +// IgnoreNodeClaimNotFoundError wrapper every already-reclaimed server would report +// result="error", which the README tells operators means a machine cannot be +// reclaimed and is still billing, and would strand its Node. +func TestReconcile_AlreadyDeletedServerStillCleansUpItsNode(t *testing.T) { + instances := &fakeInstanceProvider{ + servers: []*hcloud.Server{server(42, "worker-abc")}, + deleteErr: karpcp.NewNodeClaimNotFoundError(fmt.Errorf("server 42 not found")), + } + kubeClient := newFakeClient(nodeFor(42, "worker-abc")) + + c := newTestController(kubeClient, instances) + sweepPastGrace(t, c) + + node := &corev1.Node{} + if err := kubeClient.Get(context.Background(), types.NamespacedName{Name: "worker-abc"}, node); err == nil { + t.Error("left behind the Node of a server hcloud had already deleted") + } +} diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 8dc657b..8933123 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -3,9 +3,8 @@ // // All metrics are registered once in an init() against controller-runtime's // shared Registry so they coexist safely with karpenter-core metrics. -// Callers import this package for its side-effects and then invoke the helper -// functions (RecordServerCreate, RecordServerDelete, RecordDrift, -// RecordCacheHit, RecordCacheMiss) to instrument hot paths. +// Callers import this package for its side-effects and then invoke the exported +// helper functions to instrument hot paths. package metrics import ( @@ -45,6 +44,27 @@ var ( Help: "Total number of Hetzner server delete calls by result.", }, []string{"result"}) + // orphanGCTotal counts orphaned-server sweep outcomes. "reaped" and "error" + // are terminal; the "skipped_*" results mark a server the sweep declined to + // act on, which would otherwise be visible only as a log line repeated every + // resync interval for as long as the server bills. + orphanGCTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: "karpenter_hetzner", + Name: "orphaned_server_gc_total", + Help: "Outcomes of the orphaned-server garbage collection sweep.", + }, []string{"result"}) + + // serverAdoptTotal counts attempts to recover a server by name after a create + // call whose result was lost. Adoptions return through Create, so without this + // they are indistinguishable from ordinary successful creates -- and the + // "declined" and "error" results matter just as much, since a NodeClaim + // retrying into a collision adoption keeps refusing is otherwise invisible. + serverAdoptTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: "karpenter_hetzner", + Name: "server_adopt_total", + Help: "Outcomes of adopting a pre-existing Hetzner server after a name collision.", + }, []string{"result"}) + // hcloudAPICallsTotal counts hcloud API calls by operation and result. We // scope it to the operations we actually instrument (server_create, // server_delete, placement_group, image_list) to keep label cardinality @@ -78,9 +98,63 @@ func init() { hcloudAPICallsTotal, driftDetectedTotal, instanceTypeCacheTotal, + orphanGCTotal, + serverAdoptTotal, ) } +// Orphan garbage-collection results. +const ( + OrphanReaped = "reaped" + OrphanError = "error" + OrphanSkippedAmbiguous = "skipped_ambiguous_node" + OrphanSkippedReady = "skipped_registered_ready" + + // OrphanSkippedForeignCluster marks a server carrying another cluster's UID + // under our CLUSTER_NAME. Declining is correct, but the decline is the only + // evidence that two clusters share a name in one Hetzner project -- and the + // log that reports it fires once per process, so after any restart the + // misconfiguration is invisible. This is what stays alertable. + OrphanSkippedForeignCluster = "skipped_foreign_cluster" + + // OrphanSweepFailed marks a sweep that could not run to completion. The sweep + // swallows list failures to protect its cadence, which also hides them from + // controller_runtime_reconcile_errors_total -- so without this a permanently + // broken sweep is indistinguishable from a cluster that simply has no orphans. + OrphanSweepFailed = "sweep_failed" + + // OrphanWouldReap marks a server the sweep would have reclaimed had it not + // been running in observe mode. It is what makes the mode useful: an operator + // can watch this climb, satisfy themselves it names the right machines, and + // only then switch to enabled. + OrphanWouldReap = "would_reap" +) + +// RecordOrphanGC records one orphaned-server sweep outcome. +func RecordOrphanGC(result string) { + orphanGCTotal.WithLabelValues(result).Inc() +} + +// Adoption outcomes. +const ( + AdoptAdopted = "adopted" + AdoptDeclined = "declined" + AdoptError = "error" + + // AdoptForeignCluster marks a collision with a server carrying another + // cluster's UID. It is separated from "declined" because the remedy differs: + // an ordinary decline resolves itself once the NodeClaim expires and the sweep + // reclaims the machine, whereas this server is not ours and will never be + // reclaimed -- waiting for the sweep is exactly the wrong response. + AdoptForeignCluster = "foreign_cluster" +) + +// RecordServerAdopt records the outcome of one attempt to recover a server by +// name after a create collided on it. +func RecordServerAdopt(result string) { + serverAdoptTotal.WithLabelValues(result).Inc() +} + // RecordServerCreate records a server create result and its duration. // Call this once per Create() return, passing "success" or "error" and // the wall-clock duration measured from the call's entry point. diff --git a/pkg/operator/clusteruid.go b/pkg/operator/clusteruid.go new file mode 100644 index 0000000..fd2d5fb --- /dev/null +++ b/pkg/operator/clusteruid.go @@ -0,0 +1,39 @@ +package operator + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// clusterUIDNamespace is the namespace whose UID identifies the cluster. +// kube-system is created once at cluster bootstrap and never recreated, so its +// UID is unique per cluster and stable for the cluster's lifetime. This is the +// conventional way to derive a cluster identity in Kubernetes, and needs no +// state of our own. +const clusterUIDNamespace = "kube-system" + +// ClusterUID returns a stable identifier for this cluster. +// +// It exists because CLUSTER_NAME is operator-supplied and nothing enforces +// uniqueness. Two clusters sharing a name in one Hetzner project each read the +// other's servers as their own, which was harmless while the label only scoped +// listings and is not now that unclaimed servers are deleted. +// +// Pass an uncached reader when calling before the manager's cache has started. +func ClusterUID(ctx context.Context, reader client.Reader) (string, error) { + ns := &corev1.Namespace{} + if err := reader.Get(ctx, types.NamespacedName{Name: clusterUIDNamespace}, ns); err != nil { + return "", fmt.Errorf("reading the %s namespace to derive the cluster UID: %w", clusterUIDNamespace, err) + } + uid := string(ns.UID) + if uid == "" { + // Refuse rather than fall back to the name alone: a silently empty UID + // would reinstate the collision this exists to prevent. + return "", fmt.Errorf("the %s namespace has no UID", clusterUIDNamespace) + } + return uid, nil +} diff --git a/pkg/operator/clusteruid_test.go b/pkg/operator/clusteruid_test.go new file mode 100644 index 0000000..5ad47ed --- /dev/null +++ b/pkg/operator/clusteruid_test.go @@ -0,0 +1,46 @@ +package operator + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestClusterUID_ReadsKubeSystemUID(t *testing.T) { + const want = "34f25cbf-c7b5-49d1-833b-103bff8a34ad" + c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(&corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "kube-system", UID: want}, + }).Build() + + got, err := ClusterUID(context.Background(), c) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +// Falling back to the name alone would quietly reinstate the cross-cluster +// deletion this identifier exists to prevent, so an unusable UID is an error. +func TestClusterUID_EmptyUIDIsAnError(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(&corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "kube-system"}, + }).Build() + + if _, err := ClusterUID(context.Background(), c); err == nil { + t.Error("expected an error when the namespace carries no UID") + } +} + +func TestClusterUID_MissingNamespaceIsAnError(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() + + if _, err := ClusterUID(context.Background(), c); err == nil { + t.Error("expected an error when kube-system cannot be read") + } +} diff --git a/pkg/operator/config.go b/pkg/operator/config.go index 3590fdf..c428498 100644 --- a/pkg/operator/config.go +++ b/pkg/operator/config.go @@ -14,8 +14,36 @@ type Config struct { // ClusterName scopes all managed servers so multiple clusters can share // one Hetzner project without colliding. ClusterName string + + // InstanceGarbageCollectionMode controls the sweep that reclaims servers + // Karpenter no longer has a NodeClaim for. + // + // "observe" exists because this deletes machines. An operator adopting it + // cannot otherwise find out what it would do to their fleet except by letting + // it do it, and a kill switch only helps once something has already gone + // wrong. In observe mode every check runs and every signal is emitted; only + // the deletion is skipped. + // + // "disabled" is for maintenance that removes NodeClaims wholesale -- + // reinstalling the CRDs, restoring etcd, clearing finalizers by hand -- where + // the sweep must pause without also stopping provisioning and disruption, as + // scaling the deployment to zero would. + InstanceGarbageCollectionMode GCMode } +// GCMode selects how the orphaned-server sweep behaves. +type GCMode string + +const ( + // GCEnabled reclaims orphaned servers. + GCEnabled GCMode = "enabled" + // GCObserve runs every check and reports what it would reclaim, deleting + // nothing. + GCObserve GCMode = "observe" + // GCDisabled does not sweep at all. + GCDisabled GCMode = "disabled" +) + // LoadConfig reads provider configuration from the environment. // CLUSTER_NAME is required. func LoadConfig() (*Config, error) { @@ -26,5 +54,31 @@ func LoadConfig() (*Config, error) { if !clusterNameRE.MatchString(name) { return nil, fmt.Errorf("CLUSTER_NAME %q is not a valid Hetzner label value (must match [a-zA-Z0-9._-], max 63 chars)", name) } - return &Config{ClusterName: name}, nil + gcMode, err := parseGCMode(os.Getenv("INSTANCE_GARBAGE_COLLECTION_MODE")) + if err != nil { + return nil, err + } + return &Config{ + ClusterName: name, + InstanceGarbageCollectionMode: gcMode, + }, nil +} + +// parseGCMode reads the sweep's mode, defaulting to enabled. +// +// An unrecognised value is an error rather than a silent fallback. This gates a +// controller that deletes servers, and the moment an operator reaches for it is +// a maintenance window where a typo falling back to "enabled" would reap the +// fleet they were trying to protect. Refusing to start is the safe failure. +func parseGCMode(raw string) (GCMode, error) { + switch mode := GCMode(strings.ToLower(strings.TrimSpace(raw))); mode { + case "": + return GCEnabled, nil + case GCEnabled, GCObserve, GCDisabled: + return mode, nil + default: + return "", fmt.Errorf( + "INSTANCE_GARBAGE_COLLECTION_MODE must be one of %q, %q or %q, got %q", + GCEnabled, GCObserve, GCDisabled, raw) + } } diff --git a/pkg/operator/config_test.go b/pkg/operator/config_test.go index 71a82c1..4cd6742 100644 --- a/pkg/operator/config_test.go +++ b/pkg/operator/config_test.go @@ -20,6 +20,40 @@ func TestLoadConfig_ReadsClusterName(t *testing.T) { } } +func TestLoadConfig_InstanceGarbageCollectionModes(t *testing.T) { + t.Setenv("CLUSTER_NAME", "paperclip-prod") + for raw, want := range map[string]GCMode{ + "": GCEnabled, // unset reclaims, matching AWS and Azure + "enabled": GCEnabled, + "observe": GCObserve, + "disabled": GCDisabled, + " OBSERVE ": GCObserve, + } { + t.Setenv("INSTANCE_GARBAGE_COLLECTION_MODE", raw) + cfg, err := LoadConfig() + if err != nil { + t.Fatalf("value %q: unexpected error: %v", raw, err) + } + if cfg.InstanceGarbageCollectionMode != want { + t.Errorf("value %q gave mode %q, want %q", raw, cfg.InstanceGarbageCollectionMode, want) + } + } +} + +// An operator reaches for this during maintenance that removes NodeClaims +// wholesale. A typo silently falling back to "enabled" would reap the fleet they +// were protecting, so an unrecognised value must stop the operator starting +// rather than pick a default for them. +func TestLoadConfig_RejectsUnrecognisedGarbageCollectionMode(t *testing.T) { + t.Setenv("CLUSTER_NAME", "paperclip-prod") + for _, v := range []string{"true", "false", "off", "dry-run", "dryrun", "observer", "Enabled!"} { + t.Setenv("INSTANCE_GARBAGE_COLLECTION_MODE", v) + if _, err := LoadConfig(); err == nil { + t.Errorf("value %q was accepted; an unrecognised value must be rejected", v) + } + } +} + func TestLoadConfig_RejectsInvalidClusterName(t *testing.T) { for _, bad := range []string{"has space", "slash/name", "comma,name", "töö"} { t.Setenv("CLUSTER_NAME", bad) diff --git a/pkg/providers/instance/instance.go b/pkg/providers/instance/instance.go index 099cda8..0ffb393 100644 --- a/pkg/providers/instance/instance.go +++ b/pkg/providers/instance/instance.go @@ -40,6 +40,16 @@ type Provider struct { pgClient PlacementGroupClient waiter ActionWaiter clusterName string + + // clusterUID identifies this cluster independently of its operator-chosen + // name, so two clusters sharing a CLUSTER_NAME in one Hetzner project can + // still tell their servers apart. + // + // Empty does not mean "no check": creates then stamp no UID, but adoption + // still refuses every server that carries one, because a UID that is present + // and different is the refusal rule. Only the test constructors leave it + // empty; production reads it at startup and refuses to run without it. + clusterUID string } // NewProvider returns a Provider that does NOT wait for hcloud actions to @@ -59,8 +69,11 @@ func NewProviderWithWaiter(client ServerClient, clusterName string, waiter Actio // NewProviderWithPlacementGroups returns a Provider that supports placement // groups and waits for hcloud create actions to complete. This is the // production constructor. -func NewProviderWithPlacementGroups(client ServerClient, pgClient PlacementGroupClient, clusterName string, waiter ActionWaiter) *Provider { - return &Provider{client: client, pgClient: pgClient, waiter: waiter, clusterName: clusterName} +func NewProviderWithPlacementGroups(client ServerClient, pgClient PlacementGroupClient, clusterName, clusterUID string, waiter ActionWaiter) *Provider { + return &Provider{ + client: client, pgClient: pgClient, waiter: waiter, + clusterName: clusterName, clusterUID: clusterUID, + } } // CreateOpts contains all parameters needed to create a Hetzner server node. @@ -135,12 +148,15 @@ func (p *Provider) Create(ctx context.Context, opts CreateOpts) (*hcloud.Server, // create is the internal implementation of Create, instrumented by Create(). func (p *Provider) create(ctx context.Context, opts CreateOpts) (*hcloud.Server, error) { log := logf.FromContext(ctx) - labels := make(map[string]string, len(opts.Labels)+3) + labels := make(map[string]string, len(opts.Labels)+4) for k, v := range opts.Labels { labels[k] = v } labels[apiv1.ServerLabelManagedBy] = apiv1.ServerValueManagedBy labels[apiv1.ServerLabelCluster] = p.clusterName + if p.clusterUID != "" { + labels[apiv1.ServerLabelClusterUID] = p.clusterUID + } if opts.NodeClaim != "" { labels[apiv1.ServerLabelNodeClaim] = opts.NodeClaim } @@ -198,6 +214,30 @@ func (p *Provider) create(ctx context.Context, opts CreateOpts) (*hcloud.Server, result, _, err := p.client.Create(ctx, createOpts) if err != nil { + // Hetzner rejects duplicate server names. Reaching this means a previous + // attempt already created the server but we never recorded its ID — + // typically because the process died between the create call and the + // status write. The server is running and billing with no owner, and its + // name and labels are the only surviving record of it, so adopt it rather + // than retrying into the same collision forever. + if adopted := p.adoptOrphan(ctx, opts, err); adopted != nil { + imageID := int64(0) + if adopted.Image != nil { + imageID = adopted.Image.ID + } + pgName := "" + if adopted.PlacementGroup != nil { + pgName = adopted.PlacementGroup.Name + } + // Log the shape actually adopted, not the shape requested: these are + // the fields that would reveal a machine whose placement group or image + // diverges from what this request would have built. + log.Info("adopted orphaned server", + "name", adopted.Name, "id", adopted.ID, + "serverType", opts.ServerType, "location", opts.Location, + "imageID", imageID, "placementGroup", pgName, "status", string(adopted.Status)) + return adopted, nil + } return nil, MapCreateError(err) } @@ -211,6 +251,21 @@ func (p *Provider) create(ctx context.Context, opts CreateOpts) (*hcloud.Server, actions = append(actions, result.NextActions...) if len(actions) > 0 { if err := p.waiter.WaitFor(ctx, actions...); err != nil { + // The server exists. Walking away leaves it running and billing with + // nothing pointing at it, and a later retry would then adopt a machine + // whose provisioning actions are known to have failed -- turning a + // loud repeated error into a silent success. Clean it up so adoption's + // only input stays the crash case, where the machine is healthy. + // + // The cleanup runs on a context detached from the caller's. WaitFor + // returns ctx.Err() when that context is cancelled -- a SIGTERM, a lost + // leader lease, a reconcile deadline -- which is the likeliest way to + // reach this branch at all, and reusing the dead context would fail the + // delete before it issued a single request, leaking exactly the server + // this is here to reclaim. + if result.Server != nil { + p.deleteAfterFailedCreate(ctx, opts.Name, result.Server.ID) + } return nil, fmt.Errorf("waiting for server %q create actions: %w", opts.Name, err) } } @@ -230,6 +285,125 @@ func (p *Provider) create(ctx context.Context, opts CreateOpts) (*hcloud.Server, return result.Server, nil } +// createCleanupTimeout bounds the compensating delete issued when a create's +// actions fail. That delete runs on a context detached from the caller's, so it +// needs a deadline of its own rather than inheriting one. +const createCleanupTimeout = 30 * time.Second + +// deleteAfterFailedCreate terminates a server whose create actions did not +// complete. It goes through the instrumented Delete so the call shows up in +// server_delete_total like every other deletion this provider issues. +func (p *Provider) deleteAfterFailedCreate(ctx context.Context, name string, serverID int64) { + delCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), createCleanupTimeout) + defer cancel() + if err := p.Delete(delCtx, FormatProviderID(serverID)); err != nil && + !karpcp.IsNodeClaimNotFoundError(err) { + logf.FromContext(ctx).Error(err, "deleting a server whose create actions failed", + "name", name, "id", serverID) + } +} + +// adoptOrphan returns the server that caused a uniqueness_error, provided this +// provider owns it and it is the server this call asked for. It returns nil for +// any other error, when the lookup fails, or when the existing server does not +// match — adopting a server belonging to another cluster would hand it to +// Karpenter for deletion, and adopting one of a different shape would make the +// NodeClaim advertise capacity and a zone the machine does not have. +// +// Declining leaves the original create error to surface. Note that the garbage +// collector will NOT reclaim the server while this NodeClaim is retrying: it +// treats the nodeclaim label as proof of ownership precisely so that an +// in-flight create is never destroyed. Recovery comes when Karpenter gives up on +// the NodeClaim, after which the name frees and the sweep reclaims the machine. +func (p *Provider) adoptOrphan(ctx context.Context, opts CreateOpts, createErr error) *hcloud.Server { + if !hcloud.IsError(createErr, hcloud.ErrorCodeUniquenessError) { + return nil + } + log := logf.FromContext(ctx) + servers, err := p.client.AllWithOpts(ctx, hcloud.ServerListOpts{Name: opts.Name}) + if err != nil { + // Without this the failed recovery is invisible: the caller only ever sees + // the original uniqueness error, with no sign adoption was even attempted. + log.Error(err, "looking up the server behind a name collision", "name", opts.Name) + metrics.RecordServerAdopt(metrics.AdoptError) + return nil + } + for _, s := range servers { + if s.Name != opts.Name { + continue + } + // The name label alone is not proof of ownership: CLUSTER_NAME is + // operator-supplied and not unique. Adoption hands a live machine to + // Karpenter, which will eventually terminate it, so taking one belonging to + // a same-named cluster would destroy their node. Shared with the orphan + // sweep, which deletes on the same answer. + if !apiv1.OwnedByCluster(s.Labels, p.clusterName, p.clusterUID) { + // Refusing a server that is ours-by-name but not ours-by-UID needs its + // own signal. Folded into the generic "declined" below it would carry the + // wrong remedy: an ordinary decline clears once the NodeClaim expires and + // the sweep reclaims the machine, but the sweep refuses this server for + // the same reason adoption did, so nothing ever reclaims it. + if s.Labels[apiv1.ServerLabelManagedBy] == apiv1.ServerValueManagedBy && + s.Labels[apiv1.ServerLabelCluster] == p.clusterName { + log.Info("refusing to adopt a server stamped with a different cluster UID; "+ + "either two clusters share a CLUSTER_NAME in one Hetzner project, or this "+ + "cluster's control plane was rebuilt and this server predates it", + "name", s.Name, "ourClusterUID", p.clusterUID, + "theirClusterUID", s.Labels[apiv1.ServerLabelClusterUID]) + metrics.RecordServerAdopt(metrics.AdoptForeignCluster) + return nil + } + continue + } + // Only a server this same NodeClaim created is evidence of a lost create. + // userData, SSH keys and public-IP policy are invisible on the returned + // server and are not drift-checked either, so matching the NodeClaim is + // what makes it safe to assume the machine was built from these inputs. + // Networks and firewalls do have drift checks in pkg/cloudprovider, so a + // mismatch there self-heals. The placement group is the gap: it is visible + // (the caller logs it) but neither checked here nor drift-checked, so an + // adopted server outside its spread group stays that way for life. + if opts.NodeClaim == "" || s.Labels[apiv1.ServerLabelNodeClaim] != opts.NodeClaim { + continue + } + // The normal path waits on the create actions before returning, which is + // how the caller knows the machine is really coming up. Adoption cannot + // wait — those action handles are gone — so require the server to be + // running or still coming up. Anything else (off, stopping, deleting, + // unknown) would hand Karpenter a NodeClaim with capacity nothing starts. + if s.Status != hcloud.ServerStatusRunning && + s.Status != hcloud.ServerStatusInitializing && + s.Status != hcloud.ServerStatusStarting { + continue + } + // The caller builds the NodeClaim's capacity and zone labels from the + // offering it selected on THIS attempt, not from the server it gets back. + // A server left by an earlier attempt that selected a different offering + // would be advertised with the wrong shape and the wrong zone — the latter + // silently breaking volume scheduling. An unresolved location fails closed + // for the same reason: that is the case where the zone is most likely wrong. + if s.ServerType == nil || s.ServerType.Name != opts.ServerType { + continue + } + if opts.Location == "" || s.Location == nil || s.Location.Name != opts.Location { + continue + } + // Karpenter records the adopted server's image as the NodeClaim's and then + // compares the two to detect image drift, so a mismatch accepted here can + // never be detected again. + if opts.Image != nil && (s.Image == nil || s.Image.ID != opts.Image.ID) { + continue + } + metrics.RecordServerAdopt(metrics.AdoptAdopted) + return s + } + // A NodeClaim retrying forever into a collision adoption keeps refusing is the + // state that leaves a machine billing, so it needs its own signal rather than + // looking like an ordinary create error. + metrics.RecordServerAdopt(metrics.AdoptDeclined) + return nil +} + // Delete removes the server identified by providerID. It returns nil once the // deletion has been triggered (Hetzner deletes asynchronously), and a // cloudprovider.NodeClaimNotFoundError once the server no longer exists — the diff --git a/pkg/providers/instance/instance_test.go b/pkg/providers/instance/instance_test.go index a252d38..bd92597 100644 --- a/pkg/providers/instance/instance_test.go +++ b/pkg/providers/instance/instance_test.go @@ -29,10 +29,12 @@ type mockServerClient struct { nextID int64 deleted []int64 lastListSelector string + lastListName string action *hcloud.Action nextActions []*hcloud.Action createErr error deleteErr error + listErr error lastOpts hcloud.ServerCreateOpts } @@ -64,7 +66,12 @@ func (m *mockServerClient) DeleteWithResult(_ context.Context, server *hcloud.Se return &hcloud.ServerDeleteResult{}, nil, nil } -func (m *mockServerClient) GetByID(_ context.Context, id int64) (*hcloud.Server, *hcloud.Response, error) { +func (m *mockServerClient) GetByID(ctx context.Context, id int64) (*hcloud.Server, *hcloud.Response, error) { + // A real client fails immediately on a done context; honouring that here is + // what lets a test tell a live cleanup path from a dead one. + if err := ctx.Err(); err != nil { + return nil, nil, err + } server, ok := m.servers[id] if !ok { return nil, nil, nil @@ -74,8 +81,15 @@ func (m *mockServerClient) GetByID(_ context.Context, id int64) (*hcloud.Server, func (m *mockServerClient) AllWithOpts(_ context.Context, opts hcloud.ServerListOpts) ([]*hcloud.Server, error) { m.lastListSelector = opts.LabelSelector + m.lastListName = opts.Name + if m.listErr != nil { + return nil, m.listErr + } result := make([]*hcloud.Server, 0, len(m.servers)) for _, s := range m.servers { + if opts.Name != "" && s.Name != opts.Name { + continue + } result = append(result, s) } return result, nil @@ -338,6 +352,347 @@ func TestCreate_MapsCapacityError(t *testing.T) { } } +// orphan builds a server that a previous Create attempt left behind, labelled +// as this provider labels its own servers and shaped like the request in +// adoptOpts below. +func orphan(id int64, name, cluster string) *hcloud.Server { + return &hcloud.Server{ + ID: id, + Name: name, + ServerType: &hcloud.ServerType{Name: "cx22"}, + Location: &hcloud.Location{Name: "nbg1"}, + Labels: map[string]string{ + apiv1.ServerLabelManagedBy: apiv1.ServerValueManagedBy, + apiv1.ServerLabelCluster: cluster, + }, + } +} + +// adoptOpts is the request every adoption test replays. +func adoptOpts() CreateOpts { + return CreateOpts{ + Name: "worker-abc", ServerType: "cx22", Location: "nbg1", + Image: &hcloud.Image{ID: 1}, NodeClaim: "worker-abc", + } +} + +// ownedOrphan is a server a previous attempt created for THIS NodeClaim, with +// the image the request resolves to. +func ownedOrphan(id int64) *hcloud.Server { + s := orphan(id, "worker-abc", "test-cluster") + s.Labels[apiv1.ServerLabelNodeClaim] = "worker-abc" + s.Image = &hcloud.Image{ID: 1} + s.Status = hcloud.ServerStatusRunning + return s +} + +// A server left by a different NodeClaim tells us nothing about whether it was +// built from this request's inputs -- userData, SSH keys, networks, firewalls and +// public-IP policy are all invisible on the returned server and none of them are +// drift-checked, so a name match alone is not evidence of a match. +func TestCreate_UniquenessErrorRefusesServerOfAnotherNodeClaim(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Labels[apiv1.ServerLabelNodeClaim] = "some-other-claim" + client.servers[42] = s + client.createErr = uniquenessErr() + + _, err := NewProvider(client, "test-cluster").Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "adopted a server built for a different NodeClaim") +} + +// Only a server that is running or still coming up is a plausible launch. An +// "off" machine adopted as a success gives Karpenter a NodeClaim with capacity +// that nothing will ever start. +func TestCreate_UniquenessErrorRefusesNonRunningServer(t *testing.T) { + for _, status := range []hcloud.ServerStatus{ + hcloud.ServerStatusOff, hcloud.ServerStatusDeleting, + hcloud.ServerStatusStopping, hcloud.ServerStatusUnknown, + } { + t.Run(string(status), func(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Status = status + client.servers[42] = s + client.createErr = uniquenessErr() + + _, err := NewProvider(client, "test-cluster").Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, fmt.Sprintf("adopted a server in %q status", status)) + }) + } +} + +// An unresolved location must fail closed. Karpenter derives the NodeClaim's +// zone label from the offering it picked, so adopting a machine in an unknown +// location silently breaks hcloud CSI volume scheduling. +func TestCreate_UniquenessErrorRefusesWhenLocationUnresolved(t *testing.T) { + client := newMockServerClient() + client.servers[42] = ownedOrphan(42) + client.createErr = uniquenessErr() + + opts := adoptOpts() + opts.Location = "" + _, err := NewProvider(client, "test-cluster").Create(context.Background(), opts) + assertRefusedAdoption(t, err, "adopted a server for a request with no resolved location") +} + +// Karpenter records the adopted server's image as the NodeClaim's, and image +// drift compares those two -- so an image mismatch adopted here can never be +// detected afterwards. +func TestCreate_UniquenessErrorRefusesImageMismatch(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Image = &hcloud.Image{ID: 999} + client.servers[42] = s + client.createErr = uniquenessErr() + + _, err := NewProvider(client, "test-cluster").Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "adopted a server running a different image") +} + +// The create call succeeded, so the server exists; if waiting on its actions +// fails we must not walk away and leave it running. Deleting it here is what +// keeps adoption's input limited to crash-orphans, whose machines are healthy. +func TestCreate_WaiterFailureDeletesTheCreatedServer(t *testing.T) { + client := newMockServerClient() + client.action = &hcloud.Action{ID: 1} + waiter := &mockActionWaiter{err: fmt.Errorf("action failed")} + + p := NewProviderWithWaiter(client, "test-cluster", waiter) + if _, err := p.Create(context.Background(), adoptOpts()); err == nil { + t.Fatal("expected the wait failure to surface") + } + if len(client.deleted) != 1 { + t.Errorf("left the created server running after its actions failed: deleted=%v", client.deleted) + } +} + +// hcloud's WaitFor returns ctx.Err() when the caller's context is cancelled -- a +// SIGTERM, a lost leader lease, a reconcile deadline -- which is the likeliest +// way to reach the cleanup at all. Running the cleanup on that same context would +// fail it before it issued a single request, leaking the server it exists to +// reclaim and leaving a half-provisioned machine for a later retry to adopt. +func TestCreate_WaiterFailureDeletesTheServerOnACancelledContext(t *testing.T) { + client := newMockServerClient() + client.action = &hcloud.Action{ID: 1} + waiter := &mockActionWaiter{err: context.Canceled} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + p := NewProviderWithWaiter(client, "test-cluster", waiter) + if _, err := p.Create(ctx, adoptOpts()); err == nil { + t.Fatal("expected the wait failure to surface") + } + if len(client.deleted) != 1 { + t.Errorf("left the created server running after a cancelled create: deleted=%v", client.deleted) + } +} + +// assertRefusedAdoption asserts that Create declined to adopt AND that the +// original uniqueness error reached the caller unchanged. The contract is not +// "some error came back" but "a name collision stays an ordinary, retryable +// collision": mapping it to InsufficientCapacityError would make +// cloudprovider.Create call MarkUnavailable and blacklist a perfectly healthy +// server type/location offering on every collision, pushing the NodePool onto +// more expensive types. +func assertRefusedAdoption(t *testing.T, err error, why string) { + t.Helper() + if err == nil { + t.Fatal(why) + } + if !hcloud.IsError(err, hcloud.ErrorCodeUniquenessError) { + t.Errorf("expected the original uniqueness error to reach the caller, got %v", err) + } + if karpcp.IsInsufficientCapacityError(err) { + t.Error("a name collision was mapped to InsufficientCapacityError, blacklisting a healthy offering") + } +} + +func uniquenessErr() error { + return hcloud.Error{Code: hcloud.ErrorCodeUniquenessError, Message: "server name is already used"} +} + +const testClusterUID = "34f25cbf-c7b5-49d1-833b-103bff8a34ad" + +// providerWithUID is the production shape: a cluster name plus the UID that +// makes ownership unambiguous when two clusters share a name. It goes through +// the production constructor rather than assigning the field, so anything that +// construction derives from the UID applies here too. +func providerWithUID(client ServerClient) *Provider { + return NewProviderWithPlacementGroups(client, nil, "test-cluster", testClusterUID, nil) +} + +// Every server this installation creates must carry the cluster UID, or a +// same-named cluster sharing the Hetzner project cannot tell them apart. +func TestCreate_StampsClusterUID(t *testing.T) { + client := newMockServerClient() + + server, err := providerWithUID(client).Create(context.Background(), adoptOpts()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := server.Labels[apiv1.ServerLabelClusterUID]; got != testClusterUID { + t.Errorf("cluster UID label = %q, want %q", got, testClusterUID) + } +} + +// Adoption hands a live machine to Karpenter, which will eventually terminate +// it. Taking one belonging to a same-named cluster would destroy their node. +func TestCreate_UniquenessErrorRefusesForeignClusterUID(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Labels[apiv1.ServerLabelClusterUID] = "6e5f8dfb-e54b-41ee-8fb3-89a48a42231f" + client.servers[42] = s + client.createErr = uniquenessErr() + + _, err := providerWithUID(client).Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "adopted a server belonging to a same-named other cluster") +} + +func TestCreate_AdoptsServerWithMatchingClusterUID(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Labels[apiv1.ServerLabelClusterUID] = testClusterUID + client.servers[42] = s + client.createErr = uniquenessErr() + + server, err := providerWithUID(client).Create(context.Background(), adoptOpts()) + if err != nil { + t.Fatalf("refused to adopt our own server: %v", err) + } + if server == nil || server.ID != 42 { + t.Fatalf("expected server 42, got %+v", server) + } +} + +// Servers created before this label existed carry no UID; refusing them would +// make every pre-existing orphan unrecoverable. +func TestCreate_AdoptsLegacyServerWithoutClusterUID(t *testing.T) { + client := newMockServerClient() + client.servers[42] = ownedOrphan(42) // no UID label + client.createErr = uniquenessErr() + + server, err := providerWithUID(client).Create(context.Background(), adoptOpts()) + if err != nil { + t.Fatalf("refused to adopt a legacy server: %v", err) + } + if server == nil || server.ID != 42 { + t.Fatalf("expected server 42, got %+v", server) + } +} + +// A crash between the Hetzner create call and persisting the provider ID leaves +// a running server that Karpenter has no record of. Every retry then collides on +// the name. Adopting the existing server is the only way to recover it, since +// the server's own name and labels are the sole surviving record. +func TestCreate_AdoptsOrphanedServerOnUniquenessError(t *testing.T) { + client := newMockServerClient() + client.servers[42] = ownedOrphan(42) + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + server, err := p.Create(context.Background(), adoptOpts()) + if err != nil { + t.Fatalf("expected adoption to succeed, got error: %v", err) + } + if server == nil || server.ID != 42 { + t.Fatalf("expected adopted server 42, got %+v", server) + } + if client.lastListName != "worker-abc" { + t.Errorf("expected lookup by name %q, got %q", "worker-abc", client.lastListName) + } +} + +func TestCreate_UniquenessErrorRefusesForeignCluster(t *testing.T) { + client := newMockServerClient() + foreign := ownedOrphan(42) + foreign.Labels[apiv1.ServerLabelCluster] = "someone-elses-cluster" + client.servers[42] = foreign + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected error, adopted a server belonging to another cluster") +} + +func TestCreate_UniquenessErrorRefusesUnmanagedServer(t *testing.T) { + client := newMockServerClient() + client.servers[42] = &hcloud.Server{ID: 42, Name: "worker-abc"} // no karpenter labels + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected error, adopted a server this provider does not manage") +} + +// The caller builds the NodeClaim's capacity and zone labels from the offering +// it selected on THIS attempt, not from the server Create hands back. Adopting a +// server of a different shape would therefore advertise capacity the machine +// does not have, and a zone it is not in — which silently breaks volume +// scheduling. Declining leaves the orphan for the garbage collector. +func TestCreate_UniquenessErrorRefusesMismatchedServerType(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.ServerType = &hcloud.ServerType{Name: "cx42"} + client.servers[42] = s + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected error, adopted a server of a different server type") +} + +func TestCreate_UniquenessErrorRefusesMismatchedLocation(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Location = &hcloud.Location{Name: "fsn1"} + client.servers[42] = s + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected error, adopted a server in a different location") +} + +// Hetzner deletes asynchronously and keeps the name reserved until it finishes, +// so the garbage collector reaping an orphan leaves a window where a retry +// collides with the dying server. Adopting it would bind the NodeClaim to a +// machine that vanishes seconds later, stalling until the registration timeout. +func TestCreate_UniquenessErrorRefusesDeletingServer(t *testing.T) { + client := newMockServerClient() + s := ownedOrphan(42) + s.Status = hcloud.ServerStatusDeleting + client.servers[42] = s + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected error, adopted a server Hetzner is deleting") +} + +func TestCreate_UniquenessErrorWithNoMatchReturnsError(t *testing.T) { + client := newMockServerClient() + client.createErr = uniquenessErr() + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected the original uniqueness error when no server matches") +} + +func TestCreate_UniquenessErrorLookupFailureReturnsCreateError(t *testing.T) { + client := newMockServerClient() + client.createErr = uniquenessErr() + client.listErr = fmt.Errorf("api unavailable") + + p := NewProvider(client, "test-cluster") + _, err := p.Create(context.Background(), adoptOpts()) + assertRefusedAdoption(t, err, "expected an error when the adoption lookup fails") + if !strings.Contains(err.Error(), "already used") { + t.Errorf("expected the original create error to survive, got %v", err) + } +} + func TestCreate_WaiterErrorIsWrapped(t *testing.T) { client := newMockServerClient() client.action = &hcloud.Action{ID: 1} @@ -414,7 +769,7 @@ func (m *mockPlacementGroupClient) Create(_ context.Context, opts hcloud.Placeme func TestCreate_SpreadStrategy_CreatesPG(t *testing.T) { sc := newMockServerClient() pgc := newMockPlacementGroupClient() - p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", nil) + p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", testClusterUID, nil) _, err := p.Create(context.Background(), CreateOpts{ Name: "n", @@ -448,7 +803,7 @@ func TestCreate_SpreadStrategy_CreatesPG(t *testing.T) { func TestCreate_SpreadStrategy_EmptyStrategy_CreatesPG(t *testing.T) { sc := newMockServerClient() pgc := newMockPlacementGroupClient() - p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", nil) + p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", testClusterUID, nil) _, err := p.Create(context.Background(), CreateOpts{ Name: "n", @@ -471,7 +826,7 @@ func TestCreate_SpreadStrategy_EmptyStrategy_CreatesPG(t *testing.T) { func TestCreate_NoneStrategy_NoPG(t *testing.T) { sc := newMockServerClient() pgc := newMockPlacementGroupClient() - p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", nil) + p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", testClusterUID, nil) _, err := p.Create(context.Background(), CreateOpts{ Name: "n", @@ -502,7 +857,7 @@ func TestCreate_SpreadStrategy_ReusesPG(t *testing.T) { pgc.groups = []*hcloud.PlacementGroup{ {ID: existingID, Name: "karpenter-test-cluster-my-pool", Type: hcloud.PlacementGroupTypeSpread}, } - p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", nil) + p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", testClusterUID, nil) _, err := p.Create(context.Background(), CreateOpts{ Name: "n", @@ -530,7 +885,7 @@ func TestCreate_SpreadStrategy_ReusesPG(t *testing.T) { func TestCreate_SpreadStrategy_EmptyNodePool(t *testing.T) { sc := newMockServerClient() pgc := newMockPlacementGroupClient() - p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", nil) + p := NewProviderWithPlacementGroups(sc, pgc, "test-cluster", testClusterUID, nil) _, err := p.Create(context.Background(), CreateOpts{ Name: "n",