From ba46197c9532818472d93fa06160636beea80b59 Mon Sep 17 00:00:00 2001 From: Siddhant Sangwan Date: Thu, 29 Aug 2024 18:56:22 +0530 Subject: [PATCH 1/2] HDDS-11350. NullPointerException thrown on checking container balancer status --- .../balancer/ContainerBalancerTask.java | 16 ++++++++--- .../scm/container/balancer/MockedSCM.java | 2 +- .../TestContainerBalancerStatusInfo.java | 28 +++++++++++++++++++ 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java index 7fea44671ffa..3efbce6a7401 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java @@ -341,6 +341,16 @@ public List getCurrentIterationsStatis .max() .orElse(0); + // the balancing thread could go to sleep without having initialized findTargetStrategy, so we check for null here + Map sizeEnteringNodes = Collections.emptyMap(); + if (findTargetStrategy != null) { + sizeEnteringNodes = findTargetStrategy.getSizeEnteringNodes(); + } + Map sizeLeavingNodes = Collections.emptyMap(); + if (findSourceStrategy != null) { + sizeLeavingNodes = findSourceStrategy.getSizeLeavingNodes(); + } + ContainerBalancerTaskIterationStatusInfo currentIterationStatistic = new ContainerBalancerTaskIterationStatusInfo( lastIterationNumber + 1, null, @@ -350,8 +360,7 @@ public List getCurrentIterationsStatis metrics.getNumContainerMovesCompletedInLatestIteration(), metrics.getNumContainerMovesFailedInLatestIteration(), metrics.getNumContainerMovesTimeoutInLatestIteration(), - findTargetStrategy.getSizeEnteringNodes() - .entrySet() + sizeEnteringNodes.entrySet() .stream() .filter(Objects::nonNull) .filter(datanodeDetailsLongEntry -> datanodeDetailsLongEntry.getValue() > 0) @@ -360,8 +369,7 @@ public List getCurrentIterationsStatis entry -> entry.getValue() / OzoneConsts.GB ) ), - findSourceStrategy.getSizeLeavingNodes() - .entrySet() + sizeLeavingNodes.entrySet() .stream() .filter(Objects::nonNull) .filter(datanodeDetailsLongEntry -> datanodeDetailsLongEntry.getValue() > 0) diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/MockedSCM.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/MockedSCM.java index a3ec55d58639..0972e57df64d 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/MockedSCM.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/MockedSCM.java @@ -86,7 +86,7 @@ public MockedSCM(@Nonnull TestableCluster testableCluster) { } } - private void init(@Nonnull ContainerBalancerConfiguration balancerConfig, @Nonnull OzoneConfiguration ozoneCfg) { + void init(@Nonnull ContainerBalancerConfiguration balancerConfig, @Nonnull OzoneConfiguration ozoneCfg) { ozoneCfg.setFromObject(balancerConfig); try { doMock(balancerConfig, ozoneCfg); diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerStatusInfo.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerStatusInfo.java index b8ac648e8442..48b3ee2d0de5 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerStatusInfo.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerStatusInfo.java @@ -19,7 +19,9 @@ package org.apache.hadoop.hdds.scm.container.balancer; import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.scm.server.StorageContainerManager; import org.apache.hadoop.ozone.OzoneConsts; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import java.util.List; @@ -55,4 +57,30 @@ void testGetIterationStatistics() { }); } + + /** + * @see HDDS-11350 + */ + @Test + void testGetCurrentIterationsStatisticDoesNotThrowNullPointerExceptionWhenBalancingThreadIsSleeping() { + MockedSCM mockedScm = new MockedSCM(new TestableCluster(10, OzoneConsts.GB)); + OzoneConfiguration ozoneConfig = new OzoneConfiguration(); + ContainerBalancerConfiguration config = ozoneConfig.getObject(ContainerBalancerConfiguration.class); + + config.setIterations(2); + // the following config makes the balancing thread go to sleep while waiting for DU to be triggered in DNs and + // updated storage reports to arrive via DN heartbeats - of course, this is a unit test and NodeManager, DNs etc. + // are all mocked + config.setTriggerDuEnable(true); + mockedScm.init(config, ozoneConfig); + + // run ContainerBalancerTask in a new thread and have the current thread call getCurrentIterationsStatistic + StorageContainerManager scm = mockedScm.getStorageContainerManager(); + ContainerBalancer cb = new ContainerBalancer(scm); + ContainerBalancerTask task = new ContainerBalancerTask(scm, 0, cb, cb.getMetrics(), config, false); + Thread thread = new Thread(task); + thread.setDaemon(true); + thread.start(); + Assertions.assertDoesNotThrow(task::getCurrentIterationsStatistic); + } } From 34cb6fe02a51fd86ac1ae2971926cc5b3860f205 Mon Sep 17 00:00:00 2001 From: Siddhant Sangwan Date: Tue, 3 Sep 2024 16:58:12 +0530 Subject: [PATCH 2/2] initialise findTargetStrategy in the constructor instead of null-check --- .../balancer/ContainerBalancerTask.java | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java index 3efbce6a7401..19a2f3c2e621 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java @@ -158,6 +158,14 @@ public ContainerBalancerTask(StorageContainerManager scm, this.selectedSources = new HashSet<>(); this.selectedTargets = new HashSet<>(); findSourceStrategy = new FindSourceGreedy(nodeManager); + if (config.getNetworkTopologyEnable()) { + findTargetStrategy = new FindTargetGreedyByNetworkTopology( + containerManager, placementPolicyValidateProxy, + nodeManager, networkTopology); + } else { + findTargetStrategy = new FindTargetGreedyByUsageInfo(containerManager, + placementPolicyValidateProxy, nodeManager); + } this.iterationsStatistic = new ArrayList<>(); } @@ -341,16 +349,6 @@ public List getCurrentIterationsStatis .max() .orElse(0); - // the balancing thread could go to sleep without having initialized findTargetStrategy, so we check for null here - Map sizeEnteringNodes = Collections.emptyMap(); - if (findTargetStrategy != null) { - sizeEnteringNodes = findTargetStrategy.getSizeEnteringNodes(); - } - Map sizeLeavingNodes = Collections.emptyMap(); - if (findSourceStrategy != null) { - sizeLeavingNodes = findSourceStrategy.getSizeLeavingNodes(); - } - ContainerBalancerTaskIterationStatusInfo currentIterationStatistic = new ContainerBalancerTaskIterationStatusInfo( lastIterationNumber + 1, null, @@ -360,7 +358,8 @@ public List getCurrentIterationsStatis metrics.getNumContainerMovesCompletedInLatestIteration(), metrics.getNumContainerMovesFailedInLatestIteration(), metrics.getNumContainerMovesTimeoutInLatestIteration(), - sizeEnteringNodes.entrySet() + findTargetStrategy.getSizeEnteringNodes() + .entrySet() .stream() .filter(Objects::nonNull) .filter(datanodeDetailsLongEntry -> datanodeDetailsLongEntry.getValue() > 0) @@ -369,7 +368,8 @@ public List getCurrentIterationsStatis entry -> entry.getValue() / OzoneConsts.GB ) ), - sizeLeavingNodes.entrySet() + findSourceStrategy.getSizeLeavingNodes() + .entrySet() .stream() .filter(Objects::nonNull) .filter(datanodeDetailsLongEntry -> datanodeDetailsLongEntry.getValue() > 0) @@ -440,14 +440,7 @@ private boolean initializeIteration() { this.maxDatanodesRatioToInvolvePerIteration = config.getMaxDatanodesRatioToInvolvePerIteration(); this.maxSizeToMovePerIteration = config.getMaxSizeToMovePerIteration(); - if (config.getNetworkTopologyEnable()) { - findTargetStrategy = new FindTargetGreedyByNetworkTopology( - containerManager, placementPolicyValidateProxy, - nodeManager, networkTopology); - } else { - findTargetStrategy = new FindTargetGreedyByUsageInfo(containerManager, - placementPolicyValidateProxy, nodeManager); - } + this.excludeNodes = config.getExcludeNodes(); this.includeNodes = config.getIncludeNodes(); // include/exclude nodes from balancing according to configs