chore: Added OwnerReferences during resource creation for EphemeralRunnerSet, EphemeralRunner, and EphemeralRunnerPod

[kahirokunn]

Remove the "argocd.argoproj.io/instance" label from resources created by the controller. This prevents ArgoCD from pruning and deleting these resources when syncing.

[kahirokunn]

@nikola-jokic Hi!
This issue occurred in the following PR released in 0.9.2.
https://github.com/actions/actions-runner-controller/pull/3157/files

[rarecrumb]

@kahirokunn do you think it's also a good idea to add ownerReferences?

This would make the AutoscalingListener show up in ArgoCD as a child resource of the AutoscalingRunnerSet

  ownerReferences:
    - apiVersion: actions.github.com/v1alpha1
      blockOwnerDeletion: true
      controller: true
      kind: AutoscalingRunnerSet
      name: self-hosted

Inside of ObjectMeta:

actions-runner-controller/controllers/actions.github.com/resourcebuilder.go

Line 106 in 8ff50ee

ObjectMeta: metav1.ObjectMeta{

Just add(?):

OwnerReferences: []metav1.OwnerReference{
    {
	    APIVersion:         "actions.github.com/v1alpha1",
	    Kind:               "AutoscalingRunnerSet",
	    Name:               autoscalingRunnerSet.Name,
	    Controller:         boolPtr(true),
	    BlockOwnerDeletion: boolPtr(true),
    },
}

[kahirokunn]

@rarecrumb think adding ownerReferences is a good idea.

[mhmtsvr]

This issue still persists in 0.9.3. Is it planned to be fixed in 0.9.4? @lucasfcnunes @kahirokunn @rarecrumb

[kahirokunn]

I hope so .

[lucasfcnunes]

This issue still persists in 0.9.3. Is it planned to be fixed in 0.9.4?

@mhmtsvr @kahirokunn

You need to add the prefixes you want to exclude from propagating.

actions-runner-controller/charts/gha-runner-scale-set-controller/values.yaml

Lines 125 to 132 in a62ca3d

	## Defines a list of prefixes that should not be propagated to internal resources.
	## This is useful when you have labels that are used for internal purposes and should not be propagated to internal resources.
	## See https://github.com/actions/actions-runner-controller/issues/3533 for more information.
	##
	## By default, all labels are propagated to internal resources
	## Labels that match prefix specified in the list are excluded from propagation.
	# excludeLabelPropagationPrefixes:
	# - "argocd.argoproj.io/instance"

[kahirokunn]

@lucasfcnunes Thx. 🙏
#3575 (comment)

[MarshallOfSound]

Would love to see this merged, deploying via argo is a bit funky at the moment without this patch

[rarecrumb]

@lucasfcnunes bump

[dntosas]

@lucasfcnunes bump also ^_^

[thomaschaplin]

Bump 🙏🏼

[zavertiaev]

The release happened, but this PR wasn't included :(

[kahirokunn]

Oh...

[Nastaliss]

@Link- Any chance of seing this PR included to an upcoming release ?

[kahirokunn]

@nikola-jokic Hi 😢

[nikola-jokic]

Hey @kahirokunn, sorry for the delay!

Thinking about this change, we might want to do this for all resources. For example, EphemeralRunnerSet is owned by the AutoscalingRunnerSet, then each EphemeralRunner is owned by the EphemeralRunnerSet, and each runner pod is owned by the EphemeralRunner. Furthermore, we should also set the UID.

One suggestion, can we use GroupVersionKind.String() instead of hardcoding it?

Would you be interested in applying these changes and testing them out?

[kahirokunn]

Hi @nikola-jokic,

Thank you for your suggestion! I've implemented the changes and tested them out, but I've encountered an important limitation with Kubernetes OwnerReferences.

The Issue

OwnerReferences in Kubernetes cannot cross namespace boundaries. In the current architecture:

• AutoscalingRunnerSet exists in user namespaces
• AutoscalingListener is created in the controller's namespace (ControllerNamespace)

This means we can't establish a proper OwnerReference between these resources across different namespaces.

Verification

I've built and tested a custom image with these changes (published at quay.io/kahirokunn/gha-runner-scale-set:0.10.1) and documented the integration with ArgoCD here: https://dev.to/kahirokunn/actions-runner-controller-and-argocd-integration-guide-3a81
The test confirms that the cross-namespace OwnerReference doesn't work as expected.

Possible Solutions

I see three potential approaches to address this:

1. Make AutoscalingRunnerSet cluster-scoped: This would allow it to own resources across namespaces, but it's a significant architectural change.

2. Create AutoscalingListener in the same namespace as AutoscalingRunnerSet: This would enable proper OwnerReferences, but would require changing how the controller manages these resources.

3. Don't use OwnerReferences for cross-namespace relationships: Instead, use labels or other mechanisms to track relationships between resources in different namespaces.

I believe option 2 or 3 might be more practical for the current architecture. Option 2 would require changing where AutoscalingListener is created, while option 3 would maintain the current design but use alternative tracking mechanisms.

What are your thoughts on these approaches? I'm happy to continue working on this based on your preferred direction.

Best regards,
Kahiro

[nikola-jokic]

Hey @kahirokunn,

First of all, I want to thank you for doing all this hard work! Since the owner reference has a namespace limitation, and we have to create resources in different namespaces, I'm wondering if this comment from ArgoCD maintainer would solve it without setting the owner reference completely.

I have also created a PR allowing custom annotations which should be applied to top level resources: #3934.
Based on the previous work done here, custom annotations should resolve the issue.

As a followup, with the namespace issue, the only thing we can do properly that would make sense now is set the listener pod owner to the AutoscalingListener, and the runner pod owner to the ephemeral runner. If we proceed with that approach, we should probably set the owner reference to all resources created for the AutoscalingListener, and the EphemeralRunner.

Right now, can you please let us know if the custom annotations/labels is good enough approach?

[kahirokunn]

Hi @nikola-jokic ,

Yes, that approach works perfectly for me. Regarding the owner reference, I want to point out that everything has been fully implemented, tested, and verified in this PR. I believe all necessary preparations for merging are complete. You can reproduce the behavior by following the guide here: https://dev.to/kahirokunn/actions-runner-controller-and-argocd-integration-guide-3a81.

I've been working on this since the early stages, so I'd really appreciate it if you could consider merging my changes related to the OwnerReference. It would mean a lot to me!

Thanks for your consideration,

Best regards,
Kahiro

[foyerunix]

Hello @kahirokunn,

I'm happy to see that my code helped you improve your own PR.
May I suggest you to also use ptr.To from k8s.io/utils/ptr (https://pkg.go.dev/k8s.io/utils/ptr#To) rather than your had-hoc function ?

Regarding this solution:

"Create AutoscalingListener in the same namespace as AutoscalingRunnerSet: This would enable proper OwnerReferences, but would require changing how the controller manages these resources."

I can confirm that it run without any issue since two months in my environment.

I hope to see a solution merged soon, I will keep my own PR (#3902) open until then. Feel free to inspire yourself from it if needed.

Best Regards.

[kahirokunn]

"Create AutoscalingListener in the same namespace as AutoscalingRunnerSet: This would enable proper OwnerReferences, but would require changing how the controller manages these resources."

#3575 (comment)

Yes, we have thought about this, but if users have set NetworkPolicy etc., an error will occur, so we think it will be a breaking change.
Therefore, we can either do it gradually or all at once, but we think that the most user-friendly and fastest way is to merge it as it is now and then do additional work in a phased release.

[nikola-jokic]

LGTM, thank you!

[kallang-vgw]

Hi @nikola-jokic,

Thank you for your suggestion! I've implemented the changes and tested them out, but I've encountered an important limitation with Kubernetes OwnerReferences.

The Issue

OwnerReferences in Kubernetes cannot cross namespace boundaries. In the current architecture:

• AutoscalingRunnerSet exists in user namespaces
• AutoscalingListener is created in the controller's namespace (ControllerNamespace)

This means we can't establish a proper OwnerReference between these resources across different namespaces.

Verification

I've built and tested a custom image with these changes (published at quay.io/kahirokunn/gha-runner-scale-set:0.10.1) and documented the integration with ArgoCD here: https://dev.to/kahirokunn/actions-runner-controller-and-argocd-integration-guide-3a81 The test confirms that the cross-namespace OwnerReference doesn't work as expected.

Possible Solutions

I see three potential approaches to address this:

1. Make AutoscalingRunnerSet cluster-scoped: This would allow it to own resources across namespaces, but it's a significant architectural change.
2. Create AutoscalingListener in the same namespace as AutoscalingRunnerSet: This would enable proper OwnerReferences, but would require changing how the controller manages these resources.
3. Don't use OwnerReferences for cross-namespace relationships: Instead, use labels or other mechanisms to track relationships between resources in different namespaces.

I believe option 2 or 3 might be more practical for the current architecture. Option 2 would require changing where AutoscalingListener is created, while option 3 would maintain the current design but use alternative tracking mechanisms.

What are your thoughts on these approaches? I'm happy to continue working on this based on your preferred direction.

Best regards, Kahiro

@kahirokunn I like option 2 the best. The same way the Kubernetes Deployment controller will create a replicaset in the same namespaces the deployment object, I feel like a AutoscalingListener that is created in response to an AutoscalingRunnerSet should be created in the same namespace. For all effective purposes, it is owned by the AutoscalingRunnerSet.