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(rds): support Aurora Serverless V2 instances #25437

Merged
merged 14 commits into from May 31, 2023

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented May 4, 2023

Adding support for adding aurora serverless v2 instances to a DatabaseCluster.

For detailed information on the design decisions see the adr

This PR adds a lot of validation to try and ensure that the user is configuring the cluster correctly.

It also adds some functionality that allows users to have an easier migration experience from the deprecated properties.

closes #20197


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Starting off with just an ADR so that we can first agree on the approach
we will take in this PR. Once that has been agreed upon then I will work
on implementation.

closes #20197
@gitpod-io
Copy link

gitpod-io bot commented May 4, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team May 4, 2023 12:50
@github-actions github-actions bot added effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1 labels May 4, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 4, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

packages/aws-cdk-lib/aws-rds/adr/aurora-serverless-v2.md Outdated Show resolved Hide resolved
Comment on lines +120 to +121
pool of serverless readers with the appropriate cluster capacity range. Vertical
scaling at the serverless instance level is much faster compared to launching
Copy link
Contributor

Choose a reason for hiding this comment

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

How the hell is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the example above for the db.r6g.4xlarge instance, if you had a single db.r6g.4xlarge reader instance and were using auto scaling then when the cluster scaled it would add another db.r6g.4xlarge instance which would take time. Alternatively you could provision 2 serverless readers with min=6.5/max=64. These serverless instances would scale up faster (with the read load split between them) faster than adding a new provisioned instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the above to the doc.

packages/aws-cdk-lib/aws-rds/adr/aurora-serverless-v2.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-rds/adr/aurora-serverless-v2.md Outdated Show resolved Hide resolved
new rds.DatabaseCluster(this, 'Cluster', {
engine: rds.DatabaseClusterEngine.auroraMysql({ version: rds.AuroraMysqlEngineVersion.VER_3_03_0 }),
clusterInstances: {
writer: ClusterInstance.provisioned('writer'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely provisioned needs more parameters. Are those elided for focus on this particular decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i'll update the example to atleast show that there are properties here.

readers: [
// puts it in promition tier 0-1
ClusterInstance.serverlessV2('reader1', { scaleWithWriter: true }),
ClusterInstance.serverlessV2('reader2'),
Copy link
Contributor

Choose a reason for hiding this comment

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

...and scaling parameters for the serverless instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also: any thoughts on how we would incorporate groups/endpoints in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and scaling parameters for the serverless instance?

I'll update for this. You can't set scaling at the instance level though, you set it at the cluster level and it applies to all instances.

Also: any thoughts on how we would incorporate groups/endpoints in here?

My thought is that we could add a ClusterInstances.fromGroup or something like that once there is CloudFormation support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added both to the doc.

Comment on lines +228 to +230
The main consequence of this decision is maybe just some confusion on how
to use the new properties vs the old ones and changing some property validation
from linter to runtime validation (`instanceProps` was required and would show
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @deprecation and updating the examples in the README should be sufficient.


We can also provide instructions on how to migrate from `instanceProps` to
`clusterInstances` which would just entail overwriting the logicalId of the
instances. We could also add something like `ClusterInstance.fromInstanceProps(props: InstanceProps)`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on the migration path

otaviomacedo
otaviomacedo previously approved these changes May 18, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review May 24, 2023 18:59

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mergify
Copy link
Contributor

mergify bot commented May 25, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@corymhall corymhall marked this pull request as ready for review May 25, 2023 18:45
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 25, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test illustrates the migration path. The test migrated from the old config to the new config with no change in output.

instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE3, ec2.InstanceSize.SMALL),
vpc,
},
engine: rds.DatabaseClusterEngine.auroraMysql({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This update isn't really part of this feature, but I noticed that you can no longer create a provisioned cluster with DatabaseClusterEngine.AURORA because that uses mysql 5.6. It now assumes that you are trying to create a serverless cluster. Updated the test so that it actually deploys now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated without any change to snapshot.


this.singleUserRotationApplication = props.engine.singleUserRotationApplication;
this.multiUserRotationApplication = props.engine.multiUserRotationApplication;

const { subnetIds } = props.instanceProps.vpc.selectSubnets(props.instanceProps.vpcSubnets);
this.serverlessV2MaxCapacity = props.serverlessV2MaxCapacity ?? 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This matches the the default for provisioned (t3.small)

public static serverlessV2(id: string, props: ServerlessV2ClusterInstanceProps = {}): IClusterInstance {
return new ClusterInstance(id, {
...props,
promotionTier: props.scaleWithWriter ? 1 : 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we give users the freedom to choose the promotion tier for reader instances? I don't know if this is an important use case, but I can think of a case in which, to save costs, you want to have a smaller instance with lower priority, for the rare case in which the writer and the high priority reader fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this and I decided to do it for provisioned instances, but for serverless I decided not to. The reason is that all serverless instances in the cluster have the same min/max capacity. The only difference is whether it is in tier 0-1 or 2-15. 0-1 means it will scale with the writer and 2-15 means it will not.

I think it would be easy enough to add in later if a use case comes up, but I can't think of one right now.

throw new Error('serverlessV2MinCapacity must be >= 0.5 & <= 128');
}

if (this.serverlessV2MaxCapacity === 0.5 && this.serverlessV2MinCapacity === 0.5) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing, but we should take the opportunity to also validate that min <= max.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added!

}
} else {
// TODO: add some info around serverless instance tiers and matching scaling
Annotations.of(this).addInfo('...');
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is still a work in progress, but calling your attention to this, just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh forgot about that. I'll fix.

The PR is not a work in progress though it's ready for a final review 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@mergify
Copy link
Contributor

mergify bot commented May 31, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7b4adb5
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit fe5ed10 into aws:main May 31, 2023
7 checks passed
@mergify
Copy link
Contributor

mergify bot commented May 31, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@moltar
Copy link
Contributor

moltar commented Jun 2, 2023

@corymhall Was there no way to preserve the IDs?

I've replaced instanceProps with:

      writer: ClusterInstance.serverlessV2('Instance', {
        publiclyAccessible: databaseEngine?.public,
        parameterGroup: this.parameterGroup,
      }),

And get a diff now, which means I'll have to re-create the instance when deploying:

    -     "ClusterInstance1448F06E4": Object {
    +     "ClusterInstanceAAE2C8BD": Object {
            "DeletionPolicy": "Delete",
            "Properties": Object {
              "DBClusterIdentifier": Object {
                "Ref": "ClusterEB0386A7",
              },
              "DBInstanceClass": "db.serverless",
              "DBParameterGroupName": Object {
                "Ref": "DatabaseWarehouseParameterGroupInstanceParameterGroupE36E617C",
    -         },
    -         "DBSubnetGroupName": Object {
    -           "Ref": "ClusterSubnetsDCFA5CB7",
              },
              "Engine": "aurora-postgresql",
    +         "PromotionTier": 0,
              "PubliclyAccessible": false,
            },

@corymhall
Copy link
Contributor Author

@moltar there is a way to preserve the ids when migrating from provisioned => provisioned

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_rds-readme.html#migrating-from-instanceprops

I'm assuming that you instanceProps and then an escape hatch to change the instance class to serverless?

@moltar
Copy link
Contributor

moltar commented Jun 5, 2023

I'm assuming that you instanceProps and then an escape hatch to change the instance class to serverless?

I was using this solution:

#20197 (comment)

@hertino76
Copy link

hertino76 commented Jun 8, 2023

Was there no way to preserve the IDs when we migrated to Writer and Reader and use monitoringInterval? I've an already functional database.

I've replaced instanceProps according to the migration recommendations just changing the location of the properties like this:

` Code :

 let cluster = new rds.DatabaseCluster(scope, 'DBCluster', {
  clusterIdentifier: props.nomCluster,
  engine: rds.DatabaseClusterEngine.auroraMysql({ version: varVersion }),
  defaultDatabaseName: props.nomDB,
  credentials: rds.Credentials.fromSecret(this.dbSecret),
  parameterGroup: this.parameterGroupCluster,
  monitoringInterval: Duration.seconds(60),
  copyTagsToSnapshot: false,
  storageEncrypted: true,
  backtrackWindow: Duration.hours(6),
  backup: {
    retention: Duration.days(RetentionDays.ONE_WEEK),
    preferredWindow: '01:00-03:00',
  },
  cloudwatchLogsExports: ['audit'],
  preferredMaintenanceWindow: 'Sat:03:00-Sun:11:00',
  deletionProtection: true,
  removalPolicy: RemovalPolicy.RETAIN,
  vpc: props.vpc,
  vpcSubnets: props.vpcSubnetsData,
  securityGroups: [props.securityGroup],

  writer : rds.ClusterInstance.serverlessV2('instance1', {
    parameterGroup: this.parameterGroupInst,
    publiclyAccessible: false,
    autoMinorVersionUpgrade: true,
    allowMajorVersionUpgrade: false,
    enablePerformanceInsights: true
  }),  ...`

And I'm getting a difference with the MonitoringRole property, which means I'll have to re-create the instance when deploying.

image

},
vpc,
credentials: rds.Credentials.fromGeneratedSecret('clusteradmin'), // Optional - will default to 'admin' username and generated password
writer: rds.ClusterInstance.provisioned('writer', {

Choose a reason for hiding this comment

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

I think this is missing some lines of code

Choose a reason for hiding this comment

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

My problem is another one, the code works, the instance is serverless and the secret key works, but with the new structure using writer and reader, the new version of my cdk application destroys the MonitoringRole and recreates the instances.

@corymhall
Copy link
Contributor Author

For everyone commenting on this PR, can you please create new issues instead? It's hard to monitor and comment on closed PRs.

mergify bot pushed a commit that referenced this pull request Jul 24, 2023
…stance.serverlessV2` (#26472)

**Context**
A recent feature release #25437 has added support for Aurora Serverless V2 cluster instances. This change also introduced a new approach for defining cluster instances, deprecating `instanceProps` in the process.

The new approach uses `ClusterInstance.provisioned()` and `ClusterInstance.serverlessV2()` to define instances and their parameters on a per-instance basis. A migration flag `isFromLegacyInstanceProps` has also been added to the `ClusterInstance.provisioned()` constructor props to allow for migration to this new approach without destructive changes to the generated CFN template.

**Bug**
Because the `DatabaseCluster` construct has not previously had official support for Serverless V2 instances, the same migration flag has not been made available for `ClusterInstance.serverlessV2()`. This ignores the fact that many people have already provisioned serverless v2 instances using a common workaround described here #20197 (comment). People who have used this method previously have no clean migration path. This has been previously raised in #25942.

**Fix**
This fix simply exposes the `isFromLegacyInstanceProps` flag on **both** `ProvisionedClusterInstanceProps` and `ServerlessV2ClusterInstanceProps`. The behaviour for this flag is already implemented and applied across both instance types, so this is a type-only change. I have however added a test to capture this upgrade path for Serverless V2 instances from the common workaround.


Closes #25942.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this pull request Jul 29, 2023
…stance.serverlessV2` (aws#26472)

**Context**
A recent feature release aws#25437 has added support for Aurora Serverless V2 cluster instances. This change also introduced a new approach for defining cluster instances, deprecating `instanceProps` in the process.

The new approach uses `ClusterInstance.provisioned()` and `ClusterInstance.serverlessV2()` to define instances and their parameters on a per-instance basis. A migration flag `isFromLegacyInstanceProps` has also been added to the `ClusterInstance.provisioned()` constructor props to allow for migration to this new approach without destructive changes to the generated CFN template.

**Bug**
Because the `DatabaseCluster` construct has not previously had official support for Serverless V2 instances, the same migration flag has not been made available for `ClusterInstance.serverlessV2()`. This ignores the fact that many people have already provisioned serverless v2 instances using a common workaround described here aws#20197 (comment). People who have used this method previously have no clean migration path. This has been previously raised in aws#25942.

**Fix**
This fix simply exposes the `isFromLegacyInstanceProps` flag on **both** `ProvisionedClusterInstanceProps` and `ServerlessV2ClusterInstanceProps`. The behaviour for this flag is already implemented and applied across both instance types, so this is a type-only change. I have however added a test to capture this upgrade path for Serverless V2 instances from the common workaround.


Closes aws#25942.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(rds): support for Aurora Serverless V2
7 participants