User-Defined Node Taints on Provisioned Nodes (#41)

Signed-off-by: Erick Bourgeois <erick@jeb.ca>

Author: ebourgeois

.claude/CHANGELOG.md

@@ -13,7 +13,7 @@ CALM_DIAGRAMS_OUT := docs/src/architecture
 REGISTRY ?= ghcr.io
 IMAGE_NAME ?= 5spot
 IMAGE_TAG ?= latest-dev
-NAMESPACE ?= 5-spot-system
+NAMESPACE ?= 5spot-system
 
 # Platform configuration for builds
 # Default is linux/amd64 (most common for Kubernetes deployments)

Makefile

@@ -53,6 +53,7 @@ A cloud-native Kubernetes controller for managing time-based machine scheduling
 - 🔄 **Graceful shutdown** - Configurable grace periods with automatic node draining
 - 🎯 **Priority-based** - Resource distribution across controller instances
 - 🚨 **Kill switch** - Emergency immediate removal capability
+- 🏷️ **User-defined Node taints** - Declare taints in `spec.nodeTaints`; the controller applies them once the Node is Ready, tracks ownership, and reconciles drift
 - 📊 **Multi-instance** - Horizontal scaling with consistent hashing
 - 🔍 **Full observability** - Prometheus metrics and health checks
 

README.md

@@ -148,3 +148,82 @@ spec:
     - expression: "object.spec.infrastructureSpec.kind.size() > 0"
       message: "spec.infrastructureSpec.kind must not be empty"
       reason: Invalid
+
+    # ── 14. nodeTaints: key format (RFC-1123 qualified name) ──────────────────
+    # Mirrors validate_taint_key() in src/crd.rs. The optional "<prefix>/" part
+    # is a DNS subdomain; the name part is a qualified name. Both capped at 63
+    # chars post-split. Core/v1 admission applies the same regex — we duplicate
+    # it here so bad input never reaches the reconciler.
+    # Trivially true when nodeTaints is absent or empty.
+    - expression: |
+        !has(object.spec.nodeTaints) ||
+        object.spec.nodeTaints.all(t,
+          t.key.matches('^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$'))
+      message: >-
+        spec.nodeTaints[*].key must be an RFC-1123 qualified name
+        (optional DNS-1123 prefix '/' followed by a label of letters/digits/'-','_','.')
+      reason: Invalid
+
+    # ── 15. nodeTaints: key length ≤ 253 total, name-part ≤ 63 ────────────────
+    # Qualified-name rule: the name after the optional '/' is ≤ 63 chars.
+    # We check total length ≤ 253 which also bounds the name part since the
+    # prefix is ≤ 253 per DNS-1123. A tighter per-part cap would require CEL
+    # string splits that are verbose; the Rust-side validator does enforce
+    # the 63-char name-part cap as the authoritative guard.
+    - expression: |
+        !has(object.spec.nodeTaints) ||
+        object.spec.nodeTaints.all(t, t.key.size() <= 253)
+      message: "spec.nodeTaints[*].key must be ≤ 253 characters"
+      reason: Invalid
+
+    # ── 16. nodeTaints: value length ≤ 63 ─────────────────────────────────────
+    # Mirrors validate_taint_value() in src/crd.rs. Core/v1 enforces the same
+    # 63-char cap on taint values.
+    - expression: |
+        !has(object.spec.nodeTaints) ||
+        object.spec.nodeTaints.all(t, !has(t.value) || t.value.size() <= 63)
+      message: "spec.nodeTaints[*].value must be ≤ 63 characters"
+      reason: Invalid
+
+    # ── 17. nodeTaints: reserved 5spot.finos.org/ prefix ──────────────────────
+    # Our own controller owns this namespace for internal taints / annotations.
+    # Users must not set taints here via the CR; request routing for reserved
+    # keys is handled server-side.
+    - expression: |
+        !has(object.spec.nodeTaints) ||
+        object.spec.nodeTaints.all(t, !t.key.startsWith('5spot.finos.org/'))
+      message: >-
+        spec.nodeTaints[*].key must not start with '5spot.finos.org/' —
+        this prefix is reserved for 5Spot controller-owned taints
+      reason: Invalid
+
+    # ── 18. nodeTaints: reserved kubernetes.io / node.kubernetes.io /
+    # node-role.kubernetes.io prefixes ────────────────────────────────────────
+    # These prefixes carry special kubelet/scheduler semantics and must not
+    # be set via spec.nodeTaints — operators wanting control-plane role
+    # taints should use spec.machineTemplate instead.
+    - expression: |
+        !has(object.spec.nodeTaints) ||
+        object.spec.nodeTaints.all(t,
+          !t.key.startsWith('kubernetes.io/') &&
+          !t.key.startsWith('node.kubernetes.io/') &&
+          !t.key.startsWith('node-role.kubernetes.io/'))
+      message: >-
+        spec.nodeTaints[*].key must not start with 'kubernetes.io/',
+        'node.kubernetes.io/', or 'node-role.kubernetes.io/' —
+        use spec.machineTemplate for control-plane role taints
+      reason: Invalid
+
+    # ── 19. nodeTaints: no duplicate (key, effect) pairs ──────────────────────
+    # Mirrors the duplicate-check arm of validate_node_taints() in src/crd.rs.
+    # Identity is the tuple (key, effect); value is mutable. We check that
+    # every entry appears exactly once under (key, effect). O(n²) scan —
+    # acceptable because nodeTaints is typically ≤ 5 entries per CR.
+    - expression: |
+        !has(object.spec.nodeTaints) ||
+        object.spec.nodeTaints.all(t,
+          object.spec.nodeTaints.filter(x, x.key == t.key && x.effect == t.effect).size() == 1)
+      message: >-
+        spec.nodeTaints must not contain duplicate (key, effect) pairs —
+        taint identity is the tuple (key, effect); value is mutable
+      reason: Invalid

deploy/admission/validatingadmissionpolicy.yaml

@@ -93,7 +93,7 @@ spec:
                 description: |-
                   Optional list of process patterns that trigger an emergency node
                   reclaim. When non-empty, the 5-Spot controller installs the
-                  `5spot-reclaim-agent` DaemonSet on every Node backing this
+                  `5spot-reclaim-agent` `DaemonSet` on every Node backing this
                   `ScheduledMachine`; the agent watches `/proc` for any process whose
                   basename or argv matches one of these patterns and, on first match,
                   annotates the Node to request immediate (non-graceful) removal from
@@ -135,6 +135,49 @@ spec:
                 default: 5m
                 description: Timeout for draining the node before deletion (e.g., "5m", "10m")
                 type: string
+              nodeTaints:
+                description: |-
+                  User-defined taints applied to the Kubernetes Node once it is Ready.
+
+                  The controller owns and reconciles only the taints it applied (tracked
+                  in `status.appliedNodeTaints` plus the `5spot.finos.org/applied-taints`
+                  annotation on the Node). Admin-added taints on the same Node are left
+                  untouched. A taint is identified by the tuple `(key, effect)`; `value`
+                  is mutable. Keys prefixed with `5spot.finos.org/`, `kubernetes.io/`,
+                  `node.kubernetes.io/`, or `node-role.kubernetes.io/` are rejected.
+                items:
+                  description: |-
+                    A taint the controller applies to the Kubernetes Node once it is Ready.
+
+                    Mirrors the shape of core/v1 `Taint`. Identity is the tuple `(key, effect)`;
+                    `value` is mutable. See `ScheduledMachineSpec.node_taints` for ownership
+                    semantics.
+                  properties:
+                    effect:
+                      description: Taint effect — one of `NoSchedule`, `PreferNoSchedule`, `NoExecute`.
+                      enum:
+                      - NoSchedule
+                      - PreferNoSchedule
+                      - NoExecute
+                      type: string
+                    key:
+                      description: |-
+                        Taint key. Must be a qualified name (`[prefix/]name`), 1–63 chars on the
+                        name portion and matching `[a-z0-9A-Z]([-a-zA-Z0-9.]*[a-zA-Z0-9])?`.
+                        Reserved prefixes (`5spot.finos.org/`, `kubernetes.io/`,
+                        `node.kubernetes.io/`, `node-role.kubernetes.io/`) are rejected.
+                      type: string
+                    value:
+                      description: |-
+                        Optional taint value (max 63 chars). Matches the same qualified-name
+                        pattern as `key`'s name portion when set.
+                      nullable: true
+                      type: string
+                  required:
+                  - effect
+                  - key
+                  type: object
+                type: array
               priority:
                 default: 50
                 description: Priority for machine scheduling (higher values = higher priority)
@@ -183,6 +226,46 @@ spec:
           status:
             nullable: true
             properties:
+              appliedNodeTaints:
+                description: |-
+                  Taints the controller has applied to the Node, in the order they were
+                  applied. Maintained as the controller's record of truth so subsequent
+                  reconciles only mutate taints we own — admin-added taints on the same
+                  Node whose `(key, effect)` collides with an entry here are surfaced as
+                  a `TaintOwnershipConflict` condition rather than overwritten.
+                items:
+                  description: |-
+                    A taint the controller applies to the Kubernetes Node once it is Ready.
+
+                    Mirrors the shape of core/v1 `Taint`. Identity is the tuple `(key, effect)`;
+                    `value` is mutable. See `ScheduledMachineSpec.node_taints` for ownership
+                    semantics.
+                  properties:
+                    effect:
+                      description: Taint effect — one of `NoSchedule`, `PreferNoSchedule`, `NoExecute`.
+                      enum:
+                      - NoSchedule
+                      - PreferNoSchedule
+                      - NoExecute
+                      type: string
+                    key:
+                      description: |-
+                        Taint key. Must be a qualified name (`[prefix/]name`), 1–63 chars on the
+                        name portion and matching `[a-z0-9A-Z]([-a-zA-Z0-9.]*[a-zA-Z0-9])?`.
+                        Reserved prefixes (`5spot.finos.org/`, `kubernetes.io/`,
+                        `node.kubernetes.io/`, `node-role.kubernetes.io/`) are rejected.
+                      type: string
+                    value:
+                      description: |-
+                        Optional taint value (max 63 chars). Matches the same qualified-name
+                        pattern as `key`'s name portion when set.
+                      nullable: true
+                      type: string
+                  required:
+                  - effect
+                  - key
+                  type: object
+                type: array
               bootstrapRef:
                 description: Reference to the created bootstrap resource
                 nullable: true

deploy/crds/scheduledmachine.yaml

@@ -119,6 +119,65 @@ Optional configuration applied to the created CAPI Machine.
 | `nodeDrainTimeout` | `string` | No | `5m` | Timeout for draining the node before deletion. |
 | `killSwitch` | `bool` | No | `false` | Operator-driven kill switch. Immediately remove machine if `true`; reset to `false` to return to scheduled service. |
 | `killIfCommands` | `[]string` | No | `null` | Node-side process-match kill switch. When non-empty, the reclaim agent DaemonSet is installed on the backing node and watches `/proc` for any process whose `comm` or `cmdline` matches an entry. First match triggers `EmergencyRemove` + auto-disables the schedule. See [Emergency Reclaim](./emergency-reclaim.md). |
+| `nodeTaints` | `[]NodeTaint` | No | `[]` | User-defined taints applied to the Kubernetes Node once it is Ready. The controller owns and reconciles only the taints it applied; admin-added taints on the same Node are left untouched. See [Node Taints](#node-taints) below. |
+
+## Node Taints
+
+`spec.nodeTaints` declares taints that must exist on the Kubernetes Node once
+it joins the cluster and reports `Ready=True`. The controller patches them on
+via server-side apply, tracks what it applied in `status.appliedNodeTaints`,
+and reconciles drift on every Node change (event-driven via the Node watch —
+no polling).
+
+### Shape
+
+```yaml
+spec:
+  nodeTaints:
+    - key: workload
+      value: batch
+      effect: NoSchedule
+    - key: dedicated
+      value: ml
+      effect: NoExecute
+```
+
+| Field | Type | Required | Description |
+|-------|------|----------|-------------|
+| `key` | `string` | Yes | RFC-1123 qualified name. Max 253 chars total; name-part ≤ 63. Reserved prefixes (`5spot.finos.org/`, `kubernetes.io/`, `node.kubernetes.io/`, `node-role.kubernetes.io/`) are rejected at admission. |
+| `value` | `string` | No | Optional value, ≤ 63 chars. Mutable — changing the value on an existing taint triggers an update, not an add/remove. |
+| `effect` | `enum` | Yes | One of `NoSchedule`, `PreferNoSchedule`, `NoExecute`. Identity is the tuple `(key, effect)`. |
+
+### Ownership model
+
+Taint identity is `(key, effect)`; the `value` is mutable. The controller only
+touches taints it previously applied (tracked in `status.appliedNodeTaints` and
+recorded in the annotation `5spot.finos.org/applied-taints` on the Node).
+
+- **Admin-added taint with the same `(key, effect)`**: surfaces as a
+  `TaintOwnershipConflict` condition. The controller refuses to overwrite.
+- **Admin-added taint with a different `(key, effect)`**: ignored — left in
+  place on the Node across reconciles.
+- **Spec shrinks (taint removed)**: the controller removes only taints it
+  previously applied. If the admin has mutated the value since we applied it,
+  the controller refuses to remove and surfaces a conflict.
+
+### Status condition: `NodeTainted`
+
+| status | reason | meaning |
+|--------|--------|---------|
+| `Unknown` | `NoNodeYet` | `status.nodeRef` is populated but the Node object is not yet materialised in the API server. |
+| `False` | `NodeNotReady` | Node exists but `Ready != True`. Will re-reconcile on the Node watch event. |
+| `False` | `PatchFailed` | k8s API returned an error on the last patch. Exponential backoff applies. |
+| `False` | `TaintOwnershipConflict` | An admin taint collides with a declared `(key, effect)`. The controller refuses to overwrite until the spec changes. |
+| `True` | `Applied` | All declared taints are present on the Node. |
+
+### What it does not manage
+
+- **Tolerations** — a Pod-side concern. Workloads must declare tolerations
+  themselves to schedule onto tainted Nodes.
+- **Node labels** — separate feature with different conflict semantics.
+- **Admin-added taints** — never removed or overwritten by the controller.
 
 ## Status Fields
 
@@ -135,6 +194,7 @@ The status subresource contains the current state:
 | `infrastructureRef` | `ObjectReference` | Reference to created infrastructure resource |
 | `nodeRef` | `NodeRef` | Reference to the Kubernetes Node (apiVersion, kind, name, uid) once provisioned |
 | `providerID` | `string` | Provider-assigned machine identifier (copied from CAPI `Machine.spec.providerID`) |
+| `appliedNodeTaints` | `[]NodeTaint` | Taints the controller has applied to the Node. Source of truth for ownership — only entries here are eligible for removal. See [Node Taints](#node-taints). |
 | `lastScheduledTime` | `Time` | Last time machine was created |
 | `nextActivation` | `Time` | Next scheduled activation time |
 | `nextCleanup` | `Time` | Time when machine will be cleaned up |

docs/src/concepts/scheduled-machine.md

@@ -317,6 +317,71 @@ kubectl get events --field-selector reason=EmergencyReclaimDisabledSchedule \
 kubectl get scheduledmachine/<name> -o jsonpath='{.metadata.generation} {.status.observedGeneration}'
 ```
 
+## Node Taints
+
+### Taints not appearing on Node
+
+`spec.nodeTaints` is declared on a ScheduledMachine, the machine is Active,
+but the Node does not have the expected taints. Walk the `NodeTainted`
+condition on the CR first — it tells you exactly which layer is failing.
+
+```bash
+kubectl get scheduledmachine <name> \
+  -o jsonpath='{.status.conditions[?(@.type=="NodeTainted")]}{"\n"}'
+```
+
+Three failure reasons, each with its own fix:
+
+**`reason=NoNodeYet` (status=Unknown)**
+CAPI populated `status.nodeRef` but the Node object is not yet in the API
+server. This is usually a few seconds after Machine creation. The Node watch
+will re-enqueue us automatically — no action needed. If stuck for > 1 min,
+check that CAPI's Machine actually materialised the Node:
+
+```bash
+kubectl get machine <name>-machine -o jsonpath='{.status.nodeRef}{"\n"}'
+kubectl get nodes <node-name>
+```
+
+**`reason=NodeNotReady` (status=False)**
+Node exists but `Ready != True`. Kubelet hasn't registered, networking is
+degraded, or CNI is failing. Look at the Node's own conditions first:
+
+```bash
+kubectl describe node <node-name> | sed -n '/Conditions:/,/Addresses:/p'
+```
+
+Fix the underlying Node problem; the controller will re-reconcile on the next
+Node `Ready` transition.
+
+**`reason=TaintOwnershipConflict` (status=False)**
+An admin taint exists with the same `(key, effect)` tuple as a declared
+`spec.nodeTaints` entry. The controller refuses to overwrite admin-owned
+taints. Inspect the current state:
+
+```bash
+kubectl get node <node-name> -o jsonpath='{.spec.taints}{"\n"}'
+kubectl get node <node-name> \
+  -o jsonpath='{.metadata.annotations.5spot\.finos\.org/applied-taints}{"\n"}'
+```
+
+Resolve by either removing the admin taint (`kubectl taint nodes <node> key:effect-`)
+or changing the `spec.nodeTaints` entry so the `(key, effect)` no longer
+collides. Note: the annotation `5spot.finos.org/applied-taints` lists the
+keys we own; any taint *not* in that list belongs to the admin.
+
+**`reason=PatchFailed` (status=False)**
+A non-404, non-conflict API error on the Node PATCH. Check controller logs for
+the exact kube error (RBAC rejection, API server unreachable, etc.):
+
+```bash
+kubectl logs -n 5spot-system -l app=5spot-controller \
+  | grep -E "node_taint|appliedNodeTaints"
+```
+
+The controller retries with exponential backoff on transient failures. For
+RBAC issues, confirm the controller ClusterRole grants `patch` on `nodes`.
+
 ## Error Messages
 
 ### "Resource not owned by this instance"