feat: add aurora serverless v2 in storage by hkford · Pull Request #4075 · aws/copilot-cli

hkford · 2022-10-10T13:09:42Z

Added --serverless-version flag in storage init command.

Aurora Serverless Flags
      --engine string               The database engine used in the cluster.
                                    Must be either "MySQL" or "PostgreSQL".
      --initial-db string           The initial database to create in the cluster.
      --parameter-group string      Optional. The name of the parameter group to associate with the cluster.
      --serverless-version string   Aurora Serverless version.
                                    Must be of either "V1" or "V2"

When you pass --serverless-version V1 the storage init command generates an addon template of Aurora Serverless V1 cluster the same as before. When you pass --serverless-version V2 the storage init command generates an addon template of Aurora Serverless V2 cluster, which is recently released.

Fixes #3493

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

codecov-commenter · 2022-10-10T13:22:57Z

Codecov Report

Merging #4075 (3dfc59c) into mainline (bfedebc) will decrease coverage by 0.08%.
The diff coverage is 53.65%.

@@             Coverage Diff              @@
##           mainline    #4075      +/-   ##
============================================
- Coverage     69.09%   69.00%   -0.09%     
============================================
  Files           248      250       +2     
  Lines         35453    35768     +315     
  Branches        264      264              
============================================
+ Hits          24495    24683     +188     
- Misses         9766     9893     +127     
  Partials       1192     1192

Impacted Files	Coverage Δ
internal/pkg/addon/storage.go	`71.42% <35.71%> (-4.02%)`	⬇️
internal/pkg/cli/storage_init.go	`64.85% <62.96%> (+0.01%)`	⬆️
internal/pkg/manifest/errors.go	`51.89% <0.00%> (-23.11%)`	⬇️
internal/pkg/manifest/job.go	`52.38% <0.00%> (-21.43%)`	⬇️
internal/pkg/manifest/env.go	`73.00% <0.00%> (-5.77%)`	⬇️
internal/pkg/manifest/http.go	`80.76% <0.00%> (-5.13%)`	⬇️
internal/pkg/manifest/backend_svc.go	`83.01% <0.00%> (-4.22%)`	⬇️
internal/pkg/manifest/pipeline.go	`60.20% <0.00%> (-4.09%)`	⬇️
internal/pkg/manifest/svc.go	`78.07% <0.00%> (-2.70%)`	⬇️
internal/pkg/manifest/lb_web_svc.go	`86.36% <0.00%> (-2.53%)`	⬇️
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

huanjani · 2022-10-10T18:42:25Z

+
+const (
+	// Aurora Serverless versions.
+	ServerlessVersionV1 = "V1"


Maybe we should lowercase the 'v' (here, flag description, prompt, examples, etc.), based on https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/aurora-serverless-v2.html and Go/Semantic Versioning conventions. The flag should accept both upper and lower case input, though.

I lowercased the serverless-version flag to v1 or v2.

The flag should accept both upper and lower case input, though.

I think the flag should accept only lower case for now. The database engine flag currently accepts only "MySQL" or "PostgreSQL", not "mysql" nor "postgresql" and serverless-version flag should do the same.

Lou1415926

Thank you @hkford so much for adding this ❤️ !!

I wonder if we should just always generate the template for v2. That is, when people run

copilot storage init --storage-type Aurora

we will generate a template that creates the aurora serverless v2 cluster.

hkford · 2022-10-12T05:13:49Z

@Lou1415926
I removed the prompt to ask Aurora Serverless version and treat the default version as v2.
Unless specifying --serverless-version v1 Copilot generates a v1 cluster template.

  Create an RDS Aurora Serverless v2 cluster using PostgreSQL as the database engine.
  `$ copilot storage init -n my-cluster -t Aurora -w frontend --engine PostgreSQL --initial-db testdb`
  Create an RDS Aurora Serverless v1 cluster using MySQL as the database engine.
  `$ copilot storage init -n my-cluster -t Aurora --serverless-version v1 -w frontend --engine MySQL --initial-db testdb`

efekarakus

Awesome, thank you so much @hkford ! Apologies on the late review

efekarakus · 2022-10-13T18:38:53Z

+	ServerlessVersionV1 = "v1"
+	ServerlessVersionV2 = "v2"


nit: do these need to be public?

Suggested change

ServerlessVersionV1 = "v1"

ServerlessVersionV2 = "v2"

auroraServerlessVersionV1 = "v1"

auraraServerlessVersionV2 = "v2"

addon.AuroraServerlessVersion is used in newRDSTemplate() of cli/storage_init.go. This function calls addon.NewServerlessVxTemplate using switch statement, same as addon.RDSEngineTypeMySQL or addon.RDSEngineTypePostgreSQL.

efekarakus · 2022-10-13T18:48:04Z

-	Envs           []string // The copilot environments found inside the current app.
+	WorkloadType      string   // The type of the workload associated with the RDS addon.
+	ClusterName       string   // The name of the cluster.
+	ServerlessVersion string   // The version of Aurora Serverless


I wonder if this is a hint that we need two types of constructors? and can replace the existing RDSTemplate constructor:

type RDSProps struct { WorkloadType // Other existing fields... Envs []string serverlessVersion string } func NewServerlessV1Template(rds RDSProps) *RDSTemplate { rds.serverlessVersion = auroraServerlessVersionV1 // existing logic... } func NewServerlessV2Template(rds RDSProps) *RDSTemplate { rds.serverlessVersion = auroraServerlessVersionV2 }

I agree with you. I made two types of constructors.

efekarakus · 2022-10-13T18:49:02Z

 const (
+	serverlessVersionV1            = "v1"
+	serverlessVersionV2            = "v2"
+	defaultServerlessVersionString = "v2"


nit:

Suggested change

defaultServerlessVersionString = "v2"

defaultServerlessVersion = serverlessVersionV2

Thanks. I fixed it.

efekarakus · 2022-10-13T18:51:32Z

+	defaultServerlessVersion := defaultServerlessVersionString
+	o.rdsServerlessVersion = defaultServerlessVersion


nit:

Suggested change

defaultServerlessVersion := defaultServerlessVersionString

o.rdsServerlessVersion = defaultServerlessVersion

o.rdsServerlessVersion = defaultServerlessVersion

efekarakus · 2022-10-13T18:55:24Z

+    Properties:
+      Description: !Ref 'AWS::StackName'
+      {{- if eq .Engine "MySQL"}}
+      Family: 'aurora-mysql8.0'


Just to make sure was the addon tested in an an account with copilot deploy for both MySQL and Postgres?

efekarakus · 2022-10-13T18:58:41Z

+        # Replace "All" below with "!Ref Env" to set different autoscaling limits per environment.
+        MinCapacity: !FindInMap [{{logicalIDSafe .ClusterName}}EnvScalingConfigurationMap, All, DBMinCapacity]
+        MaxCapacity: !FindInMap [{{logicalIDSafe .ClusterName}}EnvScalingConfigurationMap, All, DBMaxCapacity]
+  {{logicalIDSafe .ClusterName}}DBWriterInstance:


Oh very interesting, can you explain to be why an explicit writer instance is needed?
We didn't set one for aurora serverless v1 so I'm curious why v2 is different?

Same question! When creating AWS::RDS::DBCluster, does RDS automatically create a writer instance for that cluster?

We had an offline discussion and I'm sharing it here for visibility:

Does RDS automatically create a writer instance for that cluster?

For v2, it doesn't. If we have only the DBCluster resource in the template, only a cluster is created - no writer instance.

We didn't set one for aurora serverless v1 so I'm curious why v2 is different?

v1 contains one and only one read/write instance. When we create a DBCluster, that one and only one read/write instance also gets automatically created. This is different from v2 and is also the reason why we didn't need to explicitly create an instance before.

Lou1415926

Thank you very much ! This is awesome. I just have some small nits and one question (same as Efe's)!

Lou1415926 · 2022-10-13T19:20:24Z

+	ServerlessVersionV1 = "v1"
+	ServerlessVersionV2 = "v2"


nit:

Suggested change

ServerlessVersionV1 = "v1"

ServerlessVersionV2 = "v2"

AuroraServerlessVersionV1 = "v1"

AuroraServerlessVersionV2 = "v2"

Thanks. I fixed it.

Lou1415926 · 2022-10-13T19:22:58Z

 			},
 			wantedBinary: []byte("mysql"),
 		},
+		"renders postgresql v2 content": {


Yayy thank you for all the unit tests !

Lou1415926 · 2022-10-13T19:49:44Z

+func validateServerlessVersion(val interface{}) error {
+	version, ok := val.(string)
+	if !ok {
+		return errValueNotAString
+	}
+	for _, valid := range serverlessVersions {
+		if version == valid {
+			return nil
+		}
+	}
+	return fmt.Errorf(fmtErrInvalidServerlessVersion, version, prettify(serverlessVersions))
+}
+


I think in this case we can move this validation function to storage_init.go, similar to

copilot-cli/internal/pkg/cli/storage_init.go

Line 333 in 08983f7

func (o *initStorageOpts) validateStorageType() error {

Suggested change

func validateServerlessVersion(val interface{}) error {

version, ok := val.(string)

if !ok {

return errValueNotAString

}

for _, valid := range serverlessVersions {

if version == valid {

return nil

}

}

return fmt.Errorf(fmtErrInvalidServerlessVersion, version, prettify(serverlessVersions))

}

func (o *initStorageOpts) validateServerlessVersion() error {

for _, valid := range serverlessVersions {

if o.serverlessVersion == valid {

return nil

}

}

return fmt.Errorf(fmtErrInvalidServerlessVersion, version, prettify(serverlessVersions))

}

The validation functions in this validate.go file accept val interface{} because these validation functions are passed to the prompt handlers which require the function signature to be (interface{}) error.

In our case, since we no longer prompt for versions, we won't need to adhere to this signature!

Thank you for your suggestion. I moved validateServerlessVersion to storage_init.go.

Lou1415926 · 2022-10-13T19:51:10Z

 	cmd.Flags().BoolVar(&vars.noLSI, storageNoLSIFlag, false, storageNoLSIFlagDescription)
 	cmd.Flags().BoolVar(&vars.noSort, storageNoSortFlag, false, storageNoSortFlagDescription)

+	cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, "", storageRDSServerlessVersionFlagDescription)


instead of using askAuroraServerlessVersion to set the default, what do you think of setting it here:

Suggested change

cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, "", storageRDSServerlessVersionFlagDescription)

cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, defaultServerlessVersion, storageRDSServerlessVersionFlagDescription)

I fixed it.

Lou1415926 · 2022-10-13T19:53:53Z

+	storageRDSServerlessVersionFlagDescription = `Optional. Aurora Serverless version. Must be either "v1" or "v2".
+(default v2)`


If we do decide to set the default in cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, "", storageRDSServerlessVersionFlagDescription) (in storage_init.go), then the cobra will automatically append (default "v2") to the end of the flag description I think!

Suggested change

storageRDSServerlessVersionFlagDescription = `Optional. Aurora Serverless version. Must be either "v1" or "v2".

(default v2)`

storageRDSServerlessVersionFlagDescription = `Optional. Aurora Serverless version. Must be either "v1" or "v2".`

I set the default value in cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, defaultServerlessVersion, storageRDSServerlessVersionFlagDescription) and cobra appended (default "v2") as expected.

Lou1415926 · 2022-10-13T19:59:22Z

+      {{- if eq $.Engine "MySQL"}}
+      "DBMinCapacity": 0.5 # AllowedValues: from 0.5 through 128
+      "DBMaxCapacity": 8   # AllowedValues: from 0.5 through 128
+      {{- else}}
+      "DBMinCapacity": 0.5 # AllowedValues: from 0.5 through 128
+      "DBMaxCapacity": 8   # AllowedValues: from 0.5 through 128
+      {{end -}}


nit: seems like we don't need to differentiate based on engine anymore (previously, the min-max was different for mysql vs. postgresql)

Suggested change

{{- if eq $.Engine "MySQL"}}

"DBMinCapacity": 0.5 # AllowedValues: from 0.5 through 128

"DBMaxCapacity": 8 # AllowedValues: from 0.5 through 128

{{- else}}

"DBMinCapacity": 0.5 # AllowedValues: from 0.5 through 128

"DBMaxCapacity": 8 # AllowedValues: from 0.5 through 128

{{end -}}

"DBMinCapacity": 0.5 # AllowedValues: from 0.5 through 128

"DBMaxCapacity": 8 # AllowedValues: from 0.5 through 128

I fixed it.

Lou1415926 · 2022-10-13T20:59:28Z

+        # Replace "All" below with "!Ref Env" to set different autoscaling limits per environment.
+        MinCapacity: !FindInMap [{{logicalIDSafe .ClusterName}}EnvScalingConfigurationMap, All, DBMinCapacity]
+        MaxCapacity: !FindInMap [{{logicalIDSafe .ClusterName}}EnvScalingConfigurationMap, All, DBMaxCapacity]
+  {{logicalIDSafe .ClusterName}}DBWriterInstance:


Same question! When creating AWS::RDS::DBCluster, does RDS automatically create a writer instance for that cluster?

hkford · 2022-10-17T08:01:41Z

@efekarakus @Lou1415926
I updated my PR based on your comments. Would you review my code again please?

efekarakus

Looks great, just some tiny comments and then we can ! thank you @hkford

efekarakus · 2022-10-18T16:42:50Z

+	AuroraServerlessVersionV1 = "v1"
+	AuroraServerlessVersionV2 = "v2"


nit: can these be private?

efekarakus · 2022-10-18T16:51:36Z

-	Envs           []string // The copilot environments found inside the current app.
+	WorkloadType            string   // The type of the workload associated with the RDS addon.
+	ClusterName             string   // The name of the cluster.
+	AuroraServerlessVersion string   // The version of Aurora Serverless


nit: can be private, and can we add a period at the end of the comment plz 🥺

Suggested change

AuroraServerlessVersion string // The version of Aurora Serverless

auroraServerlessVersion string // The version of Aurora Serverless.

changed into private field and added a period.

efekarakus · 2022-10-18T16:58:26Z

+	var version string
+	switch o.auroraServerlessVersion {
+	case auroraServerlessVersionV1:
+		version = addon.AuroraServerlessVersionV1
+		return addon.NewServerlessV1Template(addon.RDSProps{
+			ClusterName:             o.storageName,
+			AuroraServerlessVersion: version,
+			Engine:                  engine,
+			InitialDBName:           o.rdsInitialDBName,
+			ParameterGroup:          o.rdsParameterGroup,
+			Envs:                    envs,
+			WorkloadType:            o.workloadType,
+		}), nil
+	case auroraServerlessVersionV2:
+		version = addon.AuroraServerlessVersionV2
+		return addon.NewServerlessV2Template(addon.RDSProps{
+			ClusterName:             o.storageName,
+			AuroraServerlessVersion: version,
+			Engine:                  engine,
+			InitialDBName:           o.rdsInitialDBName,
+			ParameterGroup:          o.rdsParameterGroup,
+			Envs:                    envs,
+			WorkloadType:            o.workloadType,
+		}), nil
+	default:
+		return nil, errors.New("unknown serverless version")
+	}


To reduce duplication:

props := addon.RDSProps{ ClusterName: o.storageName, Engine: engine, InitialDBName: o.rdsInitialDBName, ParameterGroup: o.rdsParameterGroup, Envs: envs, WorkloadType: o.workloadType, } switch v:= o.auroraServerlessVersion; v { case auroraServerlessVersionV1: return addon.NewServerlessV1Template(props), nil case auroraServerlessVersionV2: return addon.NewServerlessV2Template(props), nil default: return nil, fmt.Errorf("unknown Aurora serverless version %q", v) }

We shouldn't need to set the AuroraServerlessVersion as that's done by the constructor in the addon pkg

Yes I noticed. I reduced duplication.

Lou1415926

@hkford Thank you very much! This looks great. I don't have much to add in addition to what Efe said. I will approve and add the do-not-merge label for you to address Efe's feedback!

Lou1415926 · 2022-10-18T20:30:23Z

+			inAppName:           "bowie",
+			inStorageType:       rdsStorageType,
+			inServerlessVersion: auroraServerlessVersionV1,
+			wantedErr:           nil,


nit: since nil is the zero value for wantedErr, it would by default be nil -

Suggested change

wantedErr: nil,

Lou1415926 · 2022-10-18T20:30:35Z

+			inAppName:           "bowie",
+			inStorageType:       rdsStorageType,
+			inServerlessVersion: auroraServerlessVersionV2,
+			wantedErr:           nil,


samesies!

Suggested change

wantedErr: nil,

I removed wantedErr: nil in two test cases and left other test cases alone. Should I remove other wantedErr: nil or do so in another PR?

Lou1415926 · 2022-10-18T20:30:58Z

+			mockStore:           func(m *mocks.Mockstore) {},
+			inAppName:           "bowie",
+			inStorageType:       rdsStorageType,
+			inServerlessVersion: "weird-serverless-version",


weird version!

I updated `copilot storage init` command's documentation to include Aurora Serverless Flags and examples of creating both Aurora Serverless v1 and v2. The aurora Serverless v2 feature is merged at [this PR](#4075). By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

feat: add aurora serverless v2 in storage

5332879

hkford requested a review from a team as a code owner October 10, 2022 13:09

hkford requested review from iamhopaul123 and removed request for a team October 10, 2022 13:09

amazon-ecs-cli-v2-pr-manager requested a review from Lou1415926 October 10, 2022 13:09

huanjani reviewed Oct 10, 2022

View reviewed changes

Comment thread internal/pkg/cli/flag.go Outdated

hkford added 2 commits October 11, 2022 16:43

fix: storageRDSServerlessVersionFlagDescription to lowercase

0ff5b37

fix: change Aurora Serverless version flag to lowercase

4024c6a

paragbhingre added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 11, 2022

Lou1415926 reviewed Oct 11, 2022

View reviewed changes

paragbhingre removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 11, 2022

hkford added 2 commits October 12, 2022 13:51

fix: set default aurora serverless version to v2

d12efd1

fix: remove unused storageInitRDSServerlessVersionPrompt

8a2ab8e

efekarakus reviewed Oct 13, 2022

View reviewed changes

Lou1415926 reviewed Oct 13, 2022

View reviewed changes

hkford added 4 commits October 17, 2022 12:22

fix: reflect comments from reviewers

6e3c1e7

fix: update NewRDSTemplate to NewServerlessV2Template

89fcad0

fix: testdata for integration test of aurora serverless

a3af1eb

fix: testdata environment name for integration test of aurora serverless

d5f2824

efekarakus reviewed Oct 18, 2022

View reviewed changes

Lou1415926 approved these changes Oct 18, 2022

View reviewed changes

Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 18, 2022

fix: auroraServerlessVersion in addon/storage.go to private field

3dfc59c

efekarakus approved these changes Oct 26, 2022

View reviewed changes

efekarakus removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 26, 2022

Merge branch 'mainline' into add-aurora-serverless-v2

dd1a3ab

mergify Bot merged commit 9701434 into aws:mainline Oct 26, 2022

hkford deleted the add-aurora-serverless-v2 branch October 28, 2022 06:50

hkford mentioned this pull request Nov 1, 2022

docs(storage): add Aurora Serverless v2 instruction #4144

Merged

	defaultServerlessVersionString = "v2"
	defaultServerlessVersion = serverlessVersionV2

		defaultServerlessVersion := defaultServerlessVersionString
		o.rdsServerlessVersion = defaultServerlessVersion

	cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, "", storageRDSServerlessVersionFlagDescription)
	cmd.Flags().StringVar(&vars.rdsServerlessVersion, storageRDSServerlessVersionFlag, defaultServerlessVersion, storageRDSServerlessVersionFlagDescription)

		storageRDSServerlessVersionFlagDescription = `Optional. Aurora Serverless version. Must be either "v1" or "v2".
		(default v2)`

		AuroraServerlessVersionV1 = "v1"
		AuroraServerlessVersionV2 = "v2"

	AuroraServerlessVersion string // The version of Aurora Serverless
	auroraServerlessVersion string // The version of Aurora Serverless.

Conversation

hkford commented Oct 10, 2022

Uh oh!

codecov-commenter commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

huanjani Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

hkford commented Oct 12, 2022

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lou1415926 Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkford commented Oct 17, 2022

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 10, 2022 •

edited

Loading

huanjani Oct 10, 2022 •

edited

Loading

Lou1415926 Oct 14, 2022 •

edited

Loading