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

feat: add spot termination watcher (beta) #3789

Merged
merged 37 commits into from
Mar 20, 2024
Merged

feat: add spot termination watcher (beta) #3789

merged 37 commits into from
Mar 20, 2024

Conversation

npalm
Copy link
Member

@npalm npalm commented Mar 2, 2024

This PR is adding a lambda function to watch termination events.

  • Log instance information for termination warnings
  • Create optional a metric with dimensions for the environment and instance type.

This PR limits to only checking the termination warning. Later we can extend on also start acting on terminations.

Testing

Spot termination can be tested by initiate a termination event via the Spot Request overview (or cli).

Todo

  • Write docs
  • Add to multi runner
  • Describe next steps in an issue.

@npalm npalm marked this pull request as draft March 2, 2024 21:26
@npalm npalm added the enhancement New feature or request label Mar 2, 2024
@npalm npalm changed the title fea: add spot termination watcher feat: add spot termination watcher Mar 3, 2024
@npalm
Copy link
Member Author

npalm commented Mar 3, 2024

@GuptaNavdeep1983 @ScottGuymer @nadavsinai-philips @saragerion Do one of you have any clue why middy is let failing the unit tests. For modules like lamdas/functions/webhook. The test is lambda.test.ts are working, where lambda.te also initiates middy.

In lambdas/functions/spot-termination-watcher the tests are failing with the error:

    TypeError: Cannot destructure property 'before' of 'middleware' as it is undefined.

      14 | import { Config } from './ConfigResolver';
      15 |
    > 16 | middy(interruptionWarning).use(captureLambdaHandler(tracer)).use(logMetrics(metrics));
         |                            ^
      17 | const config = new Config();
      18 |
      19 | export async function interruptionWarning(

      at Function.middy.use (../../node_modules/@middy/core/index.cjs:90:21)
      at Object.use (src/lambda.ts:16:28)
      at Object.<anonymous> (src/lambda.test.ts:6:1)

Even without adding the metrics part, which is the only difference comparing to the other modules, the tests are failing. CI is failing as well https://github.com/philips-labs/terraform-aws-github-runner/actions/runs/8129056369/job/22215682414

@npalm
Copy link
Member Author

npalm commented Mar 3, 2024

@GuptaNavdeep1983 @ScottGuymer @nadavsinai-philips @saragerion Do one of you have any clue why middy is let failing the unit tests. For modules like lamdas/functions/webhook. The test is lambda.test.ts are working, where lambda.te also initiates middy.

In lambdas/functions/spot-termination-watcher the tests are failing with the error:

    TypeError: Cannot destructure property 'before' of 'middleware' as it is undefined.

      14 | import { Config } from './ConfigResolver';
      15 |
    > 16 | middy(interruptionWarning).use(captureLambdaHandler(tracer)).use(logMetrics(metrics));
         |                            ^
      17 | const config = new Config();
      18 |
      19 | export async function interruptionWarning(

      at Function.middy.use (../../node_modules/@middy/core/index.cjs:90:21)
      at Object.use (src/lambda.ts:16:28)
      at Object.<anonymous> (src/lambda.test.ts:6:1)

Even without adding the metrics part, which is the only difference comparing to the other modules, the tests are failing. CI is failing as well https://github.com/philips-labs/terraform-aws-github-runner/actions/runs/8129056369/job/22215682414

Fixed, figured out the problem. Tracer handler could be undefined in unit test (at least).

ScottGuymer
ScottGuymer previously approved these changes Mar 14, 2024
Copy link
Member

@ScottGuymer ScottGuymer left a comment

Choose a reason for hiding this comment

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

Its a bit of a long one.

tried to take a look at the important stuff and it all seems ok to me.

:shipit:

docs/examples/termination-watcher.md Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
examples/termination-watcher/README.md Outdated Show resolved Hide resolved
ScottGuymer
ScottGuymer previously approved these changes Mar 18, 2024
@npalm npalm changed the title feat: add spot termination watcher feat: add spot termination watcher (beta) Mar 19, 2024
@npalm npalm merged commit b2dc794 into main Mar 20, 2024
44 checks passed
@npalm npalm deleted the npalm/spot-watcher branch March 20, 2024 19:30
npalm pushed a commit that referenced this pull request Apr 17, 2024
🤖 I have created a release *beep* *boop*
---


##
[5.10.0](v5.9.0...v5.10.0)
(2024-04-17)


### Features

* add spot termination watcher (beta)
([#3789](#3789))
([b2dc794](b2dc794))
* allow caller to provide custom userdata
([#3798](#3798))
([ac49daf](ac49daf))
* Allow to disable runner max scaling check
([#3849](#3849))
([e05a043](e05a043))


### Bug Fixes

* **lambda:** bump axios from 1.6.7 to 1.6.8 in /lambdas
([#3814](#3814))
([513b22f](513b22f))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3834](#3834))
([e7e56ea](e7e56ea))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3846](#3846))
([9303a10](9303a10))
* **lambda:** bump the aws group in /lambdas with 6 updates
([#3818](#3818))
([9a9031e](9a9031e))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants