Add object-store fencing token for remote store primary term validation - #22774
Add object-store fencing token for remote store primary term validation#22774Bukhtawar wants to merge 11 commits into
Conversation
PR Reviewer Guide 🔍(Review updated until commit 6e63b54)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 6e63b54 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 12ff413
Suggestions up to commit 61ef914
Suggestions up to commit c8faa16
Suggestions up to commit 5827e62
Suggestions up to commit 0c6ac19
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22774 +/- ##
============================================
+ Coverage 71.60% 71.63% +0.02%
- Complexity 77358 77438 +80
============================================
Files 6170 6170
Lines 359710 359987 +277
Branches 52460 52498 +38
============================================
+ Hits 257583 257882 +299
+ Misses 81693 81636 -57
- Partials 20434 20469 +35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Persistent review updated to latest commit b47b137 |
|
❌ Gradle check result for b47b137: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Persistent review updated to latest commit cec3a1b |
|
Persistent review updated to latest commit 9a7371f |
|
❌ Gradle check result for 9a7371f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Persistent review updated to latest commit b60f8c1 |
|
❌ Gradle check result for b60f8c1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Persistent review updated to latest commit 069b262 |
|
❌ Gradle check result for 069b262: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 1d04612. ⛔ Hard block: Issues at Medium severity or above will block this PR from merging.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
d9e196a to
99450f2
Compare
|
❌ Gradle check result for 3525958: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Segment-replication remote-store indices currently rely on a replica to fence a stale primary: every write fans out a no-op PRIMARY_TERM_VALIDATION request, and a partitioned primary is stopped because a replica that has learned a newer term rejects it. That makes at least one replica a safety requirement, which blocks zero-replica remote-backed indices from recovering automatically on node loss - the new primary would begin acknowledging seconds after node-left while the old one is very likely still alive and unaware. Make the object store the authority instead. Each shard keeps one small mutable object per primary term in the translog repository, fence__<invertLong(term)>, written only by compare-and-swap. The fencing token is the CAS chain, not the term: every acknowledged translog upload advances it, so a second writer holds a stale token and its next CAS fails before the write is acknowledged. Determinism comes from the key space rather than from winning a race. A takeover lists the paths and refuses if a strictly higher term owns one, creates its own with create-if-absent - a key no lower-term incumbent ever writes, so the create cannot be defeated - then deletes every lower-term object unconditionally. That delete, not the CAS, is what makes a higher-term takeover certain: the incumbent finds its object gone. A writer's only destructive act is deleting strictly below itself, so it can never touch a higher-term writer's path. A recovering primary claims the fence BEFORE reading the translog restore point it will serve from, which is what closes the acked-write-loss window: everything the previous primary acknowledged is already in that restore point, and nothing it attempts afterwards can be acknowledged. Asserted at the read choke point. The segment flow needs the fence too, which was not obvious and TLC found it. Term-scoped naming stops readers following a superseded copy's metadata, but until the new copy publishes its own that metadata is still the latest, and it defines the reference set garbage collection prunes to - so an unfenced publish makes the legitimate owner's own collection delete files it is still hydrating. Publication and collection are both gated on still owning the fence, as a read rather than a CAS, and fail-open for publication against fail-closed for collection. Relocation happens at a constant term, so source and target share one object and term-scoping cannot arbitrate. The source, which alone can write once it has drained, performs the ownership transfer as its last act before handing off and retains the resulting token; on abort it reverts with that token, and whether the revert succeeds is what distinguishes a target that took over from one that never wrote. A revert that fails means the target owns the chain, so the source fails its shard rather than serving on until its next upload loses the CAS. A conditional write whose response is lost is treated as ambiguous rather than failed. The write may have landed, leaving this copy holding a stale token whose next If-Match would fail for exactly the same reason a genuinely fenced writer's would - failing a healthy primary on a network blip. The ambiguity is resolved by identity: the object records the writing copy and its seq, so a blob carrying ours at the seq we attempted can only be the write we just issued, and its token is adopted. Anything else is reported as not-landed, which is retryable and never fences. formal-models/remote-store-fence/ holds four TLA+ specifications, each modelling the protocol as implemented and each explored to a complete state graph: seal ordering and acked-write loss, cross-term takeover determinism, the relocation handoff, and the segment flow with collection. Model checking is opt-in and the build downloads nothing - see that directory's README for why, and for what the segment flow deliberately does not cover. The specifications are the authority. Where the implementation and a model disagreed, the implementation moved: claim() checks that its sweep landed as an unconditional precondition rather than on one branch, and equal-term arbitration fences when the path has gone instead of recreating it. Constraints that come from the implementation are modelled explicitly rather than assumed away - a bounded equal-term retry, a batch delete that reports per-key failures instead of throwing, and an abort that can happen before the primary context was ever sent. Fencing is off by default, gated per index by a final setting stamped from a dynamic cluster default at creation time, and refuses any repository whose conditional writes are not enforced by the store: an emulated precondition would let two nodes both believe they hold the fence, which is worse than running unfenced because the shard would report itself protected. Replica term validation is unchanged, so this adds a second, independent check rather than replacing the existing one. Known limitations, deliberate: a fenced writer can leave an orphan metadata file, since the metadata upload and the CAS are not atomic; it is never acknowledged and readers following the highest-term lineage ignore it, but reclaiming them is a follow-up. The fence is control-flow only and is never read by snapshot restore, pinned-timestamp resolution or garbage collection, so the shallow snapshot contract is untouched. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com> CAS is defined once, on RemoteStoreFence, since the class and all four specifications are written in its terms: a CAS is a write the store accepts only if the object's current version is the one presented and refuses otherwise, and the CAS chain is the succession of tokens one object passes through, where a write must present the current token and hands back the next - so links cannot be skipped, holding the current token is what ownership means, and a writer that misses one can never rejoin. The README glossary and RemoteStoreFence.tla point at that definition rather than restating it. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
…nSearch into remote-store-fence # Conflicts: # formal-models/remote-store-fence/README.md # formal-models/remote-store-fence/tla/RemoteStoreFence.tla # server/src/main/java/org/opensearch/index/translog/transfer/RemoteStoreFence.java
Resolving the add/add conflict between the two squashes of this branch left AwsErrorDetails and ResponseBytes imported twice. Content is otherwise identical to af8b6f9, the squashed commit. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
|
Persistent review updated to latest commit 0c6ac19 |
|
❌ Gradle check result for 0c6ac19: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
A takeover claims the fence twice: the recovery seal claims via a throwaway instance and is discarded, the translog restore point is read with no live token held, and the translog's own fence instance re-adopts the chain on its first upload. That re-adoption was unguarded for a non-relocation copy, so it would take the chain back from an equal-term twin that legitimately claimed it during the window - and then serve from a restore point read before the twin's acknowledgements, losing acked writes. Require recorded ownership for every translog instance's equal-term arbitration: the seal records this copy's allocation id, so the normal flow is unaffected, while a copy whose chain a twin claimed is fenced and reassigned at a higher term instead of stealing the chain back. A relocation target already used the identical check to defer adoption until the source's ownership transfer; this unifies both under one recorded-ownership rule. Only the seal itself still arbitrates unguarded, so a new incarnation can take over a dead incumbent's path. Found by extending FenceTakeover.tla with the two-take structure: TLC violates NoAckedWriteLoss in 17 states unguarded, and verifies the guarded protocol clean at 3- and 4-writer bounds with complete state graphs. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
|
Persistent review updated to latest commit 5827e62 |
The takeover module modeled a single chain-take per writer, but the implementation takes the chain twice: the recovery seal claims via a throwaway instance, the restore point is read with no live token held, and the translog instance re-adopts on its first upload. Fold that structure into FenceTakeover.tla - a read (hydrating) state, the tokenless window, and a ReAdoptList/ReAdoptTake pair guarded by recorded ownership, matching the previous commit's fix. Measured, in the module's own style: relaxing the recorded-ownership guard violates NoAckedWriteLoss in a 17-state trace where a copy steals the chain back from an equal-term twin and serves from a restore point read before the twin's acknowledgement. As implemented, all four invariants hold over a complete state graph of 4,460,896 distinct states at the committed bounds, saturating identically at MaxTerm 5 and 8. Also align DeleteLowerPaths with the code: the sweep runs straight after create/arbitration without re-verifying possession, so guard it on having taken the path rather than on still holding the current token - the model now explores that reachable ordering instead of assuming it away. Update the README numbers and add the two-take section, and point code comments at FenceTakeover.tla. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Say what happens instead of naming it: the fence is claimed twice - first by the throwaway seal, then by the translog instance's re-adoption. Comments and docs only; TLC re-verified with the identical 4,460,896-state complete graph. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
|
Persistent review updated to latest commit c8faa16 |
|
❌ Gradle check result for c8faa16: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Three review findings, in decreasing order of blast radius: Give stale-segment collection its own fail-closed gate. The publish gate at the top of the segment sync fails open, and collection sat behind the same check - so one unreadable-fence event relaxed both, exactly the combination FenceSegmentFlow.tla measures as violating HydrationIntegrity. Collection now consults a fail-closed variant (isRemoteStoreFenceSupersededFailingClosed): an unreadable fence skips the deletion and retries next cycle, keeping the two failure directions genuinely independent. Stop classifying S3 409 ConditionalRequestConflict as a lost CAS. It means a conflicting conditional write is in progress and the outcome of this request's precondition is unknown; a retry answers definitively. Treating it as lost could fence a healthy writer whose rival lost the race. Only 412, or 409 with PreconditionFailed, now count as a lost CAS. Resolve ambiguous ownership-transfer writes by identity. The transfer and revert of a relocation handoff now adopt their own landed write after a lost response, as the ack path already did - and the handover in IndexShard#relocated moved inside the reclaim scope, so a failed or ambiguous transfer reclaims ownership instead of leaving a resuming source to be fenced on its next upload. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
|
Persistent review updated to latest commit 61ef914 |
The fs container's delete() invalidated conditional-write version tokens by prefix without a trailing separator, so deleting the container at /repo/idx-1 also invalidated live tokens under a sibling like /repo/idx-10 - spuriously fencing an unrelated shard in integration tests. Append the separator, with a regression test. Add unit tests for patch-coverage gaps that only integration tests exercised: the fence-ownership wrappers with and without a fence (InternalTranslogManager, TranslogTransferManager), the trim gate skipping remote deletion on a superseded copy, the IndexShard fence queries with fencing disabled, and the S3 SdkException fallbacks on the conditional read/write paths. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
|
Persistent review updated to latest commit 12ff413 |
The recorded-ownership guard made every translog fence instance take the same arbitration rule, so the isRelocationTarget flag threaded through TranslogConfig and buildTranslogTransferManager no longer fed anything: remove the field, the constructor parameters, and the now-redundant overload. Document two edges instead of defending them: the dynamic-durability window in getReplicationMode (a REQUEST->ASYNC flip can acknowledge one operation against neither witness, which is what ASYNC durability means), and why RemoteStoreFence.tla's single-register abstraction is faithful - it leans on the recorded-ownership guard, and the claim-twice window it cannot express is checked by FenceTakeover.tla. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
|
Persistent review updated to latest commit 6e63b54 |
|
❌ Gradle check result for 6e63b54: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 6e63b54: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Gradle-check failure triage — three failures across the last two runs, all reproduced locally as flaky:
|
|
❌ Gradle check result for 6e63b54: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 6e63b54: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 6e63b54: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 6e63b54: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| return true; | ||
| } | ||
| if (e.statusCode() != 409) { | ||
| return false; |
There was a problem hiding this comment.
If there's no current object version with the same name, or if the current object version is a delete marker, the operation fails with a 404 Not Found error.
Do we need to detect the 404 case? If I understand correctly, this is the case a partitioned writer will see once its successor is promoted and deletes the key the former primary was checking against.
| if (content.length > sizeBound) { | ||
| throw new IOException("[" + blobName + "] blob of size > [" + sizeBound + "] is too large for a versioned read"); | ||
| } |
There was a problem hiding this comment.
default value of sizeBound is 5 MB . Do we need a smaller value here for this object fencing token ?
| // when the translog construction downloads it in the snapshot V2 case), or destroys the remote | ||
| // translog lineage (the snapshot restore delete). | ||
| if (shardRouting.primary() && indexSettings.isRemoteTranslogStoreEnabled()) { | ||
| sealRemoteStoreFenceBeforeRestore(); |
There was a problem hiding this comment.
nit : rename to sealRemoteStoreFence since this is not only restore
| IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING, | ||
| IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING, | ||
| IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING, | ||
| IndexMetadata.INDEX_REMOTE_STORE_FENCING_ENABLED_SETTING, |
There was a problem hiding this comment.
one caveat for this feature is we can't enable it for remote migrating indices .
3 participants
Description
Remote store + segment replication fences a stale primary through replica-based primary term validation, which needs at least one replica as witness. Zero-replica setups therefore cannot safely recover a shard onto a new node after a node loss: nothing stops two copies writing to the same remote store paths.
This PR makes the object store itself the witness: a per-term fence object updated only by conditional writes. A copy that loses the compare-and-swap is fenced and can never acknowledge a write. This is the multi-writer safety property the zero-replica auto-restore work in #22768 needs (the allocation trigger is not in this PR). For replicated
request-durability indices it is a trade — replica witness → object store witness (NO_REPLICATIONbelow); for zero-replica indices it is a win outright.The protocol
Notation:
inv(t)=invertLong(t)— writteninv(5),inv(6)rather than the raw inverted longs.fence__inv(term), so a prefix listing returns the highest term first — that name ordering is how the object store orders cluster-manager grants it cannot interpret, with no cluster-manager I/O. Content (v1|indexUUID|shardId|term|allocationId|nodeId|seq, 68 bytes) records identity; the store's ETag is the token.seqmakes every write's bytes unique, so a content-digest ETag can never repeat (no ABA).If-Match <etag>) on its term's object, run in parallel with the metadata upload and joined before acknowledging. Losing the CAS is fatal for the copy: the shard fails instead of retrying. Only a proven precondition failure counts as a lost CAS; ambiguity (transport errors,409 ConditionalRequestConflict) stays retryable, and a lost response is resolved by re-reading the object and matching identity + seq.If-None-Match: *(uncontested — a lower-term incumbent never writes that key) → delete every lower-term object (the act that fences the incumbent) → re-list and verify. A writer's only destructive act is deleting strictly below its own term, so it can never touch a higher-term writer's path.FenceTakeover.tla: unguarded this loses acked writes in 17 states; guarded, all invariants hold).FenceSegmentFlow.tlaproves unsafe.Gating
cluster.remote_store.fencing.enabled— dynamic cluster setting (defaultfalse), baked at index creation intoindex.remote_store.fencing.enabled— final index setting (toggling a live index's write witness would leave a window checked against neither fence nor replicas). Explicit per-index value wins. A repository without store-enforced conditional writes fails shard startup fast rather than running unfenced.1. Steady state — every acknowledgement is a successful conditional PUT
sequenceDiagram autonumber participant C as Client participant A as Primary A (term 5) participant S3 as Object store Note over A: holds ETag E0 for fence__inv(5) C->>A: index op A->>S3: PUT translog data par immutable metadata upload A->>S3: PUT metadata__… and fence CAS (off the latency path) A->>S3: PUT fence__inv(5), If-Match E0, seq+1 S3-->>A: 200, new ETag E1 end A->>A: join both before acknowledging A-->>C: ack2. Failover across terms — deletion is the fencing act
sequenceDiagram autonumber participant A as Primary A (term 5, partitioned) participant S3 as Object store participant B as Copy B (appointed term 6) Note over A,S3: fence__inv(5): owner allocA B->>S3: LIST fence__* → only inv(5), nothing higher → proceed B->>S3: PUT fence__inv(6), If-None-Match * Note over B: uncontested — a lower term never writes this key B->>S3: DELETE fence__inv(5) B->>S3: LIST again — claim stands A->>S3: PUT fence__inv(5), If-Match (stale ETag) S3-->>A: refused — key is gone Note over A: fenced before acknowledging — shard fails B->>S3: only now read the translog restore point Note over B: everything A ever acknowledged is already in it3. Relocation handoff at a constant term — the ETag answers what the copies cannot ask each other
sequenceDiagram autonumber participant S as Source (term 5) participant S3 as Object store participant T as Target (term 5) S->>S: block ops, final sync, drain uploads S->>S3: PUT If-Match X → owner = allocT S3-->>S: 200, new ETag Y (retained) S->>T: hand off primary context S--xS: response lost → abort S->>S3: PUT If-Match Y → owner back to allocS (revert) alt 200 — nothing wrote since Y Note over S: target never took over → resume T->>S3: read fence → owner is allocS → stand down else refused — the target already wrote Note over S: handoff effectively completed → stand down T->>S3: continues acknowledging as owner end4. The segment-flow gates
flowchart TD R["refresh → syncSegments()"] --> Q{"publish gate:<br/>higher term owns the fence?"} Q -- no --> RUN["proceed"] Q -- yes --> SKIP["skip, do NOT retry"] Q -- "unreadable — FAIL OPEN" --> RUN RUN --> D["segment DATA upload — ungated, purely additive"] RUN --> M["uploadMetadata — publishes the reference set"] RUN --> G{"collection gate (separate check):<br/>superseded or unreadable?"} G -- no --> GC["deleteStaleSegments"] G -- "yes — FAIL CLOSED" --> GS["skip — retried next cycle"]Invariants
Documented by name in the
RemoteStoreFencejavadoc, referenced from the enforcing code:TranslogTransferManagerChanges
BlobContainergainsisConditionalWriteSupported(),readBlobWithVersion(),writeBlobConditionally()(+VersionedBlob,BlobVersionConflictException).S3BlobContainerimplements them overIf-None-Match: */If-Match; only412, or409 PreconditionFailed, counts as a lost CAS — everything ambiguous stays retryable.FsBlobContaineremulates CAS JVM-locally (lock striping + random tokens, not content digests) and reports unsupported, so fencing refuses it; only the test-onlyReloadableFsRepositoryopts in.RemoteStoreFenceowns the per-term objects, the claim/sweep/verify sequence, same-term arbitration, ownership transfer/revert, and lost-response adoption by identity.TranslogTransferManager; fenced upload is tragic. With fencing +requestdurability,TransportShardBulkActionreturnsNO_REPLICATION(replicas leave the fencing path);asynckeeps the per-op fanout.IndexShard); promotion seals inside the term transition; peer-recovery/relocation targets exempt (the source still legitimately serves).RemoteStoreRefreshListener, bothtrimUnreferencedReaders).formal-models/remote-store-fence/— four TLA+ modules checked by TLC over complete state graphs; opt-in (./gradlew :formal-models:modelCheck -PtlaToolsJar=…), deliberately not wired intocheckand downloading nothing.RemoteStoreFence.tlaFenceTakeover.tlaFenceHandoff.tlaFenceSegmentFlow.tlaCost
One conditional
PUTper translog sync (request) or sync interval (async), overlapping the metadata upload already on that path; one extra conditionalPUTper seal (recovery/promotion); oneGETper segment-sync/GC cycle on fenced indices.Testing
RemoteStoreFencingIT(10 tests): chain advancement with 0–2 replicas, fence invisibility to metadata listings, seal-over on failover, incumbent fenced before ack, superseded copies failing recovery/promotion at the seal (teeth-verified: each fails with its seal disabled), resize targets, and settings gating. Unit coverage:RemoteStoreFenceTests(codec, identity, lost-response adoption),TranslogTransferManagerTests(CAS‖upload, fatal-vs-retryable),RemoteFsTranslogTests,RemoteStoreRefreshListenerTests(split gates),InternalTranslogManagerTests,S3BlobStoreContainerTests,FsBlobContainerTests.Known limitations / follow-ups
verifyNoMultipleWritersunchanged — reviewer opinion welcome.RemoteStoreBaseIntegTestCase; force with-Dtests.remote_store.fencing=true) are follow-ups.Marking as draft while gathering feedback on the fence blob format and the immediate
NO_REPLICATIONswitch (the RFC staged it behind Phase 0 — input welcome on which staging is right).Review hardening (commits
5827e62…6e63b54)A model-assisted review pass added six commits: the recorded-ownership re-adoption guard (protocol point 4, found by extending
FenceTakeover.tlawith the claimed-twice structure); S3ConditionalRequestConflictreclassified retryable; the collection gate split fail-closed from the fail-open publish gate; lost-response transfer/revert resolved by identity with the handover insideIndexShard#relocated's reclaim scope; Fs token invalidation scoped with a trailing separator; deadisRelocationTargetplumbing removed (−55 lines). UncoveredIndexShardlines incodecov/patchare exercised end-to-end byRemoteStoreFencingIT, which patch coverage does not count.Related Issues
Relates to #22768
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.