-
Notifications
You must be signed in to change notification settings - Fork 628
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: add failover to on-demand in case request is failing #3409
Conversation
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.
As far as I can tell, it all looks good.
Do you want to address the points from CodeScene?
lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts
Outdated
Show resolved
Hide resolved
No, and actually the PR decreases the complexity by splitting the method in function. I think we can ignore codescne here |
4db0434
to
b3bdd0e
Compare
@navdeepg2021 thx, review comments are addressed |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions. |
3ded640
to
3f46965
Compare
feat: Add failover to on-demand on (spot) errors
7dc7c7c
to
dd1b293
Compare
@GuptaNavdeep1983 @ScottGuymer please can you have a look at this PR? |
|
||
it('Filter instances with status undefined, fall back to defaults.', async () => { | ||
mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); | ||
await listEC2Runners({ statuses: undefined }); |
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.
why this test is required? who is calling listec2runners with that status?
🤖 I have created a release *beep* *boop* --- ## [5.5.0](v5.4.2...v5.5.0) (2023-11-30) ### Features * add failover to on-demand in case request is failing ([#3409](#3409)) ([d71e631](d71e631)) ### Bug Fixes * add runner name prefix to context of scale-up lambda ([#3644](#3644)) ([2936edd](2936edd)) * **lambda:** bump the aws group in /lambdas with 5 updates ([#3635](#3635)) ([9615e53](9615e53)) * **lambda:** bump the octokit group in /lambdas with 1 update ([#3636](#3636)) ([876db0c](876db0c)) --- 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: forest-releaser[bot] <80285352+forest-releaser[bot]@users.noreply.github.com>
Adding the option to create on-demand instances in case spot is failing.
Problem
This module either support spot or on-demand instances. When using spot (default), creation can fail with several reasons. In case there is no sufficient capacity on AWS it makes sens to request on-demand instances. AWS does not support this kind of requests via the fleet API. This PR addresses this problme and add the option (opt-in_ to create on-demand instances in case of Insufficient capacity.
Migrations
No migrations required
Opt-in
Opt in by setting the variable
enable_runner_on_demand_failover
Verfication
Not easy to test the failover. But due to changes in multi-runner, runner module as well lambda scale-up and pool. The following checks are required