Skip to content

Fix patch on autoscaling runner set when creating a runner scale set#4502

Open
nikola-jokic wants to merge 2 commits into
masterfrom
nikola-jokic/fix-ars-patch
Open

Fix patch on autoscaling runner set when creating a runner scale set#4502
nikola-jokic wants to merge 2 commits into
masterfrom
nikola-jokic/fix-ars-patch

Conversation

@nikola-jokic
Copy link
Copy Markdown
Collaborator

@nikola-jokic nikola-jokic commented May 21, 2026

No description provided.

Copilot AI review requested due to automatic review settings May 21, 2026 11:34
@github-actions
Copy link
Copy Markdown
Contributor

Hello! Thank you for your contribution.

Please review our contribution guidelines to understand the project's testing and code conventions.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CreateRunnerScaleSet behavior 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

  • createRunnerScaleSet takes a DeepCopy of the ARS before defaulting Spec.RunnerScaleSetName, then later patches using client.MergeFrom(original). This causes the controller to persist the defaulted spec.runnerScaleSetName back 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 {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants