HDDS-14921. Improve space accounting in SCM with In-Flight container allocation tracking.

[ashishkumar50]

What changes were proposed in this pull request?

Maintain space accounting during container allocation in SCM. More detail description is in Jira.

What is the link to the Apache JIRA

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

How was this patch tested?

UT and IT.

[rakeshadr]

Thanks @ashishkumar50 for providing the patch. Added a few comments, please take care.

[rakeshadr]

Say, DN is healthy, all containers confirmed, no new allocations → that DN's bucket never rolls even though heartbeats come every 30 seconds, right?

t=0    Container C1 allocated → pending recorded in tracker

t=60-120  FCR arrives from DN
          → cid = C1
          → alreadyInDn = expectedContainersInDatanode.remove(C1) = FALSE
          → !alreadyInDn = TRUE → removePendingAllocation called → rollIfNeeded fires ✓
          → C1 added to NM DN-set

How abt rolls on every processHeartbeat, every 30 seconds regardless of container state changes ?

[ashishkumar50]

Added roll in every node report which is per minute from DN.

[rakeshadr]

Potentially hits concurrency issue. Say two threads entered this block.

Thread-1 (removePendingAllocation): bucket.isEmpty(), returns true

Thread-2 (recordPendingAllocationForDatanode): computeIfAbsent(uuid) returns same bucket
reference (key still exists), calls bucket.add(containerID) and now the bucket will be non-empty

Thread-1: datanodeBuckets.remove(uuid, bucket), then removes the non-empty bucket and now the containerID will be in a detached bucket object, right?

I think, we need to add synchronization to avoid detached bucket object.

[ashishkumar50]

Added sync at bucket level.

[rakeshadr]

Please use CHM mutation, that will simplify it.

bucket.rollIfNeeded();
removed.set(bucket.remove(containerID));
remaining.set(bucket.getCount());
LOG.debug("Removed pending container {} from DataNode {}. Removed={}, Remaining={}",
    containerID, node.getUuidString(), removed.get(), remaining.get());
return bucket.isEmpty() ? null : bucket;
});
if (removed.get() && metrics != null) {
  metrics.incNumPendingContainersRemoved();
}

Important lock design principle:-

PendingContainerTracker: Please ensure no bucket -> map mutation path anymore. You need to switch remove/roll/clear to concurrenthashmap compute-based mutations, keep bucket internals intact.

[rakeshadr]

Do we have test scenario covering roll over?

The two-window rolling behavior (container in previousWindow roll after 2× interval). Say, add C1 in currentWindow, then moves C1 to previousWindow, then wait for the roll over.

[ashishkumar50]

Added test for this testTwoWindowRollAgesOutContainerAfterTwoIntervals.

[sumitagrawl]

@ashishkumar50 Thanks for working over this, have few review comments.

[sumitagrawl]

This needs to be part of SCMNodeManager, more specific to SCMNodeStat. Reason,

1. need handle even like stale node / dead node handler as cleanup
2. May need report this when reporting to CLI for available space in the DN
3. To be used for pipeline allocation policy, where container manager does not come in role
   Its datanode space, just trying to identify already allocated space. And needs to be part of committed space at SCM when reporting to CLI, or other breakup.

[ashishkumar50]

Moved to node package

[sumitagrawl]

Please check if node report is also send in ICR, this is for reason that node information should be updated with ICR at same time.

[sumitagrawl]

Instead of calcuating all available and then removing, we can do progressive base, like,
required=pending+newAllocation
for each report
required = required - volumeUsage in roundoff value
if (required <= 0)
return true

But we need to reserve also, can do first add and check, if not present, remove containerId

OR other way,
when DN report storage handling, total consolidate value can also be added to memory to avoid looping on every call.

[ashishkumar50]

Updated the logic to break when enough space is available on any volume

[aswinshakil]

If we are not sure if we can create container here, Should we still choose this pipeline? Instead of making it generic, we can specify what to do for each exception we might see.

[ashishkumar50]

Moved the code, there is no exception here.

[aswinshakil]

Why not just add this to the ContainerManager interface? We can avoid these conversions. Is this because Recon uses the same code path and we don't want it to this? For Recon we can just make it a No-Op.

[ashishkumar50]

Moved to node package, so not required these conversions now.

[aswinshakil]

This makes the allocation little aggressive right? Even if we just have 5GB we allocate it. Should we have leave some buffer when allocating a container?

[ashishkumar50]

No need of extra buffer here, as we are anyway going to give buffer in DN by considering soft and hard limit. So in case of some overflow DN will accept it until hard limit.

[rakeshadr]

Since this PR is adding more stricter/defensive "two-window tumbling bucket" logic, there is a high chance to hit the return null code path flow. Please double check all the callers of API ContainerManagerImpl#allocateContainer() then safeguard with null check, otw it would result in NPE.

For example, SCMClientProtocolServer.java#L258, this would hit NPE.

[ashishkumar50]

Checked other places already have safeguard. SCMClientProtocolServer.java#L258 is used only for test or tools. Added a safeguard here.

[rakeshadr]

For safe coding, can you please do union,

synchronized int getCount() {
     return getAllPending().size()
}

[ashishkumar50]

getAllPending is creating two new set which may be costly. Instead this looks less costly. Sumit had given comment above to avoid creating new list. If we want other way then we need to cache size value, which i think may not be needed here to store size as well.

[rakeshadr]

Please use CHM mutation, that will simplify it.

if (node == null) {
  return;
}
UUID uuid = node.getUuid();
if (datanodeBuckets.remove(uuid) != null) {
  LOG.debug("Cleared pending container allocations for datanode {}",
      node.getUuidString());
}

Important lock design principle:-

PendingContainerTracker: Please ensure no bucket -> map mutation path anymore. You need to switch remove/roll/clear to concurrenthashmap compute-based mutations, keep bucket internals intact.

[rakeshadr]

Since datanodeBuckets is concurrent hashmap, can we simplify the logic instead of synchronized bucket and avoids unnecessary null check?

    boolean added = addContainerToBucket(node.getUuid(), containerID);
    if (added && metrics != null) {
      metrics.incNumPendingContainersAdded();
    }

  private boolean addContainerToBucket(UUID uuid, ContainerID containerID) {
    AtomicBoolean added = new AtomicBoolean(false);
    datanodeBuckets.compute(uuid, (k, existing) -> {
      TwoWindowBucket bucket = (existing != null) ? existing : new TwoWindowBucket(rollIntervalMs);
      bucket.rollIfNeeded();
      added.set(bucket.add(containerID));
      LOG.debug("Recorded pending container {} on DataNode {}. Added={}, Total pending={}",
          containerID, uuid, added.get(), bucket.getCount());
      return bucket;
    });
    return added.get();
  }

[rakeshadr]

Important lock design principle:-

PendingContainerTracker: Please ensure no bucket -> map mutation path anymore. You need to switch remove/roll/clear to concurrenthashmap compute-based mutations, keep bucket internals intact.

[rakeshadr]

Please use CHM mutation, that will simplify it.

    UUID uuid = node.getUuid();
    datanodeBuckets.computeIfPresent(uuid, (k, bucket) -> {
      bucket.rollIfNeeded();
      return bucket.isEmpty() ? null : bucket;
    });

Important lock design principle:-

PendingContainerTracker: Please ensure no bucket -> map mutation path anymore. You need to switch remove/roll/clear to concurrenthashmap compute-based mutations, keep bucket internals intact.

[rakeshadr]

Important lock design principle:-

PendingContainerTracker: Please ensure no bucket -> map mutation path anymore. You need to switch remove/roll/clear to concurrenthashmap compute-based mutations, keep bucket internals intact.

[rakeshadr]

Please use CHM mutation, that will simplify it.

bucket.rollIfNeeded();
removed.set(bucket.remove(containerID));
remaining.set(bucket.getCount());
LOG.debug("Removed pending container {} from DataNode {}. Removed={}, Remaining={}",
    containerID, node.getUuidString(), removed.get(), remaining.get());
return bucket.isEmpty() ? null : bucket;
});
if (removed.get() && metrics != null) {
  metrics.incNumPendingContainersRemoved();
}

Important lock design principle:-

PendingContainerTracker: Please ensure no bucket -> map mutation path anymore. You need to switch remove/roll/clear to concurrenthashmap compute-based mutations, keep bucket internals intact.

[aswinshakil]

@rakeshadr @ashishkr200 pipelineManager.hasEnoughSpace is not a synchronized operation on the datanodes. We take lock on the pipeline synchronized (pipeline.getId()). Still two different pipelines can still allocate container on the same datanode.

We record the pending allocation on only Line 289. Until the code reaches that point multiple pipeline will be allocated to the same space on the datanode.

Let's say we only have 6GB space available in d1.
Pipeline 1: d1, d2, d3
Pipeline 2: d1, d4, d5

When we allocate container on both the pipeline, will get past pipelineManager.hasEnoughSpace and both will be allocated container 5GB *2 = 10GB. But we only have 6GB on d1

[ashishkumar50]

@aswinshakil Some allocation may happen extra but that will be taken care by PR as DN has more buffer to handle of these extra allocation. In my opinion synchronizing completely will be too costly here.

[ashishkumar50]

@rakeshadr @aswinshakil Fixed review comments.

[szetszwo]

@ashishkumar50 , thanks for working on this!

This change is quite big and complicated. Let's split it into multiple subtasks. The first one could be adding the new PendingContainerTracker class and the related test.

(Sorry for reviewing this late.)