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

Document how to add Windows self-hosted runners #1608

Merged
merged 6 commits into from
Aug 23, 2022

Conversation

ian-flores
Copy link
Contributor

This PR documents the process we used @voltrondata to add Windows self-hosted runners to be managed by the actions-runner-controller. It specifies changes that need to be made to the cert-manager and the actions-runner-controller. It also specifies how to deploy RunnerDeployment's configured for Windows.


RUN powershell choco feature enable -n allowGlobalConfirmation

CMD [ "pwsh", "-c", "./config.cmd --name $env:RUNNER_NAME --url https://github.com/$env:RUNNER_REPO --token $env:RUNNER_TOKEN --labels $env:RUNNER_LABELS --unattended --replace --ephemeral; ./run.cmd"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ian-flores Hey! Thanks for submitting this awesome pull request 🙏

Although I'm not an expert in managing Windows machines as K8s nodes, I tried my best to review this and everything looked generally good.

My only concern is in this line- how does it handle termination signals sent by Kubernetes?

On Linux, the PID 0 of each container gets SIGTERM on pod termination and usually, the PID 0 should gracefully stop all its child processes. We implement this for ARC with dumb-init, as you can see at:
https://github.com/actions-runner-controller/actions-runner-controller/blob/8b619e7c6fa9b8b07ff184e7882cec4108d0a52e/runner/actions-runner.dockerfile#L131-L132

Is PowerShell supposed to handle it, or do you need your own way?

Copy link

@amaldonadomat amaldonadomat Jul 22, 2022

Choose a reason for hiding this comment

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

Hello @mumoshu

We are also quite new with Windows, but I'll try to answer here. As far as I know there are not termination signals on Windows, it is handled with the CRTL_SHUTDOWN_EVENT. On the kubernetes documentation I found this:

terminationGracePeriodSeconds - this is not fully implemented in Docker on Windows, see the GitHub issue. The behavior today is that the ENTRYPOINT process is sent CTRL_SHUTDOWN_EVENT, then Windows waits 5 seconds by default, and finally shuts down all processes using the normal Windows shutdown behavior. The 5 second default is actually in the Windows registry inside the container, so it can be overridden when the container is built.

I've found also this interesting article explaining windows shutdown process:

https://technoresult.com/windows-shutdown-process-behind-the-scean/

Which also says that at some point all running processes are shutdown with a grace period of 5 seconds with the same signal, the timeout for this is configurable as well.

As you know, on Linux you can handle everything in a much easier way, but Windows is different. We can play with shutdown timeouts and implement some logic for handling the CTRL_SHUTDOWN_EVENT signal with our entrypoint, but I don't think it's worth it because we would have to send more signals to processes, which the windows shutting down process already does.

Let me know what you think

Copy link
Collaborator

Choose a reason for hiding this comment

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

@amaldonadomat Good to know and totally understood. Thank you so much for sharing your knowledge and thoughts!

@ian-flores ian-flores requested a review from mumoshu August 3, 2022 15:55
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@mumoshu
Copy link
Collaborator

mumoshu commented Aug 23, 2022

@ian-flores Thank you so much for your efforts and contribution ❤️

@mumoshu mumoshu merged commit e58f82b into actions:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants