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: replace launch configuration with launch template #337

Conversation

Michenux
Copy link
Contributor

@Michenux Michenux commented Jul 6, 2021

Description

AWS recommend that we should use launch templates instead of launch configurations.

With the launch template, we can now use a CMK for storage encryption. Globally, launch template allows
to configure more things than the launch configuration.

Migrations required

NO

@npalm
Copy link
Collaborator

npalm commented Jul 8, 2021

thanks, will check the PR in the next days, sorry for the delay

Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Need a bit more time to check the PR. I ran a quick test with the default example. The plan is failing for this example. In this example runner_instance_spot_price is not set. In this case the agent will run on a NON spot instance. Which is the default. This will causse the following error:

Error: Invalid function argument

  on ../../main.tf line 219, in resource "aws_launch_template" "gitlab_runner_instance":
 219:     for_each = length(var.runner_instance_spot_price) == 0 ? [] : ["spot"]
 

Please can you also rebase with develop. I have fixed the CI jobs

@Michenux Michenux force-pushed the feature/replace-launch-configuration-with-launch-template branch from 07e1c9e to 55d066b Compare July 9, 2021 09:18
@Michenux
Copy link
Contributor Author

Michenux commented Jul 9, 2021

I have fixed the issues. Note that i have added an instance_refresh parameter on the runner autoscaling group to recreate the instance runner when a config is changed.

@npalm
Copy link
Collaborator

npalm commented Jul 21, 2021

sorry for the late response but will check the PR

@npalm
Copy link
Collaborator

npalm commented Jul 21, 2021

@Michenux great work, sorry for the late response. But I still got the following error while running the default example, see also review comment:

Error: Invalid function argument

  on ../../main.tf line 233, in resource "aws_launch_template" "gitlab_runner_instance":
 233:     for_each = var.runner_instance_spot_price != null && length(var.runner_instance_spot_price) == 0 ? [] : ["spot"]
    |----------------
    | var.runner_instance_spot_price is null

Invalid value for "value" parameter: argument must not be null.


Error: Invalid function argument

  on ../../main.tf line 271, in resource "aws_launch_template" "gitlab_runner_instance":
 271:     for_each = var.runner_instance_spot_price != null && length(var.runner_instance_spot_price) == 0 ? [] : ["spot"]
    |----------------
    | var.runner_instance_spot_price is null

Invalid value for "value" parameter: argument must not be null.

Did you fixed this issue? See only commits about formatting after my review.

Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

got it working locally, need to test a bit more, sse suggestion for my changes

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@npalm
Copy link
Collaborator

npalm commented Jul 22, 2021

Please can you also rebase develop, change in PR #348 requires a small update in the launch template.

@Michenux
Copy link
Contributor Author

I have rebased from develop and applied another solution for var.runner_instance_spot_price condition.

Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@Michenux thanks for all the hard work, sorry for the delay (once again). The current code is not working, once update the for_each on 2 places to for_each = var.runner_instance_spot_price == null || var.runner_instance_spot_price == "" ? [] : ["spot"] the default example works. Tested for spot and non spot.

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
Co-authored-by: Niek Palm <[email protected]>
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Looks like you missed on configuration for the istance market option, have made a suggestion with the fix.

Did you test one ot the examples?

main.tf Outdated Show resolved Hide resolved
Co-authored-by: Niek Palm <[email protected]>
@npalm npalm merged commit b805fb6 into cattle-ops:develop Aug 2, 2021
semantic-releaser bot pushed a commit that referenced this pull request Aug 28, 2021
## [4.29.0](4.28.0...4.29.0) (2021-08-28)

### Features

* Allow configuring docker machine egress rules, see PR [#351](#351) for upgrade instructions ([845e018](845e018))
* Parametrize runner instance launch configuration metadata options ([#348](#348)) ([a4406dc](a4406dc))
* replace launch configuration with launch template ([#337](#337)) ([b805fb6](b805fb6))
* support for settings Sentry DSN ([#352](#352)) ([2a07466](2a07466))

### Bug Fixes

* Use better ressources names ([#356](#356)) ([817e040](817e040))
@semantic-releaser
Copy link
Contributor

🎉 This PR is included in version 4.29.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

npalm pushed a commit that referenced this pull request Oct 4, 2021
## [4.29.0](4.28.0...4.29.0) (2021-08-28)

### Features

* Allow configuring docker machine egress rules, see PR [#351](#351) for upgrade instructions ([845e018](845e018))
* Parametrize runner instance launch configuration metadata options ([#348](#348)) ([a4406dc](a4406dc))
* replace launch configuration with launch template ([#337](#337)) ([b805fb6](b805fb6))
* support for settings Sentry DSN ([#352](#352)) ([2a07466](2a07466))

### Bug Fixes

* Use better ressources names ([#356](#356)) ([817e040](817e040))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants