Skip to content

HDDS-11411. Snapshot garbage collection should not run when the keys are moved from a deleted snapshot to the next snapshot in the chain #7193

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 15 commits into from
Sep 25, 2024
Merged

HDDS-11411. Snapshot garbage collection should not run when the keys are moved from a deleted snapshot to the next snapshot in the chain #7193

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b5f21a0
HDDS-11411. Snapshot garbage collection should not run when the keys …
swamirishi Sep 12, 2024
7c556b4
Merge remote-tracking branch 'apache/master' into HEAD
swamirishi Sep 20, 2024
bad7fe6
HDDS-11411. Merge with master
swamirishi Sep 20, 2024
7e66143
HDDS-11411. Merge with master
swamirishi Sep 20, 2024
83491aa
HDDS-11411. Add test cases
swamirishi Sep 24, 2024
05787df
HDDS-11411. Address review comments
swamirishi Sep 24, 2024
bf40940
HDDS-11411. FIx deep clean snapshot code
swamirishi Sep 24, 2024
7b570d5
HDDS-11411. FIx deep clean snapshot code
swamirishi Sep 24, 2024
89ca0f3
HDDS-11411. Handle bucket delete case
swamirishi Sep 24, 2024
2a707fa
HDDS-11411. Fix nullable uuid issue
swamirishi Sep 24, 2024
792db8e
HDDS-11411. Address review comments
swamirishi Sep 24, 2024
36411a6
HDDS-11411. Fix deep cleaning snapshot
swamirishi Sep 24, 2024
723c52b
HDDS-11411. Fix deep cleaning snapshot
swamirishi Sep 24, 2024
3f568db
HDDS-11411. Address review comments
swamirishi Sep 24, 2024
7cb49b7
HDDS-11411. Address review comments
swamirishi Sep 24, 2024
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
9 changes: 9 additions & 0 deletions hadoop-hdds/common/src/main/resources/ozone-default.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3743,6 +3743,15 @@
</description>
</property>

<property>
<name>ozone.snapshot.deep.cleaning.enabled</name>
<value>false</value>
<tag>OZONE, PERFORMANCE, OM</tag>
<description>
Flag to enable/disable snapshot deep cleaning.
</description>
</property>

<property>
<name>ozone.scm.event.ContainerReport.thread.pool.size</name>
<value>10</value>
Expand Down
2 changes: 2 additions & 0 deletions hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,8 @@ private OMConfigKeys() {
/**
* Configuration properties for Snapshot Directory Service.
*/
public static final String OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED = "ozone.snapshot.deep.cleaning.enabled";
public static final boolean OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED_DEFAULT = false;
public static final String OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL =
"ozone.snapshot.directory.service.interval";
public static final String OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL_DEFAULT
Expand Down
140 changes: 140 additions & 0 deletions ...on-test/src/test/java/org/apache/hadoop/fs/ozone/TestDirectoryDeletingServiceWithFSO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@

package org.apache.hadoop.fs.ozone;

import java.util.List;
import java.util.Random;
import java.util.concurrent.CompletableFuture;

import org.apache.commons.lang3.RandomStringUtils;
import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
import org.apache.hadoop.fs.FSDataOutputStream;
import org.apache.hadoop.fs.FileStatus;
Expand All @@ -32,10 +36,16 @@
import org.apache.hadoop.ozone.MiniOzoneCluster;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.TestDataUtil;
import org.apache.hadoop.ozone.client.BucketArgs;
import org.apache.hadoop.ozone.client.ObjectStore;
import org.apache.hadoop.ozone.client.OzoneBucket;
import org.apache.hadoop.ozone.client.OzoneClient;
import org.apache.hadoop.ozone.client.OzoneVolume;
import org.apache.hadoop.ozone.client.io.OzoneOutputStream;
import org.apache.hadoop.ozone.om.OMMetadataManager;
import org.apache.hadoop.ozone.om.OmSnapshotManager;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
import org.apache.hadoop.ozone.om.helpers.OzoneFileStatus;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
import org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer;
Expand All @@ -48,12 +58,16 @@
import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
import org.apache.ozone.test.GenericTestUtils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.mockito.ArgumentMatchers;
import org.mockito.Mockito;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -64,6 +78,8 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.LongSupplier;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SNAPSHOT_DELETING_SERVICE_INTERVAL;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -72,6 +88,12 @@
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_SERVICE_INTERVAL;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_FS_ITERATE_BATCH_SIZE;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.when;

/**
* Directory deletion service test cases.
Expand All @@ -97,6 +119,7 @@ public static void init() throws Exception {
conf.setInt(OMConfigKeys.OZONE_PATH_DELETING_LIMIT_PER_TASK, 5);
conf.setTimeDuration(OZONE_BLOCK_DELETING_SERVICE_INTERVAL, 100,
TimeUnit.MILLISECONDS);
conf.setTimeDuration(OZONE_SNAPSHOT_DELETING_SERVICE_INTERVAL, 1000, TimeUnit.MILLISECONDS);
conf.setBoolean(OMConfigKeys.OZONE_OM_RATIS_ENABLE_KEY, omRatisEnabled);
conf.setBoolean(OZONE_ACL_ENABLED, true);
cluster = MiniOzoneCluster.newBuilder(conf)
Expand Down Expand Up @@ -460,6 +483,123 @@ public void testDeleteFilesAndSubFiles() throws Exception {
assertEquals(prevDeletedKeyCount + 5, currentDeletedKeyCount);
}

private void createFileKey(OzoneBucket bucket, String key)
throws Exception {
byte[] value = RandomStringUtils.randomAscii(10240).getBytes(UTF_8);
OzoneOutputStream fileKey = bucket.createKey(key, value.length);
fileKey.write(value);
fileKey.close();
}

/*
* Create key d1/k1
* Create snap1
* Rename dir1 to dir2
* Delete dir2
* Wait for KeyDeletingService to start processing deleted key k2
* Create snap2 by making the KeyDeletingService thread wait till snap2 is flushed
* Resume KeyDeletingService thread.
* Read d1 from snap1.
*/
@Test
public void testAOSKeyDeletingWithSnapshotCreateParallelExecution()
throws Exception {
OMMetadataManager omMetadataManager = cluster.getOzoneManager().getMetadataManager();
Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Table<String, OmKeyInfo> deletedDirTable = omMetadataManager.getDeletedDirTable();
Table<String, String> renameTable = omMetadataManager.getSnapshotRenamedTable();
cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().shutdown();
DirectoryDeletingService dirDeletingService = cluster.getOzoneManager().getKeyManager().getDirDeletingService();
// Suspend KeyDeletingService
dirDeletingService.suspend();
GenericTestUtils.waitFor(() -> !dirDeletingService.isRunningOnAOS(), 1000, 10000);
Random random = new Random();
final String testVolumeName = "volume" + random.nextInt();
final String testBucketName = "bucket" + random.nextInt();
// Create Volume and Buckets
ObjectStore store = client.getObjectStore();
store.createVolume(testVolumeName);
OzoneVolume volume = store.getVolume(testVolumeName);
volume.createBucket(testBucketName,
BucketArgs.newBuilder().setBucketLayout(BucketLayout.FILE_SYSTEM_OPTIMIZED).build());
OzoneBucket bucket = volume.getBucket(testBucketName);

OzoneManager ozoneManager = Mockito.spy(cluster.getOzoneManager());
OmSnapshotManager omSnapshotManager = Mockito.spy(ozoneManager.getOmSnapshotManager());
when(ozoneManager.getOmSnapshotManager()).thenAnswer(i -> omSnapshotManager);
DirectoryDeletingService service = Mockito.spy(new DirectoryDeletingService(1000, TimeUnit.MILLISECONDS, 1000,
ozoneManager,
cluster.getConf()));
service.shutdown();
final int initialSnapshotCount =
(int) cluster.getOzoneManager().getMetadataManager().countRowsInTable(snapshotInfoTable);
final int initialDeletedCount = (int) omMetadataManager.countRowsInTable(deletedDirTable);
final int initialRenameCount = (int) omMetadataManager.countRowsInTable(renameTable);
String snap1 = "snap1";
String snap2 = "snap2";
createFileKey(bucket, "dir1/key1");
store.createSnapshot(testVolumeName, testBucketName, "snap1");
bucket.renameKey("dir1", "dir2");
OmKeyArgs omKeyArgs = new OmKeyArgs.Builder()
.setVolumeName(testVolumeName)
.setBucketName(testBucketName)
.setKeyName("dir2").build();
long objectId = store.getClientProxy().getOzoneManagerClient().getKeyInfo(omKeyArgs, false)
.getKeyInfo().getObjectID();
long volumeId = omMetadataManager.getVolumeId(testVolumeName);
long bucketId = omMetadataManager.getBucketId(testVolumeName, testBucketName);
String deletePathKey = omMetadataManager.getOzoneDeletePathKey(objectId,
omMetadataManager.getOzonePathKey(volumeId,
bucketId, bucketId, "dir2"));
bucket.deleteDirectory("dir2", true);


assertTableRowCount(deletedDirTable, initialDeletedCount + 1);
assertTableRowCount(renameTable, initialRenameCount + 1);
Mockito.doAnswer(i -> {
List<OzoneManagerProtocolProtos.PurgePathRequest> purgePathRequestList = i.getArgument(5);
for (OzoneManagerProtocolProtos.PurgePathRequest purgeRequest : purgePathRequestList) {
Assertions.assertNotEquals(deletePathKey, purgeRequest.getDeletedDir());
}
return i.callRealMethod();
}).when(service).optimizeDirDeletesAndSubmitRequest(anyLong(), anyLong(), anyLong(),
anyLong(), anyList(), anyList(), eq(null), anyLong(), anyInt(), Mockito.any(), any());

Mockito.doAnswer(i -> {
store.createSnapshot(testVolumeName, testBucketName, snap2);
GenericTestUtils.waitFor(() -> {
try {
SnapshotInfo snapshotInfo = store.getClientProxy().getOzoneManagerClient()
.getSnapshotInfo(testVolumeName, testBucketName, snap2);

return OmSnapshotManager.areSnapshotChangesFlushedToDB(cluster.getOzoneManager().getMetadataManager(),
snapshotInfo);
} catch (IOException e) {
throw new RuntimeException(e);
}
}, 1000, 100000);
GenericTestUtils.waitFor(() -> {
try {
return renameTable.get(omMetadataManager.getRenameKey(testVolumeName, testBucketName, objectId)) == null;
} catch (IOException e) {
throw new RuntimeException(e);
}
}, 1000, 10000);
return i.callRealMethod();
}).when(omSnapshotManager).getSnapshot(ArgumentMatchers.eq(testVolumeName), ArgumentMatchers.eq(testBucketName),
ArgumentMatchers.eq(snap1));
assertTableRowCount(snapshotInfoTable, initialSnapshotCount + 1);
service.runPeriodicalTaskNow();
service.runPeriodicalTaskNow();
assertTableRowCount(snapshotInfoTable, initialSnapshotCount + 2);
store.deleteSnapshot(testVolumeName, testBucketName, snap2);
service.runPeriodicalTaskNow();
store.deleteSnapshot(testVolumeName, testBucketName, snap1);
cluster.restartOzoneManager();
assertTableRowCount(cluster.getOzoneManager().getMetadataManager().getSnapshotInfoTable(), initialSnapshotCount);
dirDeletingService.resume();
}

@Test
public void testDirDeletedTableCleanUpForSnapshot() throws Exception {
Table<String, OmKeyInfo> deletedDirTable =
Expand Down
2 changes: 1 addition & 1 deletion .../java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDeletingServiceIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ private KeyDeletingService getMockedKeyDeletingService(AtomicBoolean keyDeletion
when(ozoneManager.getKeyManager()).thenReturn(keyManager);
KeyDeletingService keyDeletingService = Mockito.spy(new KeyDeletingService(ozoneManager,
ozoneManager.getScmClient().getBlockClient(), keyManager, 10000,
100000, cluster.getConf()));
100000, cluster.getConf(), false));
keyDeletingService.shutdown();
GenericTestUtils.waitFor(() -> keyDeletingService.getThreadCount() == 0, 1000,
100000);
Expand Down
8 changes: 8 additions & 0 deletions hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,6 +1381,8 @@ message PurgeKeysRequest {
// if set, will purge keys in a snapshot DB instead of active DB
optional string snapshotTableKey = 2;
repeated SnapshotMoveKeyInfos keysToUpdate = 3;
// previous snapshotID can also be null & this field would be absent in older requests.
optional NullableUUID expectedPreviousSnapshotID = 4;

@prashantpogde prashantpogde Sep 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do you deal with it if this field is null ?

@swamirishi swamirishi Sep 18, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

protobuf fields cannot be null.

@swamirishi swamirishi Sep 20, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to pass expectedPreviousSnapshotID = null for the case there are no snapshots in the chain. But older requests might not have a expectedPreviousSnapshotID in the request, so this validation could incorrectly run for the older requests leading to inconsistencies in the OM db on replays.

@swamirishi swamirishi Sep 20, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since there is no direct explicit way to differentiate b/w older requests & null values. I had to create a wrapper which means I can set NullableUUID which doesn't have anything inside. Since uuid field inside NullableUUID type is optional, we can signify this as a newer request and having nothing inside the field would signify a null value.

}

message PurgeKeysResponse {
Expand All @@ -1403,6 +1405,12 @@ message PurgePathsResponse {
message PurgeDirectoriesRequest {
repeated PurgePathRequest deletedPath = 1;
optional string snapshotTableKey = 2;
// previous snapshotID can also be null & this field would be absent in older requests.
optional NullableUUID expectedPreviousSnapshotID = 3;

@prashantpogde prashantpogde Sep 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same comment as above regarding this field being null.

}

message NullableUUID {
Comment thread
hemantk-12 marked this conversation as resolved.
optional hadoop.hdds.UUID uuid = 1;
}

message PurgeDirectoriesResponse {
Expand Down
8 changes: 6 additions & 2 deletions hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_TIMEOUT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_TIMEOUT_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DIRECTORY_SERVICE_TIMEOUT;
Expand Down Expand Up @@ -230,6 +232,8 @@ public KeyManagerImpl(OzoneManager om, ScmClient scmClient,

@Override
public void start(OzoneConfiguration configuration) {
boolean isSnapshotDeepCleaningEnabled = configuration.getBoolean(OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED,
OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED_DEFAULT);
if (keyDeletingService == null) {
long blockDeleteInterval = configuration.getTimeDuration(
OZONE_BLOCK_DELETING_SERVICE_INTERVAL,
Expand All @@ -241,7 +245,7 @@ public void start(OzoneConfiguration configuration) {
TimeUnit.MILLISECONDS);
keyDeletingService = new KeyDeletingService(ozoneManager,
scmClient.getBlockClient(), this, blockDeleteInterval,
serviceTimeout, configuration);
serviceTimeout, configuration, isSnapshotDeepCleaningEnabled);
keyDeletingService.start();
}

Expand Down Expand Up @@ -314,7 +318,7 @@ public void start(OzoneConfiguration configuration) {
}
}

if (snapshotDirectoryCleaningService == null &&
if (isSnapshotDeepCleaningEnabled && snapshotDirectoryCleaningService == null &&

@prashantpogde prashantpogde Sep 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

&& or
|| ?

@swamirishi swamirishi Sep 20, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It has to be && . Configuration to isSnapshotDeepCleaningEnabled should be enabled & also snapshotDirectoryCleaningService should not have been initialized before.

ozoneManager.isFilesystemSnapshotEnabled()) {
long dirDeleteInterval = configuration.getTimeDuration(
OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL,
Expand Down
Loading