aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.71k stars 3.94k forks source link

aws-rds: DatabaseCluster doesn't provide option to specify instancetype for replica instances #18685

Open vks-singla opened 2 years ago

vks-singla commented 2 years ago

Description

aws-rds: L2 construct DatabaseCluster doesn't provide option to specify instancetype for replica instances and DatabaseInstance doesn't provide an option to specify cluster name. Sometime it is required to have different instance type for writer and replica to keep high availability in cluster with keep cost low. We can do this with L1 construct but is there any plan to introduce the same in L2 construct as well ?

Use Case

We are planning to create 2 read replica along with 1 writer for each aurora cluster to handle AZ issues, so that during an AZ failure we have cluster working in 2 AZs atleast, to keep cost inline we want to keep reader on lower instance type than writer.

Proposed Solution

Currently I don't have a solution, if option can be added in L2 construct would be great.

Other information

No response

Acknowledge

skinny85 commented 2 years ago

Hey @vks-singla,

thanks for opening the issue. Just to I understand fully what the ask is, can you show me a sketch of how would you accomplish this using L1 constructs?

Thanks, Adam

vks-singla commented 2 years ago

Hi @skinny85, Thanks for replying, Here is snippet which I can use for creating cluster and instance using L1 construct. I am not sharing complete code here, but it works fine with L1 construct.

Input Props:

{
    dbclustername: 'clusterXYZ'
    writerinstanceClass: 'db.r6g.2xlarge'
    readerinstanceClass: 'db.r6g.large'
    readerNum: 3
}

L1 Constructs:

pgDbCluster = new CfnDBcluster(....);
writerInst = new CfnDBInstance(this, 'AuroraPgDbReaderInstance', {
  dbClusterIdentifier: pgDbCluster.ref,
  dbInstanceClass: props.writerinstanceClass,
});

for (let itr = 1; itr <= props.readerNum; itr++) {
  const pgDbReaderInstance = new CfnDBInstance(this, `AuroraPgDbReaderInstance${itr}`, {
    dbClusterIdentifier: pgDbCluster.ref,
    dbInstanceClass: props.readerinstanceClass,
  });
}

Thanks, Vikas

skinny85 commented 2 years ago

@vks-singla thanks! So we allow setting the instanceType property when creating the Cluster (it's in in the instanceProps property of Cluster).

Is this what you're looking for?

Thanks, Adam

vks-singla commented 2 years ago

@skinny85 not really, above configuration set instance type for all instances in the cluster with same instance type, I want to provide different instance type for writer and reader now, for example.

Writer : db.r6g.4xlarge Reader: db.r6g.large

I cannot do that with current L2 constructs. Let me know if you have more questions.

skinny85 commented 2 years ago

Ah, I see! So the problem is when you want to define different instance types for the instances in a Cluster. Interesting!

I wonder what API should we offer here, given how the current Cluster and InstanceProps have been modeled... @jogold do you have any opinions here?

jogold commented 2 years ago

Ah, I see! So the problem is when you want to define different instance types for the instances in a Cluster. Interesting!

I wonder what API should we offer here, given how the current Cluster and InstanceProps have been modeled... @jogold do you have any opinions here?

Maybe allow 0 for instances and then a addInstance() on ICluster.

skinny85 commented 2 years ago

Yeah, that's actually a really good solution 😛. Thanks Jonathan!

tehzwen commented 2 years ago

Not entirely related but wanted to tag on here that as a side note, you cannot specify storageEncrypted on the the generated instances. The cluster can have its storage encrypted but in order for the rds:CreateDBInstance call to properly have the property I had to do something like the following:

   const cluster = new rds.DatabaseCluster(this, 'cluster', {
      engine: rds.DatabaseClusterEngine.auroraPostgres({
        version: rds.AuroraPostgresEngineVersion.VER_10_11
      }),
      credentials: rds.Credentials.fromSecret(databaseCredentialsSecret),
      instances: 1,
      instanceProps: {
        vpc,
      },
      storageEncrypted: true
    });

    cluster.node.children.forEach((c) => {
      if (c instanceof(rds.CfnDBInstance)) {
        let temp = c as rds.CfnDBInstance;
        temp.storageEncrypted = true;
      }
    });

The allowing 0 instances like mentioned above then addInstance(instance) would solve this or allow instance props to contain all the fields that a normal instance has :)

skinny85 commented 2 years ago

@tehzwen yeah, I would probably create a separate issue for this problem - it's completely unrelated to what's being discussed here 🙂.

ryanolee commented 1 year ago

I think this Issue probably needs revisiting from the perspective that you cannot actually seem delegate the type of node that is allocated to each instance. According to https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbcluster.html Because Amazon RDS automatically assigns a writer and reader DB instances in the cluster, use the cluster endpoint to read and write data, not the individual DB instance endpoints..

It seems that the node roles (Reader or Writer) are allocated somewhat at random? (Or I cannot seem to find in the documentation for a clear answer on this beyond this. Nor any properties in CF that seem to relate to role allocation for DB Instances).

Not to say that mixed node sizes within the cluster for this construct would not be helpful. But more that if you cannot allocate Writer and Reader nodes in cloudformation the original reason the issue was opened under does not seem to exist. As it would be impossible allocate different node sizes to instances you could not guarantee would end up as a "writer" or "reader" nodes?

If that case there is a broader question as to whether this is functionality that should be exposed to begin with? Not sure what your take on this would be @comcalvi / @jogold (If you are still involved with this :eyes:)

ryanolee commented 1 year ago

Nevermind just saw the "SourceDBInstanceIdentifier" property and the fact this is only applicable to non Aurora DB Clusters. This should be possible. Sorry for any confusion :laughing: !