Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate runner scale set name annotation to EphemeralRunner #3098

Merged

Conversation

ordovicia
Copy link
Contributor

@ordovicia ordovicia commented Nov 22, 2023

With this PR, AutoscalingRunnerSets' annotation that describes its runner scale set name will be propagated to EphemeralRunners.
This change would make it easy to inspect EphemeralRunners for troubleshooting.

This PR changes runner-scale-set-name annotation key to actions.github.com/runner-scale-set-name to align it with actions.github.com/runner-group-name.
The annotation is used only inside the controllers, but I am not sure if this change is acceptable.

@nikola-jokic
Copy link
Contributor

Hey @ordovicia,

Can you please help me understand what problem does this PR solve? We already have a label that specifies the scale set name.

@ordovicia
Copy link
Contributor Author

Yes, EphemeralRunner resources already have actions.github.com/scale-set-name label.
But its value is the name of the parent AutoscalingRunnerSet resource's name, not a GitHub runner scale set's name.

LabelKeyGitHubScaleSetName: autoscalingRunnerSet.Name,

Runner scale set name can be different from AutoscalingRunnerSet name when the user specifies spec.runnerScaleSetName field of the AutoscalingRunnerSet.

&actions.RunnerScaleSet{
Name: autoscalingRunnerSet.Spec.RunnerScaleSetName,

So, to resolve the runner scale set name for an EphemeralRunner, users need to first get its parent AutoscalingRunnerSet, and then read the AutoscalingRunnerSet's spec.runnerScaleSetName field.
This PR allows users to obtain the runner scale set name directly from an EphemeralRunner.

@ordovicia
Copy link
Contributor Author

Hi @nikola-jokic could you please take a look at my comment above? Thank you!

@nikola-jokic
Copy link
Contributor

Hey @ordovicia,

Sorry for the delay and thank you for the explanation. I don't think adding an annotation is the right approach here. If the purpose of the field is to query it, it should be a label. Also, we use annotations mostly to include some metadata, so they should be completely scoped to the cluster resources.
Now, what I would propose is to install two more labels, like actions.github.com/runner-scale-set-name and actions.github.com/runner-scale-set-group-name and include them only in a resource builder. Let me reach out to the team for a second opinion and I will get back to you ☺️.

@nikola-jokic nikola-jokic added the gha-runner-scale-set Related to the gha-runner-scale-set mode label Dec 4, 2023
@ordovicia
Copy link
Contributor Author

Thank you @nikola-jokic for the reply!
I agree that using labels is better for this purpose.
I look forward to hearing the team’s opinion.

Copy link
Contributor

@nikola-jokic nikola-jokic left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you and sorry for the delay!

@nikola-jokic nikola-jokic merged commit 4870658 into actions:master Mar 19, 2024
@ordovicia ordovicia deleted the runner-scale-set-name-annotation branch March 24, 2024 04:25
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.

2 participants