From cdc3f4e800c837813adbb43182c3bf681967ff0d Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 19 Apr 2024 13:47:17 -0700 Subject: [PATCH 1/5] Add and test new default works --- .../apache/hadoop/hdds/scm/ScmConfigKeys.java | 2 +- .../container/common/volume/VolumeUsage.java | 19 ++++++++-------- .../volume/TestReservedVolumeSpace.java | 22 +++++++++++++++++++ 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java index caf0127d2138..1d175197b1cd 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java @@ -223,7 +223,7 @@ public final class ScmConfigKeys { "hdds.datanode.dir.du.reserved"; public static final String HDDS_DATANODE_DIR_DU_RESERVED_PERCENT = "hdds.datanode.dir.du.reserved.percent"; - public static final float HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT = 0; + public static final float HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT = 0.0001f; public static final String OZONE_SCM_HANDLER_COUNT_KEY = "ozone.scm.handler.count.key"; public static final String OZONE_SCM_CLIENT_HANDLER_COUNT_KEY = diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java index be86cdaeadf5..6bad348b7d41 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java @@ -213,17 +213,16 @@ private static long getReserved(ConfigurationSource conf, String rootDir, } // 2. If hdds.datanode.dir.du.reserved not set and - // hdds.datanode.dir.du.reserved.percent is set, fall back to this config. - if (conf.isConfigured(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT)) { - float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, - HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); - if (0 <= percentage && percentage <= 1) { - return (long) Math.ceil(capacity * percentage); - } - //If it comes here then the percentage is not between 0-1. - LOG.error("The value of {} should be between 0 to 1. Defaulting to 0.", - HDDS_DATANODE_DIR_DU_RESERVED_PERCENT); + // then fall back to hdds.datanode.dir.du.reserved.percent, using either its set value or default value if it has + // not been set. + float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); + if (0 <= percentage && percentage <= 1) { + return (long) Math.ceil(capacity * percentage); } + // If it comes here then the percentage is not between 0-1. + LOG.error("The value of {} should be between 0 to 1. Defaulting to 0.", + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT); //Both configs are not set, return 0. return 0; diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java index 2a7cae57dbf9..789af009a1f0 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java @@ -52,6 +52,28 @@ public void setup() throws Exception { .usageCheckFactory(MockSpaceUsageCheckFactory.NONE); } + @Test + public void testDefaultConfig() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); + float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); + assertEquals(percentage, HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); + + // Gets the total capacity reported by Ozone, which may be limited to less than the volume's real capacity by the + // DU reserved configurations. + long volumeCapacity = hddsVolume.getCapacity(); + VolumeUsage usage = hddsVolume.getVolumeInfo().get().getUsageForTesting(); + + // Gets the actual total capacity without accounting for DU reserved space configurations. + long totalCapacity = usage.realUsage().getCapacity(); + long reservedCapacity = usage.getReservedBytes(); + long expectedReservedCapacity = (long) Math.ceil(totalCapacity * percentage); + + assertEquals(expectedReservedCapacity, reservedCapacity); + assertEquals(totalCapacity - reservedCapacity, volumeCapacity); + } + /** * Test reserved capacity with respect to the percentage of actual capacity. * @throws Exception From e0ed6c77104e2b22c01e1537427b68471d491791 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 19 Apr 2024 14:05:36 -0700 Subject: [PATCH 2/5] Update comment --- .../hadoop/ozone/container/common/volume/VolumeUsage.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java index 6bad348b7d41..4ace93cad9a3 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java @@ -212,9 +212,8 @@ private static long getReserved(ConfigurationSource conf, String rootDir, } } - // 2. If hdds.datanode.dir.du.reserved not set and - // then fall back to hdds.datanode.dir.du.reserved.percent, using either its set value or default value if it has - // not been set. + // 2. If hdds.datanode.dir.du.reserved not set then fall back to hdds.datanode.dir.du.reserved.percent, using + // either its set value or default value if it has not been set. float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); if (0 <= percentage && percentage <= 1) { From c705cc9c69ab872e27477b1309c983dce23d55a5 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 19 Apr 2024 14:53:23 -0700 Subject: [PATCH 3/5] Fix canoncialization errors --- .../container/common/volume/VolumeUsage.java | 29 ++++++------- .../volume/TestReservedVolumeSpace.java | 42 ++++++++++++------- 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java index 4ace93cad9a3..d18998821b1e 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeUsage.java @@ -29,6 +29,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.File; +import java.io.IOException; import java.util.Collection; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_DATANODE_VOLUME_MIN_FREE_SPACE; @@ -196,19 +198,21 @@ private static long getReserved(ConfigurationSource conf, String rootDir, for (String reserve : reserveList) { String[] words = reserve.split(":"); if (words.length < 2) { - LOG.error("Reserved space should config in pair, but current is {}", + LOG.error("Reserved space should be configured in a pair, but current value is {}", reserve); continue; } - if (words[0].trim().equals(rootDir)) { - try { + try { + String path = new File(words[0]).getCanonicalPath(); + if (path.equals(rootDir)) { StorageSize size = StorageSize.parse(words[1].trim()); return (long) size.getUnit().toBytes(size.getValue()); - } catch (Exception e) { - LOG.error("Failed to parse StorageSize: {}", words[1].trim(), e); - break; } + } catch (IllegalArgumentException e) { + LOG.error("Failed to parse StorageSize {} from config {}", words[1].trim(), HDDS_DATANODE_DIR_DU_RESERVED, e); + } catch (IOException e) { + LOG.error("Failed to read storage path {} from config {}", words[1].trim(), HDDS_DATANODE_DIR_DU_RESERVED, e); } } @@ -216,15 +220,12 @@ private static long getReserved(ConfigurationSource conf, String rootDir, // either its set value or default value if it has not been set. float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); - if (0 <= percentage && percentage <= 1) { - return (long) Math.ceil(capacity * percentage); + if (percentage < 0 || percentage > 1) { + LOG.error("The value of {} should be between 0 to 1. Falling back to default value {}", + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); + percentage = HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT; } - // If it comes here then the percentage is not between 0-1. - LOG.error("The value of {} should be between 0 to 1. Defaulting to 0.", - HDDS_DATANODE_DIR_DU_RESERVED_PERCENT); - //Both configs are not set, return 0. - return 0; + return (long) Math.ceil(capacity * percentage); } - } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java index 789af009a1f0..cd9beab4b797 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java @@ -25,6 +25,8 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import java.io.File; +import java.nio.file.Files; import java.nio.file.Path; import java.util.UUID; @@ -68,9 +70,8 @@ public void testDefaultConfig() throws Exception { // Gets the actual total capacity without accounting for DU reserved space configurations. long totalCapacity = usage.realUsage().getCapacity(); long reservedCapacity = usage.getReservedBytes(); - long expectedReservedCapacity = (long) Math.ceil(totalCapacity * percentage); - assertEquals(expectedReservedCapacity, reservedCapacity); + assertEquals(getExpectedDefaultReserved(hddsVolume), reservedCapacity); assertEquals(totalCapacity - reservedCapacity, volumeCapacity); } @@ -114,17 +115,7 @@ public void testReservedWhenBothConfigSet() throws Exception { long reservedFromVolume = hddsVolume.getVolumeInfo().get() .getReservedInBytes(); - assertEquals(reservedFromVolume, 500); - } - - @Test - public void testReservedToZeroWhenBothConfigNotSet() throws Exception { - OzoneConfiguration conf = new OzoneConfiguration(); - HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); - - long reservedFromVolume = hddsVolume.getVolumeInfo().get() - .getReservedInBytes(); - assertEquals(reservedFromVolume, 0); + assertEquals(500, reservedFromVolume); } @Test @@ -158,7 +149,7 @@ public void testInvalidConfig() throws Exception { long reservedFromVolume1 = hddsVolume1.getVolumeInfo().get() .getReservedInBytes(); - assertEquals(reservedFromVolume1, 0); + assertEquals(getExpectedDefaultReserved(hddsVolume1), reservedFromVolume1); OzoneConfiguration conf2 = new OzoneConfiguration(); @@ -168,6 +159,27 @@ public void testInvalidConfig() throws Exception { long reservedFromVolume2 = hddsVolume2.getVolumeInfo().get() .getReservedInBytes(); - assertEquals(reservedFromVolume2, 0); + assertEquals(getExpectedDefaultReserved(hddsVolume2), reservedFromVolume2); + } + + @Test + public void testPathsCanonicalized() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + + // Create symlink in folder (which is the root of the volume) + Path symlink = new File(temp.toFile(), "link").toPath(); + Files.createSymbolicLink(symlink, folder); + + // Use the symlink in the configuration. Canonicalization should still match it to folder used in the volume config. + conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, symlink + ":500B"); + HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); + + long reservedFromVolume = hddsVolume.getVolumeInfo().get().getReservedInBytes(); + assertEquals(500, reservedFromVolume); + } + + private long getExpectedDefaultReserved(HddsVolume volume) { + long totalCapacity = volume.getVolumeInfo().get().getUsageForTesting().realUsage().getCapacity(); + return (long) Math.ceil(totalCapacity * HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); } } From ff6671e6473892ea37d5518732e3cf515bd84f45 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 19 Apr 2024 18:04:46 -0700 Subject: [PATCH 4/5] Do not use reserved space for volume choosing policy tests The mocked space usage class does not work it. --- .../common/volume/TestCapacityVolumeChoosingPolicy.java | 4 ++++ .../common/volume/TestRoundRobinVolumeChoosingPolicy.java | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestCapacityVolumeChoosingPolicy.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestCapacityVolumeChoosingPolicy.java index 4718df3ae3f4..1eba25c35712 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestCapacityVolumeChoosingPolicy.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestCapacityVolumeChoosingPolicy.java @@ -38,6 +38,7 @@ import java.util.Map; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_DATANODE_VOLUME_CHOOSING_POLICY; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED_PERCENT; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -60,6 +61,9 @@ public void setup() throws Exception { String volume2 = baseDir + "disk2"; String volume3 = baseDir + "disk3"; policy = new CapacityVolumeChoosingPolicy(); + // Use the exact capacity and availability specified in this test. Do not reserve space to prevent volumes from + // filling up. + CONF.setFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, 0); SpaceUsageSource source1 = MockSpaceUsageSource.fixed(500, 100); SpaceUsageCheckFactory factory1 = MockSpaceUsageCheckFactory.of( diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestRoundRobinVolumeChoosingPolicy.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestRoundRobinVolumeChoosingPolicy.java index cc6fe87e19d8..1df26365531c 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestRoundRobinVolumeChoosingPolicy.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestRoundRobinVolumeChoosingPolicy.java @@ -36,6 +36,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED_PERCENT; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -58,6 +59,10 @@ public void setup() throws Exception { String volume2 = baseDir + "disk2"; policy = new RoundRobinVolumeChoosingPolicy(); + // Use the exact capacity and availability specified in this test. Do not reserve space to prevent volumes from + // filling up. + CONF.setFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, 0); + SpaceUsageSource source1 = MockSpaceUsageSource.fixed(500, 100); SpaceUsageCheckFactory factory1 = MockSpaceUsageCheckFactory.of( source1, Duration.ZERO, SpaceUsagePersistence.None.INSTANCE); From 4fb5fc2657563a4f076b9361231bd4a04f55274f Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 19 Apr 2024 18:10:01 -0700 Subject: [PATCH 5/5] Trigger fork CI after ren-enabling