You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
First of two PRs that split #4582 apart. #4580 has merged, so this now sits on master.
af55427f in #4582 bundled two unrelated things: a behavioural change to who
owns the Running phase, and a workqueue optimisation. This PR is the
behavioural half. The predicates follow separately so that an optimisation is
never reviewed together with a semantic change.
The problem
updateRunStatusFromPod derived EphemeralRunner.Status.Phase straight from
the pod phase, so a runner flipped to Running the moment its pod started,
whether or not it had picked up a job. Running therefore meant "the pod is
up", not "the runner is busy".
That matters for scale-down. newEphemeralRunnersByStates groups runners into pending and running, and deleteIdleEphemeralRunners walks pending first
and running second. Because every started runner was Running, the two
buckets carried no information about which runners were safe to remove.
Deriving the phase from the pod also let the controller overwrite a phase it
did not own. The assignment was an unconditional cast of pod.Status.Phase, so
if a job was assigned while the pod was still PodPending — ordinary, since the
listener can promote a runner before its pod finishes starting — the next
reconcile computed Pending, saw the phase had changed, and demoted the runner
back. JobStarted is delivered once and never retried, so that demotion was
permanent: a runner executing a job stayed Pending for the rest of its life.
The same cast could also write "Unknown" on a node partition, which is not one
of the declared phases and has no arm in the metrics switch.
The change
The listener already knows exactly when a job is assigned to a named runner, so
the transition moves there:
HandleJobStarted reads the runner first and only promotes it to Running
when it is not Failed, Succeeded or Outdated and is not being deleted.
The phase goes out in the same status merge patch as the job fields it
already writes.
The read and the patch are separated by a window in which the runner can reach
a terminal phase, so the promotion carries the resource version observed by
the read and the whole sequence runs under retry.RetryOnConflict. A runner
that changed underneath the read makes the patch fail with a conflict and the
operation is retried against fresh state rather than resurrecting a terminal
runner.
The listener role gains get on ephemeralrunners for that read, split out
from the existing ephemeralrunners/status rule so status keeps only patch.
updateRunStatusFromPod still publishes the initial Pending phase while the
pod starts, and no longer promotes to Running. It now sets Pending on any
empty phase rather than only when the pod is PodPending: the reconciler
returns early until the runner container status exists, by which time the pod
has usually already gone Running, so keying off PodPending would leave
runners phase-empty and therefore absent from the phase metrics entirely.
Guarding on the empty phase alone is sufficient, because Running and every
terminal phase are non-empty and so can never be overwritten.
Runners waiting for work now stay Pending, so scale-down drains genuinely idle
runners before ones executing a job.
Upgrade note
Runners created by a previous controller can already carry Running with no JobID, because the old code derived the phase from the pod. Those objects keep
that phase until they take a job or are deleted, which skews the phase metrics
for the remainder of their life. Behaviour is unaffected: both scale-down paths
decide with HasJob() rather than the phase, so such runners are still treated
as idle and reclaimed normally.
Testing
go test ./controllers/... ./cmd/... with envtest, all green — 96 of 96 specs,
none skipped. New coverage: TestHandleJobStarted in the scaler, including the
conflict-retry path and both cases where the runner has been deleted between the
read and the patch, plus envtest cases asserting the controller publishes Pending and holds it, does not set Running from pod status, and that
readiness still tracks the pod.
This guard only observes the reconciler's cached object. If that read sees an empty phase, the listener can patch Running before this pod reconcile writes its status; the subsequent merge patch then includes Pending and can overwrite the listener-owned Running phase because it is not an optimistic-lock patch. Protect this initial phase write with a resource-version/refetch strategy so the controller cannot regress a phase that the listener has already advanced.
The controller derived Status.Phase directly from the pod phase, so a runner
became Running as soon as its pod started, whether or not it had picked up a
job. That made Running mean "the pod is up" instead of "the runner is busy",
and it left the EphemeralRunnerSet scale-down path unable to tell an idle
runner from one that is executing a job.
The listener already knows when a job is assigned to a specific runner, so
move the transition there. HandleJobStarted now reads the runner first and
only promotes it to Running when it is not terminal (Failed, Succeeded or
Outdated) and not being deleted, then patches the phase alongside the job
fields it already writes. The listener role gains "get" on ephemeralrunners
for that read.
On the controller side updateRunStatusFromPod keeps publishing the initial
Pending phase while the pod is starting, and no longer promotes to Running.
Runners waiting for work now stay Pending, so scale-down picks them before
runners that are actually executing a job.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The scaler test stub ignored metadata.resourceVersion and never returned
409, so TestHandleJobStarted passed whether or not the optimistic lock and
RetryOnConflict existed. Enforce the precondition in the stub and add a
subtest that interleaves a terminal write between the GET and the patch,
asserting the retry records the job fields without restoring Running.
Also document that Running is owned by the listener and means a job has
been assigned, on the Phase field so the meaning reaches the generated
CRDs, and regenerate them.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Running now means a job has been assigned, so an online, registered but
idle runner stays Pending. The Pending comment claimed the phase meant
"not yet online", which that case contradicts.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Both NotFound branches in patchJobStarted log and return nil, so neither
produces any observable state change and neither had coverage. The GET
branch matters most: it returns nil from inside the RetryOnConflict
closure, so a wrong predicate there ends the retry loop as a success and
a job silently never reaches Running.
Assert against a recorded request log rather than the returned error,
since nil is equally consistent with the request being skipped, rejected
or retried. A positive control establishes that the recorder captures a
promotion when one is issued, so the absent and short logs are measured
absences rather than unasked questions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keying the initial Pending phase off pod.Status.Phase == PodPending left
the runner phase-empty for its entire life on the common path. Reconcile
returns early while the runner container status does not exist, which is
most of the pod's Pending window, so by the time updateRunStatusFromPod
is first reached the pod has usually already advanced to Running. The
previous direct cast of the pod phase hid this, because PodRunning then
produced Running; removing that promotion exposed it.
An empty phase is not merely cosmetic: publishEphemeralRunnerPhaseMetric
treats it as "stop tracking", so such a runner is absent from the phase
metrics, and it contradicts the documented contract that an idle
registered runner stays Pending.
Guarding on the empty phase alone is sufficient, since Running and every
terminal phase are non-empty and therefore cannot be overwritten.
The regression test asserted the defect as expected behaviour: it drove
the pod straight to Running and required the phase to stay empty. It now
requires Pending to appear and then hold, which is strictly stronger,
because an empty phase is also what a controller that never ran at all
would leave behind.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The listener now owns the Running transition, so Running means the runner
has been assigned a job rather than that its pod is running. Update the
metric help string to match.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An object written by the pre-change controller can already have Status.Phase=Unknown after a PodUnknown event. Because this preserves every non-empty phase, an idle runner that later recovers remains in this undeclared phase indefinitely, and metrics.updateEphemeralRunner has no Unknown case, so it is omitted from phase metrics. Handle this legacy value as Pending (or perform an explicit migration) rather than only checking for empty.
Once the listener owns the Running transition, a registered runner that
is waiting for work stays in the Pending phase instead of being promoted
when its pod starts. The pending_ephemeral_runners gauge therefore counts
both runners that Kubernetes has not finished starting and runners that
are idle, so its help text and the Grafana dashboard notes no longer
describe what it measures.
Reword both to say "not been assigned a job", matching the wording
already used for running_ephemeral_runners, and correct the dashboard's
provisioning-health note: with a non-zero minRunners the gauge no longer
falls to zero, so a non-zero value is not on its own evidence of
scheduling trouble. The comparison against pods actually in a pending
state is what carries the signal, so keep that and drop the claim that
the two values typically match.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Letting the listener own the Running transition redefines what two of the
gauges described in this ADR count. A registered runner waiting for work
now stays Pending instead of being promoted when its pod starts, so
running_ephemeral_runners counts job assignment rather than pod liveness,
and pending_ephemeral_runners covers idle runners as well as ones whose
pod has not finished starting.
Correct both bullets in place and match the wording to the Help strings
in controllers/actions.github.com/metrics/metrics.go so the two can be
diffed against each other. The decision this ADR records -- to expose
these metrics -- is unchanged, so this is a referential correction rather
than a supersede: no status change and no footnote.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of two PRs that split #4582 apart. #4580 has merged, so this now sits on
master.af55427fin #4582 bundled two unrelated things: a behavioural change to whoowns the
Runningphase, and a workqueue optimisation. This PR is thebehavioural half. The predicates follow separately so that an optimisation is
never reviewed together with a semantic change.
The problem
updateRunStatusFromPodderivedEphemeralRunner.Status.Phasestraight fromthe pod phase, so a runner flipped to
Runningthe moment its pod started,whether or not it had picked up a job.
Runningtherefore meant "the pod isup", not "the runner is busy".
That matters for scale-down.
newEphemeralRunnersByStatesgroups runners intopendingandrunning, anddeleteIdleEphemeralRunnerswalkspendingfirstand
runningsecond. Because every started runner wasRunning, the twobuckets carried no information about which runners were safe to remove.
Deriving the phase from the pod also let the controller overwrite a phase it
did not own. The assignment was an unconditional cast of
pod.Status.Phase, soif a job was assigned while the pod was still
PodPending— ordinary, since thelistener can promote a runner before its pod finishes starting — the next
reconcile computed
Pending, saw the phase had changed, and demoted the runnerback.
JobStartedis delivered once and never retried, so that demotion waspermanent: a runner executing a job stayed
Pendingfor the rest of its life.The same cast could also write
"Unknown"on a node partition, which is not oneof the declared phases and has no arm in the metrics switch.
The change
The listener already knows exactly when a job is assigned to a named runner, so
the transition moves there:
HandleJobStartedreads the runner first and only promotes it toRunningwhen it is not
Failed,SucceededorOutdatedand is not being deleted.The phase goes out in the same status merge patch as the job fields it
already writes.
a terminal phase, so the promotion carries the resource version observed by
the read and the whole sequence runs under
retry.RetryOnConflict. A runnerthat changed underneath the read makes the patch fail with a conflict and the
operation is retried against fresh state rather than resurrecting a terminal
runner.
getonephemeralrunnersfor that read, split outfrom the existing
ephemeralrunners/statusrule sostatuskeeps onlypatch.updateRunStatusFromPodstill publishes the initialPendingphase while thepod starts, and no longer promotes to
Running. It now setsPendingon anyempty phase rather than only when the pod is
PodPending: the reconcilerreturns early until the runner container status exists, by which time the pod
has usually already gone
Running, so keying offPodPendingwould leaverunners phase-empty and therefore absent from the phase metrics entirely.
Guarding on the empty phase alone is sufficient, because
Runningand everyterminal phase are non-empty and so can never be overwritten.
Runners waiting for work now stay
Pending, so scale-down drains genuinely idlerunners before ones executing a job.
Upgrade note
Runners created by a previous controller can already carry
Runningwith noJobID, because the old code derived the phase from the pod. Those objects keepthat phase until they take a job or are deleted, which skews the phase metrics
for the remainder of their life. Behaviour is unaffected: both scale-down paths
decide with
HasJob()rather than the phase, so such runners are still treatedas idle and reclaimed normally.
Testing
go test ./controllers/... ./cmd/...with envtest, all green — 96 of 96 specs,none skipped. New coverage:
TestHandleJobStartedin the scaler, including theconflict-retry path and both cases where the runner has been deleted between the
read and the patch, plus envtest cases asserting the controller publishes
Pendingand holds it, does not setRunningfrom pod status, and thatreadiness still tracks the pod.