From febbb052e4d294ba0989cb225ff329f45ba1d80f Mon Sep 17 00:00:00 2001 From: andyhuangdev Date: Thu, 17 Sep 2026 16:40:48 +0800 Subject: [PATCH] HDDS-16465. Validate snapshot chains before mutation Validate global and path chains before adding or deleting snapshots to prevent partial updates when validation fails. Reject inconsistent deletion membership and add regression coverage for failed operations. Tests: TestSnapshotChain (20 passed), checkstyle, git diff --check. --- .../hadoop/ozone/om/SnapshotChainManager.java | 163 +++++++++++------- .../ozone/om/snapshot/TestSnapshotChain.java | 45 +++++ 2 files changed, 146 insertions(+), 62 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java index 5951c1303ffe..360f1f86b05a 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java @@ -70,18 +70,19 @@ public SnapshotChainManager(OMMetadataManager metadataManager) { } /** - * Add snapshot to global snapshot chain. + * Validate adding a snapshot to the global snapshot chain. */ - private void addSnapshotGlobal(UUID snapshotID, UUID prevGlobalID) + private void validateAddSnapshotGlobal(UUID snapshotID, UUID prevGlobalID) throws IOException { if (globalSnapshotChain.containsKey(snapshotID)) { throw new IOException(String.format( "Global Snapshot chain corruption. Snapshot with snapshotId: %s is " + "already present in the chain.", snapshotID)); } + if (!globalSnapshotChain.isEmpty() && prevGlobalID == null) { throw new IOException(String.format("Snapshot chain " + - "corruption. Adding snapshot %s as head node while there are %d " + + "corruption. Adding snapshot %s as head node while there are %d " + "snapshots in the global snapshot chain.", snapshotID, globalSnapshotChain.size())); } @@ -94,16 +95,22 @@ private void addSnapshotGlobal(UUID snapshotID, UUID prevGlobalID) "snapshot chain.", prevGlobalID, snapshotID)); } + if (prevGlobalID != null && globalSnapshotChain.get(prevGlobalID).hasNextSnapshotId()) { + throw new IOException(String.format( + "Global Snapshot chain corruption. Snapshot with snapshotId: %s " + + "already has the next snapshotId: %s. Adding snapshot %s " + + "with prevSnapshotId: %s will make the chain non linear.", + prevGlobalID, + globalSnapshotChain.get(prevGlobalID).getNextSnapshotId(), + snapshotID, prevGlobalID)); + } + } + + /** + * Add snapshot to global snapshot chain. + */ + private void addSnapshotGlobal(UUID snapshotID, UUID prevGlobalID) { if (prevGlobalID != null) { - if (globalSnapshotChain.get(prevGlobalID).hasNextSnapshotId()) { - throw new IOException(String.format( - "Global Snapshot chain corruption. Snapshot with snapshotId: %s " + - "already has the next snapshotId: %s. Adding snapshot %s " + - "with prevSnapshotId: %s will make the chain non linear.", - prevGlobalID, - globalSnapshotChain.get(prevGlobalID).getNextSnapshotId(), - snapshotID, prevGlobalID)); - } // On add snapshot, set previous snapshot entry nextSnapshotID = // snapshotID globalSnapshotChain.get(prevGlobalID).setNextSnapshotId(snapshotID); @@ -119,11 +126,9 @@ private void addSnapshotGlobal(UUID snapshotID, UUID prevGlobalID) } /** - * Add snapshot to bucket snapshot chain(path based). + * Validate adding a snapshot to the path snapshot chain. */ - private void addSnapshotPath(String snapshotPath, UUID snapshotID, - UUID prevPathID) throws IOException { - // On add snapshot, set previous snapshot entry nextSnapshotId = snapshotId + private void validateAddSnapshotPath(String snapshotPath, UUID snapshotID, UUID prevPathID) throws IOException { if (prevPathID != null && ((!snapshotChainByPath.containsKey(snapshotPath)) || (!snapshotChainByPath.get(snapshotPath).containsKey(prevPathID)))) { @@ -142,17 +147,22 @@ private void addSnapshotPath(String snapshotPath, UUID snapshotID, snapshotChainByPath.get(snapshotPath).size())); } + if (prevPathID != null && snapshotChainByPath.get(snapshotPath).get(prevPathID).hasNextSnapshotId()) { + throw new IOException(String.format( + "Path Snapshot chain corruption. Next snapshotId: %s is already " + + "set for snapshotId: %s. Adding snapshotId: %s with " + + "prevSnapshotId: %s will make the chain non linear.", + snapshotChainByPath.get(snapshotPath).get(prevPathID) + .getNextSnapshotId(), prevPathID, + snapshotID, prevPathID)); + } + } + + /** + * Add snapshot to bucket snapshot chain(path based). + */ + private void addSnapshotPath(String snapshotPath, UUID snapshotID, UUID prevPathID) { if (prevPathID != null && snapshotChainByPath.containsKey(snapshotPath)) { - if (snapshotChainByPath.get(snapshotPath).get(prevPathID) - .hasNextSnapshotId()) { - throw new IOException(String.format( - "Path Snapshot chain corruption. Next snapshotId: %s is already " + - "set for snapshotId: %s. Adding snapshotId: %s with " + - "prevSnapshotId: %s will make the chain non linear.", - snapshotChainByPath.get(snapshotPath).get(prevPathID) - .getNextSnapshotId(), prevPathID, - snapshotID, prevPathID)); - } snapshotChainByPath .get(snapshotPath) .get(prevPathID) @@ -170,26 +180,31 @@ private void addSnapshotPath(String snapshotPath, UUID snapshotID, latestSnapshotIdByPath.put(snapshotPath, snapshotID); } + private void validateDeleteSnapshotGlobal(UUID snapshotID) throws IOException { + UUID next = globalSnapshotChain.get(snapshotID).getNextSnapshotId(); + UUID prev = globalSnapshotChain.get(snapshotID).getPreviousSnapshotId(); + if (prev != null && !globalSnapshotChain.containsKey(prev)) { + throw new IOException(String.format( + "Global snapshot chain corruption. " + + "SnapshotId: %s to be deleted has previous snapshotId: %s " + + "but associated snapshot is not found in snapshot chain.", + snapshotID, prev)); + } + if (next != null && !globalSnapshotChain.containsKey(next)) { + throw new IOException(String.format( + "Global snapshot chain corruption. " + + "SnapshotId: {%s} to be deleted has next snapshotId: %s " + + "but associated snapshot is not found in snapshot chain.", + snapshotID, next)); + } + } + private boolean deleteSnapshotGlobal(UUID snapshotID) throws IOException { if (globalSnapshotChain.containsKey(snapshotID)) { // reset prev and next snapshot entries in chain ordered list // for node removal UUID next = globalSnapshotChain.get(snapshotID).getNextSnapshotId(); UUID prev = globalSnapshotChain.get(snapshotID).getPreviousSnapshotId(); - if (prev != null && !globalSnapshotChain.containsKey(prev)) { - throw new IOException(String.format( - "Global snapshot chain corruption. " + - "SnapshotId: %s to be deleted has previous snapshotId: %s " + - "but associated snapshot is not found in snapshot chain.", - snapshotID, prev)); - } - if (next != null && !globalSnapshotChain.containsKey(next)) { - throw new IOException(String.format( - "Global snapshot chain corruption. " + - "SnapshotId: {%s} to be deleted has next snapshotId: %s " + - "but associated snapshot is not found in snapshot chain.", - snapshotID, next)); - } globalSnapshotChain.remove(snapshotID); if (next != null) { globalSnapshotChain.get(next).setPreviousSnapshotId(prev); @@ -213,6 +228,30 @@ private boolean deleteSnapshotGlobal(UUID snapshotID) throws IOException { } } + private void validateDeleteSnapshotPath(String snapshotPath, UUID snapshotId) throws IOException { + UUID nextSnapshotId = snapshotChainByPath.get(snapshotPath).get(snapshotId).getNextSnapshotId(); + UUID previousSnapshotId = snapshotChainByPath.get(snapshotPath).get(snapshotId).getPreviousSnapshotId(); + if (previousSnapshotId != null && + !snapshotChainByPath.get(snapshotPath) + .containsKey(previousSnapshotId)) { + throw new IOException(String.format( + "Path snapshot chain corruption. " + + "SnapshotId: %s at snapshotPath: %s to be deleted has " + + "previous snapshotId: %s but associated snapshot is not " + + "found in snapshot chain.", snapshotId, snapshotPath, + previousSnapshotId)); + } + if (nextSnapshotId != null && !snapshotChainByPath.get(snapshotPath) + .containsKey(nextSnapshotId)) { + throw new IOException(String.format( + "Path snapshot chain corruption. " + + "SnapshotId: %s at snapshotPath: %s to be deleted has next " + + "snapshotId: %s but associated snapshot is not found in " + + "snapshot chain.", snapshotId, snapshotPath, + nextSnapshotId)); + } + } + private boolean deleteSnapshotPath(String snapshotPath, UUID snapshotId) throws IOException { if (snapshotChainByPath.containsKey(snapshotPath) && @@ -228,26 +267,6 @@ private boolean deleteSnapshotPath(String snapshotPath, .get(snapshotId) .getPreviousSnapshotId(); - if (previousSnapshotId != null && - !snapshotChainByPath.get(snapshotPath) - .containsKey(previousSnapshotId)) { - throw new IOException(String.format( - "Path snapshot chain corruption. " + - "SnapshotId: %s at snapshotPath: %s to be deleted has " + - "previous snapshotId: %s but associated snapshot is not " + - "found in snapshot chain.", snapshotId, snapshotPath, - previousSnapshotId)); - } - if (nextSnapshotId != null && !snapshotChainByPath.get(snapshotPath) - .containsKey(nextSnapshotId)) { - throw new IOException(String.format( - "Path snapshot chain corruption. " + - "SnapshotId: %s at snapshotPath: %s to be deleted has next " + - "snapshotId: %s but associated snapshot is not found in " + - "snapshot chain.", snapshotId, snapshotPath, - nextSnapshotId)); - } - snapshotChainByPath.get(snapshotPath).remove(snapshotId); if (nextSnapshotId != null) { snapshotChainByPath.get(snapshotPath) @@ -349,6 +368,9 @@ private boolean loadFromSnapshotInfoTable(OMMetadataManager metadataManager) { public synchronized void addSnapshot(SnapshotInfo snapshotInfo) throws IOException { validateSnapshotChain(); + validateAddSnapshotGlobal(snapshotInfo.getSnapshotId(), snapshotInfo.getGlobalPreviousSnapshotId()); + validateAddSnapshotPath(snapshotInfo.getSnapshotPath(), snapshotInfo.getSnapshotId(), + snapshotInfo.getPathPreviousSnapshotId()); addSnapshotGlobal(snapshotInfo.getSnapshotId(), snapshotInfo.getGlobalPreviousSnapshotId()); addSnapshotPath(snapshotInfo.getSnapshotPath(), @@ -375,8 +397,25 @@ public synchronized void updateSnapshot(SnapshotInfo snapshotInfo) { public synchronized boolean deleteSnapshot(SnapshotInfo snapshotInfo) throws IOException { validateSnapshotChain(); - return deleteSnapshotGlobal(snapshotInfo.getSnapshotId()) && - deleteSnapshotPath(snapshotInfo.getSnapshotPath(), snapshotInfo.getSnapshotId()); + UUID snapshotId = snapshotInfo.getSnapshotId(); + String snapshotPath = snapshotInfo.getSnapshotPath(); + boolean inGlobalChain = globalSnapshotChain.containsKey(snapshotId); + boolean inPathChain = snapshotChainByPath.containsKey(snapshotPath) + && snapshotChainByPath.get(snapshotPath).containsKey(snapshotId); + if (inGlobalChain != inPathChain) { + throw new IOException(String.format( + "Snapshot chain corruption. SnapshotId: %s has inconsistent membership for snapshot path: %s.", + snapshotId, snapshotPath)); + } + if (!inGlobalChain) { + LOG.warn("SnapshotId: {} is not found in the snapshot chain.", snapshotId); + return false; + } + validateDeleteSnapshotGlobal(snapshotId); + validateDeleteSnapshotPath(snapshotPath, snapshotId); + deleteSnapshotGlobal(snapshotId); + deleteSnapshotPath(snapshotPath, snapshotId); + return true; } /** diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotChain.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotChain.java index e62b64893254..4534e4b7d3a2 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotChain.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotChain.java @@ -207,6 +207,51 @@ public void testAddSnapshot() throws Exception { } } + @Test + public void testFailedAddPreservesChains() throws Exception { + UUID id = UUID.randomUUID(); + SnapshotInfo info = createSnapshotInfo(id, UUID.randomUUID(), null, 1L); + + assertThrows(IOException.class, () -> chainManager.addSnapshot(info)); + + assertTrue(chainManager.getGlobalSnapshotChain().isEmpty()); + assertTrue(chainManager.getSnapshotChainByPath().isEmpty()); + } + + @Test + public void testFailedAddPreservesExistingSnapshot() throws Exception { + UUID firstId = UUID.randomUUID(); + chainManager.addSnapshot(createSnapshotInfo(firstId, null, null, 1L)); + + UUID rejectedId = UUID.randomUUID(); + SnapshotInfo rejected = createSnapshotInfo(rejectedId, UUID.randomUUID(), firstId, 2L); + + assertThrows(IOException.class, () -> chainManager.addSnapshot(rejected)); + + assertEquals(1, chainManager.getGlobalSnapshotChain().size()); + assertEquals(1, chainManager.getSnapshotChainPath("vol1/bucket1").size()); + assertEquals(firstId, chainManager.getOldestGlobalSnapshotId()); + assertEquals(firstId, chainManager.getLatestGlobalSnapshotId()); + assertEquals(firstId, chainManager.getLatestPathSnapshotId("vol1/bucket1")); + assertFalse(chainManager.hasNextGlobalSnapshot(firstId)); + assertFalse(chainManager.hasNextPathSnapshot("vol1/bucket1", firstId)); + assertFalse(chainManager.getGlobalSnapshotChain().containsKey(rejectedId)); + } + + @Test + public void testWrongPathDeletePreservesGlobalChain() throws Exception { + UUID id = UUID.randomUUID(); + chainManager.addSnapshot(createSnapshotInfo(id, null, null, 1L)); + + SnapshotInfo wrongPath = createSnapshotInfo(id, null, null, 1L); + wrongPath.setSnapshotPath("vol1/bucket2"); + + assertThrows(IOException.class, () -> chainManager.deleteSnapshot(wrongPath)); + + assertTrue(chainManager.getGlobalSnapshotChain().containsKey(id)); + assertTrue(chainManager.getSnapshotChainPath("vol1/bucket1").containsKey(id)); + } + @Test public void testDeleteSnapshot() throws Exception { // add three snapshots