Skip to content

HDDS-13170. Reclaimable filter should always reclaim entries when buckets and volumes have already been deleted #8551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
swamirishi merged 2 commits into from
Jun 5, 2025
Merged

HDDS-13170. Reclaimable filter should always reclaim entries when buckets and volumes have already been deleted #8551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fe1aa6
HDDS-13170. Reclaimable filter should always reclaim entries when buc…
swamirishi Jun 3, 2025
2fb7b8a
HDDS-13170. Fix error message
swamirishi Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions ...e-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Copy link
Contributor

@jojochuang jojochuang Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not supported and is a design decision, so I think we can safely ignore it.

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();
Expand All @@ -187,7 +198,7 @@ public synchronized Boolean apply(Table.KeyValue<String, V> 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);
Expand Down
27 changes: 19 additions & 8 deletions ...c/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 + " doesn't exist", OMException.ResultCodes.VOLUME_NOT_FOUND);
}
if (!buckets.contains(bucket)) {
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))
.setBucketLayout(bucketLayout).build();
});
}

private void mockOmSnapshotManager(OzoneManager om) throws RocksDBException, IOException {
Expand Down Expand Up @@ -232,7 +243,7 @@ private void mockOmSnapshotManager(OzoneManager om) throws RocksDBException, IOE

protected List<SnapshotInfo> getLastSnapshotInfos(
String volume, String bucket, int numberOfSnapshotsInChain, int index) {
List<SnapshotInfo> infos = getSnapshotInfos().get(getKey(volume, bucket));
List<SnapshotInfo> 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());
Expand Down
2 changes: 1 addition & 1 deletion ...er/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ private void testReclaimableDirFilter(String volume, String bucket, int index,
List<SnapshotInfo> 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) {
Expand Down
30 changes: 30 additions & 0 deletions ...nager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion ...er/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ private void testReclaimableKeyFilter(String volume, String bucket, int index,
List<SnapshotInfo> 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<OmSnapshot> prevSnap = Optional.ofNullable(prevSnapshotInfo)
Expand Down
2 changes: 1 addition & 1 deletion ...est/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private void testReclaimableRenameEntryFilter(String volume, String bucket, int
throws IOException {
List<SnapshotInfo> 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<OmSnapshot> prevSnap = Optional.ofNullable(prevSnapshotInfo)
.map(info -> {
Expand Down