errose28 commented on code in PR #4379:
URL: https://github.com/apache/ozone/pull/4379#discussion_r1133026872
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteContainerHandler.java:
##########
@@ -225,6 +233,180 @@ public void testDeleteNonEmptyContainerDir()
metrics.getContainerForceDelete());
}
+ @Test(timeout = 60000)
+ public void testDeleteNonEmptyContainerBlockTable()
+ 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);
+ // Write entries to the block Table.
+ try (DBHandle dbHandle
+ = BlockUtils.getDB(
+ (KeyValueContainerData)getContainerfromDN(hddsDatanodeService,
+ containerId.getId()).getContainerData(),
+ conf)) {
+ BlockData blockData = new BlockData(new BlockID(1, 1));
+ dbHandle.getStore().getBlockDataTable().put("block1", blockData);
+ }
+
+ long containerDeleteFailedNonEmptyBefore =
+ metrics.getContainerDeleteFailedNonEmptyDir();
+ // 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);
+
+
+ // 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.assertTrue(containerDeleteFailedNonEmptyBefore <
+ metrics.getContainerDeleteFailedNonEmptyDir());
+
+ // Now empty the container Dir and try with a non empty block table
+ Container containerToDelete = getContainerfromDN(
+ hddsDatanodeService, containerId.getId());
+ File chunkDir = new File(containerToDelete.
+ getContainerData().getChunksPath());
+ File[] files = chunkDir.listFiles();
+ if (files != null) {
+ for (File file : files) {
+ FileUtils.delete(file);
+ }
+ }
+ 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);
+
+
+ // Check the log for the error message when deleting non-empty containers
+ GenericTestUtils.waitFor(() ->
+ logCapturer.getOutput().
+ contains("Non-empty blocks table for container"),
+ 500,
+ 5 * 2000);
+ Assert.assertTrue(!isContainerDeleted(hddsDatanodeService,
+ containerId.getId()));
+ Assert.assertEquals(1,
+ metrics.getContainerDeleteFailedNonEmptyBlockDB());
+ // 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());
+ }
+
+ private void clearBlocksTable(Container container) throws IOException {
+ try (DBHandle dbHandle
+ = BlockUtils.getDB(
+ (KeyValueContainerData) container.getContainerData(),
+ conf)) {
+ List<? extends Table.KeyValue<String, BlockData>>
+ blocks = dbHandle.getStore().getBlockDataTable().getRangeKVs(
+ ((KeyValueContainerData) container.getContainerData()).
+ startKeyEmpty(),
+ Integer.MAX_VALUE,
+ ((KeyValueContainerData) container.getContainerData()).
+ containerPrefix(),
+ ((KeyValueContainerData) container.getContainerData()).
+ getUnprefixedKeyFilter());
Review Comment:
Same as above, I think we can use
`dbHandle.getStore().getBlockIterator(container.getContainerData().getContainerID());`
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1226,11 +1227,67 @@ private String[] getFilesWithPrefix(String prefix, File
chunkDir) {
return chunkDir.list(filter);
}
+ private boolean logBlocksIfNonZero(Container container)
+ throws IOException {
+ boolean nonZero = false;
+ try (DBHandle dbHandle
+ = BlockUtils.getDB(
+ (KeyValueContainerData) container.getContainerData(),
+ conf)) {
+ List<? extends Table.KeyValue<String, BlockData>>
+ blocks = dbHandle.getStore().getBlockDataTable().getRangeKVs(
+ ((KeyValueContainerData) container.getContainerData()).
+ startKeyEmpty(),
+ Integer.MAX_VALUE,
+ ((KeyValueContainerData) container.getContainerData()).
+ containerPrefix(),
+ ((KeyValueContainerData) container.getContainerData()).
+ getUnprefixedKeyFilter());
+ StringBuilder stringBuilder = new StringBuilder();
+ for (Table.KeyValue<String, BlockData> kv : blocks) {
+ nonZero = true;
+ stringBuilder.append(kv.getValue());
+ if (stringBuilder.length() > StorageUnit.KB.toBytes(32)) {
+ break;
+ }
+ }
+ if (nonZero) {
+ LOG.error("blocks in rocksDB on container delete: {}",
+ stringBuilder.toString());
+ }
+ }
+ return nonZero;
+ }
+
+ private void logBlocksFoundOnDisk(Container container) throws IOException {
+ // List files left over
+ File chunksPath = new
+ File(container.getContainerData().getChunksPath());
+ Preconditions.checkArgument(chunksPath.isDirectory());
+ try (DirectoryStream<Path> dir
+ = Files.newDirectoryStream(chunksPath.toPath())) {
+ StringBuilder stringBuilder = new StringBuilder();
+ for (Path block : dir) {
+ stringBuilder.append(block);
+ stringBuilder.append(",");
+ if (stringBuilder.length() > StorageUnit.KB.toBytes(16)) {
+ break;
+ }
+ LOG.error(
+ "Received container deletion command for container {} but" +
+ " the container is not empty",
+ container.getContainerData().getContainerID());
Review Comment:
nit. If for some reason the string gets beyond 16kb in length this message
won't get printed. Might want a boolean like the previous method has.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1226,11 +1227,67 @@ private String[] getFilesWithPrefix(String prefix, File
chunkDir) {
return chunkDir.list(filter);
}
+ private boolean logBlocksIfNonZero(Container container)
+ throws IOException {
+ boolean nonZero = false;
+ try (DBHandle dbHandle
+ = BlockUtils.getDB(
+ (KeyValueContainerData) container.getContainerData(),
+ conf)) {
+ List<? extends Table.KeyValue<String, BlockData>>
+ blocks = dbHandle.getStore().getBlockDataTable().getRangeKVs(
+ ((KeyValueContainerData) container.getContainerData()).
+ startKeyEmpty(),
+ Integer.MAX_VALUE,
+ ((KeyValueContainerData) container.getContainerData()).
+ containerPrefix(),
+ ((KeyValueContainerData) container.getContainerData()).
+ getUnprefixedKeyFilter());
Review Comment:
Looks like this could be simplified with
`dbHandle.getStore().getBlockIterator(container.getContainerData().getContainerID())`;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
