Repository navigation
feat(components): hold a fenced bridge in cold standby - #1236
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a cluster fencing mechanism (ClusterFence) to the xmc_bridge component, allowing it to declare cluster ownership epochs to CMS and gate component reconciliations based on active fence status. The changes include new configuration options, RBAC permissions to read the kube-system namespace UID, and comprehensive tests. The review feedback highlights several critical issues: a compilation error in cluster_fence_test.go due to an undefined variable (cmsClient instead of cmsMock), potential data races on f.identity in Identity() and Start(), a potential deadlock in declareLocked if mutex locking is added to Identity(), and a naming conflict risk for cluster-scoped RBAC resources if multiple instances of the Helm chart are installed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cluster fencing mechanism (ClusterFence) to xmc_bridge to manage active/standby cluster ownership epochs declared to CMS, adding new configuration options, RBAC permissions to read the kube-system namespace UID, and gating reconciliations on holding the active fence. Feedback on these changes suggests refactoring ClusterFence.Declare and Check to execute the blocking gRPC call outside of the mutex lock to avoid blocking concurrent checks. Additionally, the reviewer recommends providing a --cluster-uid flag/env var fallback for restricted Kubernetes environments where reading the kube-system namespace is restricted, and using the standard status.FromError helper for cleaner gRPC error extraction.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cluster fencing mechanism to the XMC Bridge to coordinate active and standby clusters. It adds configuration options for cluster name and ownership epoch, reads the cluster's unique identity from the kube-system namespace UID, and implements a periodic epoch declaration to the CMS. The reconciler is updated to gate active reconciliations on holding the active CMS ownership fence, transitioning components to a SourceConflict state when fenced. The review feedback points out a logical issue in the ClusterFence's Declare method where transient error checks are performed before checking for stale sequences, which could cause obsolete errors from slow declarations to prematurely trigger retries.
ee159ab to
6fcf6f4
Compare
006038a to
d4f4272
Compare
0aa86d0 to
ff87d20
Compare
ff87d20 to
071cf71
Compare
9b07bc6 to
ddba095
Compare
a116869 to
d6ed91d
Compare
e6d4e26 to
ff3e5a8
Compare
5956ef0 to
3d45036
Compare
8a4e5aa to
7e2ae2f
Compare
7e2ae2f to
eff3991
Compare
|
@VLukiantsev - can you share the design doc reference that describes this "fence" mechanism? |
|
@AntonTyb this is a follow-up to the AI test of the implemented component stack. This scenario was missed in the design doc |
The previous layer makes the bridge declare its cluster epoch to CMS. This layer makes it honour the answer, so a cluster that lost the ownership fence stops writing: - ClusterFence tracks the declaration outcome: whether this cluster holds the fence, or is fenced (CLUSTER_FENCED) or behind (CLUSTER_EPOCH_REGRESSED). Check answers from the last applied status (declaring first if no declaration succeeded yet) without blocking on an in-flight CMS call, a sequence number keeps a slow declaration from overwriting a newer one, and Changes signals each transition. - reconcilePass consults the fence (WithClusterFence) before any CMS write or Helm actuation. A fenced Component reports SourceConflict=True (ClusterFenced / ClusterEpochRegressed), Ready=False and Stalled=True, keeps no status.access, and is rechecked every 15 minutes. - The Component controller watches the fence's Changes channel and re-enqueues every Component of its namespace when the fence state transitions, so a takeover or failback takes effect immediately. - extractErrorInfo reads the components-domain ErrorInfo of a wrapped FailedPrecondition and keeps the CMS message. Folds the review fixes of this layer: bounded heartbeats and transition coverage, declaring outside the fence mutex, ignoring obsolete declaration failures, keeping the CMS message of a wrapped ErrorInfo, keeping ParseConfig under the gocyclo limit, and the CMS authorizer in the tests this layer adds. Tests: cold standby against an in-process CMS (fenced standby, enabled and disabled, and a regressed epoch), transient declaration errors, Start and transition notifications, stale declarations, and an envtest of a fenced standby that takes over after an epoch bump. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…in wasComponentPreviouslyActive Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
ClusterFence logs the status a declaration moves the cluster into: an error when it is fenced out of CMS (cold standby) and an info line when it holds the fence, once on the first declaration and once per change, never on an unchanged heartbeat. A fenced bridge was otherwise silent. Comment link: #1273 (comment) Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
… bound fence checks - wasComponentPreviouslyActive no longer treats a SourceConflict Ready reason as never actuated: the fence or an ownership conflict can engage after actuation, so disabling a component after unfencing still uninstalls a workload that ran before the fence. - ClusterFence.Check shares one first declaration across concurrent callers (singleflight), detached from any single caller's cancellation and bounded by DeclareTimeout. - Explain why deletion runs before the fence check, with a TODO for the sourced deletion in a later layer, and restore the NewReconciler parameter docs. Comment links: #1236 (comment) #1236 (comment) #1236 (comment) #1236 (comment) Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…caller's context ends ClusterFence.Check shares the first DeclareClusterEpoch across reconcile workers through singleflight. Do blocked every waiter until the shared call returned, so a worker whose context was cancelled (leader loss or shutdown) could still wait up to DeclareTimeout. declareShared now uses DoChan and returns as soon as the caller's context ends; the detached, DeclareTimeout-bounded declaration keeps running for the other waiters. DoChan's result channel is buffered, so an abandoned waiter never blocks the shared call. Tests: - TestClusterFence_CheckSharesOneDeclaration: a cancelled caller returns while the shared CMS call is still in flight, and later callers still share that single call. The scripted CMS now fails a call whose context ended, so losing context.WithoutCancel is caught too. - TestClusterFence_TransitionNotifications: assert the first declaration's log line (Info when active, Error when fenced), not only the second's. Comment link: #1236 (comment) Comment link: #1236 (comment) Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Summary
A fenced xmc-bridge now enters cold standby: when CMS reports that another cluster holds the ownership fence (or this cluster's epoch regressed), it performs no CMS writes and no Helm actuation or teardown for non-deleting CRs, reports
SourceConflicton its Component CRs, and re-enqueues every Component when the fence state changes. Deleting a CR in a fenced cluster is handled in #1240 (sourcedExecuteDeletion,StaleDeletionEvent, local teardown only). The cluster declaration itself is the layer below.Motivation & Context
With the CMS fence in place (#1234/#1235) and the bridge declaring its epoch (previous layer), the bridge must stop writing when another cluster holds the fence, so a restored cluster or a DR standby cannot fight the active writer.
Key Changes
ClusterFencetracks the declaration outcome (ClusterFenced/ClusterEpochRegressedor active). The CMS call runs outside the fence mutex and stale (out-of-order) results are dropped, soChecknever waits on an in-flight RPC;Checkdeclares first if no declaration succeeded yet.Changes()signals each transition.WithClusterFencegatesreconcilePass: fenced cold standby reportsSourceConflict=True/ClusterFenced,Ready=False,Stalled=True,RequeueAfter15m; health projection only, no CMS writes and no Helm actuation or teardown.ClusterFence.Changes()through a channel source and enqueues all Components of its namespace on a fence transition.extractErrorInfo: reads the components-domainErrorInfoof a wrappedFailedPreconditionand keeps the CMS message.ClusterFencelogs each fence status transition, once on the first declaration and once per change (never per heartbeat):Errorwhen the cluster is fenced out (cold standby) andInfowhen it holds the fence. Without it a fenced bridge left no trace in its logs.ClusterFence.Checkshares one first declaration across concurrent reconcile workers (singleflight), detached from any single caller's cancellation and bounded byDeclareTimeout; a waiter whose context ends returns at once without cancelling the shared declaration.wasComponentPreviouslyActiveno longer treats aSourceConflictReady reason as never actuated: the fence can engage after actuation, so disabling a component after unfencing still uninstalls its workload.Stack Position
Assumptions
ExecuteDeletion, so CMS keeps the active owner's record).DeclareClusterEpochon a CMS that lacks it, and its controllers stay gated until CMS is upgraded.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): 251 prod / 529 test / 780 total / 0 doc lines (was 483 / 589 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).Related Issues / Bugs
Bug: 548436300
TAG=agy
CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Stack created with GitHub Stacks CLI • Give Feedback 💬