fix: issue 2614 dbcluster respect only supplying major engineVersion #237
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rmullinnix461332 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @rmullinnix461332. 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 Once the patch is verified, the new status will be reflected by the 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. |
ack-prow
bot
added
the
needs-ok-to-test
There was a problem hiding this comment.
@rmullinnix461332 Appreciate the contribution! Apologies for the delay in reviewing this.
|
|
||
| // Handle case where auto update minor version is true | ||
| // DBCluster Engine Version is major version and RDS Cluster Version is major and minor | ||
| reconcileDBClusterEngineVersion(a, b) |
There was a problem hiding this comment.
This file is generated. To make sure that this isn't accidentally removed in future changes this should be added to the delta_pre_compare.go.tpl hook. You'll also need to re-run the ack-generate tool to apply the changes to the hook. You can find information on how to do that in the contributor docs.
| if a != nil && b != nil && | ||
| a.ko.Spec.EngineVersion != nil && b.ko.Spec.EngineVersion != nil && | ||
| strings.HasPrefix(*b.ko.Spec.EngineVersion, *a.ko.Spec.EngineVersion) && | ||
| b.ko.Spec.AutoMinorVersionUpgrade != nil && *b.ko.Spec.AutoMinorVersionUpgrade { |
There was a problem hiding this comment.
Should this not be using the state of the desired AutoMinorVersionUpgrade instead of what we just read from AWS? If the latest value has drifted I don't think we'd want it to impact how the EngineVersion is handled.
| } | ||
| if ackcompare.HasNilDifference(a.ko.Spec.DatabaseInsightsMode, b.ko.Spec.DatabaseInsightsMode) { | ||
| delta.Add("Spec.DatabaseInsightsMode", a.ko.Spec.DatabaseInsightsMode, b.ko.Spec.DatabaseInsightsMode) | ||
| if a.ko.Spec.DatabaseInsightsMode != nil { |
There was a problem hiding this comment.
Would you be able to share steps to reproduce this issue? Tested adopting a DB Cluster with adopt-or-create without setting spec.databaseInsightsMode and I don't see it repeatedly showing up in the delta.
There was a problem hiding this comment.
We have been using the controller for 2-3 years. More than likely, the resources have been out there for a while. the manifest on the cluster does not have spec.databaseInsightsMode. It's marked as a delta.
spec:
allocatedStorage: 1
autoMinorVersionUpgrade: true
availabilityZones:
- us-east-1d
- us-east-1c
- us-east-1b
backupRetentionPeriod: 7
copyTagsToSnapshot: true
databaseName: XXXXXXXX
dbClusterIdentifier: XXXXXXXX
dbClusterParameterGroupRef:
from:
name: XXXXXXXX
dbSubnetGroupName: XXXXXXX
deletionProtection: true
enableCloudwatchLogsExports:
- audit
- error
- slowquery
engine: aurora-mysql
engineMode: provisioned
engineVersion: "8.0"
kmsKeyID: arn:aws:kms:us-east-1:XXXXXXXXX
masterUserPassword:
key: password
name: XXXXXXXXX
namespace: XXXXXXXXX
masterUsername: XXXXXXXX
networkType: IPV4
port: 3306
preferredBackupWindow: 03:50-04:20
preferredMaintenanceWindow: sun:12:00-sun:14:00
storageEncrypted: true
tags:
There was a problem hiding this comment.
Tried reproducing the issue by creating a DBCluster with a similar spec using the v1.5.0 version of the rds-controller/CRD and then upgrading the controller and CRD to the 1.6.0 versions. After upgrading I saw the controller detect databaseInsightsMode in the delta once and then update the DBCluster to reflect the value in AWS.
Just to confirm, did you update the DBCluster CRD along with the rds-controller deployment? I did see databaseInsightsMode repeatedly logged in the delta when the rds-controller has been upgraded to v1.6.0, but the CRD hasn't been re-applied to include the databaseInsightsMode field. The controller read the value from AWS, but when attempting to apply it to the DBCluster the patch failed with below error.
{"level":"info","ts":"2025-10-02T16:45:32.172-0700","logger":"KubeAPIWarningLogger","msg":"unknown field \"spec.databaseInsightsMode\""}
There was a problem hiding this comment.
the crds are current. we use argo-cd and the helm chart from aws public ecr for install. databaseInsightsMode is present in the dbcluster crd.
There was a problem hiding this comment.
By default Helm3 doesn't install CRDs on upgrades if the CRD is already present. Are you able to see the databaseInsightsMode field when examining the CRD directly?
kubectl get crd dbclusters.rds.services.k8s.aws -o yaml
There was a problem hiding this comment.
argo-cd doesn't use helm3, it generates the desired manifest from the helm chart, but handles the sync itself. And yes, i did verify that the databaseInsightsMode is in the CRD.
Co-authored-by: knottnt <[email protected]>
2 participants
Issue #, if available: 2614
Description of changes:
DBCluster is not respecting only supplying major egineVersion. it is reporting a delta because RDS has major/minor version. This prevents FieldExports from being populated.
In addition, existing DBCluster where DatabaseInsightsMode is nil, new controller results in continuous modifications to the RDS DB Instances. if databaseInsightsMode is not set, do not report as a delta.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.