HDDS-10923. Container Scanner should still scan unhealthy containers.

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java

@@ -225,12 +225,6 @@ ContainerReplicaProto getContainerReport()
    */
   long getBlockCommitSequenceId();
 
-  /**
-   * Returns if the container metadata should be checked. The result depends
-   * on the state of the container.
-   */
-  boolean shouldScanMetadata();
-
   /**
    * check and report the structural integrity of the container.
    * @return true if the integrity checks pass

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java

@@ -933,18 +933,6 @@ public File getContainerDBFile() {
 
   }
 
-  @Override
-  public boolean shouldScanMetadata() {
-    boolean shouldScan =
-        getContainerState() != ContainerDataProto.State.UNHEALTHY;
-    if (!shouldScan && LOG.isDebugEnabled()) {
-      LOG.debug("Container {} in state {} should not have its metadata " +
-              "scanned.",
-          containerData.getContainerID(), containerData.getState());
-    }
-    return shouldScan;
-  }
-
   @Override
   public ScanResult scanMetaData() throws InterruptedException {
     long containerId = containerData.getContainerID();
@@ -958,7 +946,8 @@ public ScanResult scanMetaData() throws InterruptedException {
   public boolean shouldScanData() {
     boolean shouldScan =
         getContainerState() == ContainerDataProto.State.CLOSED
-        || getContainerState() == ContainerDataProto.State.QUASI_CLOSED;
+        || getContainerState() == ContainerDataProto.State.QUASI_CLOSED
+        || getContainerState() == ContainerDataProto.State.UNHEALTHY;
     if (!shouldScan && LOG.isDebugEnabled()) {
       LOG.debug("Container {} in state {} should not have its data scanned.",
           containerData.getContainerID(), containerData.getState());

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/BackgroundContainerDataScanner.java

@@ -98,8 +98,10 @@ public void scanContainer(Container<?> c)
     if (!result.isHealthy()) {
       LOG.error("Corruption detected in container [{}]. Marking it UNHEALTHY.",
           containerId, result.getException());
-      metrics.incNumUnHealthyContainers();
-      controller.markContainerUnhealthy(containerId, result);
+      boolean containerMarkedUnhealthy = controller.markContainerUnhealthy(containerId, result);
+      if (containerMarkedUnhealthy) {
+        metrics.incNumUnHealthyContainers();
+      }
     }
 
     metrics.incNumContainersScanned();

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/BackgroundContainerMetadataScanner.java

@@ -79,8 +79,10 @@ public void scanContainer(Container<?> container)
     if (!result.isHealthy()) {
       LOG.error("Corruption detected in container [{}]. Marking it UNHEALTHY.",
           containerID, result.getException());
-      metrics.incNumUnHealthyContainers();
-      controller.markContainerUnhealthy(containerID, result);
+      boolean containerMarkedUnhealthy = controller.markContainerUnhealthy(containerID, result);
+      if (containerMarkedUnhealthy) {
+        metrics.incNumUnHealthyContainers();
+      }
     }
 
     // Do not update the scan timestamp after the scan since this was just a
@@ -96,7 +98,6 @@ public ContainerMetadataScannerMetrics getMetrics() {
   private boolean shouldScan(Container<?> container) {
     // Full data scan also does a metadata scan. If a full data scan was done
     // recently, we can skip this metadata scan.
-    return container.shouldScanMetadata() &&
-        !ContainerUtils.recentlyScanned(container, minScanGap, LOG);
+    return !ContainerUtils.recentlyScanned(container, minScanGap, LOG);
   }
 }

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerController.java

@@ -114,15 +114,17 @@ public void markContainerForClose(final long containerId)
    * @param reason The reason the container was marked unhealthy
    * @throws IOException in case of exception
    */
-  public void markContainerUnhealthy(final long containerId, ScanResult reason)
+  public boolean markContainerUnhealthy(final long containerId, ScanResult reason)
           throws IOException {
-    Container container = containerSet.getContainer(containerId);
-    if (container != null) {
+    Container container = getContainer(containerId);
+    if (container != null && container.getContainerState() != State.UNHEALTHY) {
       getHandler(container).markContainerUnhealthy(container, reason);
+      return true;
     } else {
       LOG.warn("Container {} not found, may be deleted, skip mark UNHEALTHY",
           containerId);
     }
+    return false;
   }
 
   /**

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OnDemandContainerDataScanner.java

@@ -144,9 +144,11 @@ private static void performOnDemandScan(Container<?> container) {
       if (!result.isHealthy()) {
         LOG.error("Corruption detected in container [{}]." +
                 "Marking it UNHEALTHY.", containerId, result.getException());
-        instance.metrics.incNumUnHealthyContainers();
-        instance.containerController.markContainerUnhealthy(containerId,
-            result);
+        boolean containerMarkedUnhealthy = instance.containerController
+            .markContainerUnhealthy(containerId, result);
+        if (containerMarkedUnhealthy) {
+          instance.metrics.incNumUnHealthyContainers();
+        }
       }
 
       instance.metrics.incNumContainersScanned();

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java

@@ -214,7 +214,6 @@ public static void setupMockContainer(
     when(data.getContainerID()).thenReturn(containerIdSeq.getAndIncrement());
     when(c.getContainerData()).thenReturn(data);
     when(c.shouldScanData()).thenReturn(shouldScanData);
-    when(c.shouldScanMetadata()).thenReturn(true);
     when(c.getContainerData().getVolume()).thenReturn(vol);
 
     try {

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java

@@ -37,12 +37,12 @@
 
 import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.UNHEALTHY;
 import static org.apache.hadoop.ozone.container.common.ContainerTestUtils.getUnhealthyScanResult;
-import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.atMost;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.atMostOnce;
@@ -101,7 +101,7 @@ public void testScannerMetrics() {
     ContainerDataScannerMetrics metrics = scanner.getMetrics();
     assertEquals(1, metrics.getNumScanIterations());
     assertEquals(2, metrics.getNumContainersScanned());
-    assertEquals(1, metrics.getNumUnHealthyContainers());
+    assertEquals(0, metrics.getNumUnHealthyContainers());
   }
 
   @Test
@@ -154,12 +154,13 @@ public void testScanTimestampUpdated() throws Exception {
 
   @Test
   @Override
-  public void testUnhealthyContainerNotRescanned() throws Exception {
+  public void testUnhealthyContainerRescanned() throws Exception {
     Container<?> unhealthy = mockKeyValueContainer();
     when(unhealthy.scanMetaData()).thenReturn(ScanResult.healthy());
     when(unhealthy.scanData(any(DataTransferThrottler.class),
         any(Canceler.class))).thenReturn(getUnhealthyScanResult());
-
+    when(controller.markContainerUnhealthy(eq(unhealthy.getContainerData().getContainerID()),
+        any())).thenReturn(true);
     setContainers(unhealthy, healthy);
 
     // First iteration should find the unhealthy container.
@@ -175,20 +176,19 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
         .setState(UNHEALTHY);
     // Update the mock to reflect this.
     when(unhealthy.getContainerState()).thenReturn(UNHEALTHY);
-    assertFalse(unhealthy.shouldScanData());
-
-    // Clear metrics to check the next run.
-    metrics.resetNumContainersScanned();
-    metrics.resetNumUnhealthyContainers();
+    assertTrue(unhealthy.shouldScanData());
 
+    when(controller.markContainerUnhealthy(eq(unhealthy.getContainerData().getContainerID()),
+        any())).thenReturn(false);
     scanner.runIteration();
-    // The only invocation of unhealthy on this container should have been from
-    // the previous scan.
-    verifyContainerMarkedUnhealthy(unhealthy, atMostOnce());
-    // This iteration should skip the already unhealthy container.
+    // The invocation of unhealthy on this container will also happen in the
+    // next iteration.
+    verifyContainerMarkedUnhealthy(unhealthy, atMost(2));
+    // This iteration should scan the unhealthy container.
     assertEquals(2, metrics.getNumScanIterations());
-    assertEquals(1, metrics.getNumContainersScanned());
-    assertEquals(0, metrics.getNumUnHealthyContainers());
+    assertEquals(4, metrics.getNumContainersScanned());
+    // numUnHealthyContainers metrics is not incremented in the 2nd iteration.
+    assertEquals(1, metrics.getNumUnHealthyContainers());
   }
 
   /**

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerMetadataScanner.java

@@ -41,9 +41,10 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.atMost;
 import static org.mockito.Mockito.atMostOnce;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -100,7 +101,7 @@ public void testScannerMetrics() {
     ContainerMetadataScannerMetrics metrics = scanner.getMetrics();
     assertEquals(1, metrics.getNumScanIterations());
     assertEquals(4, metrics.getNumContainersScanned());
-    assertEquals(1, metrics.getNumUnHealthyContainers());
+    assertEquals(0, metrics.getNumUnHealthyContainers());
   }
 
   @Test
@@ -129,13 +130,14 @@ public void testUnhealthyContainersDetected() throws Exception {
 
   @Test
   @Override
-  public void testUnhealthyContainerNotRescanned() throws Exception {
+  public void testUnhealthyContainerRescanned() throws Exception {
     Container<?> unhealthy = mockKeyValueContainer();
     when(unhealthy.scanMetaData()).thenReturn(getUnhealthyScanResult());
     when(unhealthy.scanData(
         any(DataTransferThrottler.class), any(Canceler.class)))
         .thenReturn(ScanResult.healthy());
-
+    when(controller.markContainerUnhealthy(eq(unhealthy.getContainerData().getContainerID()),
+        any())).thenReturn(true);
     setContainers(unhealthy, healthy);
 
     // First iteration should find the unhealthy container.
@@ -151,20 +153,17 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
         .setState(UNHEALTHY);
     // Update the mock to reflect this.
     when(unhealthy.getContainerState()).thenReturn(UNHEALTHY);
-    assertFalse(unhealthy.shouldScanMetadata());
-
-    // Clear metrics to check the next run.
-    metrics.resetNumContainersScanned();
-    metrics.resetNumUnhealthyContainers();
-
+    when(controller.markContainerUnhealthy(eq(unhealthy.getContainerData().getContainerID()),
+        any())).thenReturn(false);
     scanner.runIteration();
-    // The only invocation of unhealthy on this container should have been from
-    // the previous scan.
-    verifyContainerMarkedUnhealthy(unhealthy, atMostOnce());
+    // The invocation of unhealthy on this container will also happen in the
+    // next iteration.
+    verifyContainerMarkedUnhealthy(unhealthy, atMost(2));
     // This iteration should skip the already unhealthy container.
     assertEquals(2, metrics.getNumScanIterations());
-    assertEquals(1, metrics.getNumContainersScanned());
-    assertEquals(0, metrics.getNumUnHealthyContainers());
+    assertEquals(4, metrics.getNumContainersScanned());
+    // numUnHealthyContainers metrics is not incremented in the 2nd iteration.
+    assertEquals(1, metrics.getNumUnHealthyContainers());
   }
 
   /**

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerScannersAbstract.java

@@ -122,7 +122,7 @@ public abstract void testPreviouslyScannedContainerIsScanned()
   public abstract void testShutdownDuringScan() throws Exception;
 
   @Test
-  public abstract void testUnhealthyContainerNotRescanned() throws Exception;
+  public abstract void testUnhealthyContainerRescanned() throws Exception;
 
   // HELPER METHODS
 
@@ -170,8 +170,6 @@ protected Container<?> mockKeyValueContainer() {
     // and test it.
     when(unhealthy.shouldScanData()).thenCallRealMethod();
     assertTrue(unhealthy.shouldScanData());
-    when(unhealthy.shouldScanMetadata()).thenCallRealMethod();
-    assertTrue(unhealthy.shouldScanMetadata());
 
     when(unhealthy.getContainerData().getVolume()).thenReturn(vol);
 

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java

@@ -47,6 +47,7 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.atMost;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.atMostOnce;
@@ -175,7 +176,7 @@ public void testScannerMetrics() throws Exception {
     OnDemandScannerMetrics metrics = OnDemandContainerDataScanner.getMetrics();
     //Containers with shouldScanData = false shouldn't increase
     // the number of scanned containers
-    assertEquals(1, metrics.getNumUnHealthyContainers());
+    assertEquals(0, metrics.getNumUnHealthyContainers());
     assertEquals(2, metrics.getNumContainersScanned());
   }
 
@@ -258,12 +259,14 @@ public void testShutdownDuringScan() throws Exception {
 
   @Test
   @Override
-  public void testUnhealthyContainerNotRescanned() throws Exception {
+  public void testUnhealthyContainerRescanned() throws Exception {
     Container<?> unhealthy = mockKeyValueContainer();
     when(unhealthy.scanMetaData()).thenReturn(ScanResult.healthy());
     when(unhealthy.scanData(
         any(DataTransferThrottler.class), any(Canceler.class)))
         .thenReturn(getUnhealthyScanResult());
+    when(controller.markContainerUnhealthy(eq(unhealthy.getContainerData().getContainerID()),
+        any())).thenReturn(true);
 
     // First iteration should find the unhealthy container.
     scanContainer(unhealthy);
@@ -277,19 +280,19 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
         .setState(UNHEALTHY);
     // Update the mock to reflect this.
     when(unhealthy.getContainerState()).thenReturn(UNHEALTHY);
-    assertFalse(unhealthy.shouldScanData());
-
-    // Clear metrics to check the next run.
-    metrics.resetNumContainersScanned();
-    metrics.resetNumUnhealthyContainers();
+    assertTrue(unhealthy.shouldScanData());
+    when(controller.markContainerUnhealthy(eq(unhealthy.getContainerData().getContainerID()),
+        any())).thenReturn(false);
 
+    // When rescanned the metrics should increase as we scan
+    // UNHEALTHY containers as well.
     scanContainer(unhealthy);
-    // The only invocation of unhealthy on this container should have been from
-    // the previous scan.
-    verifyContainerMarkedUnhealthy(unhealthy, atMostOnce());
-    // This iteration should skip the already unhealthy container.
-    assertEquals(0, metrics.getNumContainersScanned());
-    assertEquals(0, metrics.getNumUnHealthyContainers());
+    // The invocation of unhealthy on this container will also happen in the
+    // next iteration.
+    verifyContainerMarkedUnhealthy(unhealthy, atMost(2));
+    assertEquals(2, metrics.getNumContainersScanned());
+    // numUnHealthyContainers metrics is not incremented in the 2nd iteration.
+    assertEquals(1, metrics.getNumUnHealthyContainers());
   }
 
   private void scanContainer(Container<?> container) throws Exception {