-
Notifications
You must be signed in to change notification settings - Fork 61
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
Migrate CI to Github Actions #168
Conversation
We are hitting the 1 hour time limit of Circle CI (Issue open-quantum-safe#166). This migrates the existing CircleCI job completely to Github Actions which has a 5 hour time limit. For the most part, this is pretty much a one-to-one migration. Since upstream OpenSSH provided its own set of Github Actions, I simply moved those to the `upstream-github` directory to avoid conflicts and preserve the source. I did run into two issues with getting the integration tests to pass. Beyond that, I ran into two issues that arose from migrating to Github Actions which need to be partched around. agent-subprocess zombie process reaping The combination of Github Actions' host with the OQS CI container results in a lazier reaping of zombie processes which breaks this test. In this test, ssh-agent is run as a subprocess to some arbitrary user command. This enables exclusive access to ssh-agent to that specific process. The way this works under the hood is that ssh-agent forks into a child process and the parent process exec's into the arbitrary command ([code ref](https://github.com/open-quantum-safe/openssh/blob/OQS-v9/ssh-agent.c#L2384)) which runs to completion. The child process than polls its parent process until it detects its own orphaned status and terminates itself. This, by design, results in a zombie process which must be reaped. The test's assertion uses `kill -0` to check for liveness, but that counts zombies as "alive". The workaround for this then is to add an additional check to assert that zombies are in fact "dead". percent expansion is broken due to Github's HOME override The `percent` test tests % expansions inside SSH config files (e.g. home directory, username, port number). The assertion for the home directory uses the `HOME` environmental variable. Unfortunately, when running a container on a Github Runner, they unconditionally override the value of `HOME` with `/github/home` ([issue ref](actions/runner#863)) and this breaks the test assertion. The fix here is to get a more reliable reference for the home directory and use that for the assertion. Signed-off-by: Gerardo Ravago <[email protected]>
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.
Thanks! LGTM. @dstebila you may want to click "Stop building" openssh in the CCI dashboard when this lands. It seems that also there I don't have the required permissions to do so myself.
@dstebila just tell me when you want me to do it - are you sure you can't do it? |
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 barring one nit. Since this is coming from a fork, I won't be able to trigger the workflow from liboqs
until it's merged, so I'll get that working once this lands.
Co-authored-by: Spencer Wilson <[email protected]> Signed-off-by: Gerardo Ravago <[email protected]>
I saw the CI pass prior to addressing the nit so I went ahead and merged to unblock @SWilson4's liboqs PR. CircleCI job can be removed at anytime now. |
OK, I missed some fine print in the GitHub API documentation: workflows can only be triggered if their YAML file is on the default branch. This means that in order to get the upstream trigger working, we'll need to either change the default branch to The former approach seems cleaner to me: are we OK with updating the default branch to OQS-v9? It's the active dev branch anyway. @baentsch @christianpaquin @dstebila |
Alternatively, we could always tack the changes onto #153 and finally close that PR out. |
I'd advocate for |
I agree, we can make |
Changing the default branch requires admin perms, so I've asked @ryjones to swap it out for us. |
done |
The button's disabled for me, see attached. I think it is fine to do it now. |
@dstebila done! |
The (not-yet-merged) upstream trigger is triggering as expected: https://github.com/open-quantum-safe/openssh/actions/runs/10495477066. |
We are hitting the 1 hour time limit of Circle CI (Issue #166). This migrates the existing CircleCI job completely to Github Actions which has a 5 hour time limit.
For the most part, this is pretty much a one-to-one migration. Since upstream OpenSSH provided its own set of Github Actions, I simply moved those to the
upstream-github
directory to avoid conflicts and preserve the source. I did run into two issues with getting the integration tests to pass. Beyond that, I ran into two issues that arose from migrating to Github Actions which need to be partched around.agent-subprocess zombie process reaping
The combination of Github Actions' host with the OQS CI container results in a lazier reaping of zombie processes which breaks this test. In this test, ssh-agent is run as a subprocess to some arbitrary user command. This enables exclusive access to ssh-agent to that specific process. The way this works under the hood is that ssh-agent forks into a child process and the parent process exec's into the arbitrary command (code ref) which runs to completion. The child process than polls its parent process until it detects its own orphaned status and terminates itself. This, by design, results in a zombie process which must be reaped. The test's assertion uses
kill -0
to check for liveness, but that counts zombies as "alive". The workaround for this then is to add an additional check to assert that zombies are in fact "dead".percent expansion is broken due to Github's HOME override
The
percent
test tests % expansions inside SSH config files (e.g. home directory, username, port number). The assertion for the home directory uses theHOME
environmental variable. Unfortunately, when running a container on a Github Runner, they unconditionally override the value ofHOME
with/github/home
(issue ref) and this breaks the test assertion. The fix here is to get a more reliable reference for the home directory and use that for the assertion.