Add support for tables `ContinuousBackup` #77

msvechla · 2023-07-19T16:24:00Z

Issue #, if available: #1790

Description of changes:

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

ack-prow · 2023-07-19T16:24:11Z

Hi @msvechla. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

msvechla · 2023-07-19T16:26:50Z

I'm able to set continuous backups to false and the backups are actually disabled on AWS side, however the controller keeps logging the following afterwards:

2023-07-19T16:25:23.077Z INFO ackrt desired resource state has changed {"account": "111", "role": "", "region": "eu-central-1", "kind": "Table", "namespace": "default", "name": "tenant-api-dynamodb-tenant-config", "is_adopted": false, "generation": 13, "diff": [{"Path":{"Parts":["Spec","ContinuousBackups","PointInTimeRecoveryEnabled"]},"A":false,"B":true}]}
2023-07-19T16:25:28.153Z INFO ackrt desired resource state has changed {"account": "111", "role": "", "region": "eu-central-1", "kind": "Table", "namespace": "default", "name": "my-dynamodb", "is_adopted": false, "generation": 13, "diff": [{"Path":{"Parts":["Spec","ContinuousBackups","PointInTimeRecoveryEnabled"]},"A":false,"B":true}]}

Setting backups to enabled is not working at all at the moment

a-hilaly · 2023-07-19T16:44:59Z

Setting backups to enabled is not working at all at the moment

This could be some eventual consistency of the DDB API. Maybe try the Put call that a quick Get call right after (using the CLI) to confirm?

msvechla · 2023-07-19T21:13:14Z

I found the issue, I was reading the wrong results from the DescribeContinuousBackupOutput:

{
    "ContinuousBackupsDescription": {
        "ContinuousBackupsStatus": "ENABLED",
        "PointInTimeRecoveryDescription": {
            "PointInTimeRecoveryStatus": "DISABLED"
        }
    }
}

I was checking against ContinuousBackupsStatus, when I should have checked against PointInTimeRecoveryStatus.
Everything is working as expected now.

I also bumped the acktest dependency as suggested in the issue

a-hilaly · 2023-07-19T21:29:35Z

/ok-to-test

a-hilaly · 2023-07-19T21:32:37Z

I was checking against ContinuousBackupsStatus, when I should have checked against PointInTimeRecoveryStatus.
Everything is working as expected now.

@msvechla makes sense! ~~Maybe we should name this field PointInTimeRecoveryEnabled and use the path PointInTimeRecoverySpecification.PointInTimeRecoveryEnabled instead?~~ ContinuousBackup is right here

a-hilaly · 2023-07-19T21:53:31Z

From the logs looks like the controller is struggling to understand the diff between latest and desired

2023-07-19T21:35:25.294Z	INFO	ackrt	desired resource state has changed	{"account": "021785965138", "role": "", "region": "us-west-2", "kind": "Table", "namespace": "default", "name": "table-lsi-7r1qmayaaekz3ycox3tiqw", "is_adopted": false, "generation": 1, "diff": [{"Path":{"Parts":["Spec","ContinuousBackups"]},"A":null,"B":{"pointInTimeRecoveryEnabled":false}}]}

msvechla · 2023-07-19T21:55:40Z

I was checking against ContinuousBackupsStatus, when I should have checked against PointInTimeRecoveryStatus.
Everything is working as expected now.

@msvechla makes sense! ~~Maybe we should name this field PointInTimeRecoveryEnabled and use the path PointInTimeRecoverySpecification.PointInTimeRecoveryEnabled instead?~~ ContinuousBackup is right here

Are you sure? I actually like just having pointInTimeRecoveryEnabled as a field on the Kubernetes resource. What made you rethink this?

I have the refactoring ready, just let me know whether I should push

a-hilaly

Thank you so much for working on this @msvechla - it's a great start! I left a few nit/comments below.

Also, it would be amazing if we can also have an e2e test case for this feature (very similar to the TTL ones)

apis/v1alpha1/ack-generate-metadata.yaml

a-hilaly · 2023-07-19T21:56:17Z

pkg/resource/table/hooks.go

@@ -187,6 +200,13 @@ func (rm *resourceManager) customUpdateTable(
 		}
 	}

+	if delta.DifferentAt("Spec.ContinuousBackups.PointInTimeRecoveryEnabled") {


Maybe also Spec.ContinuousBackups - a user could completly delete PointInTimeRecoveryEnabled to disable it.

pkg/resource/table/hooks.go

pkg/resource/table/hooks_continuous_backup.go

a-hilaly · 2023-07-19T22:08:31Z

Are you sure? I actually like just having pointInTimeRecoveryEnabled as a field on the Kubernetes resource. What made you rethink this?
I have the refactoring ready, just let me know whether I should push

@msvechla I initially thought that the call was PutPointInTime... but I saw it was named UpdateContinuousBackups. I'm just thinking what if one day the dynamodb team adds a new field to UpdateContinuousBackupsInput struct, do we want to keep adding new fields to the spec? or we keep everything encapsulated inside ContinuousBackups field. If you think that is very unlikely to happen i'm happy to go with a simple PointInTimeRecoveryEnabled field.

Related to this topic: We are also working on multi-version support (renaming, moving fields will be possible, with mutating webhooks)

msvechla · 2023-07-19T22:12:12Z

Are you sure? I actually like just having pointInTimeRecoveryEnabled as a field on the Kubernetes resource. What made you rethink this?
I have the refactoring ready, just let me know whether I should push

@msvechla I initially thought that the call was PutPointInTime... but I saw it was named UpdateContinuousBackups. I'm just thinking what if one day the dynamodb team adds a new field to UpdateContinuousBackupsInput struct, do we want to keep adding new fields to the spec? or we keep everything encapsulated inside ContinuousBackups field. If you think that is very unlikely to happen i'm happy to go with a simple PointInTimeRecoveryEnabled field.

Related to this topic: We are also working on multi-version support (renaming, moving fields will be possible, with mutating webhooks)

Okay thats a valid point, then I will leave the spec as it is currently 👍

pkg/resource/table/hooks.go

msvechla · 2023-07-20T10:59:51Z

Thank you so much for working on this @msvechla - it's a great start! I left a few nit/comments below.

Also, it would be amazing if we can also have an e2e test case for this feature (very similar to the TTL ones)

I have added some e2e tests, maybe you can take a look. Otherwise my manual tests also worked great now

a-hilaly

This is great @msvechla! I have two nits below and happy to merge right away

pkg/resource/table/hooks.go

a-hilaly · 2023-07-20T14:23:07Z

pkg/resource/table/hooks.go

+	if a.ko.Spec.ContinuousBackups == nil && b.ko.Spec.ContinuousBackups != nil &&
+		b.ko.Spec.ContinuousBackups.PointInTimeRecoveryEnabled != nil {
+		a.ko.Spec.ContinuousBackups = &v1alpha1.PointInTimeRecoverySpecification{
+			PointInTimeRecoveryEnabled: &DefaultPITREnabledValue,
+		}
+	}


pkg/resource/table/hooks_continuous_backup.go

test/e2e/tests/test_table.py

a-hilaly · 2023-07-20T14:31:09Z

/retitle Add support for tables ContinuousBackup

a-hilaly

Dankeshoen @msvechla ! :)
/lgtm

ack-prow · 2023-07-20T15:09:59Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

a-hilaly · 2023-07-20T15:10:37Z

Can you please send a release PR once this is merged? we can go for 1.2.0 (happy to do so as well but we can merge it CET Timezone :) )

msvechla · 2023-07-20T15:14:39Z

Can you please send a release PR once this is merged? we can go for 1.2.0

Thanks for your guidance, will do. Can I just follow this example by bumping the versions in those files manually? https://github.com/aws-controllers-k8s/dynamodb-controller/pull/72/files

Or is there some tooling around bumping the version?

a-hilaly · 2023-07-20T15:17:22Z

Thanks for your guidance, will do. Can I just follow this example by bumping the versions in those files manually? https://github.com/aws-controllers-k8s/dynamodb-controller/pull/72/files

Exactly! Those files can be regenerated with new versions using something like RELEASE_VERSION=v1.0.3 ./scripts/build-controller-release.sh dynamodb (from the code-generator repository) - full documentation is here https://aws-controllers-k8s.github.io/community/docs/contributor-docs/release/

Once the release PR is merged prow will take care of publishing new images/helm charts

msvechla · 2023-07-20T15:36:01Z

Thanks, created a release PR here: #78

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2023

ack-prow bot requested review from a-hilaly and vijtrip2 July 19, 2023 16:24

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2023

msvechla mentioned this pull request Jul 19, 2023

[dynamodb] allow specification of point-in-time recovery (PITR) aws-controllers-k8s/community#1790

Closed

msvechla force-pushed the continuous_backups branch from 24d29ad to d1bc057 Compare July 19, 2023 21:10

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2023

a-hilaly reviewed Jul 19, 2023

View reviewed changes

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

msvechla force-pushed the continuous_backups branch 2 times, most recently from 3df39b2 to b425e70 Compare July 20, 2023 09:46

msvechla changed the title ~~WIP: Continuous Backups~~ Continuous Backups Jul 20, 2023

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2023

a-hilaly reviewed Jul 20, 2023

View reviewed changes

a-hilaly changed the title ~~Continuous Backups~~ Add support for tables ContinuousBackup Jul 20, 2023

msvechla added 2 commits July 20, 2023 17:03

add continuous backup support (#1790)

4940ca5

run make build-controller (#1790)

20245ee

msvechla added 2 commits July 20, 2023 17:03

update acktest e2e test library to fix bug

4809370

add e2e tests for point in time recovery (#1790)

e41ee1c

msvechla force-pushed the continuous_backups branch from 1857a0c to e41ee1c Compare July 20, 2023 15:04

a-hilaly added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Jul 20, 2023

a-hilaly approved these changes Jul 20, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Jul 20, 2023

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

ack-prow bot added the approved label Jul 20, 2023

ack-prow bot merged commit 829b81a into aws-controllers-k8s:main Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tables `ContinuousBackup` #77

Add support for tables `ContinuousBackup` #77

msvechla commented Jul 19, 2023

ack-prow bot commented Jul 19, 2023

msvechla commented Jul 19, 2023 •

edited

Loading

a-hilaly commented Jul 19, 2023

msvechla commented Jul 19, 2023

a-hilaly commented Jul 19, 2023

a-hilaly commented Jul 19, 2023 •

edited

Loading

a-hilaly commented Jul 19, 2023

msvechla commented Jul 19, 2023 •

edited

Loading

a-hilaly left a comment

a-hilaly Jul 19, 2023 •

edited

Loading

a-hilaly commented Jul 19, 2023 •

edited

Loading

msvechla commented Jul 19, 2023

msvechla commented Jul 20, 2023

a-hilaly left a comment

a-hilaly Jul 20, 2023

a-hilaly commented Jul 20, 2023

a-hilaly left a comment

ack-prow bot commented Jul 20, 2023

a-hilaly commented Jul 20, 2023 •

edited

Loading

msvechla commented Jul 20, 2023

a-hilaly commented Jul 20, 2023

msvechla commented Jul 20, 2023

Add support for tables ContinuousBackup #77

Add support for tables ContinuousBackup #77

Conversation

msvechla commented Jul 19, 2023

ack-prow bot commented Jul 19, 2023

msvechla commented Jul 19, 2023 • edited Loading

a-hilaly commented Jul 19, 2023

msvechla commented Jul 19, 2023

a-hilaly commented Jul 19, 2023

a-hilaly commented Jul 19, 2023 • edited Loading

a-hilaly commented Jul 19, 2023

msvechla commented Jul 19, 2023 • edited Loading

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

a-hilaly commented Jul 19, 2023 • edited Loading

msvechla commented Jul 19, 2023

msvechla commented Jul 20, 2023

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly Jul 20, 2023

Choose a reason for hiding this comment

a-hilaly commented Jul 20, 2023

a-hilaly left a comment

Choose a reason for hiding this comment

ack-prow bot commented Jul 20, 2023

a-hilaly commented Jul 20, 2023 • edited Loading

msvechla commented Jul 20, 2023

a-hilaly commented Jul 20, 2023

msvechla commented Jul 20, 2023

Add support for tables `ContinuousBackup` #77

Add support for tables `ContinuousBackup` #77

msvechla commented Jul 19, 2023 •

edited

Loading

a-hilaly commented Jul 19, 2023 •

edited

Loading

msvechla commented Jul 19, 2023 •

edited

Loading

a-hilaly Jul 19, 2023 •

edited

Loading

a-hilaly commented Jul 19, 2023 •

edited

Loading

a-hilaly commented Jul 20, 2023 •

edited

Loading