errose28 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1063956324
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1239,6 +1240,29 @@ private void deleteInternal(Container container, boolean
force)
DELETE_ON_NON_EMPTY_CONTAINER);
}
}
+ if (container.getContainerData() instanceof KeyValueContainerData) {
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) container.getContainerData();
+ HddsVolume hddsVolume = keyValueContainerData.getVolume();
+
+ // Rename container location
+ boolean success = KeyValueContainerUtil.ContainerDeleteDirectory
+ .moveToTmpDeleteDirectory(keyValueContainerData, hddsVolume);
+
+ if (success) {
+ String containerPath = keyValueContainerData
+ .getContainerPath().toString();
+ File containerDir = new File(containerPath);
+
+ LOG.debug("Container {} has been successfuly moved under {}",
+ containerDir.getName(), hddsVolume.getDeleteServiceDirPath());
+ } else {
+ LOG.error("Failed to move container under " +
+ hddsVolume.getDeleteServiceDirPath());
+ throw new StorageContainerException("Moving container failed",
+ CLOSED_CONTAINER_IO);
Review Comment:
CLOSED_CONTAINER_IO is for a client trying to write to a closed container.
CONTAINER_INTERNAL_ERROR would be better in this case.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +131,75 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnShutdown(String schemaVersion) throws IOException {
+ ContainerTestVersionInfo.setTestSchemaVersion(schemaVersion, conf);
+ LOG.info("SchemaV3_enabled: " +
+ conf.get(DatanodeConfiguration.CONTAINER_SCHEMA_V3_ENABLED));
+ service.start(conf);
+
+ // Get volumeSet and store volumes in temp folders
+ // in order to access them after service.stop()
+ MutableVolumeSet volumeSet = service
+ .getDatanodeStateMachine().getContainer().getVolumeSet();
+ List<HddsVolume> volumes = StorageVolumeUtil.getHddsVolumesList(
+ volumeSet.getVolumesList());
+
+ for (HddsVolume volume : volumes) {
+ volume.format(clusterId);
+ volume.createWorkingDir(clusterId, null);
+ volume.createDeleteServiceDir(clusterId);
Review Comment:
It might be better to call `StorageVolumeUtil#checkVolume` here instead to
make the test resemble volume init more accurately.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -326,6 +345,70 @@ public void createDbStore(MutableVolumeSet dbVolumeSet)
throws IOException {
}
}
+ /**
+ * Ensure that volume is initialized properly with
+ * cleanup path. Should disk be re-inserted into
+ * cluster, cleanup path should already be on
+ * disk. This method syncs the HddsVolume
+ * with the path on disk.
+ *
+ * @param id clusterId or scmId
+ */
+ public void checkTmpDirPaths(String id) {
Review Comment:
Why do we need this in addition to createTmpPath? The tmp dir should be
created on startup or the volume failed. We shouldn't need to check that it is
still there as the DN is running.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +131,75 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnShutdown(String schemaVersion) throws IOException {
+ ContainerTestVersionInfo.setTestSchemaVersion(schemaVersion, conf);
+ LOG.info("SchemaV3_enabled: " +
+ conf.get(DatanodeConfiguration.CONTAINER_SCHEMA_V3_ENABLED));
+ service.start(conf);
+
+ // Get volumeSet and store volumes in temp folders
+ // in order to access them after service.stop()
+ MutableVolumeSet volumeSet = service
+ .getDatanodeStateMachine().getContainer().getVolumeSet();
+ List<HddsVolume> volumes = StorageVolumeUtil.getHddsVolumesList(
+ volumeSet.getVolumesList());
+
+ for (HddsVolume volume : volumes) {
+ volume.format(clusterId);
+ volume.createWorkingDir(clusterId, null);
+ volume.createDeleteServiceDir(clusterId);
+
+ // Create a container and move it under the tmp delete dir.
+ KeyValueContainer container = setUpTestContainer(volume, schemaVersion);
+ assertTrue(container.getContainerFile().exists());
+ }
+
+ service.stop();
+ service.join();
+ service.close();
+
+ for (HddsVolume hddsVolume : volumes) {
+ ListIterator<File> deleteLeftoverIt = KeyValueContainerUtil
+ .ContainerDeleteDirectory.getDeleteLeftovers(hddsVolume);
+ assertFalse(deleteLeftoverIt.hasNext());
+ }
Review Comment:
Similar comment for TestEndPoint#testTmpDirCleanup
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +131,75 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnShutdown(String schemaVersion) throws IOException {
+ ContainerTestVersionInfo.setTestSchemaVersion(schemaVersion, conf);
+ LOG.info("SchemaV3_enabled: " +
+ conf.get(DatanodeConfiguration.CONTAINER_SCHEMA_V3_ENABLED));
+ service.start(conf);
+
+ // Get volumeSet and store volumes in temp folders
+ // in order to access them after service.stop()
+ MutableVolumeSet volumeSet = service
+ .getDatanodeStateMachine().getContainer().getVolumeSet();
+ List<HddsVolume> volumes = StorageVolumeUtil.getHddsVolumesList(
+ volumeSet.getVolumesList());
+
+ for (HddsVolume volume : volumes) {
+ volume.format(clusterId);
+ volume.createWorkingDir(clusterId, null);
+ volume.createDeleteServiceDir(clusterId);
Review Comment:
Similar comment for TestEndPoint#testTmpDirCleanup
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java:
##########
@@ -165,17 +165,95 @@ public void testGetVersionTask() throws Exception {
}
}
+
+ /**
+ * Writing data to tmp dir and checking if it has been cleaned.
+ */
@Test
- public void testCheckVersionResponse() throws Exception {
+ public void testTmpDirCleanup() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_IPC_RANDOM_PORT,
true);
conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_IPC_RANDOM_PORT,
true);
- conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_DATASTREAM_ENABLED,
Review Comment:
I see a similar thing in `testGetVersionTask`
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -67,6 +70,10 @@ public class HddsVolume extends StorageVolume {
private static final Logger LOG = LoggerFactory.getLogger(HddsVolume.class);
public static final String HDDS_VOLUME_DIR = "hdds";
+ private static final String FILE_SEPARATOR = File.separator;
+ private static final String TMP_DIR = FILE_SEPARATOR + "tmp";
+ private static final String TMP_DELETE_SERVICE_DIR =
Review Comment:
Let's use standard java libraries to build paths instead of string
concatenation. We shouldn't need to save the file separator.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +413,179 @@ public static Path getMetadataDirectory(
return Paths.get(metadataPath);
}
+
+ /**
+ * Utilities for container_delete_service directory
+ * which is located under <volume>/hdds/<cluster-id>/tmp/.
+ * Containers will be moved under it before getting deleted
+ * to avoid, in case of failure, having artifact leftovers
+ * on the default container path on the disk.
+ *
+ * Delete operation for Schema < V3
+ * 1. Container directory renamed to tmp directory.
+ * 2. Container is removed from in memory container set.
+ * 3. Container is deleted from tmp directory.
+ *
+ * Delete operation for Schema V3
+ * 1. Container directory renamed to tmp directory.
+ * 2. Container is removed from in memory container set.
+ * 3. Container's entries are removed from RocksDB.
+ * 4. Container is deleted from tmp directory.
+ */
+ public static class ContainerDeleteDirectory {
+
+ /**
+ * Delete all files under
+ * <volume>/hdds/<cluster-id>/tmp/container_delete_service.
+ */
+ public static synchronized void cleanTmpDir(HddsVolume hddsVolume)
+ throws IOException {
+ if (hddsVolume.getStorageState() != StorageVolume.VolumeState.NORMAL) {
+ LOG.debug("Call to clean tmp dir container_delete_service directory "
+ + "for {} while VolumeState {}",
+ hddsVolume.getStorageDir(),
+ hddsVolume.getStorageState().toString());
+ return;
+ }
+
+ if (!hddsVolume.getClusterID().isEmpty()) {
+ // Initialize tmp and delete service directories
+ hddsVolume.checkTmpDirPaths(hddsVolume.getClusterID());
+ } else {
+ LOG.error("Volume hasn't been formatted " +
+ "properly and has no ClusterId. " +
+ "Unable to initialize tmp and delete service directories.");
+ }
Review Comment:
Same comment in a few spots below in this class.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +131,75 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnShutdown(String schemaVersion) throws IOException {
+ ContainerTestVersionInfo.setTestSchemaVersion(schemaVersion, conf);
+ LOG.info("SchemaV3_enabled: " +
+ conf.get(DatanodeConfiguration.CONTAINER_SCHEMA_V3_ENABLED));
+ service.start(conf);
+
+ // Get volumeSet and store volumes in temp folders
+ // in order to access them after service.stop()
+ MutableVolumeSet volumeSet = service
+ .getDatanodeStateMachine().getContainer().getVolumeSet();
+ List<HddsVolume> volumes = StorageVolumeUtil.getHddsVolumesList(
+ volumeSet.getVolumesList());
+
+ for (HddsVolume volume : volumes) {
+ volume.format(clusterId);
+ volume.createWorkingDir(clusterId, null);
+ volume.createDeleteServiceDir(clusterId);
+
+ // Create a container and move it under the tmp delete dir.
+ KeyValueContainer container = setUpTestContainer(volume, schemaVersion);
+ assertTrue(container.getContainerFile().exists());
+ }
+
+ service.stop();
+ service.join();
+ service.close();
+
+ for (HddsVolume hddsVolume : volumes) {
+ ListIterator<File> deleteLeftoverIt = KeyValueContainerUtil
+ .ContainerDeleteDirectory.getDeleteLeftovers(hddsVolume);
+ assertFalse(deleteLeftoverIt.hasNext());
+ }
Review Comment:
It would be good to have a similar block before the shutdown to make sure
that the test was set up properly and the container made it to the tmp
directory.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java:
##########
@@ -165,17 +165,95 @@ public void testGetVersionTask() throws Exception {
}
}
+
+ /**
+ * Writing data to tmp dir and checking if it has been cleaned.
+ */
@Test
- public void testCheckVersionResponse() throws Exception {
+ public void testTmpDirCleanup() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_IPC_RANDOM_PORT,
true);
conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_IPC_RANDOM_PORT,
true);
- conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_DATASTREAM_ENABLED,
+ conf.setFromObject(new ReplicationConfig().setPort(0));
+ try (EndpointStateMachine rpcEndPoint = createEndpoint(conf,
+ serverAddress, 1000)) {
+ DatanodeDetails datanodeDetails = randomDatanodeDetails();
+ OzoneContainer ozoneContainer = new OzoneContainer(
+ datanodeDetails, conf, getContext(datanodeDetails), null);
+ rpcEndPoint.setState(EndpointStateMachine.EndPointStates.GETVERSION);
+
+ String clusterId = scmServerImpl.getClusterId();
+
+ // write some data before calling versionTask.call()
+ MutableVolumeSet volumeSet = ozoneContainer.getVolumeSet();
+ for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
+ volumeSet.getVolumesList())) {
+ hddsVolume.format(clusterId);
+ hddsVolume.createWorkingDir(clusterId, null);
+ hddsVolume.createDeleteServiceDir(clusterId);
+
+ // Create a container and move it under the tmp delete dir.
+ KeyValueContainer container =
+ setUpTestContainer(hddsVolume, clusterId, conf);
+ assertTrue(container.getContainerFile().exists());
+ }
+
+ VersionEndpointTask versionTask = new VersionEndpointTask(rpcEndPoint,
+ conf, ozoneContainer);
+ EndpointStateMachine.EndPointStates newState = versionTask.call();
+
+ Assertions.assertEquals(EndpointStateMachine.EndPointStates.REGISTER,
+ newState);
+
+ // assert that tmp dir is empty
+ for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
+ volumeSet.getVolumesList())) {
+ Assert.assertFalse(KeyValueContainerUtil.ContainerDeleteDirectory
+ .getDeleteLeftovers(hddsVolume).hasNext());
+ }
+ }
+ }
+
+ private KeyValueContainer setUpTestContainer(
Review Comment:
This looks similar to `TestHDDSDatanodeService#setUpTestContainer`. Can we
make these one method and move it to a common place like `ContainerTestUtils`?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java:
##########
@@ -105,20 +116,13 @@ public static void setUp() throws Exception {
scmServer = SCMTestUtils.startScmRpcServer(SCMTestUtils.getConf(),
scmServerImpl, serverAddress, 10);
testDir = PathUtils.getTestDir(TestEndPoint.class);
- config = SCMTestUtils.getConf();
- config.set(DFS_DATANODE_DATA_DIR_KEY, testDir.getAbsolutePath());
- config.set(OZONE_METADATA_DIRS, testDir.getAbsolutePath());
- config
- .setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_IPC_RANDOM_PORT, true);
- config.set(HddsConfigKeys.HDDS_COMMAND_STATUS_REPORT_INTERVAL, "1s");
- config.setFromObject(new ReplicationConfig().setPort(0));
Review Comment:
Why were these configs removed?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +413,179 @@ public static Path getMetadataDirectory(
return Paths.get(metadataPath);
}
+
+ /**
+ * Utilities for container_delete_service directory
+ * which is located under <volume>/hdds/<cluster-id>/tmp/.
+ * Containers will be moved under it before getting deleted
+ * to avoid, in case of failure, having artifact leftovers
+ * on the default container path on the disk.
+ *
+ * Delete operation for Schema < V3
+ * 1. Container directory renamed to tmp directory.
+ * 2. Container is removed from in memory container set.
+ * 3. Container is deleted from tmp directory.
+ *
+ * Delete operation for Schema V3
+ * 1. Container directory renamed to tmp directory.
+ * 2. Container is removed from in memory container set.
+ * 3. Container's entries are removed from RocksDB.
+ * 4. Container is deleted from tmp directory.
+ */
+ public static class ContainerDeleteDirectory {
+
+ /**
+ * Delete all files under
+ * <volume>/hdds/<cluster-id>/tmp/container_delete_service.
+ */
+ public static synchronized void cleanTmpDir(HddsVolume hddsVolume)
+ throws IOException {
+ if (hddsVolume.getStorageState() != StorageVolume.VolumeState.NORMAL) {
+ LOG.debug("Call to clean tmp dir container_delete_service directory "
+ + "for {} while VolumeState {}",
+ hddsVolume.getStorageDir(),
+ hddsVolume.getStorageState().toString());
+ return;
+ }
+
+ if (!hddsVolume.getClusterID().isEmpty()) {
+ // Initialize tmp and delete service directories
+ hddsVolume.checkTmpDirPaths(hddsVolume.getClusterID());
+ } else {
+ LOG.error("Volume hasn't been formatted " +
+ "properly and has no ClusterId. " +
+ "Unable to initialize tmp and delete service directories.");
+ }
Review Comment:
This need would need to account for being pre-finalized for SCM HA by by
using the cluster ID or SCM ID. However I don't think this block is even
necessary here since the volume should already be initialized and that info
stored in the hddsVolume instance so we should be able to delete this. cluster
ID/scm ID choice should be made on volume init, stored in the volume, and not
needed again from outside callers.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java:
##########
@@ -165,17 +165,95 @@ public void testGetVersionTask() throws Exception {
}
}
+
+ /**
+ * Writing data to tmp dir and checking if it has been cleaned.
+ */
@Test
- public void testCheckVersionResponse() throws Exception {
+ public void testTmpDirCleanup() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_IPC_RANDOM_PORT,
true);
conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_IPC_RANDOM_PORT,
true);
- conf.setBoolean(OzoneConfigKeys.DFS_CONTAINER_RATIS_DATASTREAM_ENABLED,
Review Comment:
Looks like this test had ratis streaming enabled but it got turned off by
accident. May have happened when resolving conflicts from master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
