feat: Add last chance check before orphan termination

[stuartp44]

This pull request introduces enhancements to the AWS Lambda functions for scaling GitHub Actions runners. Key changes include the addition of functionality to manage runner tags (tagging and untagging).

• For JIT-instances the GitHub runner id is is added in the tag ghr:github_runner_id during registrartion in the scale-up lambda
• For non-JIT-instances the tag ghr:github_runner_id is added in in the start script.
• The IAM permission for the runner instances now allows to create only for the instance self to create the tag ghr:github_runner_id.
• The scale down function is using the runner id to efficient check the status of a runner.
• Orphan tags are removed when a runner seems to be active (again).

fix: #4391

Functionality Enhancements:

• Tag Management:

  • Added untag function to remove tags from EC2 instances. (lambdas/functions/control-plane/src/aws/runners.ts, lambdas/functions/control-plane/src/aws/runners.tsR117-R122)
  • Updated orphan runner handling to include untagging logic when a runner is no longer orphaned. (lambdas/functions/control-plane/src/scale-runners/scale-down.ts, lambdas/functions/control-plane/src/scale-runners/scale-down.tsL197-R261)

• JIT Runner Support:

  • Introduced logic to handle JIT runners by checking their status and activity before untagging or terminating. (lambdas/functions/control-plane/src/scale-runners/scale-down.ts, lambdas/functions/control-plane/src/scale-runners/scale-down.tsL197-R261)
  • Added runnerId to runner information for better identification and handling of JIT runners. (lambdas/functions/control-plane/src/aws/runners.ts, lambdas/functions/control-plane/src/aws/runners.tsR95)

Testing Improvements:

• Expanded Test Coverage:

  • Added tests for the new untag functionality. (lambdas/functions/control-plane/src/aws/runners.test.ts, lambdas/functions/control-plane/src/aws/runners.test.tsL232-R295)
  • Enhanced scale-down tests to cover JIT runner scenarios, including untagging and termination logic. (lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts, lambdas/functions/control-plane/src/scale-runners/scale-down.test.tsR354-R405)

• Mock Updates:

  • Updated mocks to include the new untag function for testing purposes. (lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts, lambdas/functions/control-plane/src/scale-runners/scale-down.test.tsR36)

These changes improve the scalability and robustness of the control-plane functions, ensuring better handling of dynamic runner configurations and edge cases.

[npalm]

@stuartp44 in case the PR is in WIP, can you mark the PR as draft?

[stuartp44]

@stuartp44 in case the PR is in WIP, can you mark the PR as draft?

The PR is actually ready to be review, the build failures where unrelated to my changes but I have fixed them nevertheless. @npalm

[npalm]

Thx for the PR. I only was able to a partial review so far. I have checked the Lambda code (excluding th tests). Also not tested a deployment.

This solution is solving the problem only for JIT enabled runners, but for non JIT the problem remains. Assuming the chances is less since typically less runners will be created. Do you have thoughts about alternatives? I think we should find a way / place to document this limitation.

I have made some comments, but need more time to go over the PR. Will ping you once ready, but laready sharing the feedback so far.

[stuartp44]

My test was to allow the scale-up lambda create a runner and then tag it with ghr:orphan and then watch the next run of the scale down lambda. It will test the current state of the runner against the GH API and only if its offline but busy will it terminate the runner (this can be done by terminating the GH action runner process in the system under test, which I didn't do!) else it will remove the tag. For everything else, it should follow the previous process. My only concern is if the runner is offline and idle, I am not sure if me putting anymore logic in encroaches on other areas the code should deal with.

[Copilot]

Pull Request Overview

This PR enhances control-plane Lambdas by adding EC2 tag removal, JIT runner support, and corresponding test coverage.

• Extend IAM policy to permit tag deletions
• Implement untag and last-chance orphan checks in scale-down
• Add tagging of runner IDs and test updates for JIT scenarios

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
modules/runners/policies/lambda-scale-down.json	Allow ec2:DeleteTags and tag:UntagResources in IAM
lambdas/functions/control-plane/src/scale-runners/scale-up.ts	Add runner-ID tagging in JIT flow
lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts	Mock and test new tag import
lambdas/functions/control-plane/src/scale-runners/scale-down.ts	Introduce untag, last-chance orphan logic
lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts	Cover JIT untag/terminate scenarios
lambdas/functions/control-plane/src/aws/runners.ts	Export new untag function and capture runnerId tag
lambdas/functions/control-plane/src/aws/runners.test.ts	Test listing of JIT instances and untag

Comments suppressed due to low confidence (1)

lambdas/functions/control-plane/src/aws/runners.test.ts:280

• [nitpick] The test description 'removing extra tag' is ambiguous; rename it to something like 'should remove orphan tag from runner' for clarity.

  it('removing extra tag', async () => {

[avni-ef]

@stuartp44 thx for the work. did a partial review. Made several comments. I think it would be better to keep the main control flow in a single fucntion, especiaaly the termination calls.

Besides this the PR only solve the problem for JIT enabled runners, not targetting general runners. This also required to ensure the limitation get document.

I still need to check a deployment and walk through the test code. do you know any way to reproduce the problem?

Will non-JIT runners also be targeted as part of this PR?

[stuartp44]

@avni-ef, no, as far as I am aware the JIT instances generate a runnerid (because they are pre registered) and add that via the scale-up lambda to the EC2 instance. We then look for this in the scale-down lambda and if that is present in the tags, we do the last chance check. Without that, its the current way of dealing with orphans which is tag and on the next run, terminate.

[npalm]

@avni-ef, no, as far as I am aware the JIT instances generate a runnerid (because they are pre registered) and add that via the scale-up lambda to the EC2 instance. We then look for this in the scale-down lambda and if that is present in the tags, we do the last chance check. Without that, its the current way of dealing with orphans which is tag and on the next run, terminate.

Could we not get the runner-id via the start script for non JIT, once the runner is being registered? And store this in a tag?

[avni-ef]

Thank you @stuartp44 @npalm for the quick response!
I'd appreciate if you could confirm my assumption:
When the runners (JIT/non-JIT) are removed from AWS (spots terminated), they'll still be considered as orphan runners on Github runners pool? As in, the scale-up Lambda won't try to remove them from the Github runners pool as they no longer exist on AWS side.
(Just trying to identify the root cause of the issue we're experiencing with jobs being canceled)

[npalm]

Thank you @stuartp44 @npalm for the quick response! I'd appreciate if you could confirm my assumption: When the runners (JIT/non-JIT) are removed from AWS (spots terminated), they'll still be considered as orphan runners on Github runners pool? As in, the scale-up Lambda won't try to remove them from the Github runners pool as they no longer exist on AWS side. (Just trying to identify the root cause of the issue we're experiencing with jobs being canceled)

With Orphan runners in this module we mean running aws instances not registered in GitHub. The scale-up acts based on running ec2 instances and try to remove them (scale-down) and clean orphan runners (could be caused by registration failures). The scale-down lambda is not cleaning registeried runners. In normal flow runners will be first removed from GitHub (de-register) and next terminated. For ephemeral runners the de-registration shoudl happen automacitally. Having offline runners in GitHub should not cause any side effects.

[avni-ef]

Thank you @stuartp44 @npalm for the quick response! I'd appreciate if you could confirm my assumption: When the runners (JIT/non-JIT) are removed from AWS (spots terminated), they'll still be considered as orphan runners on Github runners pool? As in, the scale-up Lambda won't try to remove them from the Github runners pool as they no longer exist on AWS side. (Just trying to identify the root cause of the issue we're experiencing with jobs being canceled)

With Orphan runners in this module we mean running aws instances not registered in GitHub. The scale-up acts based on running ec2 instances and try to remove them (scale-down) and clean orphan runners (could be caused by registration failures). The scale-down lambda is not cleaning registeried runners. In normal flow runners will be first removed from GitHub (de-register) and next terminated. For ephemeral runners the de-registration shoudl happen automacitally. Having offline runners in GitHub should not cause any side effects.

Thanks for clarifying @npalm, I now understand that I'm looking at a different problem, I'll reply on this new issue

[stuartp44]

Just a small nudge to get a review on this please

[npalm]

@stuartp44 looks good in general. I made some minor comments. I have also create a PR on top of this one #4667. This will also fix the same problem for non JIT runners. Please can you check the change and merge it in the PR. I have tested the PR to check the runner id is added for non JIT (linux /windows).

I also struggeld with testing the setup. I only checked the normal flows remains working. Which all look good.

[npalm]

@stuartp44 thx for all the work!