Repository navigation
feat(components): add the cluster ownership fence and DeclareClusterEpoch - #1234
Merged
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements an ownership fence mechanism to ensure single-cluster execution for writing component intents. It introduces the DeclareClusterEpoch RPC, updates the Spanner schema with a new ComponentOwnershipFence table, and implements the fencing and takeover logic in both the in-memory and Spanner store backends. The review feedback suggests several robustness and style improvements: explicitly validating that the cluster identity is not nil at the API boundary, defensively returning nil, err if the Spanner transaction fails, and refactoring the Spanner error check to be more idiomatic by verifying err != nil first.
This was referenced Oct 13, 2026
Collaborator
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an ownership fence mechanism to ensure single-cluster execution for writing Component intents, adding the DeclareClusterEpoch RPC, database schema updates, and store implementations for both in-memory and Spanner backends. The review feedback highlights the need to update the component's UpdateTime (and Etag for the in-memory store) during a fence takeover to ensure consistency and correct Etag generation, as well as updating the documentation to reflect these column updates.
Collaborator
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the 'Ownership Fence' mechanism to ensure single-cluster execution for writing Component intents. It adds the DeclareClusterEpoch RPC to the Component Manager Service, allowing a cluster to declare itself the active writer at a specific epoch. The implementation includes database schema updates (introducing the ComponentOwnershipFence table), in-memory and Spanner store implementations, and comprehensive tests verifying fence creation, heartbeats, epoch increments, and takeover logic where owned components are reassigned to a new active cluster. There are no review comments on this pull request, so I have no feedback to provide.
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
2 times, most recently
from
October 14, 2026 09:37
d872196 to
dabe624
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
from
October 14, 2026 09:49
dabe624 to
7b3740d
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
2 times, most recently
from
October 14, 2026 11:58
e69d54f to
fa19b55
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
from
October 14, 2026 13:54
fa19b55 to
7e0ab87
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
3 times, most recently
from
October 15, 2026 08:07
7e930c9 to
4febb6d
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
2 times, most recently
from
October 15, 2026 09:29
e92472b to
fda8206
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
from
October 15, 2026 15:43
3920066 to
2930d7e
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
from
October 16, 2026 08:53
2930d7e to
62e01a4
Compare
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
3 times, most recently
from
October 16, 2026 11:27
bb0e59b to
354c207
Compare
VLukiantsev
marked this pull request as ready for review
October 16, 2026 12:11
Base automatically changed from
feat/components-source-ownership-check-b548436300
to
main
October 16, 2026 12:18
…poch The ownership design allows one writing cluster per CMS (DD Stage 2 §3.4). A restored or rebuilt cluster, or a DR standby, must not silently take over components owned by another cluster. Moving ownership needs an explicit, reviewable step: raising a GitOps-declared epoch. Migration 015 adds the singleton ComponentOwnershipFence table (FenceId 1, reserved for partitioned fences later). The new DeclareClusterEpoch RPC takes a ClusterIdentity, which gains name = 2. The uid must be a canonical UUID (the kube-system Namespace UID) and the name a DNS-1123 label. The RPC updates the fence in one read-write transaction, as planDeclaration decides, and reports the outcome: - No fence yet: CREATED. The caller becomes active; the first owner keeps it. - Active cluster, same epoch: REFRESHED. A heartbeat that writes only LastSeenTime and, if it changed, the name, so it takes no lock on the cells that intents read. - Active cluster, higher epoch: RAISED (DeclaredTime moves). - Active cluster, lower epoch: CLUSTER_EPOCH_REGRESSED. - Another cluster, higher epoch: TAKEOVER. One DML statement moves every active owned component to the caller's cluster and epoch and clears the owner uid, keeping the namespace. The caller's next intent then adopts each component under the same ComponentId (R2, now explicit in checkSourceOwnership). - Another cluster, equal or lower epoch: CLUSTER_FENCED. Two clusters that declare the same epoch cannot both win. Rejections are FailedPrecondition with ErrorInfo metadata operation, active_cluster_name and active_epoch. Both stores implement it: the memory store under its lock, Spanner in a new spanner_ownership_fence.go. The README notes the takeover's ceiling under Spanner's per-commit mutation limit. Sourced intents do not check the fence yet; the next layer adds that, and the two layers land together. The bridge starts declaring in a later layer. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Address review feedback on the ownership fence: - Reject DeclareClusterEpoch requests without a cluster identity at the API boundary, return nil on a failed Spanner transaction, and check err before its NotFound code when reading the fence. - Adopt an owner cleared by a takeover only from the declaring cluster, so a fenced-out cluster cannot take components back (R6 instead of R2). - Add a singleton CHECK on ComponentOwnershipFence and sync the README ownership table, fence claims and ClusterIdentity.uid comment. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Assert that a fence takeover leaves the generation, UpdateTime and etag of reassigned components unchanged on both stores, and document why: the owner is not part of the Component resource. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…dateRequestSource ValidateClusterIdentity now reuses validateClusterUID and validateDNS1123Label, introduced by the #1233 hardening for ValidateRequestSource, so declared clusters and request sources apply the same identity checks. Messages are unchanged. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…over grace Migration 015 now declares LastSeenTime NOT NULL and CK_ComponentOwnershipFence_Epoch CHECK (Epoch >= 1), and drops the FenceId singleton CHECK so FenceId stays reserved for partitioned fences. A takeover sets SourceLastSeenTime to PENDING_COMMIT_TIMESTAMP(), starting the owner_stale adoption grace period, and fence rejections add active_cluster_uid to their ErrorInfo metadata. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
DeclareClusterEpoch can take over every component a cluster owns, so it now goes through the mutating-RPC authorization that #1219 added: only the configured controller service account (the bridge) or a platform admin may call it, and a server without an authorizer fails closed. The authorization check moves into Server.authorizeMutating, shared with validateMutatingRequest. TestServer_AuthorizationEnforcement now covers DeclareClusterEpoch, and a new test checks that a denied declaration leaves no fence behind. TestCMSServer_DeclareClusterEpoch configures the disabled authorizer like the other server tests. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Add PostgreSQL migration 015_component_ownership_fence and the ComponentOwnershipFence table to the canonical PostgreSQL ESS schema, mirroring the Spanner migration. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
…nce test assertions fenceError uses metadataKeyOperation, operationDeclareClusterEpoch and metadataKeyActive* constants instead of string literals. The fence tests check the status code and the ErrorInfo details separately. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Describe how a second cluster can reach the same CMS (rebuild, disaster recovery, migration, misconfiguration), why source ownership alone locks such a cluster out, and that the fence is a handover with one active writer, not multi-cluster support. Drop an internal design reference. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
VLukiantsev
force-pushed
the
feat/components-ownership-fence-b548436300
branch
from
October 16, 2026 12:18
b2f33e9 to
c2f0729
Compare
janzak-dev
previously approved these changes
Oct 16, 2026
…orInfoReason Export store.ErrorInfoReason next to ErrorInfoDomain and errorWithInfo, and use it in place of three test-local copies: reasonOf in store, errorInfoReason in store_test (which also skipped the domain check), and errorInfoReason in cms. Callers match only on the ErrorInfo reason, and the store package already exports the reasons and metadata keys, so the decoder belongs with them; production callers upstack can use it too. Comment link: #1234 (comment) Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (13 lines, excluding 65 test file lines) are within the 50-line grace delta.
Preserving prior approval by @janzak-dev on commit c2f0729.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the cluster ownership fence (migration
015) and theDeclareClusterEpochRPC, so moving component ownership between clusters needs an explicit, reviewable epoch raise.Motivation & Context
The design allows one writing cluster per CMS. A restored or rebuilt cluster, or a DR standby, must not silently take over components owned by another cluster.
Key Changes
015_component_ownership_fence(Spanner, mirrored for PostgreSQL ininternal/storage/postgres/migrations/ess/andess-schema.sql):ComponentOwnershipFencewithLastSeenTime NOT NULLandCK_ComponentOwnershipFence_Epoch CHECK (Epoch >= 1). CMS writes onlyFenceId=1; there is no singleton CHECK becauseFenceIdis reserved for partitioned fences.ClusterIdentitygainsname = 2.DeclareClusterEpochrejects a missingcluster, validates a canonical-UUID uid and a DNS-1123 name, and returnsCREATED,REFRESHED(heartbeat; writes onlyLastSeenTime/name),RAISED, orTAKEOVER. It is a mutating RPC: only callers that passAuthorizeMutating(the bridge service account or an admin) may declare; without an authorizer it returnsUnauthenticated.TAKEOVER(another cluster, higher epoch) moves every active owned component to the caller's cluster and epoch in one DML statement and clears the owner uid, settingSourceLastSeenTime = PENDING_COMMIT_TIMESTAMP()to start the stale-owner adoption grace period; the caller's next intent re-adopts each record under the sameComponentId(R2). Only the declaring cluster may adopt a cleared owner; the fenced-out cluster getsSOURCE_INCARNATION_MISMATCH(R6). The takeover is an owner-only write: generation,UpdateTimeand etag are unchanged.CLUSTER_EPOCH_REGRESSED(active cluster, lower epoch) andCLUSTER_FENCED(another cluster, equal or lower epoch), withErrorInfometadataoperation,active_cluster_uid,active_cluster_name,active_epoch.spanner_ownership_fence.gofor Spanner); README documents the takeover's ceiling under Spanner's per-commit mutation limit and the authorization.Stack Position
Assumptions
main; feat(components): enforce gRPC authentication and authorization on CMS handlers #1219's fail-closed authorizer required authorizingDeclareClusterEpoch(+15 prod), and the PostgreSQL mirror of migration015added 56 lines of DDL plus license headers to keep both schemas in sync. Review follow-ups added 20 more: named fence rejection metadata keys (+8) and the exportedstore.ErrorInfoReasondecoder (+12), which replaces three test-local copies and a production copy upstack. More than 100 prod lines are license headers on new files plus migration DDL, and executable Go is about 270 lines. The layer is one capability (the fence end to end: proto → CMS server → Store → memory and Spanner backends → migration); the only available split is horizontal (memory first, Spanner second) and would need a stub with incomplete semantics.FenceId=1) is enough for now — multi-cluster routing is deferred by the design — partitioned fences would reuse the reserved key.015is still free onmain— if another lands first, it must be renumbered.mainand before the authorization change — if wrong, the checkpoint must be re-run before merge.Verification & Testing
bazel test //pkg/components/store:store_test //pkg/components/cms:cms_test //internal/storage/spanner/migrations:migrations_test //api:protos_breaking_testTestServer_AuthorizationEnforcement(nil authorizer →Unauthenticated, viewer →PermissionDenied, bridge SA → store) andTestServer_DeclareClusterEpochAuthorization.//test/e2e:components_lifecycle_testpassed with the whole stack applied, using the real authorizer with the bridge service account.tools/analyze_diff_size.py, base = merge-base withmain< 8000 /code>): 498 prod / 501 test / 999 total / 16 doc lines (see the size exception above).015constraints, takeoverSourceLastSeenTime,active_cluster_uid) and the authorization; those are covered by the unit and Spanner-emulator tests above.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).main.015andDeclareClusterEpochSchemaMigrationsreachedVersion=15, Dirty=False. The fence row (FenceId=1) recorded the cluster'skube-systemUID, its name andEpoch=1; a refresh returnedOUTCOME_REFRESHED.CLUSTER_FENCED,CLUSTER_EPOCH_REGRESSED,InvalidArgumentCLUSTER_FENCED; the active cluster at epoch 1 against fence epoch 2 gotCLUSTER_EPOCH_REGRESSED;epoch=0gotInvalidArgument.Related Issues / Bugs
Bug: 548436300
TAG=agy
CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Stack created with GitHub Stacks CLI • Give Feedback 💬