Add `Table` resource custom hooks, terminalCodes and e2e tests #3

a-hilaly · 2021-06-02T22:28:59Z

Part of aws-controllers-k8s/community#803

Description of changes

Add custom hooks, terminalCodes, printcomns to Table resource in generator.yaml
Add e2e tests for Table create and delete operations

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

RedbackThomson · 2021-06-02T22:35:51Z

pkg/resource/table/conditions.go

+// collection that is of type ConditionTypeResourceSynced. If no such condition
+// is found, returns nil.
+//
+// TODO(jaypipes): Move to ACK code-gen templates.


?? Are these files not auto generated?

Not yet, I guess we still need to experiment with other controllers to figure out a general design

/cc @jaypipes

@RedbackThomson yeah, eventually I'd like to generate functions like these and/or move into ACK runtime. I added these for the mq-controller:

https://github.com/aws-controllers-k8s/mq-controller/blob/main/pkg/resource/broker/conditions.go
https://github.com/aws-controllers-k8s/mq-controller/blob/main/pkg/resource/broker/hooks.go

and then adapted them for the RDS controller:

https://github.com/aws-controllers-k8s/rds-controller/blob/main/pkg/resource/db_instance/conditions.go
https://github.com/aws-controllers-k8s/rds-controller/blob/main/pkg/resource/db_instance/hooks.go

I use the service controllers as a bit of a laboratory for experimentation in this way while I figure out what works as a standard.

3 controllers using same code, good time to move ?
aws-controllers-k8s/community#806

3 controllers using same code, good time to move ?
aws-controllers-k8s/community#806

after I get those logging changes pushed up, yeah...

pkg/resource/table/hooks.go

RedbackThomson · 2021-06-02T22:38:45Z

pkg/resource/table/hooks.go

+var (
+	requeueWaitWhileDeleting = ackrequeue.NeededAfter(
+		ErrTableDeleting,
+		5*time.Second,


Is this an arbitrary amount? I'd like to see requeue times standardised over all controllers, perhaps?

Not arbitrary, just remarked that tables are created super fast. The default value is 30seconds https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/requeue/requeue.go#L20-L22

pkg/resource/table/hooks.go

RedbackThomson · 2021-06-02T22:41:59Z

templates/hooks/table/sdk_delete_pre_build_request.go.tpl

+	if tableDeleting(r) {
+		return requeueWaitWhileDeleting
+	}


Table could be in deleting state before calling delete? I imagine if someone messed with it in the console (outside ACK)?

Yup - not sure if this is something we should consider at this stage.

templates/hooks/table/sdk_update_pre_build_request.go.tpl

RedbackThomson · 2021-06-02T23:03:47Z

/ok-to-test
/test dynamodb-unit-test
/test dynamodb-kind-e2e

ack-bot · 2021-06-03T08:27:04Z

@a-hilaly: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test dynamodb-unit-test

In response to this:

/test dynamodb-pre-submit

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 · 2021-06-03T08:27:49Z

/test dynamodb-kind-e2e

ack-bot · 2021-06-03T08:27:51Z

@a-hilaly: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test dynamodb-unit-test

In response to this:

/test dynamodb-kind-e2e

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 · 2021-06-03T09:14:30Z

@RedbackThomson i'm not able to run kind-e2e tests, i can see that we already have generated prow jobs for dynamodb. PTAL

a-hilaly · 2021-06-03T10:06:15Z

/test dynamodb-unit-test

a-hilaly · 2021-06-03T10:07:08Z

/test dynamodb-kind-e2e

ack-bot · 2021-06-03T10:07:10Z

@a-hilaly: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test dynamodb-unit-test

In response to this:

/test dynamodb-kind-e2e

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.

pkg/resource/table/conditions.go

pkg/resource/table/custom_update_api.go

jaypipes · 2021-06-03T14:50:14Z

templates/hooks/table/sdk_read_one_post_set_output.go.tpl

+	if isTableCreating(&resource{ko}) {
+		return &resource{ko}, requeueWaitWhileCreating
+	}
+	if isTableUpdating(&resource{ko}) {
+		return &resource{ko}, requeueWaitWhileUpdating
+	}


Why do we need the above code hooks in the sdkReadOne post-set-output code paths?

Sometimes tables needs few seconds to get created. re-queuing here is needed to sync the .status.TableStatus field.

Here what i get when i create a table and wait for few seconds

k get tables NAME TABLENAME TABLESTATUS tb1 tb1 CREATING

aws dynamodb describe-table --table-name tb1 | jq { ... "TableStatus": "ACTIVE", ... }

This is fixing a very similar issue to what @phanimullapudi was seeing in mq controller aws-controllers-k8s/community#831

for reviewers: @a-hilaly and I walked through the code and Amine patiently waited for me to understand he was totally correct. :)

apis/v1alpha1/table.go

test/e2e/resources/table.yaml

RedbackThomson · 2021-06-03T16:55:43Z

/test dynamodb-kind-e2e

a-hilaly · 2021-06-04T12:39:31Z

/test dynamodb-kind-e2e

RedbackThomson · 2021-06-04T21:38:43Z

/test dynamodb-kind-e2e

a-hilaly · 2021-06-08T15:03:46Z

/test dynamodb-kind-e2e

- Add custom hooks and terminalCodes - Regenerate `Table` resource - Add e2e tests for create and delete operations

a-hilaly · 2021-06-09T18:31:59Z

/test dynamodb-kind-e2e

jaypipes

👍

jaypipes · 2021-06-14T13:09:13Z

templates/hooks/table/sdk_read_one_post_set_output.go.tpl

+	if isTableCreating(&resource{ko}) {
+		return &resource{ko}, requeueWaitWhileCreating
+	}
+	if isTableUpdating(&resource{ko}) {
+		return &resource{ko}, requeueWaitWhileUpdating
+	}


for reviewers: @a-hilaly and I walked through the code and Amine patiently waited for me to understand he was totally correct. :)

jaypipes · 2021-06-14T13:09:45Z

/lgtm

ack-bot · 2021-06-14T13:09:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 June 2, 2021 22:29

ack-bot added the approved label Jun 2, 2021

RedbackThomson reviewed Jun 2, 2021

View reviewed changes

ack-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 2, 2021

a-hilaly force-pushed the crd/table branch from c571d3e to cfc7bda Compare June 3, 2021 10:03

This was referenced Jun 3, 2021

Add GlobalTable resource custom hooks, terminalCodes and e2e tests #4

Merged

Add Backup resource custom hooks, terminalCodes and e2e tests #5

Merged

jaypipes requested changes Jun 3, 2021

View reviewed changes

a-hilaly force-pushed the crd/table branch from cfc7bda to df909f7 Compare June 4, 2021 12:02

a-hilaly changed the title ~~Add Table resource custom hooks, terminalCodes, printcolumns and e2e tests~~ Add Table resource custom hooks, terminalCodes and e2e tests Jun 8, 2021

a-hilaly requested a review from RedbackThomson June 8, 2021 15:26

Add Table resource custom hooks, terminalCodes and e2e tests

49c67c3

- Add custom hooks and terminalCodes - Regenerate `Table` resource - Add e2e tests for create and delete operations

a-hilaly force-pushed the crd/table branch from df909f7 to 49c67c3 Compare June 9, 2021 18:11

jaypipes approved these changes Jun 14, 2021

View reviewed changes

ack-bot assigned jaypipes Jun 14, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2021

ack-bot merged commit e786654 into aws-controllers-k8s:main Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Table` resource custom hooks, terminalCodes and e2e tests #3

Add `Table` resource custom hooks, terminalCodes and e2e tests #3

a-hilaly commented Jun 2, 2021 •

edited

Loading

RedbackThomson Jun 2, 2021

a-hilaly Jun 2, 2021

jaypipes Jun 3, 2021

surajkota Jun 10, 2021

jaypipes Jun 10, 2021

RedbackThomson Jun 2, 2021

a-hilaly Jun 2, 2021

RedbackThomson Jun 2, 2021

a-hilaly Jun 2, 2021

RedbackThomson commented Jun 2, 2021

ack-bot commented Jun 3, 2021

a-hilaly commented Jun 3, 2021

ack-bot commented Jun 3, 2021

a-hilaly commented Jun 3, 2021 •

edited

Loading

a-hilaly commented Jun 3, 2021

a-hilaly commented Jun 3, 2021

ack-bot commented Jun 3, 2021

jaypipes Jun 3, 2021

a-hilaly Jun 4, 2021

a-hilaly Jun 4, 2021

a-hilaly Jun 10, 2021

jaypipes Jun 14, 2021

RedbackThomson commented Jun 3, 2021

a-hilaly commented Jun 4, 2021

RedbackThomson commented Jun 4, 2021

a-hilaly commented Jun 8, 2021

a-hilaly commented Jun 9, 2021

jaypipes left a comment

jaypipes Jun 14, 2021

jaypipes commented Jun 14, 2021

ack-bot commented Jun 14, 2021

Add Table resource custom hooks, terminalCodes and e2e tests #3

Add Table resource custom hooks, terminalCodes and e2e tests #3

Conversation

a-hilaly commented Jun 2, 2021 • 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

Choose a reason for hiding this comment

RedbackThomson commented Jun 2, 2021

ack-bot commented Jun 3, 2021

a-hilaly commented Jun 3, 2021

ack-bot commented Jun 3, 2021

a-hilaly commented Jun 3, 2021 • edited Loading

a-hilaly commented Jun 3, 2021

a-hilaly commented Jun 3, 2021

ack-bot commented Jun 3, 2021

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

RedbackThomson commented Jun 3, 2021

a-hilaly commented Jun 4, 2021

RedbackThomson commented Jun 4, 2021

a-hilaly commented Jun 8, 2021

a-hilaly commented Jun 9, 2021

jaypipes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypipes commented Jun 14, 2021

ack-bot commented Jun 14, 2021

Add `Table` resource custom hooks, terminalCodes and e2e tests #3

Add `Table` resource custom hooks, terminalCodes and e2e tests #3

a-hilaly commented Jun 2, 2021 •

edited

Loading

a-hilaly commented Jun 3, 2021 •

edited

Loading