fix(autoscaler): leave a node with no memory reading out of the pressure calculation

README.md

@@ -161,7 +161,7 @@ A scale set's static `maxRunners` has to be safe for the pool's real worst case,
 Each poll (`AUTOSCALER_POLL_SECONDS`, default 45s):
 - For every pooled profile, counts its running runners from its `EphemeralRunner` resources (every one that has not finished: a phase other than `Succeeded` or `Failed`, so a pending runner counts, because its pod already holds a slot) and reads its per-pod hard memory limit (`spec.template.spec.containers[0].resources.limits.memory`) straight from its live `AutoscalingRunnerSet`, never a static config value, so both always match what is actually deployed. The running count is not read from the `AutoscalingRunnerSet`'s status: in the ARC release this role pins (0.15) that status carries only `phase` and `observedGeneration`, so a count read from it would always be empty. A failed read of the runners stops the poll instead of counting zero. Sums the running counts into one combined total, and takes the largest of the pod memory limits as the threshold a raise or lower must leave room for.
 - Sums real, current memory usage of every pooled profile's running runner pods, from the metrics API's pod list for each profile's own namespace (`scripts/autoscaler-usage.jq`); this, not a static per-pod request or limit, is the entire point of "usage-driven". It does not use `kubectl top pod`, which fails a whole namespace while any pod in it has no metrics, as every new runner pod has none until metrics-server first samples it. A runner pod placed on a node, not finished and missing from the list is unmeasured, and assumed to use its own memory limit (its scale set's limit when it sets none), the most it can reach. Lowering is decided on the measured usage, so a new pod never lowers the pool by itself; a raise also needs a pod's room with every unmeasured pod at its limit, and the pool holds until they are measured otherwise.
-- Reads cluster-wide memory-availability pressure via `kubectl top nodes`, taking the **worst-case (minimum)** available-memory percentage across the nodes runner pods may use, not an average — a pool average can look healthy while the specific node a new runner pod would actually land on is not, consistent with the algorithm's own bias toward lowering eagerly (lowering never disrupts in-flight jobs). With the node eligibility label set (`github_runner_arc_node_label_key`, passed as `AUTOSCALER_NODE_SELECTOR`) only nodes carrying it are read, otherwise every node: a node outside the label runs no runner pod, so its pressure says nothing about where the next one lands, and when its kubelet drops out its missing metrics would otherwise fail safe on every poll. This is a genuine improvement over the pod's own earlier host-pinned design, which could only ever see one fixed machine's pressure via `/proc/meminfo`, not the whole pool's. **No swap-pressure signal**: metrics-server's API has no swap field at all, and the only alternative (the kubelet's own Summary API) needs a materially broader RBAC grant for a signal most kubelet versions don't even surface unless an off-by-default feature gate is on — dropping swap detection is a deliberate, documented trade-off once the pod is genuinely unpinned to any node, not an oversight; memory-availability pressure remains the dominant signal (the incident the values file's own comments were tuned from).
+- Reads cluster-wide memory-availability pressure via `kubectl top nodes`, taking the **worst-case (minimum)** available-memory percentage across the nodes runner pods may use, not an average — a pool average can look healthy while the specific node a new runner pod would actually land on is not, consistent with the algorithm's own bias toward lowering eagerly (lowering never disrupts in-flight jobs). With the node eligibility label set (`github_runner_arc_node_label_key`, passed as `AUTOSCALER_NODE_SELECTOR`) only nodes carrying it are read, otherwise every node: a node outside the label runs no runner pod, so its pressure says nothing about where the next one lands. This is a genuine improvement over the pod's own earlier host-pinned design, which could only ever see one fixed machine's pressure via `/proc/meminfo`, not the whole pool's. **No swap-pressure signal**: metrics-server's API has no swap field at all, and the only alternative (the kubelet's own Summary API) needs a materially broader RBAC grant for a signal most kubelet versions don't even surface unless an off-by-default feature gate is on — dropping swap detection is a deliberate, documented trade-off once the pod is genuinely unpinned to any node, not an oversight; memory-availability pressure remains the dominant signal (the incident the values file's own comments were tuned from).
 - Works out the ceiling, how many pooled runner pods the nodes can place now, from the live cluster (`scripts/autoscaler-ceiling.jq`). It reads every pooled profile's runner pod spec from its `AutoscalingRunnerSet` and takes the largest request among them, per resource, as the runner pod (a container that sets only a limit counts at its limit, as it will once its pod is created, and sidecars, init containers and pod overhead count as the scheduler counts them). A node counts when every pooled profile's runner pods could be placed on it now: it carries their `nodeSelector`, is `Ready`, is not cordoned, and their tolerations match each of its `NoSchedule` and `NoExecute` taints; a node that is not ready or unreachable is left out until it is back. On each, allocatable CPU and memory less the requests of the pods on it that are not pooled runner pods, divided by the runner pod's request, and its pod capacity (`allocatable.pods`) less those pods, give how many fit; the smallest of the three, rounded down, is the node's figure, and the ceiling is the sum. The pooled runner pods are left out so that runners already running never lower the ceiling they count towards; every other pod still holding requests counts, a non-pooled scale set's runners included. Node affinity is not read: the role gives an autoscaled profile none, and a pod one would keep off a counted node is caught as unschedulable below. `AUTOSCALER_MAX_CEILING` (`github_runner_arc_autoscaler_max_ceiling`, or, when exactly one pooled profile sets `sizing`, that profile's own derived figure) is an optional upper bound on it; empty leaves it unbounded. The ceiling worked out is kept in the status ConfigMap's `ceiling` key, and `status.json` reports `ceiling`, `ceiling_bound`, `ceiling_measured` and, per node, the runners it places or why it is not counted. When the nodes, the pods or a template cannot be read, or a figure in them is missing or is not a Kubernetes quantity, the poll prints why on stderr, keeps the last ceiling it worked out (or the bound, before it has one) for lowering and rebalancing, and does not raise the pool. Reading every node's pods needs `get`/`list` on pods cluster-wide, which the autoscaler's service account holds for this alone, as the heartbeat's does.
 - Raises the combined `maxRunners` by exactly +1 per cycle (never straight to a computed target) once headroom has covered a full pod's hard limit for `AUTOSCALER_RAISE_CONFIRM_POLLS` (default 2) consecutive polls, capped at the ceiling, and only in a poll that worked the ceiling out.
 - Lowers the combined `maxRunners` to the ceiling when the pooled total is above it, even when nothing else is wrong: each profile's own static `maxRunners` can sum to more than the ceiling, every Helm upgrade restores those, and the ceiling falls when a node goes, so the cap is enforced rather than only bounding raises.
@@ -171,7 +171,7 @@ Each poll (`AUTOSCALER_POLL_SECONDS`, default 45s):
 - Also collects the peak memory each job's runner measured for itself: the runner image's job-completed hook (`runner-hooks/job-completed.sh`, set through `ACTIONS_RUNNER_HOOK_JOB_COMPLETED`) reads the container cgroup's `memory.peak`, which a spike shorter than the poll cannot escape, and posts it as a `JobMemoryPeak` Event in the pod's namespace. The autoscaler reads those Events each poll and keeps the maximum per repository, workflow ref and job id under the status ConfigMap's `job-peak` key (`kubectl get configmap autoscaler-status -n github-runner-platform -o jsonpath='{.data.job-peak}'`), in MiB. Keeping the maximum means a forged low figure lowers nothing and re-reading an Event changes nothing. The hook never fails the job it ends. To post an Event the runner pods' service account, which otherwise holds no permission, gets `create` on `events` in its own namespace and nothing else (`github_runner_arc_record_job_peaks`, default on; `false` removes the grant). A job's code runs in the same container and can read the same token, so the worst it can do with that permission is post events in its own runner namespace.
 - Records, per job, the highest memory a runner pod running it has been seen at, in the status ConfigMap's `job-memory` key (`kubectl get configmap autoscaler-status -n github-runner-platform -o jsonpath='{.data.job-memory}'`), keyed by repository, workflow ref and job name with the peak in MiB, the number of observations and when it was last seen. It is sampled once per poll from the metrics server, so it is a lower bound that a spike shorter than the poll escapes; it shows how far below a pod's request jobs sit, which is what decides whether an accurate in-pod measurement (see the per-job sizing issue) is worth building. Entries unseen for 30 days are dropped, and a failure to keep the record never affects a capacity decision.
 - Optionally reads per-node pressure from the nodes' own `/proc/pressure` (`github_runner_arc_autoscaler_node_pressure_enabled`). A `psi-reporter` DaemonSet (`scripts/psi-reporter.sh`, in the autoscaler's image) runs on each node runner pods may use, tolerating every taint, and patches that node's own key of one ConfigMap with the `avg10` of its io, cpu and memory pressure. The reading is not taken from the kubelet's summary API, because on a cgroup v1 node that reports every pressure field as 0 whatever the node is doing. The autoscaler lowers the combined `maxRunners` by exactly one each poll, never below the running count, while any node's I/O "full" stall is at or over `AUTOSCALER_IO_PRESSURE_PCT` (default 10) or, when set above 0, its cpu "some" stall is at or over `AUTOSCALER_CPU_PRESSURE_PCT`, and it does not raise the pool while any node has no report newer than three polls, since whether that node can take more is then unknown. This is the signal for a pool whose nodes differ: memory can have room on a node whose disk is already the limit.
-- Fails safe on any memory measurement error (`kubectl top nodes` or the metrics API's pod list failing, or a figure that cannot be parsed), but not on a pod that merely has no metrics yet: lowers the combined total toward `AUTOSCALER_FLOOR` (`github_runner_arc_autoscaler_floor`) immediately rather than skipping the cycle silently. A failure reading the scale sets' own state or their pods exits non-zero instead, since fail-safe needs that state to lower anything and the API server is then unreachable anyway. This combined floor need not equal any one profile's own values-file `maxRunners`, since each profile's Helm upgrade reverts to its own static floor independently of how the pool's combined floor is set.
+- Fails safe on any memory measurement error (`kubectl top nodes` or the metrics API's pod list failing, or a figure that cannot be parsed), but not on a pod that merely has no metrics yet, nor on a node among those read that prints `<unknown>` as its MEMORY% (NotReady, its kubelet no longer reporting), which is logged by name and left out of the pressure calculation so the worst case comes from the nodes that did report (it does fail safe when no node reported a percentage, or when a MEMORY% is neither `<unknown>` nor a number): lowers the combined total toward `AUTOSCALER_FLOOR` (`github_runner_arc_autoscaler_floor`) immediately rather than skipping the cycle silently. A failure reading the scale sets' own state or their pods exits non-zero instead, since fail-safe needs that state to lower anything and the API server is then unreachable anyway. This combined floor need not equal any one profile's own values-file `maxRunners`, since each profile's Helm upgrade reverts to its own static floor independently of how the pool's combined floor is set.
 
 Because the ceiling is the tighter of CPU and memory on each node, shrinking the runner pod's CPU request raises it only on nodes where CPU is what decides. Halving a 2 CPU, 7Gi pod to 1 CPU was measured on a two-node pool and not taken: memory then decided on both nodes, so it bought one runner (a ceiling of 5 became 6), while every job that keeps more than one core busy would be throttled to one, and the autoscaler records memory only, so there was no CPU history to weigh that against. Revisit it when CPU history exists (the node pressure reporter's cpu figures, or a per-job CPU record), when one more runner is worth that throttling, or when a node with too few cores for the larger pod needs a 1 CPU pod to take work.
 

scripts/autoscaler.sh

@@ -359,6 +359,7 @@ done
 
 # Cluster-wide memory-availability pressure via `kubectl top nodes` (metrics-server), not a single node's own /proc/meminfo - the autoscaler pod is genuinely unpinned now (see docker-compose.yml's own removal of host pinning), so a single-node reading would only ever reflect wherever the scheduler happened to place THIS pod, not the pool as a whole. Takes the WORST-CASE (minimum) available-memory percentage across the nodes it reads (those matching AUTOSCALER_NODE_SELECTOR, or every node when it is empty), not an average: a pool average can look healthy while the specific node a new runner pod would actually land on is not - consistent with the algorithm's own existing bias toward lowering eagerly, since lowering never disrupts in-flight jobs. No swap-pressure signal: metrics-server's API has no swap field at all (confirmed against its type, not just "probably not"), and the only alternative (the kubelet's own Summary API) needs a materially broader RBAC grant - node/proxy, effectively "reach anything a kubelet exposes" - for a signal most kubelet versions don't even surface unless the off-by-default NodeSwap feature gate is on. Real cluster-wide swap visibility needs Prometheus/node-exporter, which this fleet doesn't run and which would be genuine scope creep to add here - dropping swap detection is a deliberate, documented trade-off, not an oversight.
 # Only the nodes runner pods may be placed on, when the role restricts them by label: a node they cannot use says nothing about where the next one lands, and one whose kubelet is unreachable reports no metrics, which would otherwise fail safe on every poll it is down.
+# A node among them whose kubelet has stopped reporting (NotReady) is printed with `<unknown>` as its MEMORY%: it has no memory reading to contribute, so it is skipped with a log line and the worst case comes from the nodes that did report, instead of holding every pool at its floor for as long as the node stays down. The fail-safe still applies to a MEMORY% that is neither `<unknown>` nor a number, and to the case that no node reported a percentage at all, which would otherwise read as no pressure.
 TOP_NODES_ARGS=(top nodes --no-headers)
 [ -n "$NODE_SELECTOR" ] && TOP_NODES_ARGS+=(-l "$NODE_SELECTOR")
 # kubectl's own error is kept for the message, since its exit status alone does not say which failure it was: "Metrics API not available" when API discovery lists no usable metrics.k8s.io version (not registered, or registered but marked unavailable), the API server's own error when the list request itself fails, and "metrics not available yet" when the list succeeds with no node in it, which is what a selector matching no node, or matching only nodes whose kubelets are down, comes to.
@@ -371,12 +372,19 @@ rm -f "$TOP_NODES_ERR_FILE"
   || fail_safe "kubectl top nodes${NODE_SELECTOR:+ -l ${NODE_SELECTOR}} failed (${TOP_NODES_ERR% }): the metrics API is missing or unreachable, or no node${NODE_SELECTOR:+ matching the selector} reports metrics"
 [ -n "$TOP_NODES_OUTPUT" ] || fail_safe "kubectl top nodes${NODE_SELECTOR:+ -l ${NODE_SELECTOR}} returned no nodes"
 mem_available_pct=100
-while read -r _name _cpu _cpu_pct _mem mem_pct; do
+reporting_nodes=0
+while read -r node_name _cpu _cpu_pct _mem mem_pct; do
   mem_pct="${mem_pct%\%}"
+  if [ "$mem_pct" = "<unknown>" ]; then
+    echo "Skipping node ${node_name}: kubectl top nodes has no memory reading for it (its kubelet is not reporting), so it is left out of the pressure calculation"
+    continue
+  fi
   case "$mem_pct" in ''|*[!0-9]*) fail_safe "could not parse a node's own MEMORY% from kubectl top nodes ('$mem_pct')" ;; esac
+  reporting_nodes=$(( reporting_nodes + 1 ))
   node_available_pct=$(( 100 - mem_pct ))
   [ "$node_available_pct" -lt "$mem_available_pct" ] && mem_available_pct=$node_available_pct
 done <<< "$TOP_NODES_OUTPUT"
+[ "$reporting_nodes" -gt 0 ] || fail_safe "no node reported a MEMORY% from kubectl top nodes (every row was <unknown>)"
 
 pressure=false
 if [ "$mem_available_pct" -lt "$MEM_AVAILABLE_PRESSURE_PCT" ]; then