feat: add STARTUP_DELAY to entrypoint.sh

[js-timbirkett]

As a starter for #591

This is the absolute simplest way that I can think of to allow Istio to get itself sorted out before registering the runner with Github.

Any discussion or ideas would be appreciated.

[mumoshu]

@js-timbirkett Hey! Thanks for the PR. I think this is a good start. Before merging, would you mind adding short documentation on this use-case and how you would use this feature to enable it?

Perhaps you'd set this STARTUP_DELAY in the Runner.Spec.Env or RunnerDeployment.Spec.Template.Spec.Env to enable this feature, right?

[toast-gear]

I don't mind introducing an arbitrary wait option, it could be useful in various scenarios. Should this be done via an API change though? It would be overkill for this specific use case however we've done all runner config via the API so far. Whilst it would be simpler to just check for the arbitrary environment variable and leave the API out of it, it would be a marked change to how we have done everything so far. I'm a fan of convention over configuration.

@mumoshu what are you thoughts on if this should be an API change? It's a more complicated changed doing it that way and not strictly needed however it would be more consistent? (this comes across like I'm advocating a API change, I don't have a view which is better). Regardless docs are needed for this new feature.

[js-timbirkett]

Thanks @toast-gear - could you elaborate on "API Change"? do you mean by adding some sort of startupDelay to the Runner and RunnerDeployment specs?

[js-timbirkett]

In terms of Istio... there's a long-running thread: istio/istio#11130 - apparently they've fixed, or attempted to fix it in version 1.7 with a: holdApplicationUntilProxyStarts configuration option which injects the sidecar as the first container in the pod (and probably breaks anything expecting containers[0] to be the primary application container.

At the moment, I'm stuck on Istio 1.5 until all of our product teams redeploy all of their applications so this fairly simple change might be "just enough" and may become redundant in the future in my case.

[callum-tait-pbx]

Thanks @toast-gear - could you elaborate on "API Change"? do you mean by adding some sort of startupDelay to the Runner and RunnerDeployment specs?

Yup, @mumoshu will need to advise. It's overkill for the actual use case, it's more a design consistency thing but it may not be desired.

configuration option which injects the sidecar as the first container in the pod (and probably breaks anything expecting containers[0] to be the primary application container.

This would break many things in this project O_O

[js-timbirkett]

Thanks @callum-tait-pbx - I can also confirm that I'm running with this change in a custom built image. I'm also running scale to 0 on an actions-runner-controller built from master - because I enjoy the pain... I'll feedback any bugs or observations. This is a pretty good star.

[callum-tait-pbx]

If you experimenting with the scale from 0 (🙇 for that, it's always helpful having people beta test!) you should be aware that this should improve somewhat soon as GitHub are making some backend changes so we don't need to create a fake registration only runner #470

[mumoshu]

Should this be done via an API change though?

@callum-tait-pbx That's a good point and I took some time to think about that.

Well, probably we'd better mark it as a temporary feature that may change or go away?

As @js-timbirkett has kindly shared and summarized, recent versions of Istio seem to have the startup-delay container injector thing. So this feature is necessary only util everyone migrates to newer versions of Istio.

An API change persists longer and we can't easily remove already added field from e.g. Runner spec. So I'd like not to make API change for an experimental feature.

Thoughts?

configuration option which injects the sidecar as the first container in the pod (and probably breaks anything expecting containers[0] to be the primary application container.

This would break many things in this project O_O

I thought so too at first. But apparently, it doesn't break at all?

We refer to it as "the first container" only when we compose the runner pod spec. The mutating webhook of Istio comes in after that. And our controller looks for the container not by index but by name after the pod creation:

https://github.com/actions-runner-controller/actions-runner-controller/blob/cb60c1ec3baed8690d4546dacfdfdc564891e852/controllers/runner_controller.go#L260-L262

@js-timbirkett It would be awesome if you could try newer Istio with the holdApplicationUntilProxyStarts thing later! In theory. actions-runner-controller should just work fine with it.

[js-timbirkett]

Rebased on to master and pushed. See if that solves the broken workflows or not 🤔

[mumoshu]

I think any of the maintainers must approve it to run for the first contributor. I just approved so it will be run shortly

[mumoshu]

Added some note about this being experimental. The build seems to be failing due to that some shell variables not available to the workflow. Perhaps GitHub changed something on their end that we need to tweak the config somehow to fix? 🤔

[mumoshu]

Thanks a lot for your contribution @js-timbirkett ☺️