Add `TableReplicas` Support for DynamoDB Table Replicas #120

rushmash91 · 2025-03-13T23:00:50Z

fixes aws-controllers-k8s/community#2077

This PR implements support for managing DynamoDB table replicas through the tableReplicas field. This enhancement allows users to manage multi-region table replicas that automatically remain in sync. This is complemented by a replicaDescription field in the Table status for tracking replica states.

Changes Overview

Added tableReplicas field to Table spec for defining replica configurations across regions
Added controller logic for replica lifecycle management (creation, updates, deletion)
Implemented validation rules:
- DynamoDB Streams must be enabled with NEW_AND_OLD_IMAGES
- Terminal error conditions for invalid configurations
E2E tests

a-hilaly

Thanks @rushmash91 !

apis/v1alpha1/table.go

pkg/resource/table/hooks.go

pkg/resource/table/hooks_replica_updates.go

templates/hooks/table/sdk_delete_pre_build_request.go.tpl

templates/hooks/table/sdk_read_one_post_set_output.go.tpl

pkg/resource/table/hooks.go

pkg/resource/table/hooks_replica_updates.go

templates/hooks/table/sdk_create_post_set_output.go.tpl

a-hilaly

Getting close!

pkg/resource/table/hooks.go

pkg/resource/table/hooks_replica_updates.go

a-hilaly · 2025-03-24T06:09:58Z

templates/hooks/table/sdk_read_one_post_set_output.go.tpl

+		ko.Spec.TableReplicas = tableReplicas
+	} else {
+		ko.Spec.TableReplicas = nil


Are we supposed to also update the status in here?

Already being handled by sdkfind. So, not needed.

a-hilaly · 2025-03-24T06:11:13Z

templates/hooks/table/sdk_read_one_post_set_output.go.tpl

+		tableReplicas := []*svcapitypes.CreateReplicationGroupMemberAction{}
+		for _, replica := range resp.Table.Replicas {
+			replicaElem := &svcapitypes.CreateReplicationGroupMemberAction{}
+			if replica.RegionName != nil {
+				replicaElem.RegionName = replica.RegionName
+			}
+			if replica.KMSMasterKeyId != nil {
+				replicaElem.KMSMasterKeyID = replica.KMSMasterKeyId
+			}
+			if replica.ProvisionedThroughputOverride != nil {
+				replicaElem.ProvisionedThroughputOverride = &svcapitypes.ProvisionedThroughputOverride{
+					ReadCapacityUnits: replica.ProvisionedThroughputOverride.ReadCapacityUnits,
+				}
+			}
+			if replica.GlobalSecondaryIndexes != nil {
+				gsiList := []*svcapitypes.ReplicaGlobalSecondaryIndex{}
+				for _, gsi := range replica.GlobalSecondaryIndexes {
+					gsiElem := &svcapitypes.ReplicaGlobalSecondaryIndex{
+						IndexName: gsi.IndexName,
+					}
+					if gsi.ProvisionedThroughputOverride != nil {
+						gsiElem.ProvisionedThroughputOverride = &svcapitypes.ProvisionedThroughputOverride{
+							ReadCapacityUnits: gsi.ProvisionedThroughputOverride.ReadCapacityUnits,
+						}
+					}
+					gsiList = append(gsiList, gsiElem)
+				}
+				replicaElem.GlobalSecondaryIndexes = gsiList
+			}
+			if replica.ReplicaTableClassSummary != nil && replica.ReplicaTableClassSummary.TableClass != "" {
+				replicaElem.TableClassOverride = aws.String(string(replica.ReplicaTableClassSummary.TableClass))
+			}
+			tableReplicas = append(tableReplicas, replicaElem)
+		}
+		ko.Spec.TableReplicas = tableReplicas
+	} else {


this logic can be move to one of the hooks files and we can just do:

if len(resp.Table.Replicas) > 0 { setTableReplicas(...) }

a-hilaly · 2025-03-24T06:13:16Z

test/e2e/tests/test_table_replicas.py

+        # Wait for both initial replicas to be created
+        table.wait_until(
+            table_name,
+            table.replicas_match([REPLICA_REGION_1, REPLICA_REGION_2]),
+            timeout_seconds=REPLICA_WAIT_AFTER_SECONDS,
+            interval_seconds=30,
+        )
+
+        # Wait for both replicas to be active
+        for region in [REPLICA_REGION_1, REPLICA_REGION_2]:
+            table.wait_until(
+                table_name,
+                table.replica_status_matches(region, "ACTIVE"),
+                timeout_seconds=REPLICA_WAIT_AFTER_SECONDS,
+                interval_seconds=30,
+            )


Question: isn't the wait in L145, sufficiant? will the table ever be marked as ACTIVE, when the replicas haven't been fully created?

No, the table can reach ACTIVE status before all of its replicas are fully active.
So, I wait for table to become active -> check if the replicas exist -> wait for all replicas to be active

a-hilaly · 2025-03-24T06:17:06Z

test/e2e/tests/test_table_replicas.py

+            if cr and "status" in cr and "conditions" in cr["status"]:
+                for condition_obj in cr["status"]["conditions"]:
+                    if condition_obj["type"] == "ACK.Terminal" and condition_obj["status"] == "True":
+                        terminal_condition_set = True
+                        # Verify the error message
+                        assert "table must have DynamoDB Streams enabled with StreamViewType set to NEW_AND_OLD_IMAGES" in condition_obj[
+                            "message"]
+                        break
+
+            if terminal_condition_set:
+                break
+


you could also use https://github.com/aws-controllers-k8s/test-infra/blob/main/src/acktest/k8s/condition.py#L29 (i know not all controllers are using them)

a-hilaly · 2025-03-24T06:21:52Z

test/e2e/tests/test_table_replicas.py

+        # Create the k8s resource
+        ref = k8s.CustomResourceReference(
+            CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+            table_name, namespace="default",
+        )


if this test fail, L528 will never be executed which will leave a dangling DDB Table in our accounts. Ideally you want to use a function that provisions the table, yields the info you need, and calls a delete function at the end.

a-hilaly

Nice work on this @rushmash91 !

michaelhtm

Great job on this one @rushmash91 🚀
left a few nits

michaelhtm · 2025-03-25T00:51:29Z

pkg/resource/table/hooks.go

@@ -224,6 +233,15 @@ func (rm *resourceManager) customUpdateTable(
 				}
 				return nil, err
 			}
+		case delta.DifferentAt("Spec.TableReplicas"):
+			if !hasStreamSpecificationWithNewAndOldImages(desired) {
+				msg := "table must have DynamoDB Streams enabled with StreamViewType set to NEW_AND_OLD_IMAGES for replica updates"


does this actually affect it? it would be nice to have comments explaining why...maybe documentation for the table as well

michaelhtm · 2025-03-25T00:57:58Z

pkg/resource/table/hooks_replica_updates.go

+)
+
+// equalCreateReplicationGroupMemberActions compares two CreateReplicationGroupMemberAction objects
+func equalCreateReplicationGroupMemberActions(a, b *v1alpha1.CreateReplicationGroupMemberAction) bool {


nit:

Suggested change

func equalCreateReplicationGroupMemberActions(a, b *v1alpha1.CreateReplicationGroupMemberAction) bool {

func areReplicaGroupsDifferent(a, b *v1alpha1.CreateReplicationGroupMemberAction) bool {

same for others below?

I think it's more intuitive to keep the 'equal' naming pattern with negation rather than introducing 'areXDifferent'. using something like '!equalX()' maintains consistency with the existing naming convention accross all ACK

michaelhtm · 2025-03-25T01:00:51Z

pkg/resource/table/hooks_replica_updates.go

+}
+
+// equalReplicaGlobalSecondaryIndexes compares two ReplicaGlobalSecondaryIndex objects
+func equalReplicaGlobalSecondaryIndexes(


another nit: Should we move the delta logic out of this file? maybe create a new one called delta_hooks.go?

the controller is currently following this convention there are hooks files for GSI and continuous backup as well.
I'll have a separate PR making a delta files and a few other optimizations for all hooks.

michaelhtm · 2025-03-25T01:04:27Z

pkg/resource/table/hooks_replica_updates.go

+		for _, gsi := range replica.GlobalSecondaryIndexes {
+			replicaGSI := svcsdktypes.ReplicaGlobalSecondaryIndex{}
+			if gsi.IndexName != nil {
+				replicaGSI.IndexName = aws.String(*gsi.IndexName)


nit: why not this? or is indexName a different type?

Suggested change

replicaGSI.IndexName = aws.String(*gsi.IndexName)

replicaGSI.IndexName = gsi.IndexName

michaelhtm · 2025-03-25T01:07:36Z

pkg/resource/table/hooks_replica_updates.go

+func updateReplicaUpdate(replica *v1alpha1.CreateReplicationGroupMemberAction) svcsdktypes.ReplicationGroupUpdate {
+	replicaUpdate := svcsdktypes.ReplicationGroupUpdate{}
+	updateAction := &svcsdktypes.UpdateReplicationGroupMemberAction{}
+	isValidUpdate := false // updates to gsi without ProvisionedThroughputOverride are invalid


nit: should we set this to true and only change it to false when ProvisionedThroughputOverride is not defined?
or we could also not use it and just return nil when provisionedTroughputOverride is not defined

michaelhtm · 2025-03-25T01:11:27Z

pkg/resource/table/hooks_replica_updates.go

+	_, err = rm.sdkapi.UpdateTable(ctx, input)
+	rm.metrics.RecordAPICall("UPDATE", "UpdateTable", err)
+	if err != nil {
+			return err


michaelhtm · 2025-03-25T01:15:53Z

pkg/resource/table/hooks_replica_updates.go

+		exit(err)
+	}()
+
+	createReplicas, updateReplicas, deleteRegions := calculateReplicaUpdates(latest, desired)


nit: instead of calculating all here, what if we returned after we find a create, an update, or a delete, and call the update and requeue immediately? not sure if that would be overengineering tho..

just keeping consistency with the mess i created with GSIs

I prefer this. Also consistent with the GSI hooks. Since the api pattern is same. But renaming functions to computeReplicaupdatesDelta

michaelhtm · 2025-03-25T01:17:55Z

pkg/resource/table/hooks_replica_updates.go

+	}
+}
+
+func checkReplicaStatus(ReplicaDescription []*svcapitypes.ReplicaDescription, regionName string) bool {


nit: can this have a better name? maybe checkIfReplicasInProgress or checkIfReplicasUpdating

ack-prow · 2025-03-25T02:05:18Z

@rushmash91: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
dynamodb-verify-attribution	`652f3f4`	link	false	`/test dynamodb-verify-attribution`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

michaelhtm

Thanks @rushmash91 🚀
/lgtm

ack-prow · 2025-03-25T02:58:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, michaelhtm, rushmash91

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [a-hilaly,michaelhtm,rushmash91]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…controllers-k8s#120)" This reverts commit 755ecd8.

…ntrollers-k8s#120) This reverts commit 755ecd8. Ensure we keep the field in helm

add replica update to crd

3cc1a09

ack-prow bot requested review from a-hilaly and jlbutler March 13, 2025 23:00

ack-prow bot added the approved label Mar 13, 2025

rushmash91 force-pushed the main branch 6 times, most recently from ca89ad3 to b53e944 Compare March 17, 2025 17:18

rushmash91 changed the title ~~add replica update support~~ add replica support dynamoDB tables Mar 17, 2025

rushmash91 force-pushed the main branch from 27e3ed3 to 8c8f8ee Compare March 20, 2025 17:50

a-hilaly reviewed Mar 20, 2025

View reviewed changes

rushmash91 force-pushed the main branch 11 times, most recently from f42a7c9 to 81e1cbd Compare March 21, 2025 07:50

add tests

0185830

rushmash91 force-pushed the main branch 2 times, most recently from 0185830 to 75da9b2 Compare March 21, 2025 19:01

rushmash91 changed the title ~~add replica support dynamoDB tables~~ Add ReplicationGroup Support for DynamoDB Table Replicas Mar 21, 2025

rushmash91 force-pushed the main branch from 75da9b2 to 3411aee Compare March 21, 2025 20:02

a-hilaly reviewed Mar 21, 2025

View reviewed changes

rushmash91 force-pushed the main branch from ae67e34 to a69fd8f Compare March 22, 2025 10:11

Rename to table replicas

534418a

rushmash91 force-pushed the main branch 4 times, most recently from 5cdbb69 to 72c06f0 Compare March 24, 2025 02:23

a-hilaly reviewed Mar 24, 2025

View reviewed changes

rushmash91 force-pushed the main branch 10 times, most recently from c6facdd to 0550f7a Compare March 24, 2025 23:14

a-hilaly approved these changes Mar 25, 2025

View reviewed changes

michaelhtm reviewed Mar 25, 2025

View reviewed changes

delete fixes

08ebf41

rushmash91 force-pushed the main branch from 0550f7a to 08ebf41 Compare March 25, 2025 01:54

change update-replica style

652f3f4

michaelhtm approved these changes Mar 25, 2025

View reviewed changes

ack-prow bot assigned michaelhtm Mar 25, 2025

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2025

ack-prow bot merged commit 755ecd8 into aws-controllers-k8s:main Mar 25, 2025
6 of 7 checks passed

michaelhtm added a commit to michaelhtm/ack-dynamodb-controller that referenced this pull request Mar 27, 2025

Revert "Add TableReplicas Support for DynamoDB Table Replicas (aws-…

ebf4803

…controllers-k8s#120)" This reverts commit 755ecd8.

michaelhtm added a commit to michaelhtm/ack-dynamodb-controller that referenced this pull request Mar 27, 2025

Revert "Add TableReplicas Support for DynamoDB Table Replicas (aws-co…

7b6a0d0

…ntrollers-k8s#120) This reverts commit 755ecd8. Ensure we keep the field in helm

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `TableReplicas` Support for DynamoDB Table Replicas #120

Add `TableReplicas` Support for DynamoDB Table Replicas #120

rushmash91 commented Mar 13, 2025 •

edited

Loading

a-hilaly left a comment

a-hilaly left a comment

a-hilaly Mar 24, 2025

rushmash91 Mar 24, 2025

a-hilaly Mar 24, 2025

a-hilaly Mar 24, 2025

rushmash91 Mar 24, 2025

a-hilaly Mar 24, 2025

a-hilaly Mar 24, 2025

a-hilaly left a comment

michaelhtm left a comment

michaelhtm Mar 25, 2025

michaelhtm Mar 25, 2025

a-hilaly Mar 25, 2025 •

edited

Loading

michaelhtm Mar 25, 2025

rushmash91 Mar 25, 2025

michaelhtm Mar 25, 2025

michaelhtm Mar 25, 2025

michaelhtm Mar 25, 2025

michaelhtm Mar 25, 2025

a-hilaly Mar 25, 2025

rushmash91 Mar 25, 2025 •

edited

Loading

michaelhtm Mar 25, 2025

ack-prow bot commented Mar 25, 2025

michaelhtm left a comment

ack-prow bot commented Mar 25, 2025

	func equalCreateReplicationGroupMemberActions(a, b *v1alpha1.CreateReplicationGroupMemberAction) bool {
	func areReplicaGroupsDifferent(a, b *v1alpha1.CreateReplicationGroupMemberAction) bool {

	replicaGSI.IndexName = aws.String(*gsi.IndexName)
	replicaGSI.IndexName = gsi.IndexName

Add TableReplicas Support for DynamoDB Table Replicas #120

Add TableReplicas Support for DynamoDB Table Replicas #120

Conversation

rushmash91 commented Mar 13, 2025 • edited Loading

Changes Overview

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly left a comment

Choose a reason for hiding this comment

michaelhtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushmash91 Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ack-prow bot commented Mar 25, 2025

michaelhtm left a comment

Choose a reason for hiding this comment

ack-prow bot commented Mar 25, 2025

Add `TableReplicas` Support for DynamoDB Table Replicas #120

Add `TableReplicas` Support for DynamoDB Table Replicas #120

rushmash91 commented Mar 13, 2025 •

edited

Loading

a-hilaly Mar 25, 2025 •

edited

Loading

rushmash91 Mar 25, 2025 •

edited

Loading