infra: migrate ASG to CDK #8507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: DanielCliftonGuardian [email protected] Co-authored-by: Charlotte [email protected]
cemms1
changed the title
|
Size Change: 0 B Total Size: 678 kB ℹ️ View Unchanged
|
ParisaTork
reviewed
Aug 8, 2023
| default: `/${stack}/${stage.toLowerCase()}/logstash.stream.name`, | ||
| }); | ||
|
|
||
| const userData = ` |
There was a problem hiding this comment.
We're doing like-for-like here, but perhaps we could move this script to a separate file and load it in our CDK code, if we get round to optimisations in future.
There was a problem hiding this comment.
This is a great idea ✨ I've moved this to a different file
ParisaTork
reviewed
Aug 8, 2023
| stage: 'PROD', | ||
| minCapacity: 15, |
There was a problem hiding this comment.
Future optimisation note to parameterise these values
ParisaTork
reviewed
Aug 8, 2023
|
|
||
| export class DotcomRendering extends GuStack { | ||
| constructor(scope: App, id: string, props: DCRProps) { | ||
| super(scope, id, props); | ||
|
|
||
| const { app, region, stack, stage } = props; |
There was a problem hiding this comment.
Wonderful destructuring here
There was a problem hiding this comment.
Maybe we could use this construct here https://github.com/guardian/cdk/blob/1d5918085d533513de0b165be815019f98e880ad/src/constructs/autoscaling/user-data.ts#L23C14-L23C24
| * Fetches user data configuration for instances in the rendering app ASG | ||
| * @see https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/user-data.html | ||
| */ | ||
| export const getUserData = ({ |
There was a problem hiding this comment.
Maybe we could use this construct here https://github.com/guardian/cdk/blob/1d5918085d533513de0b165be815019f98e880ad/src/constructs/autoscaling/user-data.ts#L23C14-L23C24
There was a problem hiding this comment.
Yeah this is a really cool idea! For now I'm going to leave the user data as a string since we are sticking to migrating the stack as is, without any changes. This is because the GuUserData abstraction is designed for a slightly more simple use case. We could definitely simplify our user data definition in the future but this would be a distinct change to the cloudformation template, which we are trying to keep fairly constant in this migration
Co-Authored-By: Charlotte Emms <[email protected]>
Co-Authored-By: Charlotte Emms <[email protected]> Co-Authored-By: Daniel Clifton <[email protected]>
…-rendering into cdk/InternalLoadBalancer
Co-Authored-By: Parisa Tork <[email protected]> Co-Authored-By: Charlotte Emms <[email protected]>
Co-Authored-By: Parisa Tork <[email protected]> Co-Authored-By: Charlotte Emms <[email protected]>
ParisaTork
reviewed
Aug 11, 2023
| type UserDataProps = Pick<DCRProps, 'app' | 'region' | 'stage'> & { | ||
| elkStreamId: string; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Future optimisation note for us to move towards launch templates, as launch configurations are deprecated:
There was a problem hiding this comment.
We already use launch templates :)
GuAutoscalingGroup does it for us here
I realise maybe the file name of this is confusing? I can call it user data if that's better. I personally find "user data" an unintuitive term, hence why I went for "launch config" as this explains to me much more directly what this is for (the config we supply for launching instances). User data is only part of the story for launch configuration though, so I'm open to suggestions!
There was a problem hiding this comment.
You're completely right - when I saw launch-config.ts, I assumed we were still using launch configurations, but you're entirely right that the GuAutoscalingGroup has a value for launch templates - in light of this, would we maybe want to change the filename to launch-template.tsor user-data.ts as you've already wonderfully suggested?
9 tasks
ParisaTork
reviewed
Aug 14, 2023
| # Keep the Node version in sync with `.nvmrc` | ||
| Recipe: dotcom-rendering-ARM-jammy-node-18.17.0 | ||
| BuiltBy: amigo | ||
| AmigoStage: PROD | ||
| rendering: | ||
| type: autoscaling | ||
| parameters: |
There was a problem hiding this comment.
Do we need to add an asgMigrationInProgress param here, like so: https://github.com/guardian/amigo/pull/632/files#diff-8bd759df9f9dfeeb4ba5ee57a1c7a0a3563f8281a526b49854ecb6972102aaa8R10
Suggested change
| parameters: | |
| parameters: | |
| asgMigrationInProgress: true |
There was a problem hiding this comment.
As discussed, not at this stage - but when we do migrate our load balancer to an ALB this might be necessary
ParisaTork
reviewed
Aug 17, 2023
| @@ -175,6 +186,40 @@ export class DotcomRendering extends GuStack { | |||
| reason: 'Retaining a stateful resource previously defined in YAML', | |||
| }); | |||
|
|
|||
| const asg = new GuAutoScalingGroup(this, 'AutoscalingGroup', { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Hey @ParisaTork this is really useful documentation, thanks for finding it and linking it. We are trying to migrate the code here (from the Cloudformation YAML to CDK) rather than the resources themselves so as long as we don't add/remove the stateful resources here, I don't think we need to "migrate" as set out in that doc.
Having said that, when I chatted to DevX last week, they hinted that this migration strategy (the dual stack approach) might have been a better choice for us rather than the way that we did it - which was a piece by piece method. Since we've already done most of this, it makes sense to continue this way unless we encounter any more large blockers to moving forward.
cemms1
commented
Aug 22, 2023
dotcom-rendering/cdk/lib/userData.ts
Outdated
| `mkdir /var/log/dotcom-rendering`, | ||
| `chown -R dotcom-rendering:frontend /var/log/dotcom-rendering`, | ||
|
|
||
| `make start-prod`, |
There was a problem hiding this comment.
Need to consider #8640 before merging as this PR moves the definition of user data out of the cloudformation.yaml file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please test both of these changes on CODE env thoroughly before removing the
DO NOT MERGElabel and merging.What does this change?
Migrates the LaunchConfig and AutoscalingGroup from Cloudformation to CDK.
Why?
Resolves #7632 and #7633, as part of our migration to CDK