Files
actions-runner-controller/controllers/actions.github.com/helpers.go
T
Nikola JokicandCopilot App 41d05325c2 Make the Outdated phase revision-aware
An outdated runner used to flip the whole scale set into the Outdated
phase permanently, which tears down the listener and switches the scale
set off. That verdict outlived the runner spec it was about: a runner
busy with a job survives the revision cleanup that follows a spec
update, and only reports Outdated once the job finishes. The result was
that a freshly applied fix could be discarded by a runner that never ran
it.

Runners are now stamped with the actionable revision they were built
from. An Outdated runner whose revision is behind the applied revision is
considered stale: it is deleted so the scaling logic replaces it with one
built from the current spec, and it no longer contributes to the set's
phase. A runner at the current revision still marks the set Outdated, so
a genuinely bad spec is still surfaced.

Two supporting fixes:

  - patchAppliedActionableRevisionStatus now recomputes the phase in both
    directions. It only ever forced Running, so the early-return path
    could leave a stale Outdated behind.

  - The AutoscalingRunnerSet only tears down on an Outdated set once that
    set has applied its current actionable revision. Otherwise a spec
    update races the EphemeralRunnerSet controller and the teardown fires
    against a phase that predates the update.

Runners created before this change parse to revision 0, which matches
the zero value of AppliedActionableRevision, so they are treated as
current until a revision is actually applied.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2026-09-08 17:27:39 +02:00

126 lines
5.6 KiB
Go

package actionsgithubcom
import (
"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
corev1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
)
// ephemeralRunnerSetActionableSpecChanged reports whether the runner spec the
// EphemeralRunnerSet is running differs from the one derived from the
// AutoscalingRunnerSet, in a way that requires re-applying it to the runners.
//
// Semantic.DeepEqual is used rather than cmp.Equal or reflect.DeepEqual because
// it treats a nil slice/map as equal to an empty one. That matters here: most
// PodSpec collection fields carry omitempty, so a template containing an
// explicitly empty value (`env: []`) is dropped when the EphemeralRunnerSet is
// written and reads back as nil. A strict comparison would report drift on every
// single reconcile, bumping ActionableRevision each time and making the
// EphemeralRunnerSet controller delete every idle and pending runner, forever.
// Semantic also knows how to compare resource.Quantity, and unlike cmp.Equal it
// cannot panic on types with unexported fields.
func ephemeralRunnerSetActionableSpecChanged(current, desired *v1alpha1.EphemeralRunnerSet) bool {
if current == nil || desired == nil {
return current != desired
}
return !apiequality.Semantic.DeepEqual(current.Spec.EphemeralRunnerSpec, desired.Spec.EphemeralRunnerSpec)
}
func nextActionableRevision(current *v1alpha1.EphemeralRunnerSet) int64 {
if current == nil {
return 1
}
if current.Spec.ActionableRevision > current.Status.AppliedActionableRevision {
return current.Spec.ActionableRevision + 1
}
return current.Status.AppliedActionableRevision + 1
}
// ephemeralRunnerSetOutdatedForAppliedRevision reports whether the set is
// Outdated *because of the runner spec it is currently running*, which is the
// only situation in which the AutoscalingRunnerSet should tear the scale set
// down.
//
// The phase alone is not enough. Outdated is deliberately sticky: it survives in
// status while the outdated runners are collected, and it is only cleared once a
// new revision is applied. So between the moment the AutoscalingRunnerSet patches
// a new runner spec onto the set and the moment the EphemeralRunnerSet controller
// processes that patch, the set still reports Outdated for a spec that no longer
// exists. Tearing down there would discard the fix the user just applied, and the
// scale set would stay switched off until something else nudged it.
//
// Requiring the applied revision to have caught up with the spec revision closes
// that window: the verdict counts only once the set is running the current spec.
func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) bool {
if ephemeralRunnerSet == nil {
return false
}
return ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated &&
ephemeralRunnerSet.Status.AppliedActionableRevision >= ephemeralRunnerSet.Spec.ActionableRevision
}
// listenerPodSpecRequiresRecreation reports whether the live listener pod must be
// deleted and rebuilt to match the desired spec.
//
// DeepDerivative, not DeepEqual: the live pod carries a large number of fields
// the desired pod never sets, written by the API server and by admission
// (nodeName, dnsPolicy, schedulerName, securityContext, enableServiceLinks,
// the default tolerations, the kube-api-access-* projected volume and its mount,
// terminationMessagePath, imagePullPolicy, secret defaultMode, ...). DeepEqual
// would therefore report drift on every reconcile of every healthy listener and
// spin in a delete/create loop. It cannot be made to work by pre-populating the
// defaults either, since nodeName is scheduler-assigned and the access-token
// volume has a generated name. See TestListenerPodSpecRequiresRecreation.
//
// The cost of DeepDerivative is that it ignores empty values on the desired side,
// so a field being *removed* is invisible to it. For everything sourced from the
// user-facing template that is harmless: the AutoscalingRunnerSet controller
// compares the whole AutoscalingListener spec with cmp.Equal and deletes the
// listener outright, which takes the pod with it. Container ports are the
// exception, because they come from the --listener-metrics-addr controller flag
// rather than from any resource, so disabling metrics would otherwise leave the
// port on the pod forever. Comparing port length is enough: additions and value
// changes are already caught by DeepDerivative, only removal is blind. The
// contents are deliberately not compared, because the API server defaults
// protocol to TCP and that would reintroduce the delete/create loop.
func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool {
if current == nil || desired == nil {
return current != desired
}
if listenerContainerPortsRemoved(current, desired) {
return true
}
return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec)
}
func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool {
for i := range desired.Spec.Containers {
desiredContainer := &desired.Spec.Containers[i]
currentContainer := findContainerByName(current.Spec.Containers, desiredContainer.Name)
if currentContainer == nil {
// A container the live pod does not have at all is drift that
// DeepDerivative already reports; nothing to decide here.
continue
}
if len(desiredContainer.Ports) < len(currentContainer.Ports) {
return true
}
}
return false
}
func findContainerByName(containers []corev1.Container, name string) *corev1.Container {
for i := range containers {
if containers[i].Name == name {
return &containers[i]
}
}
return nil
}