HDDS-15791. EC reconstructed RECOVERING container while still idle can be deleted by SCM and recreated as new partial OPEN container#10702
Conversation
…n be deleted by SCM and recreated as new partial OPEN container
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @aswinshakil for your patch. Kindly have a look into some comments.
| } | ||
|
|
||
| @Override | ||
| protected ChunkInfo.Builder decorateChunkInfo(ChunkInfo.Builder builder) { |
There was a problem hiding this comment.
I think, this might break the normal EC write as well because ECBlockOutputStream is used by both the reconstruction coordinator (ECReconstructionCoordinator) and the normal client write path (ECBlockOutputStreamEntry). So we might need to have this flag CONTAINER_CREATABLE_FALSE only for reconstruction path.
There was a problem hiding this comment.
Updated to add overloaded API for reconstruction.
| // current impl does not support caching both | ||
| ChunkInfo chunkInfo = ChunkInfo.newBuilder() | ||
| ChunkInfo chunkInfo = decorateChunkInfo(ChunkInfo.newBuilder() | ||
| .setChunkName(blockID.get().getLocalID() + "_chunk_" + ++chunkIndex) |
There was a problem hiding this comment.
decorateChunkInfo method will fall into both normal EC write path and reconstruction path. Kindly check this part.
| /** | ||
| * @return true if the DataNode may auto-create a missing container for this WriteChunk request | ||
| */ | ||
| public static boolean isContainerCreatable(ContainerCommandRequestProto request) { |
There was a problem hiding this comment.
This flag is currently only restricted to check writeChunk metadata only, but missing PutBlock / PutSmallFilecheck. In a partial/padding stripe, non-writing nodes do create containers by an empty PutBlock, so in normal EC writepath, it is fine. But in case of reconstruction path, a slight edge case where PutBlock
might land up on a force-deleted RECOVERING container can still auto-create it as OPEN
There was a problem hiding this comment.
Added for PutBlock as well.
| */ | ||
| public void updateRecoveringContainerTimeout(long containerId) { | ||
| Preconditions.checkState(containerId >= 0, "Container Id cannot be negative."); | ||
| removeRecoveringContainer(containerId); |
There was a problem hiding this comment.
This could be a performance issue, earlier removeRecoveringContainer was called when container should be marked as CLOSE, but now we are calling this a very hot path for every chunk write. Number of RECOVERING containers itself may be less, but number of chunks in lot of RECOVERING containers will add up this method call multiple times and this removeRecoveringContainer method is iterating all containers by value of hashmap entry. One possible solution: How about storing the recovering containers using containerId as key in hashmap and deadline time as value, then it will be O(1) operation
| request); | ||
| } | ||
|
|
||
| maybeUpdateRecoveringContainerTimeout(kvContainer); |
There was a problem hiding this comment.
There could be a possibility of race condition still when getTasks check here if deadline crossed and then adds to backgroundTaskQueue, but by the time queue picks up and task call markContainerUnhealthy, a writeChunk call of this method may adds a fresh deadline entry but the already-queued task still marks the container UNHEALTHY — and leaves a stale entry for a now-UNHEALTHY container.
2 participants
What changes were proposed in this pull request?
During EC offline reconstruction, the coordinator creates a RECOVERING replica on a spare DataNode and writes blocks over time. If reconstruction stalls (e.g. slow block-group processing) longer than
ozone.recovering.container.timeout(~20m, measured from creation, not last write), the DN scrubber marks the replica UNHEALTHY and SCM force-deletes it.The coordinator may still be running. The next
WriteChunkfinds no container;HddsDispatcherauto-creates a new OPEN container instead of failing. The coordinator continues and closes that replica shortly after, leaving a partial container (fewer blocks/chunks than the reference) that SCM can accept as CLOSED — silent data loss.This PR fixes the issue my not allowing create container during
WriteChunkfor RECOVERING containers. Also update the recovering container timeout based on the last write to the container.What is the link to the Apache JIRA
HDDS-15791
How was this patch tested?
Added Unit Tests