Skip to content

HDDS-15110. Fix negative container size handling in replication #10126

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

Open
ptlrs wants to merge 5 commits into
base: master
Choose a base branch
from
+61 −3
Open

HDDS-15110. Fix negative container size handling in replication #10126

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b51dde8
HDDS-15110. Fix negative container size handling in replication
ptlrs Apr 24, 2026
dc3db95
HDDS-15110. Fix redundant max calculation in container size update
ptlrs Apr 24, 2026
7723253
HDDS-15110. Add assertions to validate non-negative space usage
ptlrs Apr 24, 2026
9f51497
HDDS-15110. Adjust blockBytes calculation to handle negative values c…
ptlrs Apr 27, 2026
649cd08
Merge remote-tracking branch 'refs/remotes/upstream/master' into HDDS…
ptlrs Apr 29, 2026
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
8 changes: 4 additions & 4 deletions ...er-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,10 +707,10 @@ public synchronized void incrementBlockBytes(long delta) {

public synchronized void decDeletion(long deletedBytes, long processedBytes, long deletedBlockCount,
long processedBlockCount) {
blockBytes -= deletedBytes;
blockCount -= deletedBlockCount;
blockPendingDeletion -= processedBlockCount;
blockPendingDeletionBytes -= processedBytes;
blockBytes = Math.max(1L, blockBytes - deletedBytes);
Copy link
Copy Markdown
Contributor

@ashishkumar50 ashishkumar50 Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If blockBytes - deletedBytes == 0, we should not make blockBytes = 1L. If we do this then never blockBytes will become 0. Only when if there is negative value after calculation we can keep something like 1L.

blockCount = Math.max(0L, blockCount - deletedBlockCount);
blockPendingDeletion = Math.max(0L, blockPendingDeletion - processedBlockCount);
blockPendingDeletionBytes = Math.max(0L, blockPendingDeletionBytes - processedBytes);
}

public synchronized void updateBlocks(long bytes, long count) {
Expand Down
53 changes: 53 additions & 0 deletions ...src/test/java/org/apache/hadoop/ozone/container/replication/TestContainerReplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static org.apache.hadoop.hdds.scm.storage.ContainerProtocolCalls.createContainer;
import static org.apache.ozone.test.GenericTestUtils.waitFor;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

import com.google.common.collect.ImmutableList;
import java.io.IOException;
Expand Down Expand Up @@ -53,8 +54,11 @@
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.container.ContainerTestHelper;
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.statemachine.DatanodeStateMachine;
import org.apache.hadoop.ozone.container.common.statemachine.StateContext;
import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils;
import org.apache.hadoop.ozone.protocol.commands.ReplicateContainerCommand;
import org.apache.ozone.test.GenericTestUtils;
import org.apache.ozone.test.GenericTestUtils.LogCapturer;
Expand All @@ -65,6 +69,7 @@
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.slf4j.event.Level;

/**
Expand Down Expand Up @@ -155,6 +160,54 @@ void targetPullsFromWrongService() throws Exception {
ReplicationSupervisor::getReplicationFailureCount);
}

/**
* Replication must succeed even when the source container's persisted
* {@code CONTAINER_BYTES_USED} RocksDB counter has drifted negative.
*/
@ParameterizedTest
@ValueSource(longs = {0L, 1L, -1_234_567_890L})
void pushSucceedsWhenSourceBytesUsedIsNegative(long containerSize) throws Exception {
DatanodeDetails source = cluster.getHddsDatanodes().get(0)
.getDatanodeDetails();
DatanodeDetails target = selectOtherNode(source);

long containerID = createOverAllocatedContainer(source, 2L * 1024L * 1024L);

poisonBytesUsed(source, containerID, containerSize);

ReplicateContainerCommand cmd =
ReplicateContainerCommand.toTarget(containerID, target);

queueAndWaitForCompletion(cmd, source,
ReplicationSupervisor::getReplicationSuccessCount);

// Target must end up hosting the container.
Container<?> imported = cluster.getHddsDatanode(target)
.getDatanodeStateMachine()
.getContainer()
.getContainerSet()
.getContainer(containerID);
assertNotNull(imported, "target should import the container despite a negative bytesUsed on source");
}

private void poisonBytesUsed(DatanodeDetails dn, long containerID, long poisonValue) throws IOException {
HddsDatanodeService dnService = cluster.getHddsDatanode(dn);
Container<?> container = dnService.getDatanodeStateMachine().getContainer()
.getContainerSet().getContainer(containerID);
KeyValueContainerData data =
(KeyValueContainerData) container.getContainerData();

try (DBHandle db = BlockUtils.getDB(data, dnService.getConf())) {
db.getStore().getMetadataTable()
.put(data.getBytesUsedKey(), poisonValue);
}
// Keep the in-memory Statistics counter consistent with the on-disk
// poisoned value. The import failure is driven by the on-disk value (what
// the target reads), but this prevents any subsequent close/flush path on
// the source from silently correcting the poison before packing.
data.getStatistics().setBlockBytesForTesting(poisonValue);
}

/**
* Replication fails because source tries to push a non-existent container.
*/
Expand Down