Fix patch on autoscaling runner set when creating a runner scale set#4502
Fix patch on autoscaling runner set when creating a runner scale set#4502nikola-jokic wants to merge 2 commits into
Conversation
|
Hello! Thank you for your contribution. Please review our contribution guidelines to understand the project's testing and code conventions. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts AutoscalingRunnerSet reconciliation to better handle RunnerScaleSet creation/patching and to avoid stale AutoscalingListener references, with accompanying fake client and controller test updates.
Changes:
- Extend the multiclient fake to allow dynamic
CreateRunnerScaleSetbehavior for tests. - Update AutoscalingRunnerSet reconciliation logic to treat listeners as out-of-date when they reference a non-latest EphemeralRunnerSet, and refactor the patching approach during RunnerScaleSet creation.
- Update/add controller tests to validate stability (no listener churn) and align with the new fake client capabilities.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| controllers/actions.github.com/multiclient/fake/client.go | Adds a functional option to override CreateRunnerScaleSet behavior dynamically in tests. |
| controllers/actions.github.com/autoscalingrunnerset_controller.go | Refactors patching during RunnerScaleSet creation and tightens listener “out-of-date” detection/deletion logic. |
| controllers/actions.github.com/autoscalingrunnerset_controller_test.go | Updates fake wiring and adds a no-op stability test to ensure listeners aren’t recreated unnecessarily. |
Comments suppressed due to low confidence (1)
controllers/actions.github.com/autoscalingrunnerset_controller.go:483
createRunnerScaleSettakes a DeepCopy of the ARS before defaultingSpec.RunnerScaleSetName, then later patches usingclient.MergeFrom(original). This causes the controller to persist the defaultedspec.runnerScaleSetNameback to the API server, which is likely unintended since controllers typically shouldn’t mutate spec for defaulting (and the previous code path only patched metadata).
Consider avoiding in-place spec mutation (use a local fallback name), or move the original := ...DeepCopy() to just before patching and ensure it reflects any in-memory defaults you do not want to persist.
original := autoscalingRunnerSet.DeepCopy()
logger.Info("Creating a new runner scale set")
actionsClient, err := r.GetActionsService(ctx, autoscalingRunnerSet)
if len(autoscalingRunnerSet.Spec.RunnerScaleSetName) == 0 {
autoscalingRunnerSet.Spec.RunnerScaleSetName = autoscalingRunnerSet.Name
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| latestRunnerSet == nil || | ||
| listener.Spec.EphemeralRunnerSetName != latestRunnerSet.Name) { | ||
| log.Info("RunnerScaleSetListener is out of date. Deleting it so that it is recreated", "name", listener.Name) | ||
| if err := r.Delete(ctx, listener); err != nil { |
No description provided.