HDDS-13338. Move check for sufficient space in pipeline to allocateContainer (apache#9568)

Author: Russole

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java

@@ -237,6 +237,12 @@ private ContainerInfo createContainer(Pipeline pipeline, String owner)
   private ContainerInfo allocateContainer(final Pipeline pipeline,
                                           final String owner)
       throws IOException {
+    if (!pipelineManager.hasEnoughSpace(pipeline, maxContainerSize)) {
+      LOG.debug("Cannot allocate a new container because pipeline {} does not have the required space {}.",
+          pipeline, maxContainerSize);
+      return null;
+    }
+
     final long uniqueId = sequenceIdGen.getNextId(CONTAINER_ID);
     Preconditions.checkState(uniqueId > 0,
         "Cannot allocate container, negative container id" +
@@ -350,24 +356,17 @@ public ContainerInfo getMatchingContainer(final long size, final String owner,
       synchronized (pipeline.getId()) {
         containerIDs = getContainersForOwner(pipeline, owner);
         if (containerIDs.size() < pipelineManager.openContainerLimit(pipeline.getNodes())) {
-          if (pipelineManager.hasEnoughSpace(pipeline, maxContainerSize)) {
-            allocateContainer(pipeline, owner);
+          ContainerInfo allocated = allocateContainer(pipeline, owner);
+          if (allocated != null) {
+            // New container was created, refresh IDs so it becomes eligible.
             containerIDs = getContainersForOwner(pipeline, owner);
-          } else {
-            LOG.debug("Cannot allocate a new container because pipeline {} does not have the required space {}.",
-                pipeline, maxContainerSize);
           }
         }
         containerIDs.removeAll(excludedContainerIDs);
         containerInfo = containerStateManager.getMatchingContainer(
             size, owner, pipeline.getId(), containerIDs);
         if (containerInfo == null) {
-          if (pipelineManager.hasEnoughSpace(pipeline, maxContainerSize)) {
-            containerInfo = allocateContainer(pipeline, owner);
-          } else {
-            LOG.debug("Cannot allocate a new container because pipeline {} does not have the required space {}.",
-                pipeline, maxContainerSize);
-          }
+          containerInfo = allocateContainer(pipeline, owner);
         }
         return containerInfo;
       }

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java

@@ -99,9 +99,15 @@ void setUp() throws Exception {
     NodeManager nodeManager = new MockNodeManager(true, 10);
     sequenceIdGen = new SequenceIdGenerator(
         conf, scmhaManager, SCMDBDefinition.SEQUENCE_ID.getTable(dbStore));
-    pipelineManager = new MockPipelineManager(dbStore, scmhaManager, nodeManager);
+    PipelineManager base = new MockPipelineManager(dbStore, scmhaManager, nodeManager);
+    pipelineManager = spy(base);
+
+    // Default: allow allocation in tests unless a test overrides it.
+    doReturn(true).when(pipelineManager).hasEnoughSpace(any(Pipeline.class), anyLong());
+
     pipelineManager.createPipeline(RatisReplicationConfig.getInstance(
         ReplicationFactor.THREE));
+
     pendingOpsMock = mock(ContainerReplicaPendingOps.class);
     containerManager = new ContainerManagerImpl(conf,
         scmhaManager, sequenceIdGen, pipelineManager,
@@ -122,6 +128,8 @@ void testAllocateContainer() throws Exception {
     final ContainerInfo container = containerManager.allocateContainer(
         RatisReplicationConfig.getInstance(
             ReplicationFactor.THREE), "admin");
+
+    assertNotNull(container);
     assertEquals(1, containerManager.getContainers().size());
     assertNotNull(containerManager.getContainer(
         container.containerID()));
@@ -133,6 +141,8 @@ void testAllocateContainer() throws Exception {
    */
   @Test
   public void testGetMatchingContainerReturnsNullWhenNotEnoughSpaceInDatanodes() throws IOException {
+    doReturn(false).when(pipelineManager).hasEnoughSpace(any(), anyLong());
+
     long sizeRequired = 256 * 1024 * 1024; // 256 MB
     Pipeline pipeline = pipelineManager.getPipelines().iterator().next();
     // MockPipelineManager#hasEnoughSpace always returns false

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestContainerPlacement.java

@@ -24,7 +24,12 @@
 import static org.apache.hadoop.hdds.scm.net.NetConstants.ROOT_SCHEMA;
 import static org.apache.hadoop.hdds.upgrade.HDDSLayoutVersionManager.maxLayoutVersion;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
 import java.io.File;
@@ -154,6 +159,9 @@ SCMNodeManager createNodeManager(OzoneConfiguration config) {
 
   ContainerManager createContainerManager()
       throws IOException {
+    pipelineManager = spy(pipelineManager);
+    doReturn(true).when(pipelineManager).hasEnoughSpace(any(), anyLong());
+
     return new ContainerManagerImpl(conf,
         scmhaManager, sequenceIdGen, pipelineManager,
         SCMDBDefinition.CONTAINERS.getTable(dbStore),
@@ -224,6 +232,8 @@ public void testContainerPlacementCapacity() throws IOException,
                   SCMTestUtils.getReplicationType(conf),
                   SCMTestUtils.getReplicationFactor(conf)),
               OzoneConsts.OZONE);
+      assertNotNull(container, "allocateContainer returned null (unexpected in this placement test)");
+
       int replicaCount = 0;
       for (DatanodeDetails datanodeDetails : datanodes) {
         if (replicaCount ==