-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: allow ephemeral runner to be optional #498
Conversation
added ephemeral flag in runner spec
@thejasn Hey! Thank yoy so much for your efforts |
Hey @mumoshu, Tested it on a kind cluster, pretty much followed the procedure in the Test Output
|
runner/entrypoint.sh
Outdated
@@ -66,5 +66,10 @@ for f in runsvc.sh RunnerService.js; do | |||
sudo mv {patched,bin}/${f} | |||
done | |||
|
|||
args=() | |||
if [ "${RUNNER_EPHEMERAL}" == "true" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this breaks newly released runner images to not work with previous versions of actions-runner-controller.
I'm not saying we must support all the previous actions-runner-controller releases, but I'd love it if it could support one or two previous versions.
That being said, can we probably make this backward-compatible by changing the conditional to:
if [ "${RUNNER_EPHEMERAL}" == "true" ]; then | |
if [ "${RUNNER_EPHEMERAL}" != "false" ]; then |
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is backward-compatible as per what I tested, this condition evaluates the variable to true
when the field isn't mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thejasn Yes, it does seem to be backward-compatible in terms of Runner CRD changes.
What I was saying about was though newer runner images being deployed by older actions-runner-controller and older CRDs.
Unlike the controller, runner images are continuously built and released from our default branch, without waiting for a controller release. That implies an older version of actions-runner-controller may use newer runner images depending on the configuration on the user's side.
If the older version of actions-runner-controller instructed to use the latest runner image build after this change, wouldn't the runner pod gets an empty RUNNER_EPHEMERAL
which results in missing --once
?
That's what I wanted to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, didn't know runner would be released independently of the controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you so much for your contribution @thejasn
Hey @mumoshu, I think the docker creds are missing, actions seemed to have failed. |
Changes
add
ephemeral
option torunner.spec
defaults to
true
entrypoint.sh
in runner/Dockerfile modified to readRUNNER_EPHEMERAL
flagFixes