fix: ignore setting sdkUpdate output into DBInstance spec #221

michaelhtm · 2025-05-12T20:20:42Z

Issue #2086, #2123

Description of changes:
When we make a ModifyDBInstance call, the output returned is outdated.
Currently we've been relying on pendingModifiedValues to return the
desired field values, but based on the logs, pendingModifiedValues is not
a reliable source.

These changes ignore the sdkUpdate output spec completely, and only patch the status.

One more change here is adding support of late initializing PerformanceInsightsKMSKeyID and PerformanceInsightsRetentionPeriod fields only when PerformanceInsights is enabled.

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

michaelhtm · 2025-05-13T00:36:38Z

apis/v1alpha1/generator.yaml

+      # sdk_delete_post_request:
+      #   code: return r, fmt.Errorf("wait for delete")


should we do something like this instead? ignoring sdkUpdate output? https://github.com/aws-controllers-k8s/efs-controller/blob/af06340fb3246dc107feac45136b465ade226d3c/generator.yaml#L59-L60

Just to make sure I understand the two approaches.

In this PR you selectively ignored certain fields for the DBInstance which were receiving out of date values from the Update response. The alternative you're asking about is to completely ignore the update response for every field as a way of ensuring that no other fields are erroneously updated with out of date values from the Update response. Is that correct?

knottnt

Just left a few clarifying questions.

pkg/resource/db_instance/sdk.go

knottnt · 2025-05-13T18:26:56Z

apis/v1alpha1/generator.yaml

+      # sdk_delete_post_request:
+      #   code: return r, fmt.Errorf("wait for delete")


Just to make sure I understand the two approaches.

In this PR you selectively ignored certain fields for the DBInstance which were receiving out of date values from the Update response. The alternative you're asking about is to completely ignore the update response for every field as a way of ensuring that no other fields are erroneously updated with out of date values from the Update response. Is that correct?

knottnt · 2025-05-13T18:29:59Z

@michaelhtm: Would it be possible to create a test that reproduces the bug? Having that test would both make us a bit more confident in this fix and make sure that we don't accidentally have a regression in future changes.

test/e2e/tests/test_db_instance.py

knottnt · 2025-05-21T16:04:35Z

/lgtm
/hold

Just adding the hold in case we want a second reviewer on this one.

generator.yaml

knottnt · 2025-07-21T23:44:20Z

pkg/resource/db_instance/delta.go

-		a.ko.Spec.PerformanceInsightsKMSKeyID = b.ko.Spec.PerformanceInsightsKMSKeyID
+	if a.ko.Spec.PerformanceInsightsEnabled == nil &&
+		b.ko.Spec.PerformanceInsightsEnabled != nil {
+		a.ko.Spec.PerformanceInsightsEnabled = func() *bool { a := false; return &a }()


Q: Is this mutating PerformanceInsightEnabled to false?

If that's the case it seems odd to mutate the resource in the delta. I wouldn't expect the delta comparison to have any side effects.

We do this in a lot of controllers. The expected outcome if the field is missing is disabling it

Okay if this is an existing pattern I won't block the PR.

knottnt · 2025-07-21T23:53:39Z

templates/hooks/db_instance/sdk_update_pre_set_output.go.tpl

@@ -0,0 +1,6 @@
+	ko.Status = latest.ko.Status
+	setLastAppliedSecretReferenceAnnotation(&resource{ko})


Q: Any reason this has been moved to out of the sdk_update_post_set_output hook?

because we no longer reach post_set_output. We return here: https://github.com/aws-controllers-k8s/rds-controller/pull/221/files#diff-20a6e390c0105317eb581bd5f0bf409c375bbd090d162881ed810952ca8aef93R1998

When we make a ModifyDBInstance call, the output returned is outdated. Currently we've been relying on `pendingModifiedValues` to return the desired field values, but based on the longs, pendingModifiedValues is not a reliable source. These changes specifically ignore setting the fields after ModifyDBInstance is called. A better approach can be to ignore setting anything after Update is called. Just in case we're missing some values that we should ignore instead!!!

ack-prow · 2025-07-23T17:56:04Z

@michaelhtm: 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
rds-verify-attribution	`a3d1bd7`	link	false	`/test rds-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.

knottnt · 2025-07-23T18:02:12Z

/lgtm

ack-prow · 2025-07-23T18:02:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knottnt, michaelhtm

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~~ [knottnt,michaelhtm]

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

michaelhtm · 2025-07-23T18:35:27Z

/unhold

ack-prow bot requested review from jlbutler and knottnt May 12, 2025 20:20

ack-prow bot added the approved label May 12, 2025

michaelhtm commented May 13, 2025

View reviewed changes

knottnt reviewed May 13, 2025

View reviewed changes

michaelhtm force-pushed the fix/ignoreoutdatedfields branch 2 times, most recently from 7a87712 to ed1a046 Compare May 13, 2025 19:42

knottnt approved these changes May 21, 2025

View reviewed changes

test/e2e/tests/test_db_instance.py Show resolved Hide resolved

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2025

ack-prow bot assigned knottnt May 21, 2025

ack-prow bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels May 21, 2025

michaelhtm force-pushed the fix/ignoreoutdatedfields branch 3 times, most recently from f470a3d to 2434b87 Compare June 3, 2025 05:38

michaelhtm force-pushed the fix/ignoreoutdatedfields branch from 2434b87 to 509dc82 Compare July 2, 2025 01:28

michaelhtm unassigned knottnt Jul 2, 2025

michaelhtm force-pushed the fix/ignoreoutdatedfields branch from 36a7261 to 710c85b Compare July 2, 2025 19:24

michaelhtm force-pushed the fix/ignoreoutdatedfields branch from 710c85b to f980ff5 Compare July 17, 2025 16:40

michaelhtm changed the title ~~fix: ignore field setting in sdkUpdate DBInstance~~ fix: ignore setting sdkUpdate output into DBInstance spec Jul 17, 2025

michaelhtm force-pushed the fix/ignoreoutdatedfields branch from f980ff5 to 05da18d Compare July 17, 2025 17:26

michaelhtm requested review from a-hilaly and removed request for jlbutler July 17, 2025 18:36

knottnt reviewed Jul 21, 2025

View reviewed changes

michaelhtm added 4 commits July 23, 2025 10:37

test: DBInstance multiAZ update

dfb2f00

completely ignore sdkUpdate output setting for spec

3ff78a9

test: DBInstance PerformanceInsightsRetentionPeriod

d15a9d9

michaelhtm added 2 commits July 23, 2025 10:37

lateInitialize PerformanceInsightsRetentionPeriod/KMSKeyID

d57d40d

Skip lateInitializeIncompleteCheck

a3d1bd7

michaelhtm force-pushed the fix/ignoreoutdatedfields branch from 05da18d to a3d1bd7 Compare July 23, 2025 17:54

knottnt approved these changes Jul 23, 2025

View reviewed changes

ack-prow bot assigned knottnt Jul 23, 2025

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

ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2025

ack-prow bot merged commit 4be393b into aws-controllers-k8s:main Jul 23, 2025
6 of 7 checks passed

		# sdk_delete_post_request:
		# code: return r, fmt.Errorf("wait for delete")

		@@ -0,0 +1,6 @@
		ko.Status = latest.ko.Status
		setLastAppliedSecretReferenceAnnotation(&resource{ko})

fix: ignore setting sdkUpdate output into DBInstance spec #221

fix: ignore setting sdkUpdate output into DBInstance spec #221

Uh oh!

Conversation

michaelhtm commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knottnt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knottnt commented May 13, 2025

Uh oh!

Uh oh!

knottnt commented May 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Jul 23, 2025

Uh oh!

knottnt commented Jul 23, 2025

Uh oh!

ack-prow bot commented Jul 23, 2025

Uh oh!

michaelhtm commented Jul 23, 2025

Uh oh!

Uh oh!

Uh oh!

michaelhtm commented May 12, 2025 •

edited

Loading