search

terraform-aws-modules / terraform-aws-dms

Terraform module to create AWS DMS (Database Migration Service) resources 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/dms/aws
Apache License 2.0
64 stars 100 forks source link

feat(multi-rep-instances): Adding multiple replication instances #35

Closed sjwood00 closed 1 year ago

sjwood00 commented 1 year ago

Description

Allows the creation of multiple replication instances. Allows the creation of multiple subnet groups for the different replication instances. Allows for the creation of multiple subnet groups.

Motivation and Context

For performance reasons and because replication instances have a finite amount or resources it is common practice to create multiple replication instances and split the replication tasks between them. I observed an issue with a replication subnet group not being ready on replication instance creation so I added a separate wait for that also.

Breaking Changes

Breaking change because of the new replication_instances variable which is a map of objects rather than having multiple, separate variables. Breaking change because new subnet_groups variable which is a map of objects rather than having multiple, separate variables. Breaking change due to the use of optional variable syntax which from tf 1.3.0 onwards was no longer experimental.

How Has This Been Tested?

I have used this code to create a replication instance and replication tasks and performed data migrations. More testing/edits/improvements most welcome.

bryantbiggs commented 1 year ago

For performance reasons and because replication instances have a finite amount or resources it is common practice to create multiple replication instances and split the replication tasks between them.

This is a true statement, but the PR design is incorrect for DMS. Have you seen https://github.com/terraform-aws-modules/terraform-aws-dms#combinations ?

DMS is NOT a service that is setup for clustering, and what you are trying to do in this PR is somewhat attempt to treat it as if it were a cluster. This is problematic and does not match how DMS is setup today

Instead, to achieve your desired state of multiple DMS replication instances, you can use a for_each over a single module definition, you can use multiple module definitions, one for each DMS replication instance.

sjwood00 commented 1 year ago

DMS is NOT a service that is setup for clustering, and what you are trying to do in this PR is somewhat attempt to treat it as if it were a cluster

Fair enough, not sure I completely agree with the cluster bit though as there's no coordination between the replication instances and there doesn't necessarily have to be a link between them at all them aside from being generated from the same module (e.g. they could use different endpoints and have a different set of tasks). I do like the "for_each over a single module definition" suggestion though as that will avoid a breaking change although it's perhaps not something everyone would think of initially(me included!).

bryantbiggs commented 1 year ago

The cluster bit is not related to coordination, but stems form the fact that users will see they can pass in multiple replication instances but they have to map their replication tasks appropriately across those instances to avoid "hot spotting" on one or a few instances. DMS is really designed to support 1:1 or 1:n of instance to tasks, but it does not do n:m which is what a cluster would be used for.

github-actions[bot] commented 1 year ago

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.