Repository navigation
HDDS-17739. Fix snapshot diff dependency ordering for reoccupied paths. - #12014
Draft
SaketaChalamchala wants to merge 2 commits into
Draft
SaketaChalamchala wants to merge 2 commits into
SaketaChalamchala wants to merge 2 commits into
Conversation
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.
Found by TLA+ model checking and code review of the OM optimized snapshot diff paths under HDDS-15926, on commit d3b75c7.
The patch is generated with Specula (Claude Opus 5.5).
Should be merged after #12013
What changes were proposed in this pull request?
FSO snapshot diff dependency ordering (
SnapDiffDependencyGraph) incorrectly applied a reoccupied-path skip when adding to-snapshot path-prefix edges. That skip broke ordering in two ways:RENAME(target) under a reoccupied ancestor.Example A: Wrong order (skip too aggressive for RENAME)
Snapshot 1 → 2:
Report entries:
This leaves S2 with
b/ainstead ofa/a,b`After this patch: the graph detects a cycle (true constraint conflict). With HDDS-17738, ordered jobs should fall back to an unordered report instead of
FAILED.MODIFYentries (and missed ancestor-freed paths), so jobs failed even when a valid order existed.Example B: False cycle (skip too weak for MODIFY + nested recreate)
Snapshot 1 → 2:
Report entries (valid replay order exists):
MODIFY a/a/ais on the old tree (path in snapshot 1). It must run beforeRENAME a -> b. It must not depend onCREATE a,/ CREATE a/aon the new tree.The old logic still added (or failed to skip) a
CREATE a→MODIFY a/a/astyle edge when the path was freed via an ancestor rename (a, not a/a/a itself). That closed a false cycle → ordered jobFAILED, while unordered worked.After this patch: no to-snapshot edges for
MODIFY;expected order:
MODIFY a/a/a, RENAME a -> b, CREATE a, CREATE a/aThe fix aligns edge rules with the path namespace contract:
MODIFYnever gets to-snapshot prefix edges, andCREATE/RENAME(target)always keep those edges. The reoccupied-path skip is removed (buildReoccupiedPaths,shouldSkipReoccupiedPathToSnapshotEdge).MODIFY(andDELETE) entirely; onlyCREATEandRENAME(target) get to-snapshot prefix edges.buildReoccupiedPaths,shouldSkipReoccupiedPathToSnapshotEdge, andisStrictPathPrefix.CREATE/RENAME(target)ancestor → descendant edges (no skip).Rationale in one line:
MODIFYis from-snapshot only for hierarchy;CREATE/RENAME(target)are to-snapshot only. MixingMODIFYinto to-snapshot prefix edges (with heuristics) caused both bugs.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-17739
How was this patch tested?
TestSnapDiffDependencyGraphtestRenameSourceReuseWithDescendantRenameIntoNewOccupierDetectsCycle,testRenameSourceReuseByNestedCreateWithDescendantModify,testDeleteAndRecreateNestedPathWithDescendantModifyAndRenametestDeleteAndRecreateWithOldTreeChildModifyAndRenamenow expects a cycle (cross-parent rename into new occupier)MODIFYcases updated / still pass with the new edge rules