Skip to content

Conversation

@nikola-jokic
Copy link
Collaborator

During the delete, listener requeues after each cleanup step. This can take a long time on busy clusters.

This change aims to implement the same cleanup way we do with ephemeral runners (issuing multiple delete statements in a single loop). This way, cleanup should be much faster.

Fixes #4200

Copilot AI review requested due to automatic review settings October 28, 2025 11:52
Copy link
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 optimizes the deletion process for AutoscalingListener resources by eliminating unnecessary requeuing after each cleanup step. Instead of returning immediately after initiating each resource deletion (pod, secrets, role binding, role, service account), the controller now attempts to delete all resources in a single reconciliation loop and only requeues if any resources still exist.

Key Changes:

  • Modified cleanup logic to continue through all resource deletions instead of returning early after each one
  • Added a 1-second requeue delay when resources still exist to avoid tight reconciliation loops
  • Renamed return variable from done to requeue for clarity

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
}
return false, nil
requeue = true
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The requeue flag is set but the function doesn't return immediately, potentially overwriting this value. Since the pod still exists, you should return here to maintain the original behavior where subsequent cleanup steps are only attempted after the pod is deleted. Otherwise, this creates a logic change where all resources are attempted to be deleted regardless of pod status.

Suggested change
requeue = true
return true, nil

Copilot uses AI. Check for mistakes.
logger.Info("Listener service account is deleted")

return true, nil
return requeue, nil
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The requeue variable is uninitialized if all resources are already deleted (all get operations return NotFound). This will return false by default in Go, but it's safer to explicitly initialize requeue := false at the start of the function to make the intent clear and avoid potential issues if the function logic changes.

Copilot uses AI. Check for mistakes.
@nikola-jokic nikola-jokic added the gha-runner-scale-set Related to the gha-runner-scale-set mode label Oct 28, 2025
@nikola-jokic nikola-jokic merged commit 4d22089 into master Oct 29, 2025
31 of 34 checks passed
@nikola-jokic nikola-jokic deleted the nikola-jokic/speed-up-listener-removal branch October 29, 2025 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gha-runner-scale-set Related to the gha-runner-scale-set mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Listener pods gets removed when AutoscalingRunnerSet is patched

3 participants