public_git/actions-runner-controller
1
0
Fork 0
mirror of https://github.com/actions-runner-controller/actions-runner-controller.git synced 2026-09-30 20:41:52 +02:00
Code Issues Projects Releases Wiki Activity

Make the generation tests observe the states they claim to cover

Browse Source 
The specs added with the observed-generation work asserted on end states
only. Both the listener rebuild and a stalled reconcile pass through a
transient phase, and nothing looked at it: the tests waited for the
rebuilt listener and then checked for Running. Removing the pending update
that guards the rebuild left them green, which was pointed out in review
and is true — I checked by deleting that update and re-running, and they
passed.

The window was invisible because it closes on its own between two polls.
Hold it open instead: the AutoscalingListener controller does not run in
this suite, so a finalizer added by the test keeps the deleted listener
around until the test removes it. That turns a race into a state the test
can sit on and assert against.

The label spec now waits for the listener to carry a deletion timestamp
while the scale set reports Pending, holds that to show it is not a blip,
then releases the finalizer and checks the listener comes back with a new
UID and the scale set returns to Running. With the pending update removed
it now fails on the phase, which is the point.

The same trick gives the failure path its first coverage. A max runners
change bumps the generation and forces a listener rebuild, so blocking the
rebuild stalls the reconcile part way through. The new spec pins what the
contract claims: the observed generation stays behind the live generation
and the phase stays Pending for as long as the change cannot be applied,
and the marker only catches up once it can.

Also drops a stale comment that survived a merge and contradicted the code
next to it, still claiming label edits no longer reach the Pending phase.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Nikola Jokic
2026-09-10 22:56:47 +02:00
co-authored by Copilot App
parent 80a0a64f3f
commit b830b658d6
1 changed files with 161 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
controllers/actions.github.com/autoscalingrunnerset_controller_test.go
+161 -10
View File
@@ -523,14 +523,12 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() {
).Should(Succeed())
})
// metadata.generation only tracks spec writes, so a label-only edit no
// longer drags the scale set through the Pending phase the way the old
// label-inclusive hash did. Labels still propagate to the
// EphemeralRunnerSet, they just do not count as an update to apply.
// A label-only edit does not bump metadata.generation, so it is not an
// update to apply. It does still rebuild the listener, because the
// desired listener labels are derived from the AutoscalingRunnerSet's,
// and the phase has to report that.
// A label-only edit does not bump metadata.generation, so it never
// advances the observed generation. It does still rebuild the listener,
// because the desired listener labels are derived from the
// AutoscalingRunnerSet's, and the phase has to report that rebuild. So
// the scale set passes through Pending transiently and comes back to
// Running, while the observed generation stays exactly where it was.
It("does not re-observe a generation when only labels change, but still reports the listener rebuild", func() {
listener := new(v1alpha1.AutoscalingListener)
Eventually(
@@ -555,6 +553,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() {
autoscalingRunnerSetTestInterval,
).Should(Succeed(), "AutoscalingRunnerSet should settle with its generation observed")
// Hold the deletion window open. Without this the listener is deleted
// and re-created between two polls, so an implementation that never
// reported Pending would look identical to one that did. The
// AutoscalingListener controller does not run in this suite, so
// nothing else is going to clear this finalizer.
blockDeletion(listener)
defer unblockDeletion(listener)
patched := autoscalingRunnerSet.DeepCopy()
patched.Labels["arc.test/label-drift"] = "updated"
Expect(k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))).To(Succeed(), "failed to patch AutoScalingRunnerSet labels")
@@ -569,8 +575,39 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() {
autoscalingRunnerSetTestInterval,
).Should(Succeed())
// The listener is rebuilt to pick the new label up, so the scale set
// must not keep claiming to be running while that happens.
// The listener is on its way out, so the scale set must not claim to
// be running. This is the assertion that fails if the Pending update
// before the listener delete is removed.
Eventually(
func(g Gomega) {
current := new(v1alpha1.AutoscalingListener)
g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed())
g.Expect(current.DeletionTimestamp).NotTo(BeNil(), "listener should be marked for deletion to pick up the new label")
ars := new(v1alpha1.AutoscalingRunnerSet)
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), ars)).To(Succeed())
g.Expect(ars.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "scale set must report Pending while its listener is being rebuilt")
},
autoscalingRunnerSetTestTimeout,
autoscalingRunnerSetTestInterval,
).Should(Succeed())
// It stays Pending for as long as the rebuild has not happened, and
// the observed generation does not drift in the meantime.
Consistently(
func(g Gomega) {
ars := new(v1alpha1.AutoscalingRunnerSet)
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), ars)).To(Succeed())
g.Expect(ars.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "scale set must stay Pending until the listener is back")
g.Expect(ars.Status.ObservedGeneration).To(Equal(settledGeneration))
},
3*time.Second,
autoscalingRunnerSetTestInterval,
).Should(Succeed())
// Let the rebuild finish.
unblockDeletion(listener)
Eventually(
func(g Gomega) {
current := new(v1alpha1.AutoscalingListener)
@@ -607,6 +644,84 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() {
).Should(Succeed())
})
// The observed generation is only meant to advance once a change has
// actually been applied. A reconcile that cannot finish must leave it
// behind the live generation, so the work is retried rather than being
// mistaken for settled.
It("leaves the observed generation behind until the reconcile completes", func() {
listener := new(v1alpha1.AutoscalingListener)
Eventually(
func() error {
return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener)
},
autoscalingRunnerSetTestTimeout,
autoscalingRunnerSetTestInterval,
).Should(Succeed(), "Listener should be created")
var settledGeneration int64
Eventually(
func(g Gomega) {
current := new(v1alpha1.AutoscalingRunnerSet)
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed())
g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning))
g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation))
settledGeneration = current.Generation
},
autoscalingRunnerSetTestTimeout,
autoscalingRunnerSetTestInterval,
).Should(Succeed(), "AutoscalingRunnerSet should settle with its generation observed")
// Stall the reconcile part way through: the max runner change below
// forces the listener to be rebuilt, and the rebuild cannot complete
// while the listener is held in deletion.
blockDeletion(listener)
defer unblockDeletion(listener)
patched := autoscalingRunnerSet.DeepCopy()
updatedMax := 20
patched.Spec.MaxRunners = &updatedMax
Expect(k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))).To(Succeed(), "failed to patch AutoScalingRunnerSet max runners")
var liveGeneration int64
Eventually(
func(g Gomega) {
current := new(v1alpha1.AutoscalingRunnerSet)
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed())
g.Expect(current.Generation).To(BeNumerically(">", settledGeneration), "a spec write must bump metadata.generation")
g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "the scale set should go Pending while the change is being applied")
liveGeneration = current.Generation
},
autoscalingRunnerSetTestTimeout,
autoscalingRunnerSetTestInterval,
).Should(Succeed())
Consistently(
func(g Gomega) {
current := new(v1alpha1.AutoscalingRunnerSet)
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed())
g.Expect(current.Status.ObservedGeneration).To(Equal(settledGeneration), "observed generation must not advance while the change is still being applied")
g.Expect(current.Status.ObservedGeneration).To(BeNumerically("<", liveGeneration))
g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "the scale set must stay Pending until the change is applied")
},
3*time.Second,
autoscalingRunnerSetTestInterval,
).Should(Succeed())
// Let the reconcile complete, and only now should the marker catch up.
unblockDeletion(listener)
Eventually(
func(g Gomega) {
current := new(v1alpha1.AutoscalingRunnerSet)
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed())
g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning))
g.Expect(current.Status.ObservedGeneration).To(Equal(liveGeneration), "observed generation must catch up once the change has been applied")
},
autoscalingRunnerSetTestTimeout,
autoscalingRunnerSetTestInterval,
).Should(Succeed())
})
It("updates EphemeralRunnerSet when the runner image changes without touching the Listener", func() {
listener := new(v1alpha1.AutoscalingListener)
Eventually(
@@ -2561,3 +2676,39 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde
})
})
})
// testHoldFinalizer keeps an AutoscalingListener around after it has been
// deleted, so a test can observe the window during which the scale set has no
// usable listener. The AutoscalingListener controller is not running in this
// suite, so nothing else adds or removes finalizers on these objects.
const testHoldFinalizer = "arc.test/hold-deletion"
func blockDeletion(listener *v1alpha1.AutoscalingListener) {
GinkgoHelper()
current := new(v1alpha1.AutoscalingListener)
Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(listener), current)).To(Succeed(), "failed to get listener to block its deletion")
original := current.DeepCopy()
Expect(controllerutil.AddFinalizer(current, testHoldFinalizer)).To(BeTrue(), "listener should not already hold the test finalizer")
Expect(k8sClient.Patch(context.Background(), current, client.MergeFrom(original))).To(Succeed(), "failed to add the test finalizer to the listener")
}
// unblockDeletion is idempotent so it can be deferred as a safety net and still
// be called explicitly at the point a test wants the rebuild to proceed.
func unblockDeletion(listener *v1alpha1.AutoscalingListener) {
GinkgoHelper()
current := new(v1alpha1.AutoscalingListener)
err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(listener), current)
if errors.IsNotFound(err) {
return
}
Expect(err).NotTo(HaveOccurred(), "failed to get listener to unblock its deletion")
original := current.DeepCopy()
if !controllerutil.RemoveFinalizer(current, testHoldFinalizer) {
return
}
Expect(k8sClient.Patch(context.Background(), current, client.MergeFrom(original))).To(Succeed(), "failed to remove the test finalizer from the listener")
}