From 33c64a0552ef896ad8b027b5da79e0849511d2d6 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Thu, 29 May 2025 20:54:31 -0400 Subject: [PATCH 1/2] HDDS-13136. KeyDeleting Service should not run for already deep cleaned snapshots Change-Id: I96595118eb079852b9bcd615cace36a3b9c136b6 --- .../ozone/om/service/KeyDeletingService.java | 8 +++- .../om/service/TestKeyDeletingService.java | 43 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java index 9dba72eb8d6c..571f1911e0fc 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java @@ -172,11 +172,11 @@ public void setKeyLimitPerTask(int keyLimitPerTask) { * the blocks info in its deletedBlockLog), it removes these keys from the * DB. */ - private final class KeyDeletingTask implements BackgroundTask { + final class KeyDeletingTask implements BackgroundTask { private final KeyDeletingService deletingService; private final UUID snapshotId; - private KeyDeletingTask(KeyDeletingService service, UUID snapshotId) { + KeyDeletingTask(KeyDeletingService service, UUID snapshotId) { this.deletingService = service; this.snapshotId = snapshotId; } @@ -321,6 +321,10 @@ public BackgroundTaskResult call() { snapInfo = snapshotId == null ? null : SnapshotUtils.getSnapshotInfo(getOzoneManager(), snapshotChainManager, snapshotId); if (snapInfo != null) { + if (snapInfo.getDeepClean()) { + LOG.info("Snapshot {} has already been deep cleaned. Skipping the snapshot in this iteration.", snapInfo); + return EmptyTaskResult.newResult(); + } if (!OmSnapshotManager.areSnapshotChangesFlushedToDB(getOzoneManager().getMetadataManager(), snapInfo)) { LOG.info("Skipping snapshot processing since changes to snapshot {} have not been flushed to disk", snapInfo); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java index f799d8af6190..f1c1073d874b 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java @@ -31,11 +31,14 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; @@ -48,11 +51,14 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import org.apache.commons.lang3.RandomStringUtils; import org.apache.hadoop.hdds.client.BlockID; import org.apache.hadoop.hdds.client.RatisReplicationConfig; @@ -60,6 +66,7 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList; import org.apache.hadoop.hdds.server.ServerUtils; +import org.apache.hadoop.hdds.utils.BackgroundTaskQueue; import org.apache.hadoop.hdds.utils.db.DBConfigFromFile; import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.hdds.utils.db.TableIterator; @@ -74,6 +81,7 @@ import org.apache.hadoop.ozone.om.OzoneManager; import org.apache.hadoop.ozone.om.PendingKeysDeletion; import org.apache.hadoop.ozone.om.ScmBlockLocationTestingClient; +import org.apache.hadoop.ozone.om.SnapshotChainManager; import org.apache.hadoop.ozone.om.helpers.BucketLayout; import org.apache.hadoop.ozone.om.helpers.KeyInfoWithVolumeContext; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; @@ -583,6 +591,41 @@ void testSnapshotDeepClean() throws Exception { } } + @Test + public void testKeyDeletingServiceWithDeepCleanedSnapshots() throws Exception { + OzoneManager ozoneManager = Mockito.spy(om); + OmMetadataManagerImpl omMetadataManager = Mockito.mock(OmMetadataManagerImpl.class); + SnapshotChainManager snapshotChainManager = Mockito.mock(SnapshotChainManager.class); + OmSnapshotManager omSnapshotManager = Mockito.mock(OmSnapshotManager.class); + when(ozoneManager.getMetadataManager()).thenReturn(omMetadataManager); + when(ozoneManager.getOmSnapshotManager()).thenReturn(omSnapshotManager); + when(omMetadataManager.getSnapshotChainManager()).thenReturn(snapshotChainManager); + when(snapshotChainManager.getTableKey(any(UUID.class))) + .thenAnswer(i -> i.getArgument(0).toString()); + Table snapshotInfoTable = Mockito.mock(Table.class); + when(omMetadataManager.getSnapshotInfoTable()).thenReturn(snapshotInfoTable); + when(snapshotInfoTable.get(any(String.class))).thenAnswer(i -> { + SnapshotInfo snapshotInfo = Mockito.mock(SnapshotInfo.class); + when(snapshotInfo.getSnapshotId()).thenReturn(UUID.fromString(i.getArgument(0))); + when(snapshotInfo.getDeepClean()).thenReturn(true); + return snapshotInfo; + }); + List snapshotIds = IntStream.range(0, 10).mapToObj(i -> UUID.randomUUID()).collect(Collectors.toList()); + when(snapshotChainManager.iterator(anyBoolean())).thenAnswer(i -> snapshotIds.iterator()); + KeyDeletingService kds = Mockito.spy(new KeyDeletingService(ozoneManager, scmBlockTestingClient, 10000, + 100000, conf, 10, true)); + when(kds.getTasks()).thenAnswer(i -> { + BackgroundTaskQueue queue = new BackgroundTaskQueue(); + for (UUID id : snapshotIds) { + queue.add(kds.new KeyDeletingTask(kds, id)); + } + return queue; + }); + kds.runPeriodicalTaskNow(); + clearInvocations(omSnapshotManager); + verify(omSnapshotManager, Mockito.never()).getActiveSnapshot(any(), any(), any()); + } + @Test void testSnapshotExclusiveSize() throws Exception { Table snapshotInfoTable = From 1aa96680efdd2204049e1c70f8d06e50169ca6c0 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Fri, 30 May 2025 22:08:21 -0400 Subject: [PATCH 2/2] HDDS-13136. Address review comments Change-Id: I011bcfa97440b14c96635b4a586950bf7c8ea47f --- .../org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java | 4 ++-- .../hadoop/ozone/om/helpers/TestOmSnapshotInfo.java | 8 ++++---- .../hadoop/ozone/om/service/KeyDeletingService.java | 8 +++++--- .../om/service/SnapshotDirectoryCleaningService.java | 2 +- .../snapshot/TestOMSnapshotPurgeRequestAndResponse.java | 4 ++-- .../hadoop/ozone/om/service/TestKeyDeletingService.java | 5 +++-- 6 files changed, 17 insertions(+), 14 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java index 4f4c9038f216..680e80bfd7a2 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java @@ -152,7 +152,7 @@ public void setCheckpointDir(String checkpointDir) { this.checkpointDir = checkpointDir; } - public boolean getDeepClean() { + public boolean isDeepCleaned() { return deepClean; } @@ -627,7 +627,7 @@ public long getExclusiveReplicatedSize() { return exclusiveReplicatedSize; } - public boolean getDeepCleanedDeletedDir() { + public boolean isDeepCleanedDeletedDir() { return deepCleanedDeletedDir; } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmSnapshotInfo.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmSnapshotInfo.java index eec64b90d45b..98cc035b3c07 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmSnapshotInfo.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmSnapshotInfo.java @@ -169,8 +169,8 @@ public void testSnapshotInfoProtoToSnapshotInfo() { snapshotInfoActual.getSnapshotStatus()); assertEquals(snapshotInfoExpected.getDbTxSequenceNumber(), snapshotInfoActual.getDbTxSequenceNumber()); - assertEquals(snapshotInfoExpected.getDeepClean(), - snapshotInfoActual.getDeepClean()); + assertEquals(snapshotInfoExpected.isDeepCleaned(), + snapshotInfoActual.isDeepCleaned()); assertEquals(snapshotInfoExpected.isSstFiltered(), snapshotInfoActual.isSstFiltered()); assertEquals(snapshotInfoExpected.getReferencedSize(), @@ -181,8 +181,8 @@ public void testSnapshotInfoProtoToSnapshotInfo() { snapshotInfoActual.getExclusiveSize()); assertEquals(snapshotInfoExpected.getExclusiveReplicatedSize(), snapshotInfoActual.getExclusiveReplicatedSize()); - assertEquals(snapshotInfoExpected.getDeepCleanedDeletedDir(), - snapshotInfoActual.getDeepCleanedDeletedDir()); + assertEquals(snapshotInfoExpected.isDeepCleanedDeletedDir(), + snapshotInfoActual.isDeepCleanedDeletedDir()); assertEquals(snapshotInfoExpected.getExclusiveSizeDeltaFromDirDeepCleaning(), snapshotInfoActual.getExclusiveSizeDeltaFromDirDeepCleaning()); assertEquals(snapshotInfoExpected.getExclusiveReplicatedSizeDeltaFromDirDeepCleaning(), diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java index 571f1911e0fc..d89726fd35ef 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java @@ -172,6 +172,7 @@ public void setKeyLimitPerTask(int keyLimitPerTask) { * the blocks info in its deletedBlockLog), it removes these keys from the * DB. */ + @VisibleForTesting final class KeyDeletingTask implements BackgroundTask { private final KeyDeletingService deletingService; private final UUID snapshotId; @@ -321,8 +322,9 @@ public BackgroundTaskResult call() { snapInfo = snapshotId == null ? null : SnapshotUtils.getSnapshotInfo(getOzoneManager(), snapshotChainManager, snapshotId); if (snapInfo != null) { - if (snapInfo.getDeepClean()) { - LOG.info("Snapshot {} has already been deep cleaned. Skipping the snapshot in this iteration.", snapInfo); + if (snapInfo.isDeepCleaned()) { + LOG.info("Snapshot {} has already been deep cleaned. Skipping the snapshot in this iteration.", + snapInfo.getSnapshotId()); return EmptyTaskResult.newResult(); } if (!OmSnapshotManager.areSnapshotChangesFlushedToDB(getOzoneManager().getMetadataManager(), snapInfo)) { @@ -330,7 +332,7 @@ public BackgroundTaskResult call() { snapInfo); return EmptyTaskResult.newResult(); } - if (!snapInfo.getDeepCleanedDeletedDir()) { + if (!snapInfo.isDeepCleanedDeletedDir()) { LOG.debug("Snapshot {} hasn't done deleted directory deep cleaning yet. Skipping the snapshot in this" + " iteration.", snapInfo); return EmptyTaskResult.newResult(); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDirectoryCleaningService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDirectoryCleaningService.java index 4c80d1600f77..a14003c2245b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDirectoryCleaningService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDirectoryCleaningService.java @@ -148,7 +148,7 @@ public BackgroundTaskResult call() { // Expand deleted dirs only on active snapshot. Deleted Snapshots // will be cleaned up by SnapshotDeletingService. if (currSnapInfo == null || currSnapInfo.getSnapshotStatus() != SNAPSHOT_ACTIVE || - currSnapInfo.getDeepCleanedDeletedDir()) { + currSnapInfo.isDeepCleanedDeletedDir()) { continue; } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java index db870e8480e5..1b97e3c3dbcf 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java @@ -479,8 +479,8 @@ private void validateSnapshotOrderInSnapshotInfoTableAndSnapshotChain( } for (SnapshotInfo snapshotInfo : snapshotInfoList) { assertEquals(snapshotInfo.getLastTransactionInfo(), expectedTransactionInfos.get(snapshotInfo.getSnapshotId())); - assertEquals(snapshotInfo.getDeepClean(), expectedDeepCleanFlags.get(snapshotInfo.getSnapshotId())); - assertEquals(snapshotInfo.getDeepCleanedDeletedDir(), expectedDeepCleanFlags.get(snapshotInfo.getSnapshotId())); + assertEquals(snapshotInfo.isDeepCleaned(), expectedDeepCleanFlags.get(snapshotInfo.getSnapshotId())); + assertEquals(snapshotInfo.isDeepCleanedDeletedDir(), expectedDeepCleanFlags.get(snapshotInfo.getSnapshotId())); } OmMetadataManagerImpl metadataManager = (OmMetadataManagerImpl) getOmMetadataManager(); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java index f1c1073d874b..68d9306584ae 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java @@ -592,6 +592,7 @@ void testSnapshotDeepClean() throws Exception { } @Test + @DisplayName("KeyDeletingService should skip active snapshot retrieval for deep cleaned snapshots") public void testKeyDeletingServiceWithDeepCleanedSnapshots() throws Exception { OzoneManager ozoneManager = Mockito.spy(om); OmMetadataManagerImpl omMetadataManager = Mockito.mock(OmMetadataManagerImpl.class); @@ -607,7 +608,7 @@ public void testKeyDeletingServiceWithDeepCleanedSnapshots() throws Exception { when(snapshotInfoTable.get(any(String.class))).thenAnswer(i -> { SnapshotInfo snapshotInfo = Mockito.mock(SnapshotInfo.class); when(snapshotInfo.getSnapshotId()).thenReturn(UUID.fromString(i.getArgument(0))); - when(snapshotInfo.getDeepClean()).thenReturn(true); + when(snapshotInfo.isDeepCleaned()).thenReturn(true); return snapshotInfo; }); List snapshotIds = IntStream.range(0, 10).mapToObj(i -> UUID.randomUUID()).collect(Collectors.toList()); @@ -903,7 +904,7 @@ private static void checkSnapDeepCleanStatus(Table table, while (iterator.hasNext()) { SnapshotInfo snapInfo = iterator.next().getValue(); if (volumeName.equals(snapInfo.getVolumeName())) { - assertThat(snapInfo.getDeepClean()) + assertThat(snapInfo.isDeepCleaned()) .as(snapInfo.toAuditMap().toString()) .isEqualTo(deepClean); }