This is an automated email from the ASF dual-hosted git repository.
erose pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new 11f3d72784 HDDS-8118. Fail container delete on non empty chunks dir
(#4367)
11f3d72784 is described below
commit 11f3d72784f383f3324ad90b331b3419cafef4fe
Author: Ritesh H Shukla <[email protected]>
AuthorDate: Fri Mar 10 17:26:14 2023 -0800
HDDS-8118. Fail container delete on non empty chunks dir (#4367)
---
.../container/common/helpers/ContainerMetrics.java | 27 ++++
.../container/common/interfaces/Container.java | 7 ++
.../common/statemachine/DatanodeConfiguration.java | 7 ++
.../container/keyvalue/KeyValueContainer.java | 5 +
.../ozone/container/keyvalue/KeyValueHandler.java | 41 +++++-
.../keyvalue/helpers/KeyValueContainerUtil.java | 21 ++++
.../ozone/container/ozoneimpl/OzoneContainer.java | 9 +-
.../container/keyvalue/TestKeyValueHandler.java | 4 +-
.../commandhandler/TestDeleteContainerHandler.java | 138 ++++++++++++++++++++-
9 files changed, 248 insertions(+), 11 deletions(-)
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerMetrics.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerMetrics.java
index 09eb65dfe8..e77b0031b9 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerMetrics.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerMetrics.java
@@ -49,6 +49,10 @@ public class ContainerMetrics {
public static final String STORAGE_CONTAINER_METRICS =
"StorageContainerMetrics";
@Metric private MutableCounterLong numOps;
+ @Metric private MutableCounterLong containerDeleteFailedNonEmptyDir;
+ @Metric private MutableCounterLong containerDeleteFailedBlockCountNotZero;
+ @Metric private MutableCounterLong containerForceDelete;
+
private MutableCounterLong[] numOpsArray;
private MutableCounterLong[] opsBytesArray;
private MutableRate[] opsLatency;
@@ -125,4 +129,27 @@ public class ContainerMetrics {
public long getContainerBytesMetrics(ContainerProtos.Type type) {
return opsBytesArray[type.ordinal()].value();
}
+
+ public void incContainerDeleteFailedBlockCountNotZero() {
+ containerDeleteFailedBlockCountNotZero.incr();
+ }
+ public void incContainerDeleteFailedNonEmpty() {
+ containerDeleteFailedNonEmptyDir.incr();
+ }
+
+ public void incContainersForceDelete() {
+ containerForceDelete.incr();
+ }
+
+ public long getContainerDeleteFailedNonEmptyDir() {
+ return containerDeleteFailedNonEmptyDir.value();
+ }
+
+ public long getContainerDeleteFailedBlockCountNotZero() {
+ return containerDeleteFailedBlockCountNotZero.value();
+ }
+
+ public long getContainerForceDelete() {
+ return containerForceDelete.value();
+ }
}
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java
index 67a7a16779..ef7c4f7420 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java
@@ -57,6 +57,13 @@ public interface Container<CONTAINERDATA extends
ContainerData> extends RwLock {
*/
void delete() throws StorageContainerException;
+ /**
+ * Returns true if container is empty.
+ * @return true of container is empty
+ * @throws IOException if was unable to check container status.
+ */
+ boolean isEmpty() throws IOException;
+
/**
* Update the container.
*
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
index 74d0d19faf..9f3754c3ec 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
@@ -87,6 +87,13 @@ public class DatanodeConfiguration {
ROCKSDB_DELETE_OBSOLETE_FILES_PERIOD_MICRO_SECONDS_KEY =
"hdds.datanode.rocksdb.delete_obsolete_files_period";
+ public static final String
+ OZONE_DATANODE_CHECK_EMPTY_CONTAINER_ON_DISK_ON_DELETE =
+ "hdds.datanode.check.empty.container.delete";
+ public static final Boolean
+ OZONE_DATANODE_CHECK_EMPTY_CONTAINER_ON_DISK_ON_DELETE_DEFAULT =
+ true;
+
/**
* Number of threads per volume that Datanode will use for chunk read.
*/
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
index e1ee881142..e21664c4d3 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
@@ -308,6 +308,11 @@ public class KeyValueContainer implements
Container<KeyValueContainerData> {
}
}
+ @Override
+ public boolean isEmpty() throws IOException {
+ return KeyValueContainerUtil.noBlocksInContainer(containerData);
+ }
+
@Override
public void markContainerForClose() throws StorageContainerException {
writeLock();
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
index 1c60109431..e3396a1755 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
@@ -131,6 +131,7 @@ public class KeyValueHandler extends Handler {
private final long maxContainerSize;
private final Function<ByteBuffer, ByteString> byteBufferToByteString;
private final boolean validateChunkChecksumData;
+ private boolean checkIfNoBlockFiles;
// A striped lock that is held during container creation.
private final Striped<Lock> containerCreationLocks;
@@ -155,6 +156,14 @@ public class KeyValueHandler extends Handler {
} catch (Exception e) {
throw new RuntimeException(e);
}
+
+ checkIfNoBlockFiles =
+ conf.getBoolean(
+ DatanodeConfiguration.
+ OZONE_DATANODE_CHECK_EMPTY_CONTAINER_ON_DISK_ON_DELETE,
+ DatanodeConfiguration.
+ OZONE_DATANODE_CHECK_EMPTY_CONTAINER_ON_DISK_ON_DELETE_DEFAULT);
+
maxContainerSize = (long) config.getStorageSize(
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);
@@ -1231,19 +1240,45 @@ public class KeyValueHandler extends Handler {
}
// Safety check that the container is empty.
// If the container is not empty, it should not be deleted unless the
- // container is beinf forcefully deleted (which happens when
+ // container is being forcefully deleted (which happens when
// container is unhealthy or over-replicated).
if (container.getContainerData().getBlockCount() != 0) {
+ metrics.incContainerDeleteFailedBlockCountNotZero();
LOG.error("Received container deletion command for container {} but"
+
- " the container is not empty.",
- container.getContainerData().getContainerID());
+ " the container is not empty with blockCount {}",
+ container.getContainerData().getContainerID(),
+ container.getContainerData().getBlockCount());
throw new StorageContainerException("Non-force deletion of " +
"non-empty container is not allowed.",
DELETE_ON_NON_EMPTY_CONTAINER);
}
+
+ if (checkIfNoBlockFiles && !container.isEmpty()) {
+ metrics.incContainerDeleteFailedNonEmpty();
+ LOG.error("Received container deletion command for container {} but"
+
+ " the container is not empty",
+ container.getContainerData().getContainerID());
+ throw new StorageContainerException("Non-force deletion of " +
+ "non-empty container:" +
+ container.getContainerData().getContainerID() +
+ " is not allowed.",
+ DELETE_ON_NON_EMPTY_CONTAINER);
+ }
+ } else {
+ metrics.incContainersForceDelete();
}
long containerId = container.getContainerData().getContainerID();
containerSet.removeContainer(containerId);
+ } catch (StorageContainerException e) {
+ throw e;
+ } catch (IOException e) {
+ // All other IO Exceptions should be treated as if the container is not
+ // empty as a defensive check.
+ LOG.error("Could not determine if the container {} is empty",
+ container.getContainerData().getContainerID(), e);
+ throw new StorageContainerException("Could not determine if container "
+ + container.getContainerData().getContainerID() +
+ " is empty", DELETE_ON_NON_EMPTY_CONTAINER);
} finally {
container.writeUnlock();
}
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
index bc3d96d9b2..6d1a54b2a2 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java
@@ -19,6 +19,7 @@ package org.apache.hadoop.ozone.container.keyvalue.helpers;
import java.io.File;
import java.io.IOException;
+import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
@@ -159,6 +160,26 @@ public final class KeyValueContainerUtil {
FileUtils.deleteDirectory(containerMetaDataPath.getParentFile());
}
+ /**
+ * Returns if there are no blocks in the container.
+ * @param containerData Container to check
+ * @param conf configuration
+ * @return true if the directory containing blocks is empty
+ * @throws IOException
+ */
+ public static boolean noBlocksInContainer(KeyValueContainerData
+ containerData)
+ throws IOException {
+ Preconditions.checkNotNull(containerData);
+ File chunksPath = new File(containerData.getChunksPath());
+ Preconditions.checkArgument(chunksPath.isDirectory());
+
+ try (DirectoryStream<Path> dir
+ = Files.newDirectoryStream(chunksPath.toPath())) {
+ return !dir.iterator().hasNext();
+ }
+ }
+
/**
* Parse KeyValueContainerData and verify checksum. Set block related
* metadata like block commit sequence id, block count, bytes used and
diff --git
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
index b3db557b6f..a1d159ef35 100644
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
@@ -119,6 +119,9 @@ public class OzoneContainer {
private DatanodeDetails datanodeDetails;
private StateContext context;
+
+ private final ContainerMetrics metrics;
+
enum InitializingStatus {
UNINITIALIZED, INITIALIZING, INITIALIZED
}
@@ -162,7 +165,7 @@ public class OzoneContainer {
metadataScanner = null;
buildContainerSet();
- final ContainerMetrics metrics = ContainerMetrics.create(conf);
+ metrics = ContainerMetrics.create(conf);
handlers = Maps.newHashMap();
IncrementalReportSender<Container> icrSender = container -> {
@@ -521,4 +524,8 @@ public class OzoneContainer {
StorageVolumeChecker getVolumeChecker(ConfigurationSource conf) {
return new StorageVolumeChecker(conf, new Timer());
}
+
+ public ContainerMetrics getMetrics() {
+ return metrics;
+ }
}
diff --git
a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
index a249f66467..1df7c7a08e 100644
---
a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
+++
b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
@@ -363,9 +363,7 @@ public class TestKeyValueHandler {
Mockito.when(volumeSet.getVolumesList())
.thenReturn(Collections.singletonList(hddsVolume));
- final int[] interval = new int[1];
- interval[0] = 2;
- final ContainerMetrics metrics = new ContainerMetrics(interval);
+ final ContainerMetrics metrics = ContainerMetrics.create(conf);
final AtomicInteger icrReceived = new AtomicInteger(0);
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteContainerHandler.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteContainerHandler.java
index e39fe7ffd5..2b319365aa 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteContainerHandler.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteContainerHandler.java
@@ -17,6 +17,7 @@
package org.apache.hadoop.ozone.container.common.statemachine.commandhandler;
+import org.apache.commons.io.FileUtils;
import org.apache.hadoop.hdds.HddsConfigKeys;
import org.apache.hadoop.hdds.client.ReplicationFactor;
import org.apache.hadoop.hdds.client.ReplicationType;
@@ -34,8 +35,10 @@ import org.apache.hadoop.ozone.client.ObjectStore;
import org.apache.hadoop.ozone.client.OzoneClient;
import org.apache.hadoop.ozone.client.OzoneClientFactory;
import org.apache.hadoop.ozone.client.io.OzoneOutputStream;
+import org.apache.hadoop.ozone.container.common.helpers.ContainerMetrics;
import org.apache.hadoop.ozone.container.common.impl.ContainerData;
import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueHandler;
import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
import org.apache.hadoop.ozone.protocol.commands.CloseContainerCommand;
@@ -47,6 +50,7 @@ import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
+import java.io.File;
import java.io.IOException;
import java.util.HashMap;
import java.util.UUID;
@@ -107,6 +111,120 @@ public class TestDeleteContainerHandler {
}
}
+ @Test(timeout = 60000)
+ public void testDeleteNonEmptyContainerDir()
+ throws Exception {
+ // 1. Test if a non force deletion fails if chunks are still present with
+ // block count set to 0
+ // 2. Test if a force deletion passes even if chunks are still present
+
+ //the easiest way to create an open container is creating a key
+
+ String keyName = UUID.randomUUID().toString();
+
+ // create key
+ createKey(keyName);
+
+ // get containerID of the key
+ ContainerID containerId = getContainerID(keyName);
+
+ ContainerInfo container = cluster.getStorageContainerManager()
+ .getContainerManager().getContainer(containerId);
+
+ Pipeline pipeline = cluster.getStorageContainerManager()
+ .getPipelineManager().getPipeline(container.getPipelineID());
+
+ // We need to close the container because delete container only happens
+ // on closed containers when force flag is set to false.
+
+ HddsDatanodeService hddsDatanodeService =
+ cluster.getHddsDatanodes().get(0);
+
+ Assert.assertFalse(isContainerClosed(hddsDatanodeService,
+ containerId.getId()));
+
+ DatanodeDetails datanodeDetails = hddsDatanodeService.getDatanodeDetails();
+
+ NodeManager nodeManager =
+ cluster.getStorageContainerManager().getScmNodeManager();
+ //send the order to close the container
+ SCMCommand<?> command = new CloseContainerCommand(
+ containerId.getId(), pipeline.getId());
+ command.setTerm(
+
cluster.getStorageContainerManager().getScmContext().getTermOfLeader());
+ nodeManager.addDatanodeCommand(datanodeDetails.getUuid(), command);
+
+ Container containerInternalObj =
+ hddsDatanodeService.
+ getDatanodeStateMachine().
+ getContainer().getContainerSet().getContainer(containerId.getId());
+
+ // Write a file to the container chunks directory indicating that there
+ // might be a discrepancy between block count as recorded in RocksDB and
+ // what is actually on disk.
+ File lingeringBlock =
+ new File(containerInternalObj.
+ getContainerData().getChunksPath() + "/1.block");
+ lingeringBlock.createNewFile();
+ ContainerMetrics metrics =
+ hddsDatanodeService
+ .getDatanodeStateMachine().getContainer().getMetrics();
+ GenericTestUtils.waitFor(() ->
+ isContainerClosed(hddsDatanodeService, containerId.getId()),
+ 500, 5 * 1000);
+
+ //double check if it's really closed (waitFor also throws an exception)
+ Assert.assertTrue(isContainerClosed(hddsDatanodeService,
+ containerId.getId()));
+
+ // Check container exists before sending delete container command
+ Assert.assertFalse(isContainerDeleted(hddsDatanodeService,
+ containerId.getId()));
+
+ // Set container blockCount to 0 to mock that it is empty as per RocksDB
+ getContainerfromDN(hddsDatanodeService, containerId.getId())
+ .getContainerData().setBlockCount(0);
+
+ // send delete container to the datanode
+ command = new DeleteContainerCommand(containerId.getId(), false);
+
+ // Send the delete command. It should fail as even though block count
+ // is zero there is a lingering block on disk.
+ command.setTerm(
+
cluster.getStorageContainerManager().getScmContext().getTermOfLeader());
+ nodeManager.addDatanodeCommand(datanodeDetails.getUuid(), command);
+
+ // Deleting a non-empty container should pass on the DN when the force flag
+ // is true
+ // Check the log for the error message when deleting non-empty containers
+ GenericTestUtils.LogCapturer logCapturer =
+ GenericTestUtils.LogCapturer.captureLogs(
+ LoggerFactory.getLogger(KeyValueHandler.class));
+ GenericTestUtils.waitFor(() ->
+ logCapturer.getOutput().contains("container is not empty"),
+ 500,
+ 5 * 2000);
+ Assert.assertTrue(!isContainerDeleted(hddsDatanodeService,
+ containerId.getId()));
+ Assert.assertEquals(1,
+ metrics.getContainerDeleteFailedNonEmptyDir());
+ // Send the delete command. It should pass with force flag.
+ long beforeForceCount = metrics.getContainerForceDelete();
+ command = new DeleteContainerCommand(containerId.getId(), true);
+
+ command.setTerm(
+
cluster.getStorageContainerManager().getScmContext().getTermOfLeader());
+ nodeManager.addDatanodeCommand(datanodeDetails.getUuid(), command);
+
+ GenericTestUtils.waitFor(() ->
+ isContainerDeleted(hddsDatanodeService, containerId.getId()),
+ 500, 5 * 1000);
+ Assert.assertTrue(isContainerDeleted(hddsDatanodeService,
+ containerId.getId()));
+ Assert.assertTrue(beforeForceCount <
+ metrics.getContainerForceDelete());
+ }
+
@Test(timeout = 60000)
public void testDeleteContainerRequestHandlerOnClosedContainer()
throws Exception {
@@ -175,11 +293,23 @@ public class TestDeleteContainerHandler {
GenericTestUtils.waitFor(() -> logCapturer.getOutput().contains("Non" +
"-force deletion of non-empty container is not allowed"), 500,
5 * 1000);
-
+ ContainerMetrics metrics =
+ hddsDatanodeService
+ .getDatanodeStateMachine().getContainer().getMetrics();
+ Assert.assertEquals(1,
+ metrics.getContainerDeleteFailedBlockCountNotZero());
// Set container blockCount to 0 to mock that it is empty
- getContainerfromDN(hddsDatanodeService, containerId.getId())
- .getContainerData().setBlockCount(0);
-
+ Container containerToDelete = getContainerfromDN(
+ hddsDatanodeService, containerId.getId());
+ containerToDelete.getContainerData().setBlockCount(0);
+ File chunkDir = new File(containerToDelete.
+ getContainerData().getChunksPath());
+ File[] files = chunkDir.listFiles();
+ if (files != null) {
+ for (File file : files) {
+ FileUtils.delete(file);
+ }
+ }
// Send the delete command again. It should succeed this time.
command.setTerm(
cluster.getStorageContainerManager().getScmContext().getTermOfLeader());
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
