You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/09/07 16:49:48 UTC
[GitHub] [ozone] xBis7 opened a new pull request, #3741: HDDS-6449. Failed container delete can leave artifacts on disk
xBis7 opened a new pull request, #3741:
URL: https://github.com/apache/ozone/pull/3741
## What changes were proposed in this pull request?
In case there is a failure during a container delete there will be artifact leftovers on disk. To avoid that, containers will be renamed to a new location under a tmp directory and will be deleted from that location. This solution only works if SchemaV3 is enabled. For previous Schema versions, delete operation is exactly the same.
To facilitate this implementation, a helper class called `CleanUpManager` was created which initializes the tmp directory on each disk under the path `/<volume>/<clusterId>/tmp/container_delete_service` and handles all operations related to this directory. Before every container delete, if SchemaV3 is enabled then we rename the container, set the path to the new location and then delete it from there.
On datanode startup and shutdown we are checking the tmp directory for any leftovers and then clean it up. The datanode startup hook is in `VersionEndpointTask.checkVolumeSet()` and the shutdown is in `HddsDatanodeService.stop()`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6449
## How was this patch tested?
This patch was tested with new unit tests. We added `TestContainerPersistence.testDeleteContainerWithSchemaV3Enabled()` for testing `CleanUpManager` and `TestKeyValueHandler.testDeleteContainerWithSchemaV3Enabled()` for testing `KeyValueHandler.deleteInternal()`. In addition, we tested cleaning the tmp directory on datanode startup in `TestEndPoint.testGetVersionTask()` and on datanode shutdown in `TestHddsDatanodeService`.
There were some issues with the existing code for `TestContainerPersistence.testDeleteContainer()`. The code block that was expected to throw an non-empty container exception, was never executed while the method was exiting upon the first exception thrown. Furthermore, the command that was used for deleting the container wasn’t checking if it was empty or not. In order to fix that, the method was splitted into two new ones and `KeyValueHandler.deleteContainer()` was used instead of `container.delete()`.
Due to the changes related to SchemaV3 some tests under `container-service` package were failing, so we had to make small changes to fix their setups.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r965665476
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private void setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException();
Review Comment:
@myskov In the catch statement we are logging both the message and the exception.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122608079
##########
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:
@xBis7 , we can remove the need for the `checkTmpDir` methods in the `HddsVolume` with a few changes that cleanup the code and also resolves this issue.
1.) remove `HddsVolume.checkTmpPaths`
2.) remove `checkCleanupDirs `method and calls in `StorageVolumeUtil`
3.) in `HddsVolume.createTmpPath` get the `id` from a call to new` HddsVolume` method,
`private String getIdDir() `that returns versions id : `return VersionedDatanodeFeatures.ScmHA.chooseContainerPathID(this, getClusterID() == null ? "" : getClusterID());`
4.) In `keyValueContainerUtil.getLeftovers` and `keyValueContainerUtil.moveToTmpDirectory` call `createDeleteServiceDir `instead of `checkTmpPaths`. The `createDeleteServiceDir` will check whether the path exists in hddsvolume intstance or on disk as we want it to.
With the above changes, the code is simplified in the datanode start `VersionEndpointTask`, we no longer need the checkTmpPaths, it takes into account the volume path layout for different versions (scmid, clusterid) and it seems to pass all the unit tests. I'll supply you with the diff. Note the unit test failures in the current CI workflow.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064764590
##########
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:
@errose28 The tmp is created when creating a working dir. There are cases were this step is missed because working dir already exists but the paths used for the tmp dir are null and we need to set them again. Check a workflow I launched where we are skipping this method.
https://github.com/xBis7/ozone/actions/runs/3874176967/jobs/6605289826
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133021713
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java:
##########
@@ -178,32 +187,61 @@ public void testTmpDirCleanup() throws Exception {
String clusterId = scmServerImpl.getClusterId();
- // write some data before calling versionTask.call()
+ // VolumeSet for this test, contains only 1 volume
MutableVolumeSet volumeSet = ozoneContainer.getVolumeSet();
- for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
- volumeSet.getVolumesList())) {
- StorageVolumeUtil.checkVolume(hddsVolume, clusterId,
- clusterId, conf, null, null);
-
- // Create a container and move it under the tmp delete dir.
- KeyValueContainer container = ContainerTestUtils.
- setUpTestContainerUnderTmpDir(hddsVolume, clusterId,
- conf, OzoneConsts.SCHEMA_V3);
- Assertions.assertTrue(container.getContainerFile().exists());
- }
+ Assertions.assertEquals(1, volumeSet.getVolumesList().size());
+ StorageVolume volume = volumeSet.getVolumesList().get(0);
- VersionEndpointTask versionTask = new VersionEndpointTask(rpcEndPoint,
- conf, ozoneContainer);
- EndpointStateMachine.EndPointStates newState = versionTask.call();
+ // Check instanceof and typecast
+ Assertions.assertTrue(volume instanceof HddsVolume);
+ HddsVolume hddsVolume = (HddsVolume) volume;
- Assertions.assertEquals(EndpointStateMachine.EndPointStates.REGISTER,
- newState);
+ StorageVolumeUtil.checkVolume(hddsVolume, clusterId,
Review Comment:
We should re-use the `ContainerTestUtils.createDbInstancesForTestIfNeeded` to setup test here instead of the `checkVolume`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1300590409
Hi @errose28 , In dev email communication, this PR is required by Ozone 1.3. Currently the release of Ozone-1.3 is blocked on this PR. Could you help continue to review this PR?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1128112525
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +125,44 @@ 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) {
+ StorageVolumeUtil.checkVolume(volume, clusterId,
+ clusterId, conf, LOG, null);
+ // Create a container and move it under the tmp delete dir.
+ KeyValueContainer container = ContainerTestUtils
+ .setUpTestContainer(volume, clusterId, conf, schemaVersion);
+ assertTrue(container.getContainerFile().exists());
Review Comment:
@neils-dev I've added the test, but it's on `TestEndPoint`. `TestHddsDatanodeService` tests tmp dir cleanup on shutdown where db is closed, so I added the check on startup where we don't have such issue.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064968407
##########
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:
There are no issues. I proceeded with the changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064743619
##########
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:
You are right, we should be able to get rid of the null check. Looking for any failures...
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064699917
##########
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:
@errose28 `config` field was never getting used. The tests that actually need these confs, are setting them under their scope and never use `config`. The majority of the tests don't need them, so it made most sense to get rid of `config` altogether.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1252777196
@errose28 Thanks for your comments, I've made the changes and also modified the tests to use the new code path. Let me know what you think.
> One other thing to check is what happens with schema V3 when the container is moved to the tmp dir but the RocksDB update fails. Will the datanode restart cleanly? What happens to the RocksDB entries for that container?
I will be looking at that next.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1258157162
Thanks @errose28 for our offline discussions on this container delete / rename change and for the PR review and comments.
> One other thing to check is what happens with schema V3 when the container is moved to the tmp dir but the RocksDB update fails.
During the design discussions with @swagle and @nandakumar131 on this case for failed access to db to delete container on container delete operation from,
https://github.com/apache/ozone/blob/0e9ea9fba0d78becc393945205fa5450ab31f066/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java#L149
`BlockUtils.removeContainerFromDB(containerData, conf);` it was deemed to be unrecoverable.
At the time we called it an unrecoverable error and should the access to delete the container from the db fail, then the disk should be replaced.
Right now for that case, we log the error and throw an exception that is translated to a `StorageContainerException`. Presently, there is no recoverable path for the db container delete failure like was discussed in the design discussions. If it should be recoverable, what do you suggest? @swagle , @nandakumar131 thoughts?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1121673330
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -116,8 +117,25 @@ public HddsDatanodeService(boolean printBanner, String[] args) {
this.args = args != null ? Arrays.copyOf(args, args.length) : null;
}
+ private void cleanTmpDir() {
+ if (datanodeStateMachine != null) {
+ MutableVolumeSet volumeSet =
+ datanodeStateMachine.getContainer().getVolumeSet();
+ for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
Review Comment:
@neils-dev In `ScmHAFinalizeUpgradeActionDatanode` we are iterating `volumeSet.getVolumesList()` which returns all volumes and in that case, it makes sense to check their instance.
Here, we are wraping the `volumeSet.getVolumesList()` with `StorageVolumeUtil.getHddsVolumesList(volume list)` which filters the volumes and returns only the HddsVolumes.
https://github.com/apache/ozone/blob/master/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/StorageVolumeUtil.java#L66
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1121791171
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +413,162 @@ 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;
+ }
+
+ // Initialize tmp and delete service directories
+ hddsVolume.checkTmpDirPaths(hddsVolume.getClusterID());
Review Comment:
I'll remove this check here and replace it with a comment.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122231707
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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) {
+ Path tmpPath = createTmpPath(id);
+ deleteServiceDirPath = tmpPath.resolve(TMP_DELETE_SERVICE_DIR);
+ }
+
+ private Path createTmpPath(String id) {
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String vol = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ Path volPath = Paths.get(vol);
+ Path idPath = Paths.get(id);
Review Comment:
We should be able to use `chooseContainerPathID(StorageVolume volume, String clusterID) `inside the original `checkTmpDirPaths` so one place sets the proper location for the subdir of hdds. The `chooseContainerPathID `should resolve the proper location of the hdds subdir independent of version. Inside `checkTmpDirPaths`, just check if id is null, if so pass an emptyString for `clusterID `to `chooseContainerPathId `and it should return the proper location of the hdds subdir (scmid or clusterid). Let me know how it goes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1304366228
@captainzmc we still need some more work to get this PR ready. I'm not sure it needs to be a release blocker. The bug is if there is an IO error while deleting the container, some pieces may be left behind in the container directory. This will cause the datanode to log a warning on startup but has no impact on functionality. Users concerned about the log message can check the container ID using SCM admin commands to see that it is deleted and manually remove the artifacts safely if they wish.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1311855851
> Did you get a chance to try the downgrade to 1.1.0 testing I mentioned earlier?
@errose28 I was able to run this script successfully twice `hadoop-ozone/dist/target/ozone-1.3.0-SNAPSHOT/compose/upgrade/test.sh`, once with `run_test non-rolling-upgrade 1.1.0 1.3.0` and once with `run_test non-rolling-upgrade 1.2.1 1.3.0`. For some weird reason, I had to install robotframework under `/compose/upgrade` to get it to work, even though I have it installed in my machine. Apart from that, no issues. I also checked the result logs and I couldn't find any errors related to our changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122580398
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
+
+ KeyValueHandler kvHandler = new KeyValueHandler(conf,
+ datanodeId, containerSet, volumeSet, metrics,
+ c -> icrReceived.incrementAndGet());
+
exception.expect(StorageContainerException.class);
exception.expectMessage(
- "Container cannot be deleted because it is not empty.");
- container2.delete();
+ "Non-force deletion of non-empty container is not allowed.");
+ kvHandler.deleteContainer(container2, false);
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+ }
+
+ /**
+ * If SchemaV3 is enabled, HddsVolume has already been
+ * formatted and initialized in
+ * setupPaths#createDbInstancesForTestIfNeeded.
+ * @throws Exception
+ */
+ @Test
+ public void testDeleteContainerWithRenaming()
+ throws Exception {
+ HddsVolume hddsVolume = null;
+ if (!schemaVersion.contains(OzoneConsts.SCHEMA_V3)) {
Review Comment:
_Are we not handling container delete with rename for all schemas now?_ Ok, the schema check is to setup the the test for layout of the volume.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1123079883
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -116,8 +117,25 @@ public HddsDatanodeService(boolean printBanner, String[] args) {
this.args = args != null ? Arrays.copyOf(args, args.length) : null;
}
+ private void cleanTmpDir() {
+ if (datanodeStateMachine != null) {
+ MutableVolumeSet volumeSet =
+ datanodeStateMachine.getContainer().getVolumeSet();
+ for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
Review Comment:
I didn't realize it was casting to `HddsVolume`. You are right, in that case, it's best to iterate all volumes and check the instance. I'll fix it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1037311277
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
Review Comment:
No, we just need it to initialize `KeyValueHandler`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1061077986
##########
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",
+ IO_EXCEPTION);
Review Comment:
@xBis7 , thanks for adding the `StorageContainerException` on container move to tmp directory failure. Looks like this exception will be sent back to the SCM to retry the container delete by the` keyValueHandler.handleDeleteContainer`. Looks great, but would be better to throw a more descriptive error with the `StorageContainerException`. Instead of `IO_EXCEPTION`, use either `CONTAINER_INTERNAL_ERROR` or `CLOSED_CONTAINER_IO`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] DaveTeng0 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1408100362
hey @xBis7 ~ please help take a look some failed test~ thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1134742012
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -48,10 +48,7 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
-import static org.junit.jupiter.api.Assertions.assertFalse;
Review Comment:
Let's just revert this for now.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133006904
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java:
##########
@@ -207,16 +207,21 @@ public static KeyValueContainer setUpTestContainer(
UUID.randomUUID().toString(),
UUID.randomUUID().toString());
keyValueContainerData.setSchemaVersion(schemaVersion);
+ keyValueContainerData.setState(ContainerProtos
Review Comment:
Is this change needed in the tests? I removed this and the `parseKVContainerData` addition to the test util without affecting the tests. We can omit them if not needed for tests.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1535975597
@neils-dev please trim "extended description" when merging PR. I think listing all individual commit messages from the PR (which Github does by default) is unnecessary, and in this case it is really overly verbose:
```
HDDS-6449. Failed container delete can leave artifacts on disk (#3741)
* cleanUpManager - tmpDir - rename container path - delete container in tmpDir
* TestContainerDeleteWithSchemaV3Enabled improvement, cleanup
* cleanTmpDir on datanode start and shutdown, cleanup
* File.renameTo - rename before deletes if V3 enabled
* datanode service start/stop cleanTmpDir()
* HddsDatanodeService cleanTmpDir() getting schema
* testing cleanTmpDir on datanode start/stop
* modify /tmp path based on volume
* adding cluster id to /tmp path
* remove flag SchemaV3Enabled - clusterId/scmId on the /tmp path
* move cleanTmpDir startup hook to VersionEndpointTask
* no changes
* checking if schema v3 enabled
* check schema v3 is enabled using configuration in CleanUpManager
* initialize CleanUpManager if schemaV3 - testDeleteContainerWithSchemaV3Enabled execute conditionally
* TestHddsDatanodeService - getTmpDirPath from CleanUpManager
* TestKeyValueHandler testDeleteContainerWithSchemaV3Enabled
* TestContainerPersistence fix
* findbugs errors fixed
* TestEndpoint test clean tmp dir on datanode startup
* TestContainerPersistence fix - datanode startup hook moved
* checkstyle fix
* failing tests fixed
* cleanup
* cleanup - moving CleanUpManager under common.helpers package
* check instance of containerdata - cleanup
* TestContainerPersistence findbugs error fix
* CleanUpManager.renameDir javadoc comment
* CleanUpManager.setTmpDirPath exception message
* CleanUpManager tmpDirPath made final
* moving tmp dir under HddsVolume
* IntegrationTest error fixed
* deleted tmp files
* TestDatanodeUpgradeToScmHA#testImportContainer fix
* cleanup
* unit tests fixed
* checkstyle fixed
* remove container db failure exception
* db failure rethrow exception
* cleanup
* db failure exception
* container delete operations moved under KeyValueContainerUtil
* workflow errors fixed
* rebasing - TestHddsDatanodeService migrating to junit5
* build errors from rebasing, fixed - KeyValueContainerUtil cleanup
* tests cleaned up
* findbugs error fixed
* null pointer exception for tmp dir during datanode startup fixed
* calling ContainerUtils#getContainerFile from cleanTmpDir - tests updated
* scm exception if container move failed
* rebasing
* createTmpDir if null, while creating delete service dir
* deleteInternal more specific SCE thrown on container rename
* HddsVolume using Paths lib - cleanup
* remove null check before setting tmp dir paths
* cleanup
* logging error on removeContainerFromDB failure
* cleanup
* removing checkTmpDirs() - tests cleanup
* delete test generated directories
* tests cleanup
* test db entries on tmp dir cleanup
* merging errors fixed
* tests cleanup
* reducing test diffs
---------
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1092503593
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -59,6 +67,8 @@ private KeyValueContainerUtil() {
private static final Logger LOG = LoggerFactory.getLogger(
KeyValueContainerUtil.class);
+ private static final String FILE_SEPARATOR = File.separator;
Review Comment:
Ozone system file separator can be used here: `OZONE_URI_DELIMITER`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1301705048
Yes, I'm working on a re-review to get this through.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1015829690
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.DATANODE_SCHEMA_V3)) {
+ // Get container file
+ File containerFile = getContainerFile(file);
+
+ // If file exists
+ if (containerFile != null) {
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ // Remove container from Rocks DB
+ String dbPath = hddsVolume.getDbParentDir().getAbsolutePath();
+ DatanodeStoreSchemaThreeImpl store =
+ new DatanodeStoreSchemaThreeImpl(hddsVolume.getConf(),
+ dbPath, false);
+ store.removeKVContainerData(keyValueContainerData.getContainerID());
+ }
+ }
+
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", hddsVolume.getDeleteServiceDirPath().toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Search recursively for the container file under a
+ * directory. Return null if the file is not found.
+ * @param file
+ * @return container file or null if it doesn't exist
+ * @throws IOException
+ */
+ public static File getContainerFile(File file) throws IOException {
Review Comment:
@errose28 There is a chance that cleaning the tmp directory can fail and then the container directory won't be in it's normal structure and format. In that case, ContainerUtils.getContainerFile() won't work as expected. That's why I had created my own method that searches recursively the directory for the container file.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1250252204
@errose28 Thanks for reviewing this. I will make the changes and modify the tests accordingly. Prior to `SchemaV3`, the clusterId doesn't appear to be available. We were told that clusterId should be available in the tmp dir path and that was an issue with previous versions.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r973483825
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,20 @@ private void deleteInternal(Container container, boolean force)
DELETE_ON_NON_EMPTY_CONTAINER);
}
}
+ if (container.getContainerData() instanceof KeyValueContainerData) {
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) container.getContainerData();
+ if (CleanUpManager
+ .checkContainerSchemaV3Enabled(keyValueContainerData)) {
+ HddsVolume hddsVolume = keyValueContainerData.getVolume();
+
+ // Initialize the directory
+ CleanUpManager cleanUpManager =
+ new CleanUpManager(hddsVolume);
+ // Rename
+ cleanUpManager.renameDir(keyValueContainerData);
Review Comment:
In context this might be easier to understand if `renameDir` was named to describe the location of the rename or the purpose the rename serves. A name like `moveToTmpDirectory` might be better.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private final Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ this.tmpDirPath = setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private Path setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException("Volume has not been initialized," +
+ " clusterId is null.");
+ }
+ } catch (IOException ex) {
+ LOG.error("Exception in CleanUpManger setTmpDirPath.", ex);
+ }
+
+ String pathId = "";
+ try {
+ pathId += VersionedDatanodeFeatures.ScmHA
+ .chooseContainerPathID(hddsVolume, clusterId);
+ } catch (IOException ex) {
+ LOG.error("Failed to get the container path Id", ex);
+ }
+
+ stringBuilder.append(pathId);
+ stringBuilder.append(TMP_DELETE_SERVICE_DIR);
+
+ String tmpPath = stringBuilder.toString();
+ return Paths.get(tmpPath);
+ }
+
+ /**
+ * Renaming container directory path to a new location
+ * under "/tmp/container_delete_service" and
+ * updating metadata and chunks path.
+ * @param keyValueContainerData
+ * @return true if renaming was successful
+ */
+ public boolean renameDir(KeyValueContainerData keyValueContainerData) {
+ String containerPath = keyValueContainerData.getContainerPath();
+ File container = new File(containerPath);
+ String containerDirName = container.getName();
+
+ String destinationDirPath = tmpDirPath
+ .resolve(Paths.get(containerDirName)).toString();
+
+ boolean success = container.renameTo(new File(destinationDirPath));
+
+ if (success) {
+ keyValueContainerData.setMetadataPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.CONTAINER_META_PATH);
+ keyValueContainerData.setChunksPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.STORAGE_DIR_CHUNKS);
Review Comment:
Just a note on this part: These only update the in memory values of the container's location. This is good enough to communicate the information to `KeyValueContainerUtil#removeContainer`. It will not survive a restart, but that is ok since the whole tmp delete directory will be wiped then. This would probably be good to explain in a comment here.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
Review Comment:
Let's expand the javadoc to specify that this tmp directory exists on a per volume basis. /tmp looks like it refers to the shared OS /tmp directory instead of the one on this volume. When tmp is being referred to in other places for comments, we should probably omit the leading slash to indicate the comment is not referring to an absolute path.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/StaleRecoveringContainerScrubbingService.java:
##########
@@ -84,6 +88,19 @@ static class RecoveringContainerScrubbingTask implements BackgroundTask {
@Override
public BackgroundTaskResult call() throws Exception {
+ Container container = containerSet.getContainer(containerID);
Review Comment:
This class is used for EC offline reconstruction. Let's leave that code as is for now. It uses its own tmp directory to build the new container, and may be able to use that for deleting failed reconstructions as well.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -556,6 +569,9 @@ public void terminateDatanode() {
@Override
public void stop() {
+ if (CleanUpManager.checkContainerSchemaV3Enabled(conf)) {
+ cleanTmpDir();
+ }
Review Comment:
The shutdown hook should be moved inside the atomic boolean check to make sure it is only invoked once.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private final Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ this.tmpDirPath = setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private Path setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException("Volume has not been initialized," +
+ " clusterId is null.");
+ }
+ } catch (IOException ex) {
+ LOG.error("Exception in CleanUpManger setTmpDirPath.", ex);
+ }
+
+ String pathId = "";
+ try {
+ pathId += VersionedDatanodeFeatures.ScmHA
+ .chooseContainerPathID(hddsVolume, clusterId);
+ } catch (IOException ex) {
+ LOG.error("Failed to get the container path Id", ex);
+ }
+
+ stringBuilder.append(pathId);
+ stringBuilder.append(TMP_DELETE_SERVICE_DIR);
+
+ String tmpPath = stringBuilder.toString();
+ return Paths.get(tmpPath);
+ }
+
+ /**
+ * Renaming container directory path to a new location
+ * under "/tmp/container_delete_service" and
+ * updating metadata and chunks path.
+ * @param keyValueContainerData
+ * @return true if renaming was successful
+ */
+ public boolean renameDir(KeyValueContainerData keyValueContainerData) {
+ String containerPath = keyValueContainerData.getContainerPath();
+ File container = new File(containerPath);
+ String containerDirName = container.getName();
+
+ String destinationDirPath = tmpDirPath
+ .resolve(Paths.get(containerDirName)).toString();
+
+ boolean success = container.renameTo(new File(destinationDirPath));
+
+ if (success) {
+ keyValueContainerData.setMetadataPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.CONTAINER_META_PATH);
+ keyValueContainerData.setChunksPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.STORAGE_DIR_CHUNKS);
+ }
+ return success;
+ }
+
+ /**
+ * Get direct files under /tmp/container_delete_service
+ * and store them in a list.
+ * @return iterator to the list of the leftover files
+ */
+ public ListIterator<File> getDeleteLeftovers() {
+ List<File> leftovers = new ArrayList<>();
+
+ try {
+ File tmpDir = new File(tmpDirPath.toString());
+
+ for (File file : tmpDir.listFiles()) {
+ leftovers.add(file);
+ }
+ } catch (NullPointerException ex) {
+ LOG.error("Tmp directory is null, path doesn't exist", ex);
+ }
+
+ ListIterator<File> leftoversListIt = leftovers.listIterator();
+
+ return leftoversListIt;
+ }
+
+ /**
+ * @return true if tmpDir is empty
+ */
+ public boolean tmpDirIsEmpty() {
Review Comment:
I don't think this method is necessary. `cleanTmpDir` will be a no-op if the directory is empty so we can just call it directly.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java:
##########
@@ -296,8 +297,11 @@ public void delete() throws StorageContainerException {
} catch (StorageContainerException ex) {
throw ex;
} catch (IOException ex) {
- // TODO : An I/O error during delete can leave partial artifacts on the
- // disk. We will need the cleaner thread to cleanup this information.
+ if (CleanUpManager.checkContainerSchemaV3Enabled(containerData)) {
+ HddsVolume hddsVolume = containerData.getVolume();
+ CleanUpManager cleanUpManager = new CleanUpManager(hddsVolume);
+ cleanUpManager.cleanTmpDir();
+ }
Review Comment:
I think we can remove this part. The delete path is:
1. Container is moved to tmp dir
2. Container is deleted from tmp dir
If 1 failed, the container is not in the tmp dir to delete. If 2 failed, I don't think retrying the same thing a second time here is going to make progress. We can wait until shutdown/restart either wipes the volume's tmp dir or results in the volume being removed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1016998322
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -416,6 +418,8 @@ ContainerCommandResponseProto handleDeleteContainer(
deleteInternal(kvContainer, forceDelete);
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
+ } catch (IOException ex) {
Review Comment:
@errose28 Just logged the error and removed the exception alltogether, since you mentioned that container and volume scans will deal with it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122294243
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -116,8 +117,25 @@ public HddsDatanodeService(boolean printBanner, String[] args) {
this.args = args != null ? Arrays.copyOf(args, args.length) : null;
}
+ private void cleanTmpDir() {
+ if (datanodeStateMachine != null) {
+ MutableVolumeSet volumeSet =
+ datanodeStateMachine.getContainer().getVolumeSet();
+ for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
Review Comment:
Thanks @xBis7 , that util, it seems to _cast_ each entry in the volume stream to an `HddsVolume` and returns the corresponding list. It would be better to iterate the volume set and only use `HddsVolume`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1121130398
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +413,162 @@ 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;
+ }
+
+ // Initialize tmp and delete service directories
+ hddsVolume.checkTmpDirPaths(hddsVolume.getClusterID());
Review Comment:
The `checkTmpDirPaths` is called again in the line below within the `getDeleteLeftovers`. In this case it is called twice. Where is it better placed?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133881228
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -48,10 +48,7 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
-import static org.junit.jupiter.api.Assertions.assertFalse;
Review Comment:
I know that it seems we are creating unnecessary diffs here but it's much cleaner having one import instead of five. Apart from the existing, we will add 2 more (`assertEquals`, `assertTrue`) and generate new diffs anyway.
```java
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertEquals;
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133931954
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java:
##########
@@ -207,16 +207,21 @@ public static KeyValueContainer setUpTestContainer(
UUID.randomUUID().toString(),
UUID.randomUUID().toString());
keyValueContainerData.setSchemaVersion(schemaVersion);
+ keyValueContainerData.setState(ContainerProtos
Review Comment:
It made sense to set the state to DELETED, since it can't be anything else but you are right. We don't need it or the other addition and we can get rid of them.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1470805278
@errose28 , we've raised a ticket on an issue we found while performing end to end tests with Container Delete with `V3 Schema Rocks db`, https://issues.apache.org/jira/browse/HDDS-8173 .
On container delete processed by the KeyValueHandler.deleteInternal and handled by the `KeyValueContainerUtil.removeContainer`, the rocksdb V3 entries for the container are not cleared. After container delete the container metadata and chunks directories are deleted however rocksdb V3 container table entries are not removed.
Within the filed ticket for that issue a Test is included (patch to `TestContainerPersistence`) can be used to verify the behavior rocksdb V3 container delete behavior. https://issues.apache.org/jira/secure/attachment/13056380/rocksDBContainerDelete.diff
`Schema V2` rocksdb should be unaffected as there is a rocksDB per container. On container delete, the rocksDB is moved to the tmp directory along with the container metadata and chunks directory and deleted.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122573075
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
Review Comment:
Does this test not work with schema V3 as well? Do we skip it for V3? It is testing deleting a non-empty container, which is independent of the db schema (whether db per datanode or db per volume).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1015832132
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
Review Comment:
A reasonable test would be adding `Assert.assertEquals(0, icrReceived.get());` after `kvHandler.deleteContainer()` to check that the operation failed, but it's redundant. After line 328 runs, the exception gets thrown and nothing executes below that.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1015455413
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/VersionEndpointTask.java:
##########
@@ -122,6 +124,19 @@ private void checkVolumeSet(MutableVolumeSet volumeSet,
boolean result = StorageVolumeUtil.checkVolume(volume,
scmId, clusterId, configuration, LOG,
ozoneContainer.getDbVolumeSet());
+
+ // Clean <HddsVolume>/tmp/container_delete_service dir.
+ if (volume instanceof HddsVolume) {
Review Comment:
> I would say to not fail the volume, as the tmp dir being clean on startup is not required for functionality.
Tmp dir will also be cleaned on shutdown. I agree, let's leave it as it and just `LOG.error()` upon catching the exception.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1036615240
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +130,67 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnStartup(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());
+ int volumeSetSize = volumes.size();
+ File[] tempHddsVolumes = new File[volumeSetSize];
+
+ for (int i = 0; i < volumeSetSize; i++) {
+ HddsVolume volume = volumes.get(i);
+ volume.format(clusterId);
+ volume.createWorkingDir(clusterId, null);
+ volume.createDeleteServiceDir();
+ if (this.tempDir.isDirectory()) {
+ tempHddsVolumes[i] = tempDir;
+ }
+
+ // Write to tmp dir under volume
+ File testFile = new File(volume.getDeleteServiceDirPath() +
+ FILE_SEPARATOR + "testFile.txt");
+ Files.touch(testFile);
+ assertTrue(testFile.exists());
+
+ ListIterator<File> tmpDirIter = KeyValueContainerUtil
+ .ContainerDeleteDirectory.getDeleteLeftovers(volume);
+ List<File> tmpDirFileList = new LinkedList<>();
+ boolean testFileExistsUnderTmp = false;
+
+ while (tmpDirIter.hasNext()) {
+ tmpDirFileList.add(tmpDirIter.next());
+ }
+
+ if (tmpDirFileList.contains(testFile)) {
+ testFileExistsUnderTmp = true;
+ }
+ assertTrue(testFileExistsUnderTmp);
+ }
+ conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_KEY,
+ Arrays.toString(tempHddsVolumes));
Review Comment:
I'm not sure why this config key is being set right before shutdown. The test passes without this. We can remove the array with this config.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,208 @@ 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.");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // Check the case where we have Schema V3 and
+ // removing the container's entries from RocksDB fails.
+ // --------------------------------------------
+ // On datanode restart, we populate the container set
+ // based on the available datanode volumes and
+ // populate the container metadata based on the values in RocksDB.
+ // The container is in the tmp directory,
+ // so it won't be loaded in the container set
+ // but there will be orphaned entries in the volume's RocksDB.
+ // --------------------------------------------
+ // For every .container file we find under /tmp,
+ // we use it to get the RocksDB entries and delete them.
+ // If the .container file doesn't exist then the contents of the
+ // directory are probably leftovers of a failed delete and
+ // the RocksDB entries must have already been removed.
+ // In any case we can proceed with deleting the directory's contents.
+ if (VersionedDatanodeFeatures.isFinalized(
Review Comment:
What we want to check here is if the container is using schema V3, not if schema v3 is finalized. So we need to load the container file first and check the schema version. Only if that schema version is v3 do we need to call removeContainerFromDB.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +130,67 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnStartup(String schemaVersion) throws IOException {
Review Comment:
It's called test temp dir on startup, but it actually tests it on shutdown. Do we want to test startup cleaning here as well?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
Review Comment:
I don't see the value getting read anywhere so I was just wondering if it was being used in the lambda somehow and I missed it.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.DATANODE_SCHEMA_V3)) {
+ // Get container file
+ File containerFile = getContainerFile(file);
+
+ // If file exists
+ if (containerFile != null) {
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ // Remove container from Rocks DB
+ String dbPath = hddsVolume.getDbParentDir().getAbsolutePath();
+ DatanodeStoreSchemaThreeImpl store =
+ new DatanodeStoreSchemaThreeImpl(hddsVolume.getConf(),
+ dbPath, false);
+ store.removeKVContainerData(keyValueContainerData.getContainerID());
+ }
+ }
+
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", hddsVolume.getDeleteServiceDirPath().toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Search recursively for the container file under a
+ * directory. Return null if the file is not found.
+ * @param file
+ * @return container file or null if it doesn't exist
+ * @throws IOException
+ */
+ public static File getContainerFile(File file) throws IOException {
Review Comment:
What would be an example of this failure case? `ContainerUtils#getContainerFile` just requires that the container file is in a certain subdirectory of the base container directory. The delete does not move this file relative to the base location.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -416,6 +418,8 @@ ContainerCommandResponseProto handleDeleteContainer(
deleteInternal(kvContainer, forceDelete);
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
+ } catch (IOException ex) {
Review Comment:
We still need to throw the StorageContainerException if the move fails so that SCM will retry the delete. My point about the scanners was that it is ok if SCM keeps retrying even though there is some IO issue with the volume, because the scanners will eventually pick up on it and fail the volume, after which the deletions will not be sent again.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -33,24 +48,52 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Test class for {@link HddsDatanodeService}.
*/
+
public class TestHddsDatanodeService {
+
+ @TempDir
+ private File tempDir;
private File testDir;
- private OzoneConfiguration conf;
- private HddsDatanodeService service;
- private String[] args = new String[] {};
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestHddsDatanodeService.class);
+
+ private final String clusterId = UUID.randomUUID().toString();
+ private final OzoneConfiguration conf = new OzoneConfiguration();
+ private final String[] args = new String[] {};
+ private final HddsDatanodeService service =
+ HddsDatanodeService.createHddsDatanodeService(args);
Review Comment:
```suggestion
private final HddsDatanodeService service =
HddsDatanodeService.createHddsDatanodeService(new String[]{});
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1061424780
##########
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",
+ IO_EXCEPTION);
Review Comment:
@neils-dev Thanks for the suggestion. I made the change.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064699917
##########
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:
@errose28 `config` field was never getting used. The tests that actually need these confs, are setting them under their scope and never using `config`. The majority of the tests don't need them, so it made most sense to get rid of `config` altogether.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064699917
##########
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:
@errose28 These configs were unecessary for the existing tests, so I thought about cleaning them up. If there is something I missed, I will put them back.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r965664609
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private void setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException();
+ }
+ } catch (IOException ex) {
+ LOG.error("Volume has not been initialized, " +
+ "clusterId is null.", ex);
+ }
+
+ String pathId = "";
+ try {
+ pathId += VersionedDatanodeFeatures.ScmHA
+ .chooseContainerPathID(hddsVolume, clusterId);
+ } catch (IOException ex) {
+ LOG.error("Failed to get the container path Id", ex);
+ }
+
+ stringBuilder.append(pathId);
+ stringBuilder.append(TMP_DELETE_SERVICE_DIR);
+
+ String tmpPath = stringBuilder.toString();
+ this.tmpDirPath = Paths.get(tmpPath);
+ }
+
+ public boolean renameDir(KeyValueContainerData keyValueContainerData) {
+ String containerPath = keyValueContainerData.getContainerPath();
+ File container = new File(containerPath);
+ String containerDirName = container.getName();
+
+ String destinationDirPath = tmpDirPath
+ .resolve(Paths.get(containerDirName)).toString();
+
+ boolean success = container.renameTo(new File(destinationDirPath));
+
+ if (success) {
+ keyValueContainerData.setMetadataPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.CONTAINER_META_PATH);
+ keyValueContainerData.setChunksPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.STORAGE_DIR_CHUNKS);
+ }
+ return success;
+ }
+
+ /**
+ * Get direct files under /tmp/container_delete_service
+ * and store them in a list.
+ * @return iterator to the list of the leftover files
+ */
+ public ListIterator<File> getDeleteLeftovers() {
Review Comment:
@myskov This is used in `TestHddsDatanodeService, TestContainerPersistence, TestEndPoint`. I would rather keep it public. People might want to use it later to gather statistics or info about the leftover files.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122584949
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
+
+ KeyValueHandler kvHandler = new KeyValueHandler(conf,
+ datanodeId, containerSet, volumeSet, metrics,
+ c -> icrReceived.incrementAndGet());
+
exception.expect(StorageContainerException.class);
exception.expectMessage(
- "Container cannot be deleted because it is not empty.");
- container2.delete();
+ "Non-force deletion of non-empty container is not allowed.");
+ kvHandler.deleteContainer(container2, false);
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+ }
+
+ /**
+ * If SchemaV3 is enabled, HddsVolume has already been
+ * formatted and initialized in
+ * setupPaths#createDbInstancesForTestIfNeeded.
+ * @throws Exception
+ */
+ @Test
+ public void testDeleteContainerWithRenaming()
+ throws Exception {
+ HddsVolume hddsVolume = null;
+ if (!schemaVersion.contains(OzoneConsts.SCHEMA_V3)) {
+ Files.createDirectories(Paths.get(hddsPath));
+
+ HddsVolume.Builder volumeBuilder =
+ new HddsVolume.Builder(hddsPath)
+ .datanodeUuid(DATANODE_UUID)
+ .conf(conf)
+ .usageCheckFactory(MockSpaceUsageCheckFactory.NONE);
+
+ hddsVolume = volumeBuilder.build();
+
+ hddsVolume.format(SCM_ID);
+ hddsVolume.createWorkingDir(SCM_ID, null);
+ }
+
+ long testContainerID1 = getTestContainerID();
+ Thread.sleep(100);
+ long testContainerID2 = getTestContainerID();
+
+ Container container1 = addContainer(containerSet, testContainerID1);
+ container1.close();
+
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID1));
Assert.assertTrue(containerSet.getContainerMapCopy()
.containsKey(testContainerID2));
+
+ KeyValueContainerData container1Data =
+ (KeyValueContainerData) container1.getContainerData();
+
+ KeyValueContainerData container2Data =
+ (KeyValueContainerData) container2.getContainerData();
+
+ if (schemaVersion.contains(OzoneConsts.SCHEMA_V3)) {
+ hddsVolume = container1Data.getVolume();
Review Comment:
Is this condition needed?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122580398
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
+
+ KeyValueHandler kvHandler = new KeyValueHandler(conf,
+ datanodeId, containerSet, volumeSet, metrics,
+ c -> icrReceived.incrementAndGet());
+
exception.expect(StorageContainerException.class);
exception.expectMessage(
- "Container cannot be deleted because it is not empty.");
- container2.delete();
+ "Non-force deletion of non-empty container is not allowed.");
+ kvHandler.deleteContainer(container2, false);
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+ }
+
+ /**
+ * If SchemaV3 is enabled, HddsVolume has already been
+ * formatted and initialized in
+ * setupPaths#createDbInstancesForTestIfNeeded.
+ * @throws Exception
+ */
+ @Test
+ public void testDeleteContainerWithRenaming()
+ throws Exception {
+ HddsVolume hddsVolume = null;
+ if (!schemaVersion.contains(OzoneConsts.SCHEMA_V3)) {
Review Comment:
Are we not handling container delete with rename for all schemas now? If so, this test does the same thing 3 times, once for each schema without altering behavior. The differences between < V3 and V3 was the db delete. It doesn;t look to be involved in this test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1121775624
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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) {
+ Path tmpPath = createTmpPath(id);
+ deleteServiceDirPath = tmpPath.resolve(TMP_DELETE_SERVICE_DIR);
+ }
+
+ private Path createTmpPath(String id) {
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String vol = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ Path volPath = Paths.get(vol);
+ Path idPath = Paths.get(id);
Review Comment:
I've been looking at all the usage and [this](https://github.com/apache/ozone/blob/f291058fa8974fe3a2844df5d27ec444e9826742/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/StorageVolumeUtil.java#L241-L265) seems the only case where clusterID might not be available and we need to set it.
For calling `VersionDatanodeFeatures.ScmHA.chooseContainerPathID` we need to know clusterId which might not be available in the corner case we are trying to cover here.
It makes more sense to me to overload `checkTmpDirPaths()` and have a separate `checkTmpDirPaths(String id)` where we choose the id in StorageVolumeUtil and pass it as a parameter.
@neils-dev What do you think?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1121086980
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java:
##########
@@ -116,8 +117,25 @@ public HddsDatanodeService(boolean printBanner, String[] args) {
this.args = args != null ? Arrays.copyOf(args, args.length) : null;
}
+ private void cleanTmpDir() {
+ if (datanodeStateMachine != null) {
+ MutableVolumeSet volumeSet =
+ datanodeStateMachine.getContainer().getVolumeSet();
+ for (HddsVolume hddsVolume : StorageVolumeUtil.getHddsVolumesList(
Review Comment:
Would be safer to check if each volume in `MutableVolumeSet` to be `instanceof ``HddsVolume`. Similar to https://github.com/apache/ozone/blob/9f77588334854c2e610cce9e0d59391207d6849a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/upgrade/ScmHAFinalizeUpgradeActionDatanode.java#L55
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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:
With restructuring the location of configuring the tmp directory into the `HddsVolume`, passing the `clusterID` to the tmp dir path config is redundant. _Remove_ all interfaces with `id `(clusterid) as an argument as the `clusterID `is a private variable in the `HddsVolume `(`StorageVolume`) instance (use `getClusterID`).
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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) {
+ Path tmpPath = createTmpPath(id);
+ deleteServiceDirPath = tmpPath.resolve(TMP_DELETE_SERVICE_DIR);
+ }
+
+ private Path createTmpPath(String id) {
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String vol = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ Path volPath = Paths.get(vol);
+ Path idPath = Paths.get(id);
+
+ return volPath.resolve(idPath).resolve(TMP_DIR);
+ }
+
+ private void createTmpDir(String id) {
Review Comment:
update interface, unnecessary `id` argument. (clusterid defined in HddsVolume / StorageVolume)
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1236,6 +1237,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) {
Review Comment:
This block (success) can be restructured to only be used and output only if debug logging is enabled. Something like:
```
if (!success) {
LOG.error("Failed to move container under " +
hddsVolume.getDeleteServiceDirPath());
throw new StorageContainerException("Moving container failed",
CONTAINER_INTERNAL_ERROR);
}
if (LOG.isDebugEnabled()) {
String containerPath = keyValueContainerData
.getContainerPath().toString();
File containerDir = new File(containerPath);
LOG.debug("Container {} has been successfuly moved under {}",
containerDir.getName(), hddsVolume.getDeleteServiceDirPath());
}
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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) {
+ Path tmpPath = createTmpPath(id);
+ deleteServiceDirPath = tmpPath.resolve(TMP_DELETE_SERVICE_DIR);
+ }
+
+ private Path createTmpPath(String id) {
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String vol = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ Path volPath = Paths.get(vol);
+ Path idPath = Paths.get(id);
Review Comment:
The id is the <clusterid / scmid> based on the version. It may be best to check and set the `id` from `VersionDatanodeFeatures.chooseContainerPathID` here. Once set here, all calls to the tmp dir container delete service path can be independent of the version, having properly set the path correctly in one place.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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) {
+ Path tmpPath = createTmpPath(id);
+ deleteServiceDirPath = tmpPath.resolve(TMP_DELETE_SERVICE_DIR);
+ }
+
+ private Path createTmpPath(String id) {
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String vol = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ Path volPath = Paths.get(vol);
+ Path idPath = Paths.get(id);
+
+ return volPath.resolve(idPath).resolve(TMP_DIR);
+ }
+
+ private void createTmpDir(String id) {
+ tmpDirPath = createTmpPath(id);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public void createDeleteServiceDir(String id) {
Review Comment:
update interface, unnecessary `id` argument. (clusterid defined in HddsVolume / StorageVolume)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1092502430
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -33,24 +47,48 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Test class for {@link HddsDatanodeService}.
*/
+
public class TestHddsDatanodeService {
+
private File testDir;
- private OzoneConfiguration conf;
- private HddsDatanodeService service;
- private String[] args = new String[] {};
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestHddsDatanodeService.class);
+
+ private final String clusterId = UUID.randomUUID().toString();
+ private final OzoneConfiguration conf = new OzoneConfiguration();
+ private final HddsDatanodeService service =
+ HddsDatanodeService.createHddsDatanodeService(new String[] {});
+ private static final int SCM_SERVER_COUNT = 1;
+ private static final String FILE_SEPARATOR = File.separator;
Review Comment:
Would be better to use Ozone separator delimiter: `OzoneConts.OZONE_URI_DELIMITER `
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1092503593
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -59,6 +67,8 @@ private KeyValueContainerUtil() {
private static final Logger LOG = LoggerFactory.getLogger(
KeyValueContainerUtil.class);
+ private static final String FILE_SEPARATOR = File.separator;
Review Comment:
Ozone system file separator can be used here: `OZONE_URI_DELIMITER`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1411337922
In the `KeyValueHandler.importContainerData`, it too tries to cleanup the container should the import fail through container delete. This would be a good candidate for the container _move_ to `tmp` directory and delete to ensure no artifacts are left in case the delete fails. https://github.com/apache/ozone/blob/6389d01605053a9a5ae5510b67fb2bc6204db9d2/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java#L560
@errose28 , is this what you had in mind for reusing the `tmp dir` and cleanup for the volume? In this case should the container import fails, it would move the container to the `tmp `directory, try to delete from db. In the event of failure it should be picked up by the cleanup on datanode shutdown/start and retried.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133935683
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -267,45 +267,45 @@ public void testAddingBlockToDeletedContainer() throws Exception {
// so skip check the behaviors related to it.
assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
- long testContainerID1 = getTestContainerID();
+ long testContainerID = getTestContainerID();
Review Comment:
I was looking again at the tests and saw some cosmetics we can cleanup. Maybe, we can keep these changes if it's okay with you.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1258673684
I've looked through the existing code, and if the RocksDB update fails for schema v3 I think we will be ok with a minor improvement in the current design. The volume may eventually be marked unhealthy but even if it is not I think this error is recoverable.
## Schema < V3
For schema < V3 we have the following operations:
1. Container directory renamed to tmp directory.
2. Container is removed from in memory container set.
3. Container is deleted from tmp directory.
We had initially designed to handle a failure in steps 1 or 3. Step 2 is in memory so it should fail. For schema V3, however, we have an extra operation since the volume's RocksDB needs to be updated.
## Schema V3
For schema V3 we have the following operations:
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.
The original design still handles failures in steps 1 and 4 above as intended, but what happens if step 3 fails? The container has been removed from the in memory container set in step 2, so there should be any correctness issues while the datanode is running. On restart, the container set is rebuilt by scanning the available datanode volumes, and container metadata is populated based on the values in RocksDB. Since the container is already in the tmp directory, it will not be seen in this step and will not be loaded in to the container set, so we are still behaving correctly.
This does leave some orphaned entries in the volume's RocksDB, however I think these should be relatively easy to clean up. On the startup/shutdown hooks where we are deleting the containers from the tmp directory, we can check if the container is schema V3. If it is, we should first delete any remaining RocksDB entries it may have (if they exist) and then we can delete the container from the tmp directory. If this RocksDB delete fails the container should be skipped and its delete will happen again on the next startup/shutdown. Since the container move to the tmp directory is atomic, we can check RocksDB for each schema V3 .container file we find in the tmp directory. If the contents of the tmp directory are partially deleted and there is no .container file (maybe a failed delete from the tmp directory in the past) then the RocksDB entries must have already been removed so we can proceed to delete those contents.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1307186070
> @captainzmc we still need some more work to get this PR ready. I'm not sure it needs to be a release blocker. The bug is if there is an IO error while deleting the container, some pieces may be left behind in the container directory. This will cause the datanode to log a warning on startup but has no impact on functionality. Users concerned about the log message can check the container ID using SCM admin commands to see that it is deleted and manually remove the artifacts safely if they wish.
Thanks to @errose28 for explaining the impact of this bug. It seems that this does not affect the use of the cluster, and the user can manually delete these artifacts.
Hi @neils-dev. The release of 1.3.0 is now nearly two months behind schedule. If this PR takes too long to improve, I recommend removing it from the block list. In addition, Some new bugs may be found after the 1.3.0 release. So we can cherry pick the new bug fix from the master later, and then we can continue to release 1.3.1. What do you think?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r981384862
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ 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();
+
+ if (hddsVolume.getClusterID() != null) {
Review Comment:
If there is not `ClusterID` then the volume has not been formatted and mounted. There is no tmp directory available, so we don't want to do a rename. You might argue here that since the container is available then the volume should be as well. We can remove it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] myskov commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
myskov commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r965862906
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,214 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
Review Comment:
can be final?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1015476729
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -416,6 +418,8 @@ ContainerCommandResponseProto handleDeleteContainer(
deleteInternal(kvContainer, forceDelete);
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
+ } catch (IOException ex) {
Review Comment:
This IOException comes from [here](https://github.com/apache/ozone/blob/78447eb08463a89a2ad0c75d5ce0b2e3597705a6/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java#L568). I don't think we should surpress this or change it to `StorageContainerException`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1307392666
Thanks @captainzmc, @errose28 , @xBis7 ,
> Hi @neils-dev. The release of 1.3.0 is now nearly two months behind schedule... I recommend removing it from the block list
Sure, let's remove it from the block list. Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1307677591
@errose28 I'm done with the changes you requested. I left a few comments based on some details that are unclear to me.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064751059
##########
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:
@errose28 Tmp directories are only created when needed and not along with volume initialization. Without this method setting the paths everything breaks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1092502430
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -33,24 +47,48 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Test class for {@link HddsDatanodeService}.
*/
+
public class TestHddsDatanodeService {
+
private File testDir;
- private OzoneConfiguration conf;
- private HddsDatanodeService service;
- private String[] args = new String[] {};
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestHddsDatanodeService.class);
+
+ private final String clusterId = UUID.randomUUID().toString();
+ private final OzoneConfiguration conf = new OzoneConfiguration();
+ private final HddsDatanodeService service =
+ HddsDatanodeService.createHddsDatanodeService(new String[] {});
+ private static final int SCM_SERVER_COUNT = 1;
+ private static final String FILE_SEPARATOR = File.separator;
Review Comment:
Would be better to use Ozone separator delimiter: `OzoneConsts.OZONE_URI_DELIMITER `
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122555019
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +125,44 @@ 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(
Review Comment:
Similar to previous comment. Good to check each volume in list instanceof `HddsVolume`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1123160088
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
Review Comment:
If you check the diffs, you can see this was part of a bigger test that I refactored. It was returning in case we had SchemaV3 so I didn't check it out or made any changes. I just broke it down to smaller tests.
You are right, it works regardless of Schema.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev merged pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev merged PR #3741:
URL: https://github.com/apache/ozone/pull/3741
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122557212
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +125,44 @@ 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) {
+ StorageVolumeUtil.checkVolume(volume, clusterId,
+ clusterId, conf, LOG, null);
+ // Create a container and move it under the tmp delete dir.
+ KeyValueContainer container = ContainerTestUtils
+ .setUpTestContainer(volume, clusterId, conf, schemaVersion);
+ assertTrue(container.getContainerFile().exists());
Review Comment:
For Schema_V3, an added entry into the db and deleting entry based on container in tmp directory would be a great addition.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133006661
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -48,10 +48,7 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
-import static org.junit.jupiter.api.Assertions.assertFalse;
Review Comment:
Having the Junit 5 assert methods fully qualified is fine. We can leave them in the file and prevent unneeded changes having to be merged. We can leave the fully qualified imports for `assertFalse`, `assertTrue`, `assertNotNull`, `assertNull `and revert the changes in the file due to their omission.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133006969
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java:
##########
@@ -207,16 +207,21 @@ public static KeyValueContainer setUpTestContainer(
UUID.randomUUID().toString(),
UUID.randomUUID().toString());
keyValueContainerData.setSchemaVersion(schemaVersion);
+ keyValueContainerData.setState(ContainerProtos
+ .ContainerDataProto.State.DELETED);
KeyValueContainer container =
new KeyValueContainer(keyValueContainerData, conf);
container.create(volume.getVolumeSet(), volumeChoosingPolicy, clusterId);
+ KeyValueContainerUtil
+ .parseKVContainerData(keyValueContainerData, conf);
+
Review Comment:
Same as prev comment, needed in test util for tests? If not, we can omit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1123162888
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
+
+ KeyValueHandler kvHandler = new KeyValueHandler(conf,
+ datanodeId, containerSet, volumeSet, metrics,
+ c -> icrReceived.incrementAndGet());
+
exception.expect(StorageContainerException.class);
exception.expectMessage(
- "Container cannot be deleted because it is not empty.");
- container2.delete();
+ "Non-force deletion of non-empty container is not allowed.");
+ kvHandler.deleteContainer(container2, false);
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+ }
+
+ /**
+ * If SchemaV3 is enabled, HddsVolume has already been
+ * formatted and initialized in
+ * setupPaths#createDbInstancesForTestIfNeeded.
+ * @throws Exception
+ */
+ @Test
+ public void testDeleteContainerWithRenaming()
+ throws Exception {
+ HddsVolume hddsVolume = null;
+ if (!schemaVersion.contains(OzoneConsts.SCHEMA_V3)) {
+ Files.createDirectories(Paths.get(hddsPath));
+
+ HddsVolume.Builder volumeBuilder =
+ new HddsVolume.Builder(hddsPath)
+ .datanodeUuid(DATANODE_UUID)
+ .conf(conf)
+ .usageCheckFactory(MockSpaceUsageCheckFactory.NONE);
+
+ hddsVolume = volumeBuilder.build();
+
+ hddsVolume.format(SCM_ID);
+ hddsVolume.createWorkingDir(SCM_ID, null);
+ }
+
+ long testContainerID1 = getTestContainerID();
+ Thread.sleep(100);
+ long testContainerID2 = getTestContainerID();
+
+ Container container1 = addContainer(containerSet, testContainerID1);
+ container1.close();
+
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID1));
Assert.assertTrue(containerSet.getContainerMapCopy()
.containsKey(testContainerID2));
+
+ KeyValueContainerData container1Data =
+ (KeyValueContainerData) container1.getContainerData();
+
+ KeyValueContainerData container2Data =
+ (KeyValueContainerData) container2.getContainerData();
+
+ if (schemaVersion.contains(OzoneConsts.SCHEMA_V3)) {
+ hddsVolume = container1Data.getVolume();
Review Comment:
It seems to pass without it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1470830706
Other than the issue raised in HDDS-8173, the end to end testing of container delete with atomic move works without issue. Normal container delete, attempts to remove container entries from `rocksdb (schema V3)`, moves the container directories on the volume to the tmp directory and deletes. Should an error occur either from an exception generated by `rocksDB `helper functions or from the deleting the `tmp` directory after move, the delete from `rocksdb (schema V3) `and delete from `tmp` is retried on shutdown of datanode and restart.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1037307978
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -88,6 +130,67 @@ public void testStartup() throws IOException {
service.close();
}
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
+ public void testTmpDirOnStartup(String schemaVersion) throws IOException {
Review Comment:
You are right. I got carried away by the name of the other method. I'll fix it!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1335566131
@errose28 thanks for reviewing this. I made the changes and updated the tests. Also, added an scm exception in case moving the container failed. Let me know how it looks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1015476729
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -416,6 +418,8 @@ ContainerCommandResponseProto handleDeleteContainer(
deleteInternal(kvContainer, forceDelete);
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
+ } catch (IOException ex) {
Review Comment:
This IOException comes from [here](https://github.com/apache/ozone/blob/78447eb08463a89a2ad0c75d5ce0b2e3597705a6/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java#L568). I don't think we should surpress this or change it to `StorageContainerException`. Maybe catch this exception in `deleteInternal` and log an error.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1281120977
cc @aswinshakil
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] myskov commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
myskov commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r965629751
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private void setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException();
Review Comment:
I think the exception should contain a message
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private void setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException();
+ }
+ } catch (IOException ex) {
+ LOG.error("Volume has not been initialized, " +
+ "clusterId is null.", ex);
+ }
+
+ String pathId = "";
+ try {
+ pathId += VersionedDatanodeFeatures.ScmHA
+ .chooseContainerPathID(hddsVolume, clusterId);
+ } catch (IOException ex) {
+ LOG.error("Failed to get the container path Id", ex);
+ }
+
+ stringBuilder.append(pathId);
+ stringBuilder.append(TMP_DELETE_SERVICE_DIR);
+
+ String tmpPath = stringBuilder.toString();
+ this.tmpDirPath = Paths.get(tmpPath);
+ }
+
+ public boolean renameDir(KeyValueContainerData keyValueContainerData) {
Review Comment:
It would be great to have a javadoc for this method
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private void setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException();
+ }
+ } catch (IOException ex) {
+ LOG.error("Volume has not been initialized, " +
+ "clusterId is null.", ex);
+ }
+
+ String pathId = "";
+ try {
+ pathId += VersionedDatanodeFeatures.ScmHA
+ .chooseContainerPathID(hddsVolume, clusterId);
+ } catch (IOException ex) {
+ LOG.error("Failed to get the container path Id", ex);
+ }
+
+ stringBuilder.append(pathId);
+ stringBuilder.append(TMP_DELETE_SERVICE_DIR);
+
+ String tmpPath = stringBuilder.toString();
+ this.tmpDirPath = Paths.get(tmpPath);
+ }
+
+ public boolean renameDir(KeyValueContainerData keyValueContainerData) {
+ String containerPath = keyValueContainerData.getContainerPath();
+ File container = new File(containerPath);
+ String containerDirName = container.getName();
+
+ String destinationDirPath = tmpDirPath
+ .resolve(Paths.get(containerDirName)).toString();
+
+ boolean success = container.renameTo(new File(destinationDirPath));
+
+ if (success) {
+ keyValueContainerData.setMetadataPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.CONTAINER_META_PATH);
+ keyValueContainerData.setChunksPath(destinationDirPath +
+ FILE_SEPARATOR + OzoneConsts.STORAGE_DIR_CHUNKS);
+ }
+ return success;
+ }
+
+ /**
+ * Get direct files under /tmp/container_delete_service
+ * and store them in a list.
+ * @return iterator to the list of the leftover files
+ */
+ public ListIterator<File> getDeleteLeftovers() {
Review Comment:
The only usage of this method outside of the class is test (TestContainerPersistence). Can we avoid keeping it public?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] myskov commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
myskov commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r965858045
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/CleanUpManager.java:
##########
@@ -0,0 +1,207 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.helpers;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ListIterator;
+
+/**
+ * Helper class for handling /tmp/container_delete_service
+ * operations used for container delete when Schema V3 is enabled.
+ */
+public class CleanUpManager {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(CleanUpManager.class);
+
+ private static final String FILE_SEPARATOR = File.separator;
+
+ private static final String TMP_DELETE_SERVICE_DIR =
+ FILE_SEPARATOR + "tmp" + FILE_SEPARATOR + "container_delete_service";
+
+ private Path tmpDirPath;
+
+ public CleanUpManager(HddsVolume hddsVolume) {
+ setTmpDirPath(hddsVolume);
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ public Path getTmpDirPath() {
+ return tmpDirPath;
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ KeyValueContainerData keyValueContainerData) {
+ return (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3));
+ }
+
+ public static boolean checkContainerSchemaV3Enabled(
+ ConfigurationSource config) {
+ return VersionedDatanodeFeatures.SchemaV3
+ .isFinalizedAndEnabled(config);
+ }
+
+ private void setTmpDirPath(HddsVolume hddsVolume) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = hddsVolume.getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ String clusterId = "";
+ try {
+ clusterId += hddsVolume.getClusterID();
+
+ if (clusterId == null) {
+ throw new IOException();
Review Comment:
@xBis7 yes, but it's better for error description to be part of the exception. If catch statement will be changed in the future, this exception may be left as is.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1251531676
The cluster ID directory on the datanode has a complicated history, but it is not related to schema V3. For a detailed look see HDDS-5432 and the design document linked there. Basically before SCM HA the cluster ID directory on each volume was actually using the SCM ID. This became a problem when SCM HA was introduced since there are now 3 SCMs, but renaming the directory was an incompatible change. At the end of it all the volume could be formatted in the following ways:
- Only cluster ID directory
- The volume was formatted with Ozone 1.3.0
- SCM ID directory pointed to by a cluster ID symlink
- The volume was formatted with Ozone <= 1.2.0 and SCM HA was finalized.
- Only SCM ID
- The volume was formatted with Ozone <= 1.2.0 and SCM HA has not yet been finalized.
- This would happen on the upgrade path from 1.1.0 to 1.3.0
In general you should not need to worry about this. Just call `VersionedDatanodeFeatures#ScmHA#chooseContainerPathID` and use the result as the parent directory.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1092665501
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +413,158 @@ 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;
+ }
+
+ // Initialize tmp and delete service directories
+ hddsVolume.checkTmpDirPaths(hddsVolume.getClusterID());
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // Check the case where we have Schema V3 and
+ // removing the container's entries from RocksDB fails.
+ // --------------------------------------------
+ // On datanode restart, we populate the container set
+ // based on the available datanode volumes and
+ // populate the container metadata based on the values in RocksDB.
+ // The container is in the tmp directory,
+ // so it won't be loaded in the container set
+ // but there will be orphaned entries in the volume's RocksDB.
+ // --------------------------------------------
+ // For every .container file we find under /tmp,
+ // we use it to get the RocksDB entries and delete them.
+ // If the .container file doesn't exist then the contents of the
+ // directory are probably leftovers of a failed delete and
+ // the RocksDB entries must have already been removed.
+ // In any case we can proceed with deleting the directory's contents.
+ // --------------------------------------------
+ // Get container file and check Schema version. If Schema is V3
+ // then remove the container from RocksDB.
+ File containerFile = ContainerUtils.getContainerFile(file);
+
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ if (keyValueContainerData.getSchemaVersion()
+ .equals(OzoneConsts.SCHEMA_V3)) {
+ // Container file doesn't include the volume
+ // so we need to set it here in order to get the db file.
+ keyValueContainerData.setVolume(hddsVolume);
+ File dbFile = KeyValueContainerLocationUtil
+ .getContainerDBFile(keyValueContainerData);
+ keyValueContainerData.setDbFile(dbFile);
+ // Remove container from Rocks DB
+ BlockUtils.removeContainerFromDB(keyValueContainerData,
+ hddsVolume.getConf());
+ }
Review Comment:
The `removeContainerFromDB` can throw an exception should the delete from the volume DB fail. We should catch this here, _log the error_, and continue iterating through the list of containers to delete. On the next call for this volume, the failed container db entry will be retried to remove from the db and subsequently delete the container should it succeed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1412208504
@neils-dev I've updated the patch.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1133014383
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -267,45 +267,45 @@ public void testAddingBlockToDeletedContainer() throws Exception {
// so skip check the behaviors related to it.
assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
- long testContainerID1 = getTestContainerID();
+ long testContainerID = getTestContainerID();
Review Comment:
Pls try to keep changes made to code under review to just address review comments.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1251535746
One other thing to check is what happens with schema V3 when the container is moved to the tmp dir but the RocksDB update fails. Will the datanode restart cleanly? What happens to the RocksDB entries for that container?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1014416675
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -416,6 +418,8 @@ ContainerCommandResponseProto handleDeleteContainer(
deleteInternal(kvContainer, forceDelete);
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
+ } catch (IOException ex) {
Review Comment:
I don't think we need the new exception type. If the move operation fails the container remains on the disk, but SCM won't get an exception back saying it was not deleted. This means the operation will never be retried until the DN sends the next full container report. #3788 will be doing container and volume scans on errors like these in the future to determine if those need to be marked unhealthy, but for now letting SCM know the delete failed is consistent with the current behavior.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -71,21 +124,76 @@ public void tearDown() {
FileUtil.fullyDelete(testDir);
}
- @Test
- public void testStartup() throws IOException {
- service = HddsDatanodeService.createHddsDatanodeService(args);
+ @ParameterizedTest
+ @ValueSource(strings = {OzoneConsts.SCHEMA_V1,
+ OzoneConsts.SCHEMA_V2, OzoneConsts.SCHEMA_V3})
Review Comment:
I think the testing of temp dir cleanup should be moved to its own named test in this class, since the bulk of the code here currently is about that instead of just a general startup test.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
// Deleting a non-empty container should fail.
BlockID blockID2 = ContainerTestHelper.getTestBlockID(testContainerID2);
BlockData someKey2 = new BlockData(blockID2);
someKey2.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container2, someKey2);
+ // KeyValueHandler setup
+ String datanodeId = UUID.randomUUID().toString();
+
+ int[] interval = new int[1];
+ interval[0] = 2;
+ ContainerMetrics metrics = new ContainerMetrics(interval);
+
+ AtomicInteger icrReceived = new AtomicInteger(0);
Review Comment:
Do we want to test the value of this somewhere?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/VersionEndpointTask.java:
##########
@@ -122,6 +124,19 @@ private void checkVolumeSet(MutableVolumeSet volumeSet,
boolean result = StorageVolumeUtil.checkVolume(volume,
scmId, clusterId, configuration, LOG,
ozoneContainer.getDbVolumeSet());
+
+ // Clean <HddsVolume>/tmp/container_delete_service dir.
+ if (volume instanceof HddsVolume) {
Review Comment:
We should not run this block if `result` in the above line comes back false. If it comes back true and we get an exception while cleaning tmp, we need to decide whether or not this should fail the volume. I would say to not fail the volume, as the tmp dir being clean on startup is not required for functionality. If there's further issues as the DN is running the volume scanner will handle it more thoroughly.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.DATANODE_SCHEMA_V3)) {
+ // Get container file
+ File containerFile = getContainerFile(file);
+
+ // If file exists
+ if (containerFile != null) {
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ // Remove container from Rocks DB
+ String dbPath = hddsVolume.getDbParentDir().getAbsolutePath();
+ DatanodeStoreSchemaThreeImpl store =
+ new DatanodeStoreSchemaThreeImpl(hddsVolume.getConf(),
+ dbPath, false);
+ store.removeKVContainerData(keyValueContainerData.getContainerID());
Review Comment:
Looks like we can call `BlockUtils#removeContainerFromDB` here instead. We have a `KeyValueContainerData` and the two places calling this method have configuration objects they can pass down.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.DATANODE_SCHEMA_V3)) {
+ // Get container file
+ File containerFile = getContainerFile(file);
+
+ // If file exists
+ if (containerFile != null) {
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ // Remove container from Rocks DB
+ String dbPath = hddsVolume.getDbParentDir().getAbsolutePath();
+ DatanodeStoreSchemaThreeImpl store =
+ new DatanodeStoreSchemaThreeImpl(hddsVolume.getConf(),
+ dbPath, false);
+ store.removeKVContainerData(keyValueContainerData.getContainerID());
+ }
+ }
+
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", hddsVolume.getDeleteServiceDirPath().toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Search recursively for the container file under a
+ * directory. Return null if the file is not found.
+ * @param file
+ * @return container file or null if it doesn't exist
+ * @throws IOException
+ */
+ public static File getContainerFile(File file) throws IOException {
+ try {
+ if (file.isDirectory()) {
+ for (File subFile : file.listFiles()) {
+ if (subFile.isDirectory()) {
+ File containerFile = getContainerFile(subFile);
+ if (containerFile != null) {
+ return containerFile;
+ }
+ } else {
+ if (FilenameUtils.getExtension(subFile.getName())
+ .equals("container")) {
+ return subFile;
+ }
+ }
+ }
+ } else {
+ if (FilenameUtils.getExtension(file.getName())
+ .equals("container")) {
+ return file;
+ }
+ }
+ } catch (NullPointerException ex) {
+ LOG.error("File object is null.", ex);
+ }
+ return null;
+ }
+
+ /**
+ * In the future might be used to gather metrics
+ * for the files left under
+ * <volume>/hdds/<cluster-id>/tmp/container_delete_service.
+ * @return ListIterator to all the files
+ */
+ public static ListIterator<File> getDeleteLeftovers(HddsVolume hddsVolume) {
+ List<File> leftovers = new ArrayList<>();
+
+ try {
+ File tmpDir = new File(hddsVolume.getDeleteServiceDirPath().toString());
+
+ if (tmpDir.exists()) {
+ for (File file : tmpDir.listFiles()) {
+ leftovers.add(file);
+ }
+ }
+ } catch (NullPointerException ex) {
+ LOG.error("tmp delete directory is null, path doesn't exist", ex);
+ }
+
+ ListIterator<File> leftoversListIt = leftovers.listIterator();
+ return leftoversListIt;
Review Comment:
```suggestion
List<File> leftovers = new ArrayList<>();
File tmpDir = hddsVolume.getDeleteServiceDirPath().toFile();
if (tmpDir.exists() && tmpDir.isDirectory()) {
File[] tmpFiles = tmpDir.listFiles();
if (tmpFiles != null) {
leftovers.addAll(Arrays.asList(tmpFiles));
}
}
return leftovers.listIterator();
```
Writing the method this way removes many intellij warnings that are otherwise present, and does not require catching an unchecked exception which is usually bad practice.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1139,27 @@ 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.info("Container {} has been successfuly moved under {}",
Review Comment:
nit. This might be better as a debug log since it is just confirming that expected behavior happened.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
Review Comment:
Might be good to put a comment block here somewhat sucinctly summarizing [this comment](https://github.com/apache/ozone/pull/3741#issuecomment-1258673684). It's not very intuitive why we need to check and remove RocksDB entries here otherwise.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java:
##########
@@ -135,6 +136,11 @@ private KeyValueContainerData createContainer(Path dir) throws IOException {
Files.createDirectories(dbDir);
Files.createDirectories(dataDir);
+ if (schemaVersion.equals(SCHEMA_V3)) {
+ Path schemaV3DBDir = metaDir.resolve("db");
+ Files.createDirectories(schemaV3DBDir);
+ }
+
Review Comment:
This test passed locally for me without these changes present. Are they needed?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java:
##########
@@ -137,16 +142,47 @@ public void testGetVersion() throws Exception {
/**
* We make getVersion RPC call, but via the VersionEndpointTask which is
* how the state machine would make the call.
+ * Writing data to tmp dir and checking if it has been cleaned.
Review Comment:
This should probably be moved to its own test method for clarity.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -33,24 +51,59 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Test class for {@link HddsDatanodeService}.
*/
+
public class TestHddsDatanodeService {
+
+ @TempDir
+ private File tempDir;
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestHddsDatanodeService.class);
+
+ private final String clusterId = UUID.randomUUID().toString();
+ private final OzoneConfiguration conf = new OzoneConfiguration();
+ private static final int SCM_SERVER_COUNT = 1;
+ private static final String FILE_SEPARATOR = File.separator;
+
private File testDir;
- private OzoneConfiguration conf;
- private HddsDatanodeService service;
- private String[] args = new String[] {};
+ private List<String> serverAddresses;
+ private List<RPC.Server> scmServers;
+ private List<ScmTestMock> mockServers;
Review Comment:
Check on some of the warnings showing after the changes and see if the suggestions can be incorporated. For example, I'm seeing "contents updated but never queried" on these last two fields, and similarly on `hddsDirs` below. Basically looks like the variables are unused.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.DATANODE_SCHEMA_V3)) {
+ // Get container file
+ File containerFile = getContainerFile(file);
+
+ // If file exists
+ if (containerFile != null) {
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ // Remove container from Rocks DB
+ String dbPath = hddsVolume.getDbParentDir().getAbsolutePath();
+ DatanodeStoreSchemaThreeImpl store =
+ new DatanodeStoreSchemaThreeImpl(hddsVolume.getConf(),
+ dbPath, false);
+ store.removeKVContainerData(keyValueContainerData.getContainerID());
+ }
+ }
+
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", hddsVolume.getDeleteServiceDirPath().toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Search recursively for the container file under a
+ * directory. Return null if the file is not found.
+ * @param file
+ * @return container file or null if it doesn't exist
+ * @throws IOException
+ */
+ public static File getContainerFile(File file) throws IOException {
Review Comment:
Can we use `ContainerUtils#getContainerFile` instead of this new method?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1311856319
> Did you get a chance to try the downgrade to 1.1.0 testing I mentioned earlier?
@errose28 I was able to run this script successfully twice `hadoop-ozone/dist/target/ozone-1.3.0-SNAPSHOT/compose/upgrade/test.sh`, once with `run_test non-rolling-upgrade 1.1.0 1.3.0` and once with `run_test non-rolling-upgrade 1.2.1 1.3.0`. For some weird reason, I had to install robotframework under `/compose/upgrade` to get it to work, even though I have it installed in my machine. Apart from that, no issues. I also checked the result logs and I couldn't find any errors related to our changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1307218149
> If this PR takes too long to improve, I recommend removing it from the block list. In addition, Some new bugs may be found after the 1.3.0 release. So we can cherry pick the new bug fix from the master later, and then we can continue to release 1.3.1
@captainzmc okay, let's do that.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r980557006
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -329,6 +354,169 @@ 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
+ public void checkCleanupDirs(String id) {
+ String tmpPath = createTmpPath(id);
+ String deleteServicePath = tmpPath + TMP_DELETE_SERVICE_DIR;
+ deleteServiceDirPath = Paths.get(deleteServicePath);
+ }
+
+ private String createTmpPath(String id) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ stringBuilder.append(id);
+ stringBuilder.append(TMP_DIR);
+
+ return stringBuilder.toString();
+ }
+
+
+ private void createTmpDir(String id) {
+ tmpDirPath = Paths.get(createTmpPath(id));
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ private void createDeleteServiceDir() {
+ try {
+ if (tmpDirPath == null) {
+ throw new IOException("tmp directory under volume " +
+ "has not been initialized");
+ }
+ String tmpPath = tmpDirPath.toString();
+ String deleteServicePath = tmpPath + TMP_DELETE_SERVICE_DIR;
+
+ deleteServiceDirPath = Paths.get(deleteServicePath);
+
+ if (Files.notExists(deleteServiceDirPath)) {
+ try {
+ Files.createDirectories(deleteServiceDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", deleteServiceDirPath.toString(), ex);
+ }
+ }
+ } catch (IOException ex) {
+ String hddsRoot = getHddsRootDir().toString();
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ LOG.error("Failed to create container_delete_service directory under {}",
+ volPath, ex);
+ }
+ }
+
+ /**
+ * Delete all files under tmp/container_delete_service.
+ */
+ public synchronized void cleanTmpDir() throws IOException {
+ if (getStorageState() != VolumeState.NORMAL) {
+ LOG.debug("Call to clean tmp dir container_delete_service directory "
+ + "for {} while VolumeState {}",
+ getStorageDir(), getStorageState().toString());
+ return;
+ }
+
+ if (!getClusterID().isEmpty()) {
+ checkCleanupDirs(getClusterID());
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers();
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", deleteServiceDirPath.toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Keep public, in the future might be used to gather metrics
+ * for the files left under tmp/container_delete_service.
+ * @return ListIterator to all the files
+ */
+ public ListIterator<File> getDeleteLeftovers() {
+ List<File> leftovers = new ArrayList<>();
+
+ try {
+ File tmpDir = new File(deleteServiceDirPath.toString());
+
+ for (File file : tmpDir.listFiles()) {
+ leftovers.add(file);
+ }
+ } catch (NullPointerException ex) {
+ LOG.error("tmp directory is null, path doesn't exist", ex);
Review Comment:
`tmpDir.exists()` or an equivalent call is probably a better way to handle this. Usually we want to avoid catching unchecked exceptions.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/StorageVolumeUtil.java:
##########
@@ -238,6 +238,11 @@ public static boolean checkVolume(StorageVolume volume, String scmId,
// skipped. Create cluster ID symlink now.
// Else, We are still pre-finalized.
// The existing directory should be left for backwards compatibility.
+ if (volume instanceof HddsVolume) {
+ String id = VersionedDatanodeFeatures.ScmHA.chooseContainerPathID(conf,
+ scmId, clusterId);
+ ((HddsVolume)volume).checkCleanupDirs(id);
+ }
Review Comment:
This will run on fresh clusters or clusters pre-finalized for SCM HA, because the volume directory has the VERSION file and one ID directory (either the cluster ID or SCM ID). For clusters that have been upgraded from 1.1.0 the else block will execute, because there will be 3 files (VERSION file, SCM ID directory, and symlink to SCM ID directory named after cluster ID). So the else block needs the cleanup code as well.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/StorageVolumeUtil.java:
##########
@@ -238,6 +238,11 @@ public static boolean checkVolume(StorageVolume volume, String scmId,
// skipped. Create cluster ID symlink now.
// Else, We are still pre-finalized.
// The existing directory should be left for backwards compatibility.
+ if (volume instanceof HddsVolume) {
+ String id = VersionedDatanodeFeatures.ScmHA.chooseContainerPathID(conf,
+ scmId, clusterId);
+ ((HddsVolume)volume).checkCleanupDirs(id);
+ }
Review Comment:
You can refactor the if/else branching to avoid code duplication when checking the cleanup directory once this change is made.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ 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();
+
+ if (hddsVolume.getClusterID() != null) {
Review Comment:
Why do we need this check?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ 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();
+
+ if (hddsVolume.getClusterID() != null) {
+ try {
+ // Rename container location
+ boolean success = hddsVolume
+ .moveToTmpDeleteDirectory(keyValueContainerData);
+
+ if (success) {
Review Comment:
If this fails we need to throw an exception to prevent further steps from executing.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ 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();
+
+ if (hddsVolume.getClusterID() != null) {
+ try {
+ // Rename container location
+ boolean success = hddsVolume
+ .moveToTmpDeleteDirectory(keyValueContainerData);
+
+ if (success) {
+ String containerPath = keyValueContainerData
+ .getContainerPath().toString();
+ File containerDir = new File(containerPath);
+
+ LOG.info("Container {} has been successfuly moved under {}",
+ containerDir.getName(), hddsVolume.getDeleteServiceDirPath());
+ }
+ } catch (IOException ex) {
Review Comment:
Same idea as above, we want to let this exception propagate to abort the delete process. Logging on error might be better done inside the method that is doing the move. Right now it is not clear which methods in the try/catch block throw the exception and why that means the volume is not initialized.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1408214769
hey @DaveTeng0, if you are referring to the workflow, it's unrelated to the PR. I got a green workflow for the latest 2 commits on my fork. Check below
[cleanup](https://github.com/xBis7/ozone/actions/runs/3876726389)
[remove null check before setting tmp dir paths](https://github.com/xBis7/ozone/actions/runs/3876633980)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1092502430
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/TestHddsDatanodeService.java:
##########
@@ -33,24 +47,48 @@
import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_TOKEN_ENABLED;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Test class for {@link HddsDatanodeService}.
*/
+
public class TestHddsDatanodeService {
+
private File testDir;
- private OzoneConfiguration conf;
- private HddsDatanodeService service;
- private String[] args = new String[] {};
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestHddsDatanodeService.class);
+
+ private final String clusterId = UUID.randomUUID().toString();
+ private final OzoneConfiguration conf = new OzoneConfiguration();
+ private final HddsDatanodeService service =
+ HddsDatanodeService.createHddsDatanodeService(new String[] {});
+ private static final int SCM_SERVER_COUNT = 1;
+ private static final String FILE_SEPARATOR = File.separator;
Review Comment:
Would be better to use Ozone separator delimiter: `OzoneConsts.OZONE_URI_DELIMITER `
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1037312795
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -395,4 +416,174 @@ 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.
+ */
+ 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 delete directory
+ hddsVolume.createDeleteServiceDir();
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers(hddsVolume);
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+
+ // If SchemaV3 is enabled and we have a RocksDB
+ if (VersionedDatanodeFeatures.isFinalized(
+ HDDSLayoutFeature.DATANODE_SCHEMA_V3)) {
+ // Get container file
+ File containerFile = getContainerFile(file);
+
+ // If file exists
+ if (containerFile != null) {
+ ContainerData containerData = ContainerDataYaml
+ .readContainerFile(containerFile);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+
+ // Remove container from Rocks DB
+ String dbPath = hddsVolume.getDbParentDir().getAbsolutePath();
+ DatanodeStoreSchemaThreeImpl store =
+ new DatanodeStoreSchemaThreeImpl(hddsVolume.getConf(),
+ dbPath, false);
+ store.removeKVContainerData(keyValueContainerData.getContainerID());
+ }
+ }
+
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", hddsVolume.getDeleteServiceDirPath().toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Search recursively for the container file under a
+ * directory. Return null if the file is not found.
+ * @param file
+ * @return container file or null if it doesn't exist
+ * @throws IOException
+ */
+ public static File getContainerFile(File file) throws IOException {
Review Comment:
> What would be an example of this failure case?
I don't really remember what I was thinking at that point.
> The delete does not move this file relative to the base location.
You are right. I'll make the change and fix the tests.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1122573075
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
Review Comment:
Does this test not work with schema V3 as well. It is testing deleting a non-empty container, which is independent of the db schema (whether db per datanode or db per volume).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1123160088
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -286,19 +288,161 @@ public void testDeleteContainer() throws Exception {
BlockData someKey1 = new BlockData(blockID1);
someKey1.setChunks(new LinkedList<ContainerProtos.ChunkInfo>());
blockManager.putBlock(container1, someKey1);
+ }
+
+ @Test
+ public void testDeleteNonEmptyContainer() throws Exception {
+ long testContainerID2 = getTestContainerID();
+ Container container2 = addContainer(containerSet, testContainerID2);
+ container2.close();
+
+ Assert.assertTrue(containerSet.getContainerMapCopy()
+ .containsKey(testContainerID2));
+
+ // With schema v3, we don't have a container dedicated db,
+ // so skip check the behaviors related to it.
+ assumeFalse(schemaVersion.contains(OzoneConsts.SCHEMA_V3));
Review Comment:
If you check the diffs, this was part of a bigger test that I refactored and it was returning in case we had SchemaV3 so I didn't check it out or made any changes. I just broke it into smaller tests.
You are right, it works regardless of Schema.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #3741:
URL: https://github.com/apache/ozone/pull/3741#issuecomment-1458428208
@neils-dev I've added the new test for checking that the container has been removed from the DB during tmp dir cleanup.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1123133506
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -350,6 +368,63 @@ 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) {
+ Path tmpPath = createTmpPath(id);
+ deleteServiceDirPath = tmpPath.resolve(TMP_DELETE_SERVICE_DIR);
+ }
+
+ private Path createTmpPath(String id) {
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String vol = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ Path volPath = Paths.get(vol);
+ Path idPath = Paths.get(id);
Review Comment:
This is handled in the new method `getIdDir()`. I'll resolve this conversation.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1123131176
##########
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:
@neils-dev This approach is simpler and seems to work as expected. Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xBis7 commented on a diff in pull request #3741: HDDS-6449. Failed container delete can leave artifacts on disk
Posted by GitBox <gi...@apache.org>.
xBis7 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r1064706850
##########
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:
Yes, that must have been an accident. I only meant to cleanup the `setUp()`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org