rakeshadr commented on code in PR #10054:
URL: https://github.com/apache/ozone/pull/10054#discussion_r3158877849
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/AvailableSpaceFilter.java:
##########
@@ -39,9 +42,25 @@ public AvailableSpaceFilter(long requiredSpace) {
@Override
public boolean test(HddsVolume vol) {
StorageLocationReport report = vol.getReport();
- long available = report.getUsableSpace();
+ long capacity = report.getCapacity();
+ long spareAtHardLimit = vol.getFreeSpaceToSpare(capacity);
Review Comment:
What if a user wants to completely turn off the soft limit space?
Say, if I keep the soft limit value same as hard limit. Behavior is
identical to the old code, right?
Can we change it to ->
`long availableAtReportedSpare = report.getUsableSpace();`
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -285,25 +290,36 @@ public class DatanodeConfiguration extends
ReconfigurableConfig {
defaultValue = "-1",
type = ConfigType.SIZE,
tags = { OZONE, CONTAINER, STORAGE, MANAGEMENT },
- description = "This determines the free space to be used for closing
containers" +
- " When the difference between volume capacity and used reaches this
number," +
- " containers that reside on this volume will be closed and no new
containers" +
- " would be allocated on this volume." +
- " Max of min.free.space and min.free.space.percent will be used as
final value."
+ description = "Minimum free space (bytes) applied together with
min.free.space.percent "
+ + "(reported to SCM in heartbeat as freeSpaceToSpare) and "
+ + "min.free.space.hard.limit.percent (local write enforcement). "
+ + "The effective value for each tier is max(this bytes, capacity *
ratio)."
)
private long minFreeSpace = getDefaultFreeSpace();
@Config(key = "hdds.datanode.volume.min.free.space.percent",
defaultValue = "0.02", // match
HDDS_DATANODE_VOLUME_MIN_FREE_SPACE_PERCENT_DEFAULT
type = ConfigType.FLOAT,
tags = { OZONE, CONTAINER, STORAGE, MANAGEMENT },
- description = "This determines the free space percent to be used for
closing containers" +
- " When the difference between volume capacity and used reaches
(free.space.percent of volume capacity)," +
- " containers that reside on this volume will be closed and no new
containers" +
- " would be allocated on this volume." +
- " Max of min.free.space or min.free.space.percent will be used as
final value."
+ description = "Minimum fraction of volume capacity reported to SCM as
freeSpaceToSpare "
+ + "(heartbeat / storage reports). Local write rejection uses "
+ + "hdds.datanode.volume.min.free.space.hard.limit.percent instead. "
+ + "The soft band is the gap between these two (e.g. 2000GB disk: 2%
= 40GB reported vs "
+ + "1.5% = 30GB hard → 10GB band) where the DN may send
close-container actions while "
+ + "writes still succeed."
)
private float minFreeSpaceRatio =
HDDS_DATANODE_VOLUME_MIN_FREE_SPACE_PERCENT_DEFAULT;
+ @Config(key = "hdds.datanode.volume.min.free.space.hard.limit.percent",
+ defaultValue = "0.015",
+ type = ConfigType.FLOAT,
+ tags = { OZONE, CONTAINER, STORAGE, MANAGEMENT },
+ description = "Minimum fraction of volume capacity reserved for local
enforcement: "
Review Comment:
I could see existing docs describing the min free space calculation and
behavior. Could you please go through it and would be good to describe the
`Soft and Hard Min-Free-Space Limits` and how does it help to the users/admins
- "improves write continuity near disk capacity limits".
https://github.com/apache/ozone/blob/master/hadoop-hdds/docs/content/design/dn-min-space-configuration.md
https://github.com/apache/ozone/blob/master/hadoop-hdds/docs/content/design/full-volume-handling.md
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestAvailableSpaceFilter.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.volume;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AvailableSpaceFilter}.
+ */
+public class TestAvailableSpaceFilter {
+
+ @Test
+ public void testIncrementsSoftBandWhenBetweenReportedAndHard() {
+ HddsVolume volume = mock(HddsVolume.class);
+ VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
+ StorageLocationReport report = mock(StorageLocationReport.class);
+ when(volume.getReport()).thenReturn(report);
+ when(report.getCapacity()).thenReturn(1000L);
+ when(report.getRemaining()).thenReturn(100L);
+ when(report.getCommitted()).thenReturn(0L);
+ when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
+ when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
+ when(volume.getVolumeInfoStats()).thenReturn(metrics);
+
+ AvailableSpaceFilter filter = new AvailableSpaceFilter(50L);
+ assertTrue(filter.test(volume));
+
+ verify(metrics).incNumContainerCreateRequestsInSoftBandMinFreeSpace();
+ verify(metrics,
never()).incNumContainerCreateRequestsRejectedHardMinFreeSpace();
+ }
+
+ @Test
+ public void testIncrementsHardRejectWhenHardLimitViolated() {
+ HddsVolume volume = mock(HddsVolume.class);
+ VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
+ StorageLocationReport report = mock(StorageLocationReport.class);
+ when(volume.getReport()).thenReturn(report);
+ when(report.getCapacity()).thenReturn(1000L);
+ when(report.getRemaining()).thenReturn(100L);
+ when(report.getCommitted()).thenReturn(0L);
+ when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
+ when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
+ when(volume.getVolumeInfoStats()).thenReturn(metrics);
+
+ AvailableSpaceFilter filter = new AvailableSpaceFilter(80L);
+ assertFalse(filter.test(volume));
+
+ verify(metrics).incNumContainerCreateRequestsRejectedHardMinFreeSpace();
+ verify(metrics,
never()).incNumContainerCreateRequestsInSoftBandMinFreeSpace();
+ }
+
+ @Test
+ public void testNoMetricIncrementWhenWellAboveSoftBand() {
+ HddsVolume volume = mock(HddsVolume.class);
+ VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
+ StorageLocationReport report = mock(StorageLocationReport.class);
+ when(volume.getReport()).thenReturn(report);
+ when(report.getCapacity()).thenReturn(1000L);
+ when(report.getRemaining()).thenReturn(1000L);
+ when(report.getCommitted()).thenReturn(0L);
+ when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
+ when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
+ when(volume.getVolumeInfoStats()).thenReturn(metrics);
+
+ AvailableSpaceFilter filter = new AvailableSpaceFilter(50L);
+ assertTrue(filter.test(volume));
+
+ verify(metrics,
never()).getNumContainerCreateRequestsInSoftBandMinFreeSpace();
+ verify(metrics, never()).incNumWriteRequestsRejectedHardMinFreeSpace();
Review Comment:
Can you double check the assertions. Should it be like this?
```
// remaining(1000) - committed(0) - hardSpare(30) = 970 > 50: well above
both limits
verify(metrics,
never()).incNumContainerCreateRequestsInSoftBandMinFreeSpace();
verify(metrics,
never()).incNumContainerCreateRequestsRejectedHardMinFreeSpace();
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/helpers/TestContainerUtils.java:
##########
@@ -227,4 +234,41 @@ private static void assertDetailsEquals(DatanodeDetails
expected,
assertEquals(expected.getInitialVersion(), actual.getInitialVersion());
assertEquals(expected.getIpAddress(), actual.getIpAddress());
}
+
+ @Test
+ public void
assertSpaceAvailabilityIncrementsSoftBandWhenBetweenReportedAndHard()
+ throws Exception {
+ HddsVolume volume = mock(HddsVolume.class);
+ VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
+ SpaceUsageSource.Fixed usage = new SpaceUsageSource.Fixed(1000L, 100L,
900L);
+ when(volume.getCurrentUsage()).thenReturn(usage);
+ when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
+ when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
+ when(volume.getVolumeInfoStats()).thenReturn(metrics);
+ when(volume.toString()).thenReturn("mockVolume");
+
+ ContainerUtils.assertSpaceAvailability(1L, volume, 50);
+
+ verify(metrics).incNumWriteRequestsInSoftBandMinFreeSpace();
+ verify(metrics, never()).incNumWriteRequestsRejectedHardMinFreeSpace();
+ }
+
+ @Test
+ public void
assertSpaceAvailabilityIncrementsHardRejectWhenHardLimitViolated() {
+ HddsVolume volume = mock(HddsVolume.class);
+ VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
+ SpaceUsageSource.Fixed usage = new SpaceUsageSource.Fixed(1000L, 100L,
900L);
+ when(volume.getCurrentUsage()).thenReturn(usage);
+ when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
+ when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
+ when(volume.getVolumeInfoStats()).thenReturn(metrics);
+ when(volume.toString()).thenReturn("mockVolume");
+
+ StorageContainerException ex =
assertThrows(StorageContainerException.class,
+ () -> ContainerUtils.assertSpaceAvailability(1L, volume, 80));
+ assertEquals(Result.DISK_OUT_OF_SPACE, ex.getResult());
+
+ verify(metrics).incNumWriteRequestsRejectedHardMinFreeSpace();
+ verify(metrics, never()).incNumWriteRequestsInSoftBandMinFreeSpace();
+ }
Review Comment:
Can we add a few more tests to cover boundary/edge cases. Probably, you can
refer below cursor-generated tests.
```
/**
* available(100) - hardSpare(30) = 70 == sizeRequested(70).
* The check is strict less-than, so the write passes at the exact
boundary.
* The volume is still inside the soft band (available - softSpare = 0 <
70),
* so the soft-band metric fires.
*/
@Test
public void assertSpaceAvailabilityPassesAtExactHardBoundary() throws
Exception {
HddsVolume volume = mock(HddsVolume.class);
VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
SpaceUsageSource.Fixed usage = new SpaceUsageSource.Fixed(1000L, 100L,
900L);
when(volume.getCurrentUsage()).thenReturn(usage);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(metrics);
when(volume.toString()).thenReturn("mockVolume");
// available(100) - hardSpare(30) = 70 == sizeRequested → passes (< not
<=)
assertDoesNotThrow(() -> ContainerUtils.assertSpaceAvailability(1L,
volume, 70));
verify(metrics).incNumWriteRequestsInSoftBandMinFreeSpace();
verify(metrics, never()).incNumWriteRequestsRejectedHardMinFreeSpace();
}
/**
* available(100) - hardSpare(30) = 70 < sizeRequested(71).
* One byte past the hard boundary; write must be rejected.
*/
@Test
public void assertSpaceAvailabilityRejectsOneByteOverHardBoundary() {
HddsVolume volume = mock(HddsVolume.class);
VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
SpaceUsageSource.Fixed usage = new SpaceUsageSource.Fixed(1000L, 100L,
900L);
when(volume.getCurrentUsage()).thenReturn(usage);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(metrics);
when(volume.toString()).thenReturn("mockVolume");
// available(100) - hardSpare(30) = 70 < 71 → rejected
StorageContainerException ex =
assertThrows(StorageContainerException.class,
() -> ContainerUtils.assertSpaceAvailability(1L, volume, 71));
assertEquals(Result.DISK_OUT_OF_SPACE, ex.getResult());
verify(metrics).incNumWriteRequestsRejectedHardMinFreeSpace();
verify(metrics, never()).incNumWriteRequestsInSoftBandMinFreeSpace();
}
/**
* available(200) is well above both soft(100) and hard(30) spares.
* available - hardSpare = 170 > sizeRequested(50): write passes.
* available - softSpare = 100 > sizeRequested(50): not in soft band.
* Neither metric should fire.
*/
@Test
public void assertSpaceAvailabilityFiresNoMetricWhenWellAboveBothLimits()
throws Exception {
HddsVolume volume = mock(HddsVolume.class);
VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
SpaceUsageSource.Fixed usage = new SpaceUsageSource.Fixed(1000L, 200L,
800L);
when(volume.getCurrentUsage()).thenReturn(usage);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(metrics);
when(volume.toString()).thenReturn("mockVolume");
// available(200) - hardSpare(30) = 170 > 50, and 200 - softSpare(100) =
100 > 50
ContainerUtils.assertSpaceAvailability(1L, volume, 50);
verify(metrics, never()).incNumWriteRequestsInSoftBandMinFreeSpace();
verify(metrics, never()).incNumWriteRequestsRejectedHardMinFreeSpace();
}
/**
* When VolumeInfoMetrics is null (volume not yet initialised or metrics
disabled),
* assertSpaceAvailability must not throw NullPointerException.
*/
@Test
public void assertSpaceAvailabilityHandlesNullMetrics() throws Exception {
HddsVolume volume = mock(HddsVolume.class);
SpaceUsageSource.Fixed usage = new SpaceUsageSource.Fixed(1000L, 100L,
900L);
when(volume.getCurrentUsage()).thenReturn(usage);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(null);
when(volume.toString()).thenReturn("mockVolume");
assertDoesNotThrow(() -> ContainerUtils.assertSpaceAvailability(1L,
volume, 50));
}
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestAvailableSpaceFilter.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.common.volume;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AvailableSpaceFilter}.
+ */
+public class TestAvailableSpaceFilter {
+
+ @Test
+ public void testIncrementsSoftBandWhenBetweenReportedAndHard() {
+ HddsVolume volume = mock(HddsVolume.class);
+ VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
+ StorageLocationReport report = mock(StorageLocationReport.class);
+ when(volume.getReport()).thenReturn(report);
+ when(report.getCapacity()).thenReturn(1000L);
+ when(report.getRemaining()).thenReturn(100L);
+ when(report.getCommitted()).thenReturn(0L);
Review Comment:
Missing Scenario: non-zero committed bytes AvailableSpaceFilter subtracts
report.getCommitted() but every test sets it to 0L. No test exercises the
committed-bytes path.
How about adding below tests.
Note: Auto-generated via cursor.
```
/**
* Without committed bytes: remaining(200) - hardSpare(30) = 170 >
requiredSpace(60),
* and 200 - softSpare(100) = 100 > 60 → well above both limits, no metric.
* With committed(80): 200 - 80 - hardSpare(30) = 90 > 60 → still passes
hard,
* but 200 - 80 - softSpare(100) = 20 ≤ 60 → now inside the soft band.
* Committed bytes representing in-flight pipeline allocations push the
volume into
* the soft band even though raw remaining space looks healthy.
*/
@Test
public void testCommittedBytesCanPushVolumeIntoSoftBand() {
HddsVolume volume = mock(HddsVolume.class);
VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
StorageLocationReport report = mock(StorageLocationReport.class);
when(volume.getReport()).thenReturn(report);
when(report.getCapacity()).thenReturn(1000L);
when(report.getRemaining()).thenReturn(200L);
when(report.getCommitted()).thenReturn(80L);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(metrics);
// available = 200 - 80 - 30 = 90 > requiredSpace(60) → passes hard check
// availableAtReported = 200 - 80 - 100 = 20 <= 60 → inside soft band
AvailableSpaceFilter filter = new AvailableSpaceFilter(60L);
assertTrue(filter.test(volume));
verify(metrics).incNumContainerCreateRequestsInSoftBandMinFreeSpace();
verify(metrics,
never()).incNumContainerCreateRequestsRejectedHardMinFreeSpace();
}
/**
* Committed bytes representing in-flight pipeline allocations cause a
hard reject
* that would not occur if committed were zero.
* remaining(130) - committed(80) - hardSpare(30) = 20 < requiredSpace(50)
→ rejected.
* Without committed: 130 - 0 - 30 = 100 > 50 → would have passed.
*/
@Test
public void testCommittedBytesCanCauseHardReject() {
HddsVolume volume = mock(HddsVolume.class);
VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
StorageLocationReport report = mock(StorageLocationReport.class);
when(volume.getReport()).thenReturn(report);
when(report.getCapacity()).thenReturn(1000L);
when(report.getRemaining()).thenReturn(130L);
when(report.getCommitted()).thenReturn(80L);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(metrics);
// available = 130 - 80 - 30 = 20 < requiredSpace(50) → hard rejected
AvailableSpaceFilter filter = new AvailableSpaceFilter(50L);
assertFalse(filter.test(volume));
verify(metrics).incNumContainerCreateRequestsRejectedHardMinFreeSpace();
verify(metrics,
never()).incNumContainerCreateRequestsInSoftBandMinFreeSpace();
}
/**
* Even with non-zero committed bytes, if remaining space is large enough,
* the volume remains well above both limits and no metric fires.
* remaining(300) - committed(50) - hardSpare(30) = 220 >
requiredSpace(50): passes hard.
* 300 - 50 - softSpare(100) = 150 > 50: not in soft band.
*/
@Test
public void testCommittedBytesDoNotAffectMetricsWhenVolumeStillHealthy() {
HddsVolume volume = mock(HddsVolume.class);
VolumeInfoMetrics metrics = mock(VolumeInfoMetrics.class);
StorageLocationReport report = mock(StorageLocationReport.class);
when(volume.getReport()).thenReturn(report);
when(report.getCapacity()).thenReturn(1000L);
when(report.getRemaining()).thenReturn(300L);
when(report.getCommitted()).thenReturn(50L);
when(volume.getFreeSpaceToSpare(1000L)).thenReturn(30L);
when(volume.getReportedFreeSpaceToSpare(1000L)).thenReturn(100L);
when(volume.getVolumeInfoStats()).thenReturn(metrics);
// available = 300 - 50 - 30 = 220 > 50, availableAtReported = 300 - 50
- 100 = 150 > 50
AvailableSpaceFilter filter = new AvailableSpaceFilter(50L);
assertTrue(filter.test(volume));
verify(metrics,
never()).incNumContainerCreateRequestsInSoftBandMinFreeSpace();
verify(metrics,
never()).incNumContainerCreateRequestsRejectedHardMinFreeSpace();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
