Add `Table` update code path #30

a-hilaly · 2022-02-11T17:04:02Z

Issues:

Description of changes:

Add Table custom update code
Add Table custom preCompare delta code (to properly compare
GlobalSecondaryIndexes ,AttributeDefinitions and KeySchemas)
Add unit tests for custom compare code for GlobalSecondaryIndexes
Make KeySchema an immutable field.
Gradually update GlobalSecondaryIndexes when multiple ones are
update/removed or added.
Add custom code to properly populate BillingMode and
SSESpecification and TableClass
Add e2e tests for each new support field update + one test to ensure
that multi-field updates at once can be properly handled

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

ack-bot · 2022-02-11T17:04:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly

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]

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

RedbackThomson

Looks pretty ready. Just found some typos in your diffs, and I have a question regarding re-queueing because of the nature of asynchronous updates.

pkg/resource/table/hooks.go

pkg/resource/table/hooks_tags.go

a-hilaly · 2022-02-23T14:08:01Z

/retest all

ack-bot · 2022-02-23T14:08:03Z

@a-hilaly: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test dynamodb-kind-e2e
/test dynamodb-release-test
/test dynamodb-recommended-policy-test
/test dynamodb-unit-test

Use /test all to run all jobs.

In response to this:

/retest all

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.

a-hilaly · 2022-02-23T14:08:19Z

/test dynamodb-kind-e2e

jaypipes · 2022-02-22T14:48:40Z

generator.yaml

+          is_ignored: true
+      KeySchema:
+        compare:
+          is_immutable: true


is it necessary to have is_immutable when is_ignored is true? :)

is_immutable doesn't ignore the field comparison in the generated delta function. The reason I added an is_ignored: true is that the Dynamodb control plane sorts the KeySchema array, and sometimes causes the controller to reconcile for ever since reflect.DeepEqual takes in consideration the order to elements. I added a custom equalKeySchemaArrays function which properly compares the Custom Resource Spec.KeySchema with the sorted one returned by the dynamodb control plane.

@a-hilaly note that is_immutable is a configuration on the FieldConfig, not the CompareConfig.

@a-hilaly is_immutable is a configuration on the FieldConfig, not the CompareConfig, so this is being ignored entirely...

You're I brain lagged here. Addressed

pkg/resource/table/hooks.go

jaypipes · 2022-06-01T19:09:47Z

@a-hilaly you want to rebase this and get it going again?

jaypipes · 2022-07-06T16:33:28Z

@a-hilaly where are we with this?

ack-bot · 2022-10-04T17:46:39Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

ack-bot · 2022-11-03T17:47:03Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten

ack-bot · 2022-12-03T17:47:27Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

ack-bot · 2022-12-03T17:47:30Z

@ack-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

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.

a-hilaly · 2022-12-03T17:49:46Z

/reopen
/lifecycle frozen

a-hilaly · 2023-04-04T23:18:44Z

@jaypipes @Julian-Chu rebased and pushed latest changes PTAL

a-hilaly · 2023-04-05T16:57:10Z

/retest

pkg/resource/table/hooks.go

test/e2e/resources/table_billing.yaml

test/e2e/tests/test_table.py

jaypipes · 2023-04-05T18:40:11Z

pkg/resource/table/hooks_global_secondary_indexes.go

+// equalGlobalSecondaryIndexesArrays returns true if two GlobalSecondaryIndex
+// arrays are equal regardless of the order of their elements.
+func equalGlobalSecondaryIndexesArrays(
+	a []*v1alpha1.GlobalSecondaryIndex,
+	b []*v1alpha1.GlobalSecondaryIndex,
+) bool {
+	added, updated, removed := computeGlobalSecondaryIndexDelta(a, b)
+	return len(added) == 0 && len(updated) == 0 && len(removed) == 0
+}
+


I don't see this function being used anywhere?

It is used in customerPreCompare:

if len(a.ko.Spec.GlobalSecondaryIndexes) != len(b.ko.Spec.GlobalSecondaryIndexes) { delta.Add("Spec.GlobalSecondaryIndexes", a.ko.Spec.GlobalSecondaryIndexes, b.ko.Spec.GlobalSecondaryIndexes) } else if a.ko.Spec.GlobalSecondaryIndexes != nil && b.ko.Spec.GlobalSecondaryIndexes != nil { if !equalGlobalSecondaryIndexesArrays(a.ko.Spec.GlobalSecondaryIndexes, b.ko.Spec.GlobalSecondaryIndexes) { delta.Add("Spec.GlobalSecondaryIndexes", a.ko.Spec.GlobalSecondaryIndexes, b.ko.Spec.GlobalSecondaryIndexes) } }

pkg/resource/table/hooks_global_secondary_indexes.go

pkg/resource/table/hooks_key_schema.go

pkg/resource/table/hooks_local_secondary_indexes.go

jaypipes · 2023-04-05T18:47:50Z

pkg/resource/table/hooks_local_secondary_indexes.go

+	"github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1"
+)
+
+func computeLocalSecondaryIndexDelta(


Are local secondary indexes ever updated?

Local secondary indexes can only be set during the CREATE op. They are immutable. computeLocalSecondaryIndexDelta is used to set a terminal condition if users try to modify them

test/e2e/tag.py

jaypipes · 2023-04-05T18:49:44Z

test/e2e/tests/test_table.py

+        assert self.table_exists(table_name)
+
+        # Get CR latest revision
+        cr = k8s.wait_resource_consumed_by_controller(ref)


above tests are great. I don't see where local secondary indexes are ever tested though? or updated...

Same reason here. LSIs are only mentioned during the creation, and the API will reject any non-valid LSIs, checking that the table is active is equivalent to checking the LSIs

test/e2e/tests/test_global_table.py

test/e2e/tests/test_table.py

jaypipes

Great work on this @a-hilaly. I'm super excited to finally see this getting merged. 👍

Signed-off-by: Amine Hilaly <[email protected]>

a-hilaly · 2023-04-06T16:56:06Z

/retest

jljaco · 2023-04-06T17:26:10Z

/lgtm

ack-prow · 2023-04-06T17:26:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, jljaco

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,jaypipes]

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

ack-bot requested review from jaypipes and vijtrip2 February 11, 2022 17:04

ack-bot added the approved label Feb 11, 2022

a-hilaly force-pushed the table/updates branch 2 times, most recently from cc124fb to a156e26 Compare February 11, 2022 17:08

RedbackThomson suggested changes Feb 15, 2022

View reviewed changes

a-hilaly force-pushed the table/updates branch 2 times, most recently from 1e3c998 to 58f4ec0 Compare February 23, 2022 13:47

jaypipes requested changes Feb 25, 2022

View reviewed changes

a-hilaly force-pushed the table/updates branch 2 times, most recently from a95deaf to 5ff5e02 Compare March 3, 2022 14:13

ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2022

aws-controllers-k8s deleted a comment from ack-bot Jun 1, 2022

jaypipes removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2022

ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 4, 2022

ack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 3, 2022

ack-bot closed this Dec 3, 2022

ack-bot reopened this Dec 3, 2022

a-hilaly removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 4, 2023

a-hilaly force-pushed the table/updates branch 8 times, most recently from ffcbe1b to a048d49 Compare April 5, 2023 16:34

RedbackThomson reviewed Apr 5, 2023

View reviewed changes

pkg/resource/table/hooks.go Outdated Show resolved Hide resolved

test/e2e/resources/table_billing.yaml Outdated Show resolved Hide resolved

test/e2e/tests/test_table.py Outdated Show resolved Hide resolved

a-hilaly force-pushed the table/updates branch 2 times, most recently from de4670c to 700ca2a Compare April 5, 2023 18:14

jaypipes reviewed Apr 5, 2023

View reviewed changes

jljaco reviewed Apr 5, 2023

View reviewed changes

a-hilaly force-pushed the table/updates branch from 700ca2a to 117749e Compare April 5, 2023 21:38

a-hilaly requested review from jljaco, RedbackThomson and jaypipes and removed request for vijtrip2 April 5, 2023 21:39

jaypipes approved these changes Apr 6, 2023

View reviewed changes

Add dynamodb table updates

e55b0e6

Signed-off-by: Amine Hilaly <[email protected]>

a-hilaly force-pushed the table/updates branch from 117749e to e55b0e6 Compare April 6, 2023 16:30

ack-prow bot assigned jljaco Apr 6, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023

ack-prow bot merged commit b0b0d59 into aws-controllers-k8s:main Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Table` update code path #30

Add `Table` update code path #30

a-hilaly commented Feb 11, 2022 •

edited

Loading

ack-bot commented Feb 11, 2022

RedbackThomson left a comment

a-hilaly commented Feb 23, 2022

ack-bot commented Feb 23, 2022

a-hilaly commented Feb 23, 2022

jaypipes Feb 22, 2022

a-hilaly Mar 3, 2022

jaypipes Dec 9, 2022

jaypipes Apr 5, 2023

a-hilaly Apr 5, 2023

jaypipes commented Jun 1, 2022

jaypipes commented Jul 6, 2022

ack-bot commented Oct 4, 2022

ack-bot commented Nov 3, 2022

ack-bot commented Dec 3, 2022

ack-bot commented Dec 3, 2022

a-hilaly commented Dec 3, 2022

a-hilaly commented Apr 4, 2023

a-hilaly commented Apr 5, 2023

jaypipes Apr 5, 2023

a-hilaly Apr 5, 2023

jaypipes Apr 5, 2023

a-hilaly Apr 5, 2023

jaypipes Apr 5, 2023

a-hilaly Apr 5, 2023

jaypipes left a comment

a-hilaly commented Apr 6, 2023

jljaco commented Apr 6, 2023

ack-prow bot commented Apr 6, 2023

Add Table update code path #30

Add Table update code path #30

Conversation

a-hilaly commented Feb 11, 2022 • edited Loading

ack-bot commented Feb 11, 2022

RedbackThomson left a comment

Choose a reason for hiding this comment

a-hilaly commented Feb 23, 2022

ack-bot commented Feb 23, 2022

a-hilaly commented Feb 23, 2022

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

jaypipes commented Jun 1, 2022

jaypipes commented Jul 6, 2022

ack-bot commented Oct 4, 2022

ack-bot commented Nov 3, 2022

ack-bot commented Dec 3, 2022

ack-bot commented Dec 3, 2022

a-hilaly commented Dec 3, 2022

a-hilaly commented Apr 4, 2023

a-hilaly commented Apr 5, 2023

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

jaypipes left a comment

Choose a reason for hiding this comment

a-hilaly commented Apr 6, 2023

jljaco commented Apr 6, 2023

ack-prow bot commented Apr 6, 2023

Add `Table` update code path #30

Add `Table` update code path #30

a-hilaly commented Feb 11, 2022 •

edited

Loading