diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java index 14aa7efdc38..7515d93a747 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java @@ -250,7 +250,6 @@ private boolean canBalancerStop() { @Override public void start() throws IllegalContainerBalancerStateException, InvalidContainerBalancerConfigurationException { - startedAt = OffsetDateTime.now(); lock.lock(); try { // should be leader-ready, out of safe mode, and not running already @@ -277,6 +276,9 @@ public void start() throws IllegalContainerBalancerStateException, ozoneConfiguration); validateConfiguration(configuration); this.config = configuration; + // Set startedAt only after validation has passed, so a rejected start + // does not overwrite the recorded start time of the previous run. + startedAt = OffsetDateTime.now(); startBalancingThread(proto.getNextIterationIndex(), true); } finally { lock.unlock(); @@ -298,7 +300,6 @@ public void start() throws IllegalContainerBalancerStateException, public void startBalancer(ContainerBalancerConfiguration configuration) throws IllegalContainerBalancerStateException, InvalidContainerBalancerConfigurationException, IOException { - startedAt = OffsetDateTime.now(); lock.lock(); try { // validates state, config, and then saves config @@ -307,6 +308,9 @@ public void startBalancer(ContainerBalancerConfiguration configuration) saveConfiguration(configuration, true, 0); this.config = configuration; + // Set startedAt only after validation has passed, so a rejected start + // does not overwrite the recorded start time of the previous run. + startedAt = OffsetDateTime.now(); //start balancing task startBalancingThread(0, false); } finally { diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java index 8931c9d6719..440cf590f02 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java @@ -38,6 +38,7 @@ import com.google.protobuf.ByteString; import java.io.IOException; +import java.time.OffsetDateTime; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -459,6 +460,55 @@ public void testRejectInvalidStartupConfiguration() throws Exception { assertSame(ContainerBalancerTask.Status.STOPPED, containerBalancer.getBalancerStatus()); } + /** + * Verifies that a start request rejected during validation does not update startedAt of last run. + * Prevents startedAt from advancing past stoppedAt, which previously caused a negative duration and CLI status crash. + */ + @Test + public void testRejectedStartDoesNotModifyStartedAt() throws Exception { + // The test first starts and immediately stops the balancer normally. This creates a realistic "previous state" + // in system memory and later trigger a bad request and verify that startedAt remains identical to startedAtBefore, + // rather than being wrongly overwritten with "now", which would make startedAt come after stoppedAt which resulted + // in a negative balancing duration. + startBalancer(balancerConfiguration); + assertSame(ContainerBalancerTask.Status.RUNNING, containerBalancer.getBalancerStatus()); + stopBalancer(); + assertSame(ContainerBalancerTask.Status.STOPPED, containerBalancer.getBalancerStatus()); + + ContainerBalancerStatusInfo before = containerBalancer.getBalancerStatusInfo(); + assertNotNull(before); + OffsetDateTime startedAtBefore = before.getStartedAt(); + OffsetDateTime stoppedAt = before.getStoppedAt(); + assertNotNull(startedAtBefore); + assertNotNull(stoppedAt); + + // Pause so that system time advances before the second start attempt so a timestamp overwrite becomes measurable. + Thread.sleep(20); + + // A start that is rejected during validation. Here we used an invalid config, this is just one of many rejection + // paths (safe mode, non-leader SCM, already running, etc.). + + balancerConfiguration.setMoveReplicationTimeout(TimeUnit.MINUTES.toMillis(60)); + balancerConfiguration.setMoveTimeout(TimeUnit.MINUTES.toMillis(59)); + assertThrows(InvalidContainerBalancerConfigurationException.class, + () -> containerBalancer.startBalancer(balancerConfiguration)); + + // The rejected start must leave the balancer STOPPED. + assertSame(ContainerBalancerTask.Status.STOPPED, containerBalancer.getBalancerStatus()); + + ContainerBalancerStatusInfo after = containerBalancer.getBalancerStatusInfo(); + assertNotNull(after); + + // startedAt must still equal the previous run's start time, a rejected start must not reset it. + assertEquals(startedAtBefore, after.getStartedAt(), + "Rejected start reset startedAt to 'now' (set before validation), replacing the previous run's start time"); + + // startedAt must never be after stoppedAt. Violating this is + // what produced the negative duration and the status --verbose crash. + assertFalse(after.getStartedAt().isAfter(stoppedAt), + "startedAt ended up after stoppedAt -> negative balancing duration that crashes 'status --verbose'"); + } + private static List createEligibleDatanodes(int count) { List datanodes = new ArrayList<>(count); for (int i = 0; i < count; i++) {