Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
errose28 merged 3 commits into from
Jun 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90987b9
HDDS-10923. Container Scanner should still scan unhealthy containers.
aswinshakil Jun 12, 2024
3bb03a5
HDDS-10923. Update Tests.
aswinshakil Jun 21, 2024
dd55fc1
HDDS-10923. Fix unhealthy container count and update Tests.
aswinshakil Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions ...-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 2 additions & 13 deletions ...r-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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());
Expand Down
3 changes: 1 addition & 2 deletions .../java/org/apache/hadoop/ozone/container/ozoneimpl/BackgroundContainerMetadataScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,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);
}
}
1 change: 0 additions & 1 deletion ...er-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
14 changes: 7 additions & 7 deletions .../java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@

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;
Expand All @@ -49,6 +48,7 @@
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.internal.verification.VerificationModeFactory.atMost;

/**
* Unit tests for the background container data scanner.
Expand Down Expand Up @@ -154,7 +154,7 @@ 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),
Expand All @@ -175,7 +175,7 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
.setState(UNHEALTHY);
// Update the mock to reflect this.
when(unhealthy.getContainerState()).thenReturn(UNHEALTHY);
assertFalse(unhealthy.shouldScanData());
assertTrue(unhealthy.shouldScanData());

// Clear metrics to check the next run.
metrics.resetNumContainersScanned();
Expand All @@ -184,11 +184,11 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
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.
verifyContainerMarkedUnhealthy(unhealthy, atMost(2));
// This iteration should scan the unhealthy container.
assertEquals(2, metrics.getNumScanIterations());
assertEquals(1, metrics.getNumContainersScanned());
assertEquals(0, metrics.getNumUnHealthyContainers());
assertEquals(2, metrics.getNumContainersScanned());
assertEquals(1, metrics.getNumUnHealthyContainers());
}

/**
Expand Down
11 changes: 5 additions & 6 deletions ...a/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerMetadataScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@
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.Mockito.any;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.atMostOnce;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.internal.verification.VerificationModeFactory.atMost;

/**
* Unit tests for the background container metadata scanner.
Expand Down Expand Up @@ -129,7 +129,7 @@ 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(
Expand All @@ -151,7 +151,6 @@ 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();
Expand All @@ -160,11 +159,11 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
scanner.runIteration();
// The only invocation of unhealthy on this container should have been from
// the previous scan.
verifyContainerMarkedUnhealthy(unhealthy, atMostOnce());
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(2, metrics.getNumContainersScanned());
assertEquals(1, metrics.getNumUnHealthyContainers());
}

/**
Expand Down
4 changes: 1 addition & 3 deletions .../test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerScannersAbstract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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);

Expand Down
16 changes: 8 additions & 8 deletions ...st/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;
import static org.mockito.internal.verification.VerificationModeFactory.atMost;

/**
* Unit tests for the on-demand container scanner.
Expand Down Expand Up @@ -258,7 +259,7 @@ 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(
Expand All @@ -277,19 +278,18 @@ public void testUnhealthyContainerNotRescanned() throws Exception {
.setState(UNHEALTHY);
// Update the mock to reflect this.
when(unhealthy.getContainerState()).thenReturn(UNHEALTHY);
assertFalse(unhealthy.shouldScanData());
assertTrue(unhealthy.shouldScanData());

// Clear metrics to check the next run.
metrics.resetNumContainersScanned();
metrics.resetNumUnhealthyContainers();

// 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());
verifyContainerMarkedUnhealthy(unhealthy, atMost(2));
assertEquals(1, metrics.getNumContainersScanned());
assertEquals(1, metrics.getNumUnHealthyContainers());
}

private void scanContainer(Container<?> container) throws Exception {
Expand Down