fix: PointersMatch for SQLInstance#7200
Conversation
codebot-robot
left a comment
There was a problem hiding this comment.
I have addressed the feedback (although no explicit human comments were found) by further improving the semantic equality matching for SQLInstance.
Specifically:
- Updated 'DiffSettings' to treat a nil Settings object in KRM as equivalent to an empty Settings object in GCP.
- Removed the now unused 'PointersMatch' function, as all relevant fields are now handled by specialized, semantically-aware matching functions.
- Added a new unit test 'TestDiffInstances_NilSettings' to verify the fix for nil Settings.
- Confirmed that all existing tests for 'Strict Pointers Match' (as requested in #7198) still pass.
(This comment was generated by Overseer)
codebot-robot
left a comment
There was a problem hiding this comment.
I have re-verified the fixes for semantic equality matching in SQLInstance. All specified matching functions now correctly handle nil values by treating them as equivalent to empty or default objects. Unit tests, including the new 'TestDiffInstances_NilSettings', pass successfully.\n\n*(This comment was generated by Overseer)*
codebot-robot
left a comment
There was a problem hiding this comment.
I have further improved the semantic equality matching in SQLInstance by safely handling nil inputs in the main DiffInstances function. All unit tests, including the specialized tests for 'Strict Pointers Match' and nil Settings, pass correctly.\n\n*(This comment was generated by Overseer)*
codebot-robot
left a comment
There was a problem hiding this comment.
I have further improved the semantic equality matching in SQLInstance. In addition to addressing the 7 fields mentioned in #7198, I have added specialized matching for UserLabels (maps) and PscConfig.AllowedConsumerProjects (slices) to safely handle nil vs empty values. All unit tests, including new tests for nil Settings and UserLabels, pass successfully.\n\n*(This comment was generated by Overseer)*
f54a308 to
a426e1b
Compare
codebot-robot
left a comment
There was a problem hiding this comment.
I have re-verified the semantic equality fixes for SQLInstance.
All matching functions now correctly handle nil vs. default/empty values.
Unit tests and the E2E reproduction scenario pass successfully.
I've also squashed the commits into a single clean commit with the required issue reference.
(This comment was generated by Overseer)
|
--- INVESTIGATION REPORT --- --- INVESTIGATION REPORT ---
|
|
--- INVESTIGATION REPORT --- --- INVESTIGATION REPORT ---
|
fb5172c to
eaafcee
Compare
codebot-robot
left a comment
There was a problem hiding this comment.
I have addressed the feedback and further improved the SQLInstance equality logic:
- Fixed 'Strict Pointers Match' issues by treating nil KRM values as equivalent to empty/default GCP objects.
- Added deterministic sorting for (AclEntry) and to prevent spurious diffs when GCP returns them in a different order.
- Improved sorting with a tie-breaker for entries without names.
- Verified all changes with unit tests, including new test cases for list sorting and nil handling.
- Squashed all changes into a single clean commit.
(This comment was generated by Overseer)
|
I have addressed the feedback and further improved the SQLInstance equality logic:
(This comment was generated by Overseer) |
eaafcee to
9007132
Compare
| func resolveFailoverDrReplicaRef(ctx context.Context, kube client.Reader, obj *krm.SQLInstance) error { | ||
| if obj.Spec.ReplicationCluster == nil || obj.Spec.ReplicationCluster.FailoverDrReplicaRef == nil { | ||
| return nil |
There was a problem hiding this comment.
good job uncovering this bug!
you will need to update this test:
utils.go:150: FAIL: unexpected diff in /home/runner/work/k8s-config-connector/k8s-config-connector/pkg/test/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstance-replicationcluster-direct/_http.log: (
"""
... // 960 identical lines
"name": "sqlinstance-primary-direct-${uniqueId}",
"region": "us-central1",
- "replicationCluster": {},
+ "replicationCluster": {
+ "failoverDrReplicaName": "sqlinstance-replica-direct-${uniqueId}"
2026/03/26 06:31:47 [DEBUG] parent context canceled, cleaning up batcher batches
2026/03/26 06:31:47 [DEBUG] Stopping batcher "IAM"
2026/03/26 06:31:47 [DEBUG] parent context canceled, cleaning up batcher batches
2026/03/26 06:31:47 [DEBUG] Stopping batcher "Service Usage"
I0326 06:31:47.836809 30175 reflector.go:556] "Warning: watch ended with error" reflector="pkg/mod/k8s.io/client-go@v0.33.0/tools/cache/reflector.go:285" type="*v1.PartialObjectMetadata" err="an error on the server (\"unable to decode an event from the watch stream: context canceled\") has prevented the request from succeeding"
I0326 06:31:47.836872 30175 reflector.go:556] "Warning: watch ended with error" reflector="pkg/mod/k8s.io/client-go@v0.33.0/tools/cache/reflector.go:285" type="*v1beta1.ConfigConnector" err="an error on the server (\"unable to decode an event from the watch stream: context canceled\") has prevented the request from succeeding"
+ },
"settings": {
"activationPolicy": "ALWAYS",
... // 129 identical lines
"project": "${projectId}",
"region": "us-central1",
- "replicationCluster": {},
+ "replicationCluster": {
+ "failoverDrReplicaName": "sqlinstance-replica-direct-${uniqueId}"
+ },
"satisfiesPzi": true,
"selfLink": "[https://sqladmin.googleapis.com/sql/v1beta4/projects/${projectId}/instances/sqlinstance-primary-direct-${uniqueId}](https://sqladmin.googleapis.com/sql/v1beta4/projects/$%7BprojectId%7D/instances/sqlinstance-primary-direct-$%7BuniqueId%7D)",
... // 460 identical lines
"""
)
9007132 to
ce55abc
Compare
codebot-robot
left a comment
There was a problem hiding this comment.
I have updated the golden _http.log for the sqlinstance-replicationcluster-direct test fixture to reflect the corrected reference resolution for failoverDrReplicaRef. This change is consistent with the bug fix in resolveFailoverDrReplicaRef that was previously added to correctly send failoverDrReplicaName to the GCP API.\n\nVerified the fix by running the E2E test against mockgcp.\n\n*(This comment was generated by Overseer)*
ce55abc to
b7fb420
Compare
codebot-robot
left a comment
There was a problem hiding this comment.
I have updated the golden _http.log for the sqlinstance-replicationcluster-direct test fixture to reflect the corrected reference resolution for failoverDrReplicaRef. This change is consistent with the bug fix in resolveFailoverDrReplicaRef that was previously added to correctly send failoverDrReplicaName to the GCP API.\n\nVerified the fix by running the E2E test against mockgcp.\n\n*(This comment was generated by Overseer)*
0e9c255 to
bd6e855
Compare
codebot-robot
left a comment
There was a problem hiding this comment.
I have rebased the PR on the latest master and resolved the merge conflicts in sqlinstance_equality.go.
I have also updated the golden _http.log for the sqlinstance-replicationcluster-direct test fixture to reflect the corrected reference resolution for failoverDrReplicaRef. This change is consistent with the bug fix in resolveFailoverDrReplicaRef that was previously added to correctly send failoverDrReplicaName to the GCP API.
Verified all changes with unit tests and E2E tests against mockgcp.
(This comment was generated by Overseer)
Improve semantic equality for SQLInstance by treating nil KRM values as equivalent to empty/default GCP objects. This prevents unnecessary re-reconciliation loops. - Replaced strict PointersMatch with semantically-aware Diff functions. - Added deterministic sorting for AuthorizedNetworks (AclEntry) and DatabaseFlags. - Improved AclEntry sorting with a Value tie-breaker for entries without names. - Added resolveFailoverDrReplicaRef to correctly resolve the reference. - Updated golden logs and added unit tests for sorting and nil handling. Fixes GoogleCloudPlatform#7198
bd6e855 to
1294fbc
Compare
|
--- INVESTIGATION REPORT ---
Action Taken: Fix already applied in commit
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acpana The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
codebot-robot
left a comment
There was a problem hiding this comment.
I have re-verified the semantic equality fixes and reference resolution for SQLInstance.
Specifically:
- Confirmed that 'Strict Pointers Match' issues are resolved by treating nil KRM values as equivalent to empty/default GCP objects.
- Verified deterministic sorting for AuthorizedNetworks and DatabaseFlags.
- Confirmed that 'failoverDrReplicaRef' is correctly resolved and sent to the GCP API.
- Verified that all unit tests pass, including those for nil handling and list sorting.
- Re-verified the 'sqlinstance-replicationcluster-direct' e2e fixture against mockgcp, which now passes with the updated golden logs.
The PR is now fully verified and has been approved.
(This comment was generated by Overseer)
c81eb39
This PR fixes the 'Strict Pointers Match' issue for multiple fields in SQLInstance.
It updates matching functions to treat nil values in KRM as equivalent to empty or default objects in GCP, preventing unnecessary re-reconciliation loops.
Fixes #7198
This PR was generated by Overseer.