Replace integrity-hash annotations with explicit API state fields

The `actions.github.com/integrity-hash` annotation was used as an opaque
fingerprint to detect spec drift across AutoscalingRunnerSet,
EphemeralRunnerSet and the listener resources. Hashes are brittle: they
change whenever unrelated serialization details change, they are invisible
to users, and they are not restart-safe. FNV-32a also carries a real
collision risk, where the consequence is an update silently never applied.

Replace it with explicit, typed state:

- `AutoscalingRunnerSetStatus.ObservedGeneration` drives the Pending phase
  transition via `metadata.generation` instead of an annotation hash.
- `EphemeralRunnerSetSpec.ActionableRevision` and
  `EphemeralRunnerSetStatus.AppliedActionableRevision` form a restart-safe
  applied marker. The revision is bumped by the AutoscalingRunnerSet
  controller when `EphemeralRunnerSpec` changes, and only advanced in status
  after idle/pending runner cleanup succeeds.
- `EphemeralRunnerSetStatus.FinishedRunnerCleanupPatchID` records the
  listener patch ID for which finished runners were reaped, so scale-up is
  suppressed until the listener publishes a fresh desired state. This
  prevents creating a replacement runner for a job that already completed.
- Listener pod recreation compares pod specs semantically instead of
  comparing hash annotations.

Drift detection uses `apiequality.Semantic`, not `cmp` or `reflect`:

- `Semantic.DeepEqual` for the EphemeralRunnerSpec. 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 reports drift on every reconcile,
  bumping ActionableRevision each time and deleting every idle and pending
  runner, forever. Semantic treats nil and empty as equal, understands
  resource.Quantity, and cannot panic on unexported fields the way cmp can.
  It is also roughly six times cheaper than cmp.Equal on a realistic spec.
- `Semantic.DeepDerivative` for the listener pod, because the live pod
  carries many fields the desired pod never sets (nodeName, dnsPolicy,
  default tolerations, the kube-api-access volume, ...). DeepEqual there
  would spin in a delete/create loop. Container port length is checked
  separately, since ports come from the --listener-metrics-addr flag rather
  than from a resource, so disabling metrics would otherwise leave the port
  on the pod forever.

Drift detection is deliberately not short-circuited on
metadata.generation. Re-registration changes the runner scale set ID
through an annotation, and metadata changes do not bump generation, so a
generation-based shortcut would leave the EphemeralRunnerSet pointing at a
scale set that no longer exists. The measured saving did not justify the
risk.

Additionally:

- Count deleting runners toward the scale-up total so terminating runners
  are not double-replaced.
- Cleanup of finished runners is no longer deferred; failures now surface as
  reconcile errors instead of being logged and swallowed.
- Status patches for the new fields use `RetryOnConflict` against a freshly
  read object.
- Keep merging EphemeralRunnerSet annotations and labels rather than
  overwriting them, so metadata applied by admission webhooks or other
  controllers is preserved. Drift detection compares against the merge
  result so foreign keys cannot cause a permanent patch loop.
- Add unit tests and benchmarks for both drift checks, including a guard
  that fails if the listener comparison is ever tightened to DeepEqual.
- Cover the re-registration path, which previously had no assertion that
  the new runner scale set ID reaches the EphemeralRunnerSet at all.
This commit is contained in:
Nikola Jokic
2026-09-07 22:08:17 +02:00
parent 54147cfa5e
commit e481f69cff
21 changed files with 1724 additions and 250 deletions
@@ -35,6 +35,7 @@ import (
kerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/util/retry"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -133,11 +134,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, nil
}
// If hash spec has changed, delete idle ephemeral runners
// in order to apply the change to the runners that did not yet receive a job.
ephemeralRunnerIntegrityHash := ephemeralRunnerSetIntegrityHash(&ephemeralRunnerSet)
if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != ephemeralRunnerIntegrityHash {
log.Info("EphemeralRunnerSpec has changed, deleting idle ephemeral runners to apply the new spec")
if ephemeralRunnerSet.Spec.ActionableRevision > ephemeralRunnerSet.Status.AppliedActionableRevision {
log.Info(
"EphemeralRunnerSpec revision has changed, deleting idle or pending ephemeral runners to apply the new spec",
"specActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision,
"statusAppliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision,
)
if _, err := r.cleanUpEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil {
log.Error(err, "Failed to clean up EphemeralRunners")
return ctrl.Result{}, err
@@ -148,18 +150,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, err
}
log.Info("Updating EphemeralRunnerSet with new spec hash")
original := ephemeralRunnerSet.DeepCopy()
if ephemeralRunnerSet.Annotations == nil {
ephemeralRunnerSet.Annotations = make(map[string]string)
}
ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerIntegrityHash
if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil {
log.Error(err, "Failed to update ephemeral runner set with new spec hash")
if err := r.patchAppliedActionableRevisionStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.ActionableRevision); err != nil {
log.Error(err, "Failed to update EphemeralRunnerSet applied actionable revision status")
return ctrl.Result{}, err
}
log.Info("Updated ephemeral runner set with new spec hash")
log.Info("Updated EphemeralRunnerSet applied actionable revision status", "appliedActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision)
return ctrl.Result{}, nil
}
@@ -205,15 +201,31 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
total := ephemeralRunnersByState.scaleTotal()
if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != ephemeralRunnersByState.latestPatchID {
defer func() {
if len(ephemeralRunnersByState.finished) > 0 {
if err := r.cleanupFinishedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil {
log.Error(err, "failed to cleanup finished ephemeral runners")
return ctrl.Result{}, err
}
}()
log.Info("Scaling comparison", "current", total, "desired", ephemeralRunnerSet.Spec.Replicas)
if err := r.patchFinishedRunnerCleanupPatchIDStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.PatchID); err != nil {
log.Error(err, "failed to update finished runner cleanup patch ID status")
return ctrl.Result{}, err
}
ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID = ephemeralRunnerSet.Spec.PatchID
log.Info("Finished ephemeral runners were cleaned up, deferring scaling decision")
return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log)
}
scaleUpTotal := total + len(ephemeralRunnersByState.deleting)
log.Info("Scaling comparison", "current", total, "deleting", len(ephemeralRunnersByState.deleting), "desired", ephemeralRunnerSet.Spec.Replicas)
switch {
case total < ephemeralRunnerSet.Spec.Replicas: // Handle scale up
count := ephemeralRunnerSet.Spec.Replicas - total
case scaleUpTotal < ephemeralRunnerSet.Spec.Replicas: // Handle scale up
if ephemeralRunnerSet.Spec.PatchID > 0 && ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID {
log.Info("Skipping scale up until listener publishes a fresh desired state after finished runner cleanup", "patchID", ephemeralRunnerSet.Spec.PatchID)
return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log)
}
count := ephemeralRunnerSet.Spec.Replicas - scaleUpTotal
log.Info("Creating new ephemeral runners (scale up)", "count", count)
if err := r.createEphemeralRunners(ctx, &ephemeralRunnerSet, count, log); err != nil {
log.Error(err, "failed to make ephemeral runner")
@@ -245,6 +257,51 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log)
}
func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error {
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
var latest v1alpha1.EphemeralRunnerSet
if err := r.Get(ctx, key, &latest); err != nil {
return err
}
original := latest.DeepCopy()
latest.Status.AppliedActionableRevision = targetAppliedRevision
ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList)
if err := r.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace), client.MatchingFields{resourceOwnerKey: latest.Name}); err != nil {
return fmt.Errorf("failed to list child ephemeral runners: %w", err)
}
if len(newEphemeralRunnersByStates(ephemeralRunnerList).outdated) == 0 {
latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning
}
if original.Status == latest.Status {
return nil
}
return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
})
}
func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error {
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
var latest v1alpha1.EphemeralRunnerSet
if err := r.Get(ctx, key, &latest); err != nil {
return err
}
original := latest.DeepCopy()
latest.Status.FinishedRunnerCleanupPatchID = patchID
if original.Status == latest.Status {
return nil
}
return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
})
}
func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error {
original := ephemeralRunnerSet.DeepCopy()
var phase v1alpha1.EphemeralRunnerSetPhase
@@ -257,7 +314,9 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer
phase = ephemeralRunnerSet.Status.Phase
}
desiredStatus := v1alpha1.EphemeralRunnerSetStatus{
Phase: phase,
Phase: phase,
AppliedActionableRevision: ephemeralRunnerSet.Status.AppliedActionableRevision,
FinishedRunnerCleanupPatchID: ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID,
}
// Update the status if needed.