From 6fe1aa6076e8ec8cfe1371094704703afa0f60d8 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Tue, 3 Jun 2025 05:33:39 -0400 Subject: [PATCH 1/2] HDDS-13170. Reclaimable filter should always reclaim entries when buckets and volumes have already been deleted Change-Id: I16dc9d8f00686320b4e98fa5691420294a7f1e2f --- .../om/snapshot/filter/ReclaimableFilter.java | 21 +++++++++---- .../filter/AbstractReclaimableFilterTest.java | 27 ++++++++++++----- .../filter/TestReclaimableDirFilter.java | 2 +- .../filter/TestReclaimableFilter.java | 30 +++++++++++++++++++ .../filter/TestReclaimableKeyFilter.java | 2 +- .../TestReclaimableRenameEntryFilter.java | 2 +- 6 files changed, 68 insertions(+), 16 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java index 0bb53e628032..5dc78e708fcb 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java @@ -33,6 +33,7 @@ import org.apache.hadoop.ozone.om.OmSnapshotManager; import org.apache.hadoop.ozone.om.OzoneManager; import org.apache.hadoop.ozone.om.SnapshotChainManager; +import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock; @@ -167,11 +168,21 @@ private void initializePreviousSnapshotsFromChain(String volume, String bucket) previousOmSnapshots.add(null); previousSnapshotInfos.add(null); } - - // NOTE: Getting volumeId and bucket from active OM. - // This would be wrong on volume & bucket renames support. - bucketInfo = ozoneManager.getBucketInfo(volume, bucket); + } + // NOTE: Getting volumeId and bucket from active OM. + // This would be wrong on volume & bucket renames support. + try { + bucketInfo = ozoneManager.getBucketManager().getBucketInfo(volume, bucket); volumeId = ozoneManager.getMetadataManager().getVolumeId(volume); + } catch (OMException e) { + // If Volume or bucket has been deleted then all keys should be reclaimable as no snapshots would exist. + if (OMException.ResultCodes.VOLUME_NOT_FOUND == e.getResult() || + OMException.ResultCodes.BUCKET_NOT_FOUND == e.getResult()) { + bucketInfo = null; + volumeId = null; + return; + } + throw e; } } catch (IOException e) { this.cleanup(); @@ -187,7 +198,7 @@ public synchronized Boolean apply(Table.KeyValue keyValue) throws IOE if (!validateExistingLastNSnapshotsInChain(volume, bucket) || !snapshotIdLocks.isLockAcquired()) { initializePreviousSnapshotsFromChain(volume, bucket); } - boolean isReclaimable = isReclaimable(keyValue); + boolean isReclaimable = (bucketInfo == null) || isReclaimable(keyValue); // This is to ensure the reclamation ran on the same previous snapshot and no change occurred in the chain // while processing the entry. return isReclaimable && validateExistingLastNSnapshotsInChain(volume, bucket); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java index fc7a53422c50..4f0205a0e15a 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java @@ -48,12 +48,14 @@ import org.apache.hadoop.hdds.utils.db.RDBStore; import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB; +import org.apache.hadoop.ozone.om.BucketManager; import org.apache.hadoop.ozone.om.KeyManager; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OmSnapshot; import org.apache.hadoop.ozone.om.OmSnapshotManager; import org.apache.hadoop.ozone.om.OzoneManager; import org.apache.hadoop.ozone.om.SnapshotChainManager; +import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.helpers.BucketLayout; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; @@ -159,19 +161,28 @@ protected void teardown() throws IOException { private void mockOzoneManager(BucketLayout bucketLayout) throws IOException { OMMetadataManager metadataManager = mock(OMMetadataManager.class); + BucketManager bucketManager = mock(BucketManager.class); when(ozoneManager.getMetadataManager()).thenReturn(metadataManager); + when(ozoneManager.getBucketManager()).thenReturn(bucketManager); long volumeCount = 0; - long bucketCount = 0; for (String volume : volumes) { when(metadataManager.getVolumeId(eq(volume))).thenReturn(volumeCount); - for (String bucket : buckets) { - when(ozoneManager.getBucketInfo(eq(volume), eq(bucket))) - .thenReturn(OmBucketInfo.newBuilder().setVolumeName(volume).setBucketName(bucket) - .setObjectID(bucketCount).setBucketLayout(bucketLayout).build()); - bucketCount++; - } volumeCount++; } + + when(bucketManager.getBucketInfo(anyString(), anyString())).thenAnswer(i -> { + String volume = i.getArgument(0, String.class); + String bucket = i.getArgument(1, String.class); + if (!volumes.contains(volume)) { + throw new OMException("Volume " + volume + " already exists", OMException.ResultCodes.VOLUME_NOT_FOUND); + } + if (!buckets.contains(bucket)) { + throw new OMException("Bucket " + bucket + " already exists", OMException.ResultCodes.BUCKET_NOT_FOUND); + } + return OmBucketInfo.newBuilder().setVolumeName(volume).setBucketName(bucket) + .setObjectID((long) volumes.indexOf(volume) * buckets.size() + buckets.indexOf(bucket)) + .setBucketLayout(bucketLayout).build(); + }); } private void mockOmSnapshotManager(OzoneManager om) throws RocksDBException, IOException { @@ -232,7 +243,7 @@ private void mockOmSnapshotManager(OzoneManager om) throws RocksDBException, IOE protected List getLastSnapshotInfos( String volume, String bucket, int numberOfSnapshotsInChain, int index) { - List infos = getSnapshotInfos().get(getKey(volume, bucket)); + List infos = getSnapshotInfos().getOrDefault(getKey(volume, bucket), Collections.emptyList()); int endIndex = Math.min(index - 1, infos.size() - 1); return IntStream.range(endIndex - numberOfSnapshotsInChain + 1, endIndex + 1).mapToObj(i -> i >= 0 ? infos.get(i) : null).collect(Collectors.toList()); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java index a85da9900a03..c2fcfa30b097 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java @@ -72,7 +72,7 @@ private void testReclaimableDirFilter(String volume, String bucket, int index, List snapshotInfos = getLastSnapshotInfos(volume, bucket, 1, index); assertEquals(snapshotInfos.size(), 1); SnapshotInfo prevSnapshotInfo = snapshotInfos.get(0); - OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket); + OmBucketInfo bucketInfo = getOzoneManager().getBucketManager().getBucketInfo(volume, bucket); long volumeId = getOzoneManager().getMetadataManager().getVolumeId(volume); KeyManager keyManager = getKeyManager(); if (prevSnapshotInfo != null) { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java index 2b986f8fb32a..7b50cff3f388 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java @@ -130,6 +130,36 @@ public void testReclaimableFilterSnapshotChainInitialization( false); } + @ParameterizedTest + @MethodSource("testReclaimableFilterArguments") + public void testReclaimableFilterSnapshotChainInitializationWithInvalidVolume( + int numberOfPreviousSnapshotsFromChain, int actualNumberOfSnapshots) + throws IOException, RocksDBException { + SnapshotInfo currentSnapshotInfo = + setup(numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots, actualNumberOfSnapshots + 1, 4, 2); + String volume = "volume" + 6; + String bucket = getBuckets().get(1); + testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1, + currentSnapshotInfo, true, true); + testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1, + currentSnapshotInfo, false, true); + } + + @ParameterizedTest + @MethodSource("testReclaimableFilterArguments") + public void testReclaimableFilterSnapshotChainInitializationWithInvalidBucket( + int numberOfPreviousSnapshotsFromChain, int actualNumberOfSnapshots) + throws IOException, RocksDBException { + SnapshotInfo currentSnapshotInfo = + setup(numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots, actualNumberOfSnapshots + 1, 4, 2); + String volume = getVolumes().get(3); + String bucket = "bucket" + 6; + testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1, + currentSnapshotInfo, true, true); + testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1, + currentSnapshotInfo, false, true); + } + @ParameterizedTest @MethodSource("testReclaimableFilterArguments") public void testReclaimableFilterWithBucketVolumeMismatch( diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java index 9db680c18f97..5e781ddfec17 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java @@ -101,7 +101,7 @@ private void testReclaimableKeyFilter(String volume, String bucket, int index, List snapshotInfos = getLastSnapshotInfos(volume, bucket, 2, index); SnapshotInfo previousToPreviousSapshotInfo = snapshotInfos.get(0); SnapshotInfo prevSnapshotInfo = snapshotInfos.get(1); - OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket); + OmBucketInfo bucketInfo = getOzoneManager().getBucketManager().getBucketInfo(volume, bucket); long volumeId = getOzoneManager().getMetadataManager().getVolumeId(volume); UncheckedAutoCloseableSupplier prevSnap = Optional.ofNullable(prevSnapshotInfo) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java index 4fad10f248f7..59f4cf0ca02e 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java @@ -80,7 +80,7 @@ private void testReclaimableRenameEntryFilter(String volume, String bucket, int throws IOException { List snapshotInfos = getLastSnapshotInfos(volume, bucket, 1, index); SnapshotInfo prevSnapshotInfo = snapshotInfos.get(0); - OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket); + OmBucketInfo bucketInfo = getOzoneManager().getBucketManager().getBucketInfo(volume, bucket); if (prevSnapshotInfo != null) { UncheckedAutoCloseableSupplier prevSnap = Optional.ofNullable(prevSnapshotInfo) .map(info -> { From 2fb7b8ab1e1bae6f2df0a10e4bd917b33bf7d893 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Wed, 4 Jun 2025 12:58:09 -0400 Subject: [PATCH 2/2] HDDS-13170. Fix error message Change-Id: I223b99d0f9d6ccf97db0312ecf57250698442ebf --- .../om/snapshot/filter/AbstractReclaimableFilterTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java index 4f0205a0e15a..e8c362d9a5f4 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java @@ -174,10 +174,10 @@ private void mockOzoneManager(BucketLayout bucketLayout) throws IOException { String volume = i.getArgument(0, String.class); String bucket = i.getArgument(1, String.class); if (!volumes.contains(volume)) { - throw new OMException("Volume " + volume + " already exists", OMException.ResultCodes.VOLUME_NOT_FOUND); + throw new OMException("Volume " + volume + " doesn't exist", OMException.ResultCodes.VOLUME_NOT_FOUND); } if (!buckets.contains(bucket)) { - throw new OMException("Bucket " + bucket + " already exists", OMException.ResultCodes.BUCKET_NOT_FOUND); + throw new OMException("Bucket " + bucket + " doesn't exist", OMException.ResultCodes.BUCKET_NOT_FOUND); } return OmBucketInfo.newBuilder().setVolumeName(volume).setBucketName(bucket) .setObjectID((long) volumes.indexOf(volume) * buckets.size() + buckets.indexOf(bucket))