feat: Add last chance check before orphan termination #4595

stuartp44 · 2025-05-22T08:25:02Z

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.

…nality for incorrectly tagged runners

…tion

…minateOrphan function

npalm · 2025-05-22T15:04:28Z

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

stuartp44 · 2025-05-23T06:54:53Z

@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.

lambdas/functions/control-plane/src/scale-runners/scale-up.ts

lambdas/functions/control-plane/src/scale-runners/scale-down.ts

lambdas/functions/control-plane/src/scale-runners/scale-up.ts

lambdas/functions/control-plane/src/scale-runners/scale-down.ts

Co-authored-by: Niek Palm <[email protected]>

…function

…mports in scale-down.ts

…on logic

npalm · 2025-07-09T14:07:15Z

@npalm this is now ready for re-review. I have tested it against a test deployment and it works fine.

Can you share a scenario how to test this as well?

…fringe on other checks

stuartp44 · 2025-07-09T14:11:59Z

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 () => {

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

avni-ef · 2025-07-10T08:28:56Z

@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 · 2025-07-10T08:34:50Z

@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 · 2025-07-10T08:48:51Z

@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 · 2025-07-10T08:51:34Z

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 · 2025-07-14T06:10:54Z

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 · 2025-07-14T06:48:58Z

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 · 2025-07-18T07:21:54Z

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.

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

lambdas/functions/control-plane/src/scale-runners/scale-down.ts

modules/runners/policies/lambda-scale-down.json

…sistency

⚠️ This PR is on top of #4539 This pull request is an enhancement on top of #4539. The PR also introduces tagging via the start scripts for non-JIT configured instances. And it adds a state diagram to the ever growing complexity of the state diagram below terminating instances. ### Instance Tagging Enhancements: * Add function to tag runners with runner id for non JIT instances. Functions are designed to ignore errors to avoid causing a crash of the runner registration process. * side effect is that the instance is allowed to create the tag `ghr:github_runner_id` the instance is allowed to create the tag only on itself. ### Added docs * State diagram for scale-down added <img width="2084" height="3840" alt="Untitled diagram _ Mermaid Chart-2025-07-20-140732" src="https://github.com/user-attachments/assets/e88af647-98e5-46cb-8da4-28110c608d8d" /> --------- Co-authored-by: github-aws-runners-pr|bot <github-aws-runners-pr[bot]@users.noreply.github.com>

npalm · 2025-07-21T19:55:46Z

@stuartp44 thx for all the work!

🤖 I have created a release *beep* *boop* --- ## [6.7.0](v6.6.2...v6.7.0) (2025-07-22) ### Features * Add last chance check before orphan termination ([#4595](#4595)) ([7cbd9e0](7cbd9e0)) ### Bug Fixes * [#4391](#4391) ([7cbd9e0](7cbd9e0)) * **lambda:** bump form-data from 2.5.1 to 2.5.5 in /lambdas ([#4672](#4672)) ([0ec1c9a](0ec1c9a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: runners-releaser[bot] <194412594+runners-releaser[bot]@users.noreply.github.com>

stuartp44 added 2 commits May 21, 2025 18:39

feat(runners): add runnerId to RunnerList and implement untag functio…

3fa09d2

…nality for incorrectly tagged runners

fix(tests): improve clarity of orphaned runner untagging test descrip…

14df9fe

…tion

stuartp44 requested a review from a team as a code owner May 22, 2025 08:25

stuartp44 linked an issue May 22, 2025 that may be closed by this pull request

Pagination Data Slippage Issue causing EC2 instance to be scaled down #4584

Closed

stuartp44 requested a review from Copilot May 22, 2025 08:29

This comment was marked as outdated.

Sign in to view

fmt

9d7c89a

stuartp44 changed the title ~~Add last chance check before termination for JIT instances~~ feat: Add last chance check before termination for JIT instances May 22, 2025

stuartp44 changed the title ~~feat: Add last chance check before termination for JIT instances~~ feat: Add last chance check before orphan termination for JIT instances May 22, 2025

stuartp44 added 2 commits May 22, 2025 10:42

fix(scale-down): remove unnecessary logging of runner variable in ter…

e09337f

…minateOrphan function

fix(scale-up): remove unused import of run function

9064512

fix(scale-down): remove unused import of metricGitHubAppRateLimit

716e079

Merge branch 'main' into stu/add_tag_plus_check

e826fbe

npalm self-requested a review May 23, 2025 13:56

npalm reviewed May 24, 2025

View reviewed changes

stuartp44 and others added 9 commits May 26, 2025 11:34

Update lambdas/functions/control-plane/src/scale-runners/scale-down.ts

a5fcc88

Co-authored-by: Niek Palm <[email protected]>

Update lambdas/functions/control-plane/src/scale-runners/scale-down.ts

bb8ba2b

Co-authored-by: Niek Palm <[email protected]>

Update lambdas/functions/control-plane/src/scale-runners/scale-up.ts

97de234

Co-authored-by: Niek Palm <[email protected]>

Update lambdas/functions/control-plane/src/scale-runners/scale-up.ts

8b61d39

Co-authored-by: Niek Palm <[email protected]>

Remove warning log for orphan runners without runnerId in scale-down …

9883b0b

…function

Remove logging of runner ID marking in addGhRunnerIdToEC2InstanceTag …

43468a7

…function

readded metricGitHubAppRateLimit

bc995ef

Merge branch 'main' into stu/add_tag_plus_check

1182c8b

Refactor runner interfaces: remove RunnerState interface and update i…

6e1c72c

…mports in scale-down.ts

stuartp44 marked this pull request as draft June 3, 2025 15:49

stuartp44 added 3 commits June 4, 2025 10:13

Add headers to runner state return and update logging for busy state

097c14d

Remove redundant comment describing RunnerState type

971ec2d

Implement last chance check for orphan runners and refactor terminati…

5a275e5

…on logic

fix: enhance orphan runner judgment logic for clarity

3617608

fix: simplify orphan runner judgment logic by removing checks that in…

b8af3f0

…fringe on other checks

npalm requested a review from Copilot July 9, 2025 19:21

Copilot AI reviewed Jul 9, 2025

View reviewed changes

lambdas/functions/control-plane/src/aws/runners.test.ts Show resolved Hide resolved

avni-ef mentioned this pull request Jul 10, 2025

Pagination Data Slippage Issue causing EC2 instance to be scaled down #4584

Closed

avni-ef mentioned this pull request Jul 14, 2025

Retries exausthed when jobs are still queued #4653

Open

Merge branch 'main' into stu/add_tag_plus_check

1ddc98e

npalm requested changes Jul 20, 2025

View reviewed changes

stuartp44 and others added 5 commits July 21, 2025 17:48

fix(scale-down): rename unmarkOrphan function to unMarkOrphan for con…

835ca85

…sistency

fix(lambda): remove unused tag:UntagResources action from policy

3551c77

fix(runner): update orphan tag value to 'true' in untag runner test

441c4f4

Merge branch 'main' into stu/add_tag_plus_check

1a59d77

npalm requested a review from a team as a code owner July 21, 2025 19:08

npalm changed the title ~~feat: Add last chance check before orphan termination for JIT instances~~ feat: Add last chance check before orphan termination Jul 21, 2025

npalm approved these changes Jul 21, 2025

View reviewed changes

npalm merged commit 7cbd9e0 into main Jul 21, 2025
44 checks passed

npalm deleted the stu/add_tag_plus_check branch July 21, 2025 19:52

runners-releaser bot mentioned this pull request Jul 21, 2025

chore(main): release 6.7.0 #4669

Merged

feat: Add last chance check before orphan termination #4595

feat: Add last chance check before orphan termination #4595

Conversation

stuartp44 commented May 22, 2025 • edited by npalm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Functionality Enhancements:

Testing Improvements:

Uh oh!

This comment was marked as outdated.

Uh oh!

npalm commented May 22, 2025

Uh oh!

stuartp44 commented May 23, 2025

Uh oh!

npalm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

npalm commented Jul 9, 2025

Uh oh!

stuartp44 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

avni-ef commented Jul 10, 2025

Uh oh!

stuartp44 commented Jul 10, 2025

Uh oh!

npalm commented Jul 10, 2025

Uh oh!

avni-ef commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npalm commented Jul 14, 2025

Uh oh!

avni-ef commented Jul 14, 2025

Uh oh!

stuartp44 commented Jul 18, 2025

Uh oh!

npalm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

npalm commented Jul 21, 2025

Uh oh!

Uh oh!

stuartp44 commented May 22, 2025 •

edited by npalm

Loading

stuartp44 commented Jul 9, 2025 •

edited

Loading

avni-ef commented Jul 10, 2025 •

edited

Loading