HDDS-15541. PlacementPolicy to use PendingContainerTracker to check space availability. by ashishkumar50 · Pull Request #10497 · apache/ozone

ashishkumar50 · 2026-06-12T08:30:34Z

What changes were proposed in this pull request?

PlacementPolicy to consider pending container movements tracked by PendingContainerTracker while evaluating datanode space availability for container placement decisions.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15541

How was this patch tested?

Unit tests and existing tests

…pace availability.

rakeshadr

Thanks @ashishkumar50. Added a few comments, please check it.

rakeshadr · 2026-06-15T13:43:34Z


+    // Data-space check: use PendingContainerTracker slot availability.
+    // This accounts for both current disk usage and in-flight allocations.
    if (dataSizeRequired > 0) {


#hasAvailableSpace() function checks for a full 5 GB maxContainerSize slot, so a dn with 4.9 GB free gets rejected.

Remove dataSizeRequired from the signature entirely and rename the method to make the slot-based contract clear — since the value is no longer used and tracker's approach is always maxContainerSize.

rakeshadr · 2026-06-15T15:10:32Z

+        return true;
+      }
+    }
+    LOG.debug("Datanode {} has no available container slots. Pending: {}, Allocatable: {}",


Suggestion: Please check whether we need to add a safety warn check?

// Warn if pending count is physically impossible given current disk sizes. // This indicates container reports are significantly delayed or the disk // shrank faster than the heartbeat cycle. Entries age out after 2 * rollInterval. if (pendingCount > allocatableTotal * 2) { LOG.warn("Datanode {} has unusually high pending count: pending={}, allocatable={}. " + "Container reports may be delayed. Slots will age out in ~{}min.", datanodeInfo.getID(), pendingCount, allocatableTotal, (2 * rollIntervalMs) / 60_000); }

Example case:

How the numbers flow through with the example from before

Volume 1: usable=44GB → 44/5 = 8 slots Volume 2: usable=44GB → 8 slots Volume 3: usable=44GB → 8 slots allocatableTotal = 24 Normal: pendingCount=5, 5 > 48? NO warn. 24 > 5? → return true Burst: pendingCount=20, 20 > 48? NO warn. 24 > 20? → return true (4 free) Full: pendingCount=24, 24 > 48? NO warn. 24 > 24? → return false, metric++ Delayed: pendingCount=55, 55 > 48? YES → LOG.warn "age out in ~10min" 24 > 55? → return false, metric++

Seems this may not happen(pendingCount is so high than allocatable space) unless disk size has shrunk.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR updates SCM placement and pipeline selection logic to consider in-flight container allocations tracked by PendingContainerTracker when evaluating datanode space availability.

Changes:

Add NodeManager#hasAvailableSpace(DatanodeInfo) and implement it in SCMNodeManager (and test node managers) backed by PendingContainerTracker.
Update placement/pipeline logic to use slot-based availability (pending-aware) rather than per-request data-size checks.
Adjust and extend unit tests to mock/stub the new space-availability path and validate slot accounting behavior.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java	Adds the new `hasAvailableSpace` API for pending-aware slot checks.
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/PendingContainerTracker.java	Implements read-only slot availability check and improves logging/slot math safety.
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java	Wires `PendingContainerTracker` into `hasAvailableSpace` and node writability filtering.
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java	Switches “enough space” checks to `NodeManager#hasAvailableSpace` (slot-based).
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineProvider.java	Uses updated “enough space” helper which now depends on `NodeManager`.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestPendingContainerTracker.java	Adds tests for slot exhaustion via pending allocations and empty storage-report behavior.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/TestSCMCommonPlacementPolicy.java	Updates validity tests to use `hasAvailableSpace` semantics instead of committed-bytes delta.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/MockNodeManager.java	Implements `hasAvailableSpace` and adds a helper to reset tracker max container size for tests.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/SimpleMockNodeManager.java	Implements `hasAvailableSpace` (always true) for tests.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestRatisPipelineProvider.java	Sets pending tracker max container size to match configured container size.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelinePlacementPolicy.java	Adjusts test to drive slot exhaustion by inflating pending container max size.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelinePlacementFactory.java	Stubs `hasAvailableSpace` in mocks/spies to keep tests focused.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestSCMContainerPlacement*.java	Stubs `hasAvailableSpace` on NodeManager mocks to align with new placement checks.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestContainerPlacementFactory.java	Stubs `hasAvailableSpace` to satisfy new placement flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    LOG.debug("Datanode {} has no available container slots. Pending: {}, Allocatable: {}",
+        datanodeInfo.getID(), pendingCount, allocatableCount);
+    if (metrics != null) {
+      metrics.incNumSkippedFullNodeContainerAllocation();
+    }
+    return false;


ashishkumar50 · 2026-06-18T05:50:19Z

@szetszwo @sodonnel Can you please help to review this PR.

rakeshadr

Thanks @ashishkumar50 , added a few comments. Please check & handle it.

rakeshadr · 2026-06-18T11:11:53Z

+        if (dropped > 0) {
+          LOG.warn("PendingContainerTracker: force-dropped {} unconfirmed pending containers "
+              + "on DN {} after {}ms (2x rollInterval). "
+              + "Container reports may have been lost.", dropped, datanodeID, elapsed);


Can we add metrics as well. Its good to monitor the case of lost reports - "containers dropped after 2× roll."

if (metrics != null) { metrics.incNumPendingContainersForceDropped(dropped); }

Adding metrics here means, we need to update TwoWindowBucket constructor and also DatanodeInfo constructor to accept metrics object. Since DatanodeInfo class is used by many test class, it requires changing in them too. If we need it then it's better to do in other Jira.

Yeah, make sense to me. Please create a follow-up task

rakeshadr · 2026-06-18T12:34:44Z

+  /**
+   * Pending in-flight replications recorded via checkSpaceAndRecordAllocation count against
+   * slots, same as write-path containers. hasAvailableSpace reflects the combined total.
+   */


Add missing tests related to tracker metrics:-

Note: LLM generated, please use this as reference code and validate before adding it.

private StorageReportProto createFailedStorageReport(DatanodeInfo dn) { return HddsTestUtils.createStorageReport(dn.getID(), "/data/failed", 0, 0, 0, StorageTypeProto.DISK, true); } private PendingContainerTracker trackerWithMetrics(SCMNodeMetrics metrics) { return new PendingContainerTracker(MAX_CONTAINER_SIZE, HddsTestUtils.ROLL_INTERVAL_MS_DEFAULT, metrics); } /** * hasAvailableSpace is a read-only advisory check called by placement policy * for every candidate DN. Firing numSkippedFullNodeContainerAllocation there * would inflate the counter O(healthyNodes) per container allocation. * Only checkSpaceAndRecordAllocation (the actual gate) should fire it. */ @Test public void testSkippedMetricFiresOnlyFromActualAllocationNotFromReadOnlyCheck() { SCMNodeMetrics metrics = mock(SCMNodeMetrics.class); PendingContainerTracker t = trackerWithMetrics(metrics); // fill dn1 to capacity: 1 slot, consume it dn1.updateStorageReports(List.of( createStorageReport(dn1, 10 * MAX_CONTAINER_SIZE, MAX_CONTAINER_SIZE, 0))); t.checkSpaceAndRecordAllocation(dn1, container1); // slot consumed clearInvocations(metrics); // read-only check on a full DN — must NOT fire the metric assertFalse(t.hasAvailableSpace(dn1)); verify(metrics, never()).incNumSkippedFullNodeContainerAllocation(); // actual allocation attempt on a full DN — MUST fire the metric assertFalse(t.checkSpaceAndRecordAllocation(dn1, container2)); verify(metrics, times(1)).incNumSkippedFullNodeContainerAllocation(); } @Test public void testAddedAndRemovedMetricsFireCorrectly() { SCMNodeMetrics metrics = mock(SCMNodeMetrics.class); PendingContainerTracker t = trackerWithMetrics(metrics); assertTrue(t.checkSpaceAndRecordAllocation(dn1, container1)); verify(metrics, times(1)).incNumPendingContainersAdded(); verify(metrics, never()).incNumSkippedFullNodeContainerAllocation(); assertTrue(t.checkSpaceAndRecordAllocation(dn1, container2)); verify(metrics, times(2)).incNumPendingContainersAdded(); t.removePendingAllocation(dn1.getPendingContainerAllocations(), container1); verify(metrics, times(1)).incNumPendingContainersRemoved(); // removing non-existent — must NOT fire removed metric again t.removePendingAllocation(dn1.getPendingContainerAllocations(), container1); verify(metrics, times(1)).incNumPendingContainersRemoved(); } @Test @Timeout(10) public void testForceDropMetricFiresOnDoubleRollInterval() throws InterruptedException { long rollMs = 100L; SCMNodeMetrics metrics = mock(SCMNodeMetrics.class); DatanodeInfo dn = new DatanodeInfo( MockDatanodeDetails.randomLocalDatanodeDetails(), NodeStatus.inServiceHealthy(), null, rollMs); setupDefaultStorageReport(dn); PendingContainerTracker t = new PendingContainerTracker(MAX_CONTAINER_SIZE, rollMs, metrics); assertTrue(t.checkSpaceAndRecordAllocation(dn, container1)); assertTrue(t.checkSpaceAndRecordAllocation(dn, container2)); assertEquals(2, dn.getPendingContainerAllocations().getCount()); Thread.sleep(2 * rollMs + 50); // skip past double interval — triggers force-drop branch assertFalse(t.hasAvailableSpace(dn)); // triggers rollIfNeeded → force-drop assertEquals(0, dn.getPendingContainerAllocations().getCount()); verify(metrics, times(1)).incNumPendingContainersForceDropped(2); verify(metrics, never()).incNumPendingContainersAgedOut(anyInt()); } @Test @Timeout(10) public void testAgedOutMetricFiresOnNormalRoll() throws InterruptedException { long rollMs = 100L; SCMNodeMetrics metrics = mock(SCMNodeMetrics.class); DatanodeInfo dn = new DatanodeInfo( MockDatanodeDetails.randomLocalDatanodeDetails(), NodeStatus.inServiceHealthy(), null, rollMs); setupDefaultStorageReport(dn); PendingContainerTracker t = new PendingContainerTracker(MAX_CONTAINER_SIZE, rollMs, metrics); assertTrue(t.checkSpaceAndRecordAllocation(dn, container1)); // goes into currentWindow assertEquals(1, dn.getPendingContainerAllocations().getCount()); Thread.sleep(rollMs + 50); // single roll: container1 moves to previousWindow // trigger roll via hasAvailableSpace (or getPendingContainerAllocations which rolls) t.hasAvailableSpace(dn); assertEquals(1, dn.getPendingContainerAllocations().getCount()); // still visible in previousWindow Thread.sleep(rollMs + 50); // second roll: previousWindow (holding container1) cleared t.hasAvailableSpace(dn); assertEquals(0, dn.getPendingContainerAllocations().getCount()); verify(metrics, times(1)).incNumPendingContainersAgedOut(1); verify(metrics, never()).incNumPendingContainersForceDropped(anyInt()); } @Test public void testRemovalFreesSlotForNextAllocation() { dn1.updateStorageReports(List.of( createStorageReport(dn1, 10 * MAX_CONTAINER_SIZE, MAX_CONTAINER_SIZE, 0))); // 1 slot assertTrue(tracker.checkSpaceAndRecordAllocation(dn1, container1)); assertFalse(tracker.checkSpaceAndRecordAllocation(dn1, container2)); // slot full tracker.removePendingAllocation(dn1.getPendingContainerAllocations(), container1); assertTrue(tracker.hasAvailableSpace(dn1)); // slot freed assertTrue(tracker.checkSpaceAndRecordAllocation(dn1, container2)); // succeeds now }

Same as above comment, it requires lot of file changes just for metrics.

Please provide the jira link and close this comment!

rakeshadr · 2026-06-25T05:14:05Z

+      if (report.hasFailed() && report.getFailed()) {
+        continue;
+      }
+      allocatableCount += Math.max(0L, VolumeUsage.getUsableSpace(report)) / maxContainerSize;


Please add this also to the metrics list totalAllocatableContainerSlots sub-task and its good metrics to understand:

Total allocatable slots across the cluster - sum(floor(usableSpace/maxContainerSize)) across all healthy DNs

Cluster-wide slot pressure - how close the pending count is to the allocatable ceiling

rakeshadr

Thanks @ashishkumar50 for the continuous efforts. Please provide sub-task link and resolve all comments, then proceed with merge.

+1 LGTM

ashishkumar50 · 2026-06-25T16:40:15Z

Thanks @ashishkumar50 for the continuous efforts. Please provide sub-task link and resolve all comments, then proceed with merge.

+1 LGTM

Jira link: https://issues.apache.org/jira/browse/HDDS-15666

HDDS-15541. PlacementPolicy to use PendingContainerTracker to check s…

a881db0

…pace availability.

ashishkumar50 requested review from rakeshadr and szetszwo June 12, 2026 08:30

ashishkumar50 marked this pull request as ready for review June 12, 2026 10:57

ashishkumar50 mentioned this pull request Jun 15, 2026

HDDS-14924. Account replication in PendingContainerTracker for space usage. #10440

Merged

rakeshadr reviewed Jun 15, 2026

View reviewed changes

Fix review comments

11d32b9

ashishkumar50 requested review from rakeshadr and sodonnel June 17, 2026 12:19

rakeshadr requested a review from Copilot June 17, 2026 13:43

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Copilot started reviewing on behalf of rakeshadr June 17, 2026 13:51 View session

Fix comments

8303848

rakeshadr reviewed Jun 18, 2026

View reviewed changes

Fix review comments

91be97b

rakeshadr reviewed Jun 25, 2026

View reviewed changes

rakeshadr approved these changes Jun 25, 2026

View reviewed changes

ashishkumar50 merged commit 0dd906c into apache:master Jun 25, 2026
47 checks passed

Uh oh!

Conversation

ashishkumar50 commented Jun 12, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

rakeshadr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

ashishkumar50 commented Jun 18, 2026

Uh oh!

rakeshadr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rakeshadr left a comment

Choose a reason for hiding this comment

Uh oh!

ashishkumar50 commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants