Skip to content

HDDS-8142. Check if no entries in Block DB for a container on container delete #4379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
errose28 merged 3 commits into from
Mar 12, 2023
Merged

HDDS-8142. Check if no entries in Block DB for a container on container delete #4379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
962f306
HDDS-8142. Check if no entries in Block DB for a container on contain…
kerneltime Mar 11, 2023
9aebe4c
Address review comments
kerneltime Mar 11, 2023
d47c64b
Address checkstyle
kerneltime Mar 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...vice/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public class ContainerMetrics {
@Metric private MutableCounterLong containerDeleteFailedNonEmptyDir;
@Metric private MutableCounterLong containerDeleteFailedBlockCountNotZero;
@Metric private MutableCounterLong containerForceDelete;
@Metric private MutableCounterLong containerDeleteFailedNonEmptyBlockDB;

private MutableCounterLong[] numOpsArray;
private MutableCounterLong[] opsBytesArray;
Expand Down Expand Up @@ -152,4 +153,12 @@ public long getContainerDeleteFailedBlockCountNotZero() {
public long getContainerForceDelete() {
return containerForceDelete.value();
}

public void incContainerDeleteFailedNonEmptyBlocksDB() {
containerDeleteFailedNonEmptyBlockDB.incr();
}

public long getContainerDeleteFailedNonEmptyBlockDB() {
return containerDeleteFailedNonEmptyBlockDB.value();
}
}
132 changes: 111 additions & 21 deletions ...ner-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
Expand All @@ -42,7 +45,6 @@
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandRequestProto;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandResponseProto;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerType;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.GetSmallFileRequestProto;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.KeyValue;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.PutSmallFileRequestProto;
Expand All @@ -64,7 +66,9 @@
import org.apache.hadoop.ozone.container.common.impl.ContainerLayoutVersion;
import org.apache.hadoop.ozone.container.common.impl.ContainerData;
import org.apache.hadoop.ozone.container.common.impl.ContainerSet;
import org.apache.hadoop.ozone.container.common.interfaces.BlockIterator;
import org.apache.hadoop.ozone.container.common.interfaces.Container;
import org.apache.hadoop.ozone.container.common.interfaces.DBHandle;
import org.apache.hadoop.ozone.container.common.interfaces.Handler;
import org.apache.hadoop.ozone.container.common.interfaces.VolumeChoosingPolicy;
import org.apache.hadoop.ozone.container.common.report.IncrementalReportSender;
Expand Down Expand Up @@ -124,14 +128,13 @@ public class KeyValueHandler extends Handler {
private static final Logger LOG = LoggerFactory.getLogger(
KeyValueHandler.class);

private final ContainerType containerType;
private final BlockManager blockManager;
private final ChunkManager chunkManager;
private final VolumeChoosingPolicy volumeChoosingPolicy;
private final long maxContainerSize;
private final Function<ByteBuffer, ByteString> byteBufferToByteString;
private final boolean validateChunkChecksumData;
private boolean checkIfNoBlockFiles;
private final boolean checkIfNoBlockFiles;

// A striped lock that is held during container creation.
private final Striped<Lock> containerCreationLocks;
Expand All @@ -143,7 +146,6 @@ public KeyValueHandler(ConfigurationSource config,
ContainerMetrics metrics,
IncrementalReportSender<Container> icrSender) {
super(config, datanodeId, contSet, volSet, metrics, icrSender);
containerType = ContainerType.KeyValueContainer;
blockManager = new BlockManagerImpl(config);
validateChunkChecksumData = conf.getObject(
DatanodeConfiguration.class).isChunkDataValidationCheck();
Expand Down Expand Up @@ -398,11 +400,10 @@ ContainerCommandResponseProto handleCreateContainer(
private void populateContainerPathFields(KeyValueContainer container,
HddsVolume hddsVolume) throws IOException {
volumeSet.readLock();
HddsVolume containerVolume = hddsVolume;
try {
String idDir = VersionedDatanodeFeatures.ScmHA.chooseContainerPathID(
containerVolume, clusterId);
container.populatePathFields(idDir, containerVolume);
hddsVolume, clusterId);
container.populatePathFields(idDir, hddsVolume);
} finally {
volumeSet.readUnlock();
}
Expand Down Expand Up @@ -659,8 +660,8 @@ ContainerCommandResponseProto handleListBlock(
}
List<BlockData> responseData =
blockManager.listBlock(kvContainer, startLocalId, count);
for (int i = 0; i < responseData.size(); i++) {
returnData.add(responseData.get(i).getProtoBufMessage());
for (BlockData responseDatum : responseData) {
returnData.add(responseDatum.getProtoBufMessage());
}
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
Expand Down Expand Up @@ -1226,11 +1227,64 @@ 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)) {
BlockIterator<BlockData>
blockIterator = dbHandle.getStore().
getBlockIterator(container.getContainerData().getContainerID());
StringBuilder stringBuilder = new StringBuilder();
while (blockIterator.hasNext()) {
nonZero = true;
stringBuilder.append(blockIterator.nextBlock());
if (stringBuilder.length() > StorageUnit.KB.toBytes(32)) {
break;
}
}
if (nonZero) {
LOG.error("blocks in rocksDB on container delete: {}",
stringBuilder.toString());
}
}
return nonZero;
}

private boolean logBlocksFoundOnDisk(Container container) throws IOException {
// List files left over
File chunksPath = new
File(container.getContainerData().getChunksPath());
Preconditions.checkArgument(chunksPath.isDirectory());
boolean notEmpty = false;
try (DirectoryStream<Path> dir
= Files.newDirectoryStream(chunksPath.toPath())) {
StringBuilder stringBuilder = new StringBuilder();
for (Path block : dir) {
if (notEmpty) {
stringBuilder.append(",");
}
stringBuilder.append(block);
notEmpty = true;
if (stringBuilder.length() > StorageUnit.KB.toBytes(16)) {
break;
}
}
if (notEmpty) {
LOG.error("Files still part of the container on delete: {}",
stringBuilder.toString());
}
}
return notEmpty;
}

private void deleteInternal(Container container, boolean force)
throws StorageContainerException {
container.writeLock();
try {
// If force is false, we check container state.
// If force is false, we check container state.
if (!force) {
// Check if container is open
if (container.getContainerData().isOpen()) {
Expand All @@ -1245,24 +1299,60 @@ private void deleteInternal(Container container, boolean force)
if (container.getContainerData().getBlockCount() != 0) {
metrics.incContainerDeleteFailedBlockCountNotZero();
LOG.error("Received container deletion command for container {} but" +
" the container is not empty with blockCount {}",
" 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);
// This is a defensive check to make sure there is no data loss if
// 1. There are one or more blocks on the filesystem
// 2. There are one or more blocks in the block table
// This can lead to false positives as
// 1. Chunks written to disk that did not get recorded in RocksDB can
// occur due to failures during write
// 2. Blocks that were deleted from blocks table but the deletion of
// the underlying file could not be completed
// 3. Failures between files being deleted from disk but not being
// cleaned up.
// 4. Bugs in the code.
// Blocks stored on disk represent data written by a client and should
// be treated with care at the expense of creating artifacts on disk
// that might be unreferenced.
// https://issues.apache.org/jira/browse/HDDS-8138 will move the
// implementation to only depend on consistency of the chunks folder

// First check if any files are in the chunks folder. If there are
// to help with debugging also dump the blocks table data.
if (checkIfNoBlockFiles) {
if (!container.isEmpty()) {
metrics.incContainerDeleteFailedNonEmpty();
logBlocksFoundOnDisk(container);
logBlocksIfNonZero(container);
// List Blocks from Blocks Table
throw new StorageContainerException("Non-force deletion of " +
"non-empty container dir:" +
container.getContainerData().getContainerID() +
" is not allowed.",
DELETE_ON_NON_EMPTY_CONTAINER);
}

// The chunks folder is empty, not check if the blocks table has any
// blocks still referenced. This will avoid cleaning up the
// blocks table for future debugging.
// List rocks
if (logBlocksIfNonZero(container)) {
LOG.error("Non-empty blocks table for container {}",
container.getContainerData().getContainerID());
metrics.incContainerDeleteFailedNonEmptyBlocksDB();
throw new StorageContainerException("Non-force deletion of " +
"non-empty container block table:" +
container.getContainerData().getContainerID() +
" is not allowed.",
DELETE_ON_NON_EMPTY_CONTAINER);
}
}
} else {
metrics.incContainersForceDelete();
Expand Down
Loading