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

Ephemeral Runner: Can we make this optional? #457

Closed
thejasn opened this issue Apr 14, 2021 · 8 comments
Closed

Ephemeral Runner: Can we make this optional? #457

thejasn opened this issue Apr 14, 2021 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@thejasn
Copy link
Contributor

thejasn commented Apr 14, 2021

We run a few bazel jobs which need persistent workflows, have a workaround in place right now after looking at this.

Keeping a custom entrypoint.sh and overriding the entire script in the custom docker image feels more like an extra step. Plus any updates that happen on summerwind/actions-runner.entrypoint.sh will need to manually be updated in the copy.

Can we have an ENV variable or a command-line arg in entrypoint.sh which defaults to --once but can be overridden from the custom docker image? Or any other different mechanism to override this value?

Would be happy to contribute.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 16, 2021

@thejasn Hey! I agree this should be optional. My quick think says that, in the beginning, it should be enough to make it configurable via envvars in entrypoint.sh, like:

#snip

args=()
if [ "${EPHEMERAL}" = "false" ]; then
else
  args+=(--once)
fi

unset RUNNER_NAME RUNNER_REPO RUNNER_TOKEN
exec ./bin/runsvc.sh "${args[@]}"

But I can't promise there's no breaking change in the entrypoint.sh around this envvar name and the usage so sooner or later I'd like to add some API change to make it configurable via Runner Spec. Perhaps:

kind: RunnerDeployment:
spec:
  template:
    spec:
      ephemeral: false

@callum-tait-pbx
Copy link
Contributor

callum-tait-pbx commented Apr 17, 2021

actions/runner#510 it's worth noting that --once flag isn't considered stable or safe for production environments, it's probably worth doing it properly first time with an API change.

@thejasn
Copy link
Contributor Author

thejasn commented Apr 19, 2021

Hey @callum-tait-pbx @mumoshu,
I agree with doing it with the API change. Although not very familiar with custom controllers would be happy to contribute to this. Just had a few question w.r.t to the API change,

kind: RunnerDeployment:
spec:
  template:
    spec:
      ephemeral: false

So, what would the controller do based on the value set at spec.ephemeral?

  • Add a respective env (assuming we will still be making the change in the entrypoint.sh)
  • Would this value be considered while deciding whether to restart the container or not (assuming we will not use --once anymore)
  • Or something else that i missed

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 20, 2021

@thejasn Hey! Thanks for confirming and trying to contribute ☺️

Add a respective env (assuming we will still be making the change in the entrypoint.sh)

Yes, you will definitely need this

Would this value be considered while deciding whether to restart the container or not (assuming we will not use --once anymore)

No, as far as I can imagine.

The runner controller detects the runner pod of status Succeeded or the primary runner container to have exited with code 0 to be restarted. I suppose that logic to work regardless of the runner is ephemeral or not.

https://github.com/summerwind/actions-runner-controller/blob/3b2d2c052eb843cd7f1509960681d501c81f8eab/controllers/runner_controller.go#L224-L236

@thejasn
Copy link
Contributor Author

thejasn commented May 12, 2021

@mumoshu hey,

I'll have to wait for a release to use the updated CRD right? Any timelines on the same?

@funkypenguin
Copy link
Contributor

FWIW, I used the latest chart with the canary tag, and added this to my runnerDeployment spec.template.spec:

      env:
      - name: RUNNER_EPHEMERAL
        value: 'false'

@thejasn
Copy link
Contributor Author

thejasn commented May 27, 2021

hey @mumoshu
our CI/CD is set-up to use helm to install (without cloning the repository) the CRDs, and since the chart's publish job seems to be failing we couldn't use helm to install it.

Looks like there is a new repo being set-up for charts, should we just wait for that?

@mumoshu
Copy link
Collaborator

mumoshu commented May 28, 2021

@thejasn hey! the chart release is being delayed until the next release of the controller #562 (comment)

you can try using e.g. helm-git or temporarily copy the latest unreleased chart locally if you want to use it earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants