Skip to content

Commit a634d60

Browse files
authored
fix(memorystore): prevent infinite reconciliation drift loop by aligning connections list length (#11547)
### Why is this change necessary? During E2E testing of network connectivity topologies, we observed that `MemorystoreInstance` controllers can fall into an infinite reconciliation loop due to false drift detection on the `endpoints[i].connections` list. Specifically: - In the KRM spec (`desired`), a user defines the target VPC network(s) under `spec.endpoints[i].connections` (typically 1 connection item representing the intended PSC network target). - Under the hood, GCP automatically allocates and returns **multiple** server-generated connection entries per target network (e.g., one connection per shard or node attachment). - Because the GCP API returns an `actual` connections list that is longer than the user's `desired` list, `tags.DiffForTopLevelFields` flags the length mismatch as a drift. KCC repeatedly sends update requests attempting to shrink the connections list, which GCP ignores, resulting in an infinite reconciliation loop. ### What does this change do? 1. **Normalizes `Connections` list length before diffing (`pkg/controller/direct/memorystore/memorystoreinstance_controller.go`)**: Truncates the server-generated extra `Connections` in `maskedActual` to match `len(desiredEndpoint.Connections)` before comparison (`actualEndpoint.Connections = actualEndpoint.Connections[:len(desiredEndpoint.Connections)]`). - **Why truncating is correct and does not cause data loss**: Truncation only applies to the temporary `maskedActual` copy used for drift comparison against the user's `spec`. It does not discard any user-configured target connections, nor does it affect the full observed state reported in `status.endpoints`. It ensures we compare exactly the user-declared intent (`desired`) against the corresponding actual connection items without failing on extra server-created replica entries. 2. **Prevents partial status overwrites (`pkg/controller/direct/directbase/operations.go`)**: Updates `directbase/operations.go` to prevent partial status updates from overwriting existing status fields (such as `conditions` or `externalRef`) with `nil`. 3. **Sorts HTTP logs in E2E tests**: Aligns/sorts HTTP log entries for the `MemorystoreInstance` test fixtures to prevent test flakiness.
2 parents 26dfc23 + cf34c59 commit a634d60

4 files changed

Lines changed: 537 additions & 516 deletions

File tree

pkg/controller/direct/memorystore/memorystoreinstance_controller.go

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,13 +396,89 @@ func compareInstance(ctx context.Context, actual, desired *memorystorepb.Instanc
396396
desired.CrossInstanceReplicationConfig = maskedActual.CrossInstanceReplicationConfig
397397
}
398398

399+
// Align actual endpoints/connections with desired when desired is a subset of actual.
400+
alignEndpointsWithDesired(maskedActual, desired)
401+
399402
diffs, _, err := tags.DiffForTopLevelFields(ctx, desired.ProtoReflect(), maskedActual.ProtoReflect())
400403
if err != nil {
401404
return nil, err
402405
}
403406
return diffs, nil
404407
}
405408

409+
// alignEndpointsWithDesired aligns actual endpoints with desired when desired is a subset of actual.
410+
// It matches each desired endpoint to a corresponding actual endpoint and retains only matched endpoints
411+
// in actual so that unmanaged server-generated extra endpoints do not trigger false drift.
412+
func alignEndpointsWithDesired(actual, desired *memorystorepb.Instance) {
413+
if len(desired.Endpoints) == 0 {
414+
actual.Endpoints = nil
415+
return
416+
}
417+
418+
alignedEndpoints := make([]*memorystorepb.Instance_InstanceEndpoint, 0, len(desired.Endpoints))
419+
420+
for _, desiredEndpoint := range desired.Endpoints {
421+
for _, actualEndpoint := range actual.Endpoints {
422+
if isConnectionSubset(desiredEndpoint.GetConnections(), actualEndpoint.GetConnections()) {
423+
matchedEndpoint := &memorystorepb.Instance_InstanceEndpoint{
424+
Connections: desiredEndpoint.Connections,
425+
}
426+
alignedEndpoints = append(alignedEndpoints, matchedEndpoint)
427+
break
428+
}
429+
}
430+
}
431+
432+
if len(alignedEndpoints) == len(desired.Endpoints) {
433+
actual.Endpoints = alignedEndpoints
434+
}
435+
}
436+
437+
// isConnectionSubset checks if every desired connection is matched by at least one actual connection.
438+
func isConnectionSubset(desiredConnections, actualConnections []*memorystorepb.Instance_ConnectionDetail) bool {
439+
for _, desiredConn := range desiredConnections {
440+
matched := false
441+
442+
for _, actualConn := range actualConnections {
443+
if connectionsMatch(desiredConn, actualConn) {
444+
matched = true
445+
break
446+
}
447+
}
448+
449+
if !matched {
450+
return false
451+
}
452+
}
453+
454+
return true
455+
}
456+
457+
// connectionsMatch checks if all specified fields in a desired connection match an actual connection.
458+
func connectionsMatch(desired, actual *memorystorepb.Instance_ConnectionDetail) bool {
459+
if desired == nil || actual == nil {
460+
return desired == actual
461+
}
462+
463+
desiredPsc := desired.GetPscAutoConnection()
464+
actualPsc := actual.GetPscAutoConnection()
465+
if desiredPsc == nil || actualPsc == nil {
466+
return desiredPsc == actualPsc
467+
}
468+
469+
// Only match against project and network since CRD implementation uses project and network refs.
470+
// So no PSC fields will be present in the desired spec.
471+
if desiredPsc.GetProjectId() != "" && desiredPsc.GetProjectId() != actualPsc.GetProjectId() {
472+
return false
473+
}
474+
475+
if desiredPsc.GetNetwork() != "" && desiredPsc.GetNetwork() != actualPsc.GetNetwork() {
476+
return false
477+
}
478+
479+
return true
480+
}
481+
406482
// Export maps the GCP object to a Config Connector resource `spec`.
407483
func (a *InstanceAdapter) Export(ctx context.Context) (*unstructured.Unstructured, error) {
408484
if a.actual == nil {

0 commit comments

Comments
 (0)