fix(autoscaler): leave a NotReady node out of the memory check

.github/workflows/autoscaler-ceiling.yml

@@ -9,6 +9,7 @@ on:
     paths:
       - scripts/autoscaler.sh
       - scripts/autoscaler-ceiling.jq
+      - scripts/autoscaler-node.jq
       - roles/github_runner_arc/tasks/install_platform.yml
       - plugins/filter/arc.py
       - tests/autoscaler_ceiling/**

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, and when its kubelet drops out its missing metrics would otherwise fail safe on every poll. The same holds for a node inside the label that is not `Ready` (its kubelet stopped reporting or reported a fault): it is left out of this check by the definition the ceiling uses (`scripts/autoscaler-node.jq`), since it places no runner pod and `kubectl top nodes` prints `<unknown>` for it. 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 the node list failing, or a `Ready` node's figure that cannot be parsed), but not on a pod that merely has no metrics yet, nor on a node that is not `Ready`: 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.
 

autoscaler/Dockerfile

@@ -21,6 +21,7 @@ COPY scripts/autoscaler.sh /app/scripts/autoscaler.sh
 COPY scripts/autoscaler-ceiling.jq /app/scripts/autoscaler-ceiling.jq
 COPY scripts/autoscaler-usage.jq /app/scripts/autoscaler-usage.jq
 COPY scripts/autoscaler-quantity.jq /app/scripts/autoscaler-quantity.jq
+COPY scripts/autoscaler-node.jq /app/scripts/autoscaler-node.jq
 COPY scripts/psi-reporter.sh /app/scripts/psi-reporter.sh
 COPY scripts/node-cache-pruner.sh /app/scripts/node-cache-pruner.sh
 RUN chmod +x /app/loop.sh /app/scripts/autoscaler.sh /app/scripts/psi-reporter.sh /app/scripts/node-cache-pruner.sh

scripts/autoscaler-ceiling.jq

@@ -8,6 +8,8 @@
 
 # The quantity parsing shared with the usage program (scripts/autoscaler-quantity.jq), found through jq's -L, which the autoscaler points at its own directory.
 include "autoscaler-quantity";
+# Which nodes are Ready, shared with the autoscaler's memory check (scripts/autoscaler-node.jq).
+include "autoscaler-node";
 
 # A container's request for one resource. A container that sets only a limit is given that limit as its request when its pod is created, which a scale set's template has not been yet; a running pod already carries the request, so the fallback changes nothing there.
 def container_request($resource): (.resources.requests[$resource] // .resources.limits[$resource] // 0) | amount($resource);
@@ -33,9 +35,8 @@ def tolerates($tolerations; $taint):
 # Why a node cannot take a runner pod of the template $spec now, or "" when it can.
 def unplaceable($spec):
   (.metadata.labels // {}) as $labels
-  | ([.status.conditions[]? | select(.type == "Ready") | .status] | first) as $ready
   | if any(($spec.nodeSelector // {}) | to_entries[]; $labels[.key] != (.value | tostring)) then "does not carry the runner pods' nodeSelector"
-    elif $ready != "True" then "is not Ready"
+    elif is_ready | not then "is not Ready"
     elif .spec.unschedulable == true then "is cordoned"
     else [.spec.taints[]? | select(.effect == "NoSchedule" or .effect == "NoExecute") | select(tolerates($spec.tolerations // []; .) | not) | "has the taint \(.key):\(.effect)"] | first // ""
     end;

scripts/autoscaler-node.jq

@@ -0,0 +1,4 @@
+# What the autoscaler's jq programs and shell share about a Node object (scripts/autoscaler-ceiling.jq, and the memory check in scripts/autoscaler.sh), found through jq's -L, which the autoscaler points at its own directory, and pulled in with `include "autoscaler-node";`.
+
+# Whether the node's Ready condition is True. A node whose kubelet has stopped reporting (Unknown) or has reported a fault (False) is not Ready, and so is a node with no Ready condition at all: the scheduler places no pod on it and metrics-server has no figures for it, so it is left out of every count of where runner pods can go.
+def is_ready: ([.status.conditions[]? | select(.type == "Ready") | .status] | first) == "True";