Repository navigation
feat(components): send RequestSource from bridge and record status.componentGeneration - #1237
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces componentGeneration to track the CMS component generation in the Component status, independent of the Kubernetes custom resource's observedGeneration. It also implements single-writer ownership checks using RequestSource (with UID and Cluster fields) to safely handle same-cluster custom resource recreations and prevent stale incarnations from overwriting newer statuses. Feedback on the changes highlights a bug in sourceChanged where requiring entry.componentGeneration > 0 prevents syncing the transition from an unknown/zero generation to a valid positive generation, which can be resolved by simplifying the condition.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces tracking for componentGeneration in the Component status, allowing CMS component generations to be tracked independently of the Kubernetes custom resource observedGeneration. It also implements single-writer ownership checks and same-cluster recreation takeovers using a new LiveUIDReader. The review feedback focuses on defensive programming, suggesting explicit nil checks for info (returned by extractErrorInfo), setResp, r.fence, and t.entries to prevent potential nil pointer dereferences and improve overall code robustness.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'componentGeneration' to track CMS component generation independently of 'observedGeneration', updating the CRD and reconciliation logic accordingly. It enhances CMS intent handling by implementing ownership verification via 'RequestSource' and 'liveUIDReader', allowing for safer same-cluster custom resource recreation. Additionally, it adds conflict handling for ownership mismatches and stale incarnations. The review feedback correctly identifies potential safety issues, recommending the addition of defensive nil checks when accessing 'ErrorInfo' fields and 'SetComponentIntent' response structures to prevent runtime panics.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust ownership and synchronization mechanism for XMC Components by adding componentGeneration to track CMS state independently of Kubernetes metadata generation. It includes logic to handle source conflicts (e.g., incarnation mismatches, stale generations, and cluster fencing) using google.rpc.ErrorInfo for machine-readable error handling. The changes involve updating the Reconciler to verify live component UIDs, enhancing the cmsSyncTracker to detect source changes, and updating the CRD and status handling to support the new generation tracking. Several tests were added to verify the new adoption and conflict resolution logic.
ca35c8f to
a35295c
Compare
a35295c to
a3137cf
Compare
2c11ac7 to
09b57ba
Compare
e2afa75 to
a4e95f0
Compare
a4e95f0 to
ab0dac6
Compare
40d751d to
fcf9ec3
Compare
ffee966 to
2b6ffb6
Compare
…mponentGeneration Every bridge intent and lifecycle RPC now carries a RequestSource (CR UID, generation, namespace and the declared cluster identity), so CMS enforces single-writer ownership and the cluster fence on it: - callWithOwnership attaches the source, records fence rejections on the ClusterFence (RecordConflict), and on CLUSTER_NOT_DECLARED re-declares the cluster epoch and retries once. CMS ownership and fence rejections (SOURCE_INCARNATION_MISMATCH, SOURCE_NAMESPACE_CONFLICT, CLUSTER_FENCED, CLUSTER_EPOCH_REGRESSED) report SourceConflict=True without Helm side effects. - STALE_GENERATION means the informer snapshot is older than a generation CMS already applied: the pass writes no status (StaleIncarnation) and requeues after about 1s, never reporting a SourceConflict. - The bridge matches DEPENDENCY_NOT_REGISTERED and DEPENDENCY_CYCLE by ErrorInfo reason instead of by message prefix. - The CMS component generation returned by SetComponentIntent is kept in status.componentGeneration (new CRD field) and fed back into the sync tracker and observed-generation updates; the actuator checks it. A Component recreated in the same cluster while CMS still holds the old incarnation's record reports SourceConflict=SourceIncarnationMismatch in this layer; the next layer adopts it via expected_owner_uid. Folds this layer's review fixes (DEPENDENCY_NOT_REGISTERED by reason, first-known component generation, nil-safe RecordConflict, explicit nil guards, staleComponentRetryDelay, envtest intents at the overriding fence epoch, skipping sync records at unknown generation) and the CMS authorizer in the tests this layer adds. Bug: 548436300 TAG=agy CONV=b2ebecbe-c04a-4fb7-a16c-d23de82ed3e5
…test file Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…h ErrorInfo The CMS sync-condition case added for observedGeneration gating built a plain FailedPrecondition status, which the ErrorInfo-reason matcher introduced here no longer recognizes. Use the dependencyNotRegisteredErr helper like the other gating tests. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…tusInt table-driven Run the team-b enabled/disabled namespace-conflict cases in TestReconcileComponent_SourceNamespaceConflictAndStaleGeneration as named subtests, keeping the shared reconciler state and case order. Run each TestExtractStatusInt case as a subtest and fold the nil-object check into the table. Inputs and assertions are unchanged. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…alls The test file targets reconciler.go, and this layer has no live-UID takeover, so the header no longer mentions it. sourceCallRecorder counts calls instead of recording ExpectedOwnerUid, which this layer never sets; the owner-UID recording arrives with the takeover. Comment link: #1237 (comment) Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…vely TestApplyIntentResponse now compares the whole ReconcileResult with cmp.Diff instead of checking State and ComponentGeneration with chained inequalities, so stray field writes also fail the test. The populated TestBuildStatusAndBuildStatusMap subtest now declares the full expected status map (generations, state, access env vars and the condition with its fixed lastTransitionTime) and compares it with cmp.Diff, like the nil subtest. Comment link: #1237 (comment) Comment link: #1237 (comment) Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Summary
The bridge now sends
RequestSourceon its CMS intent RPC (SetComponentIntent), records the CMS generation instatus.componentGeneration, and projects ownership rejections as aSourceConflictcondition. Same-cluster recreation takeover is the layer above.Motivation & Context
CMS can enforce ownership (#1232–#1235) only for sourced requests. The bridge must send its source, re-declare when CMS has no fence for it, and keep the CMS generation so the sync tracker and the actuator can compare against it.
Key Changes
RequestSourcebuilt from the CR (uid, generation, namespace) and the declaredClusterIdentity;callWithOwnershipattaches it, records fence rejections on theClusterFence(RecordConflict), and onCLUSTER_NOT_DECLAREDre-declares the epoch and retries once.SourceConflict=True(SourceIncarnationMismatch,SourceNamespaceConflict,ClusterFenced,ClusterEpochRegressed) withReady=False/SourceConflictandStalled=True. Policy blocks returnRequeueAfter15m without an error log.STALE_GENERATIONis a stale informer snapshot, not a conflict: no status write, requeue after ~1s.DEPENDENCY_CYCLEandDEPENDENCY_NOT_REGISTEREDare matched byErrorInforeason, never by message text.status.componentGeneration(the CMS generation), used by the sync tracker and observed-generation updates. The actuator drift monitor compares it with the locked generation with no fallback;componentGeneration == 0means not synced and blocks placement.TestApplyIntentResponse(full-structcmp.Diff),BuildStatusMapchecked against a full expected map,TestCallWithOwnership_WithoutFence,TestCallWithOwnership_Retries(re-declare and stale-generation paths), sourced intent / namespace conflict / stale generation against an in-process CMS, reconciler and actuator tests.Stack Position
expected_owner_uidCAS retry, the owner-gone retry, the stale-incarnation outcome and the status SSA UID precondition moved to feat(components): adopt a same-cluster recreated Component via expected_owner_uid #1274 (its tree equals the previous feat(components): send RequestSource from bridge and record status.componentGeneration #1237 tip).Assumptions
SetComponentIntentis sourced in this layer, so CMS ownership and fencing do not yet cover observation, deactivation or deletion from a fenced cluster or a stale incarnation — feat(components): enforce source ownership on observation and deactivation RPCs #1238 sourcesUpdateObservedGeneration,PrepareDeactivationandCompleteDeactivation, and feat(components): enforce deletion ownership and fenced CR cleanup #1240 sourcesExecuteDeletion— if those layers do not land with this one, a fenced cluster can still deactivate or delete records it does not own.SourceConflict=SourceIncarnationMismatch— acceptable while Components are unlaunched.componentGeneration; the actuator therefore has noobservedGenerationfallback — impact if wrong: placement is blocked (retryable) until the bridge writescomponentGeneration.mainand before the split — if wrong, the checkpoint must be re-run before merge.Verification & Testing
bazel test //pkg/components/... //cmd/xmc_bridge/... //cmd/experiment_state_server/... //cmd/actuator/... //test/e2e:kind_3p_harness_unit_testand the//test/helmpy tests at this layer's tip;golangci-lint run(v1.64.6);gofmt -l.//test/e2e:components_lifecycle_testpassed with the whole stack applied.tools/analyze_diff_size.py): 305 prod / 593 test / 898 total / 5 doc lines (was 424 / 670 before the split).Live Deployment & Manual Testing
enable_components: true, Spanner-backed CMS), upgraded with the dev-environment upgrade script including Spanner migrations014/015.5/5,16/16pods Ready, baseline Component CRs Ready and Synced).RequestSourceandstatus.componentGenerationmt-cp3-recreate(1 → 2 → 3) persist the CMS-assignedstatus.componentGeneration, with matchingSource*columns in Spanner.SourceConflictprojection on ownership rejectionmt-cp3-del2inxmcwhile CMS recordedother-nsas its owner projectedSourceConflict=True (SourceNamespaceConflict),Ready=False (SourceConflict)andStalled=True.Related Issues / Bugs
Bug: 548436300
TAG=agy
CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Stack created with GitHub Stacks CLI • Give Feedback 💬