From 0669d739fbd292070e8c63faeda07db314599c54 Mon Sep 17 00:00:00 2001 From: Aswin Shakil Balasubramanian Date: Fri, 17 Jun 2022 14:36:05 -0700 Subject: [PATCH 1/4] HDDS-6901. Configure HDDS volume reserved as percentage of the volume space. --- .../apache/hadoop/hdds/scm/ScmConfigKeys.java | 2 + .../src/main/resources/ozone-default.xml | 8 ++ .../container/common/volume/VolumeInfo.java | 52 ++++++--- .../container/common/volume/VolumeUsage.java | 9 +- .../volume/TestReservedVolumeSpace.java | 105 ++++++++++++++++++ 5 files changed, 158 insertions(+), 18 deletions(-) create mode 100644 hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java 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 b10f05cd1a91..fe5c94fcbaba 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 @@ -214,6 +214,8 @@ public final class ScmConfigKeys { public static final String HDDS_DATANODE_DIR_KEY = "hdds.datanode.dir"; public static final String HDDS_DATANODE_DIR_DU_RESERVED = "hdds.datanode.dir.du.reserved"; + public static final String HDDS_DATANODE_VOLUME_RESERVED = + "hdds.datanode.volume.reserved"; public static final String HDDS_REST_CSRF_ENABLED_KEY = "hdds.rest.rest-csrf.enabled"; public static final boolean HDDS_REST_CSRF_ENABLED_DEFAULT = false; diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index 21c580b8cd69..69734451ff05 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -166,6 +166,14 @@ Such as /dir1:100B, /dir2:200MB, means dir1 reserves 100 bytes and dir2 reserves 200 MB. + + hdds.datanode.volume.reserved + + OZONE, CONTAINER, STORAGE, MANAGEMENT + Percentage of volume that should be reserved. This space is left free for other usage. + The value should be between 0-1. Such as 0.1 which means 10% of volume space will be reserved. + + hdds.datanode.volume.choosing.policy diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java index 71063e23fc88..ae8ff281490c 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java @@ -33,6 +33,7 @@ import org.slf4j.LoggerFactory; import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_VOLUME_RESERVED; /** * Stores information about a disk/volume. @@ -134,25 +135,45 @@ public VolumeInfo build() throws IOException { } private long getReserved(ConfigurationSource conf) { + final float defaultValue = Float.MAX_VALUE; + float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED, + defaultValue); Collection reserveList = conf.getTrimmedStringCollection( HDDS_DATANODE_DIR_DU_RESERVED); - for (String reserve : reserveList) { - String[] words = reserve.split(":"); - if (words.length < 2) { - LOG.error("Reserved space should config in pair, but current is {}", - reserve); - continue; - } + //Both the configs are set. Log it and return 0 + if (reserveList.size() > 0 && percentage != defaultValue) { + LOG.error("Both {} and {} are set. Set either one, not both. " + + "Setting reserved to 0.", HDDS_DATANODE_VOLUME_RESERVED, + HDDS_DATANODE_DIR_DU_RESERVED); + return 0; + } - if (words[0].trim().equals(rootDir)) { - try { - 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); - return 0; + if (reserveList.size() > 0) { + for (String reserve : reserveList) { + String[] words = reserve.split(":"); + if (words.length < 2) { + LOG.error("Reserved space should config in pair, but current is {}", + reserve); + continue; } + + if (words[0].trim().equals(rootDir)) { + try { + 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); + return 0; + } + } + } + } else if (percentage != defaultValue) { + if (0 <= percentage && percentage <= 1) { + return (long) Math.ceil(this.usage.getCapacity() * 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_VOLUME_RESERVED); } return 0; @@ -183,8 +204,9 @@ private VolumeInfo(Builder b) throws IOException { SpaceUsageCheckParams checkParams = usageCheckFactory.paramsFor(root); + this.usage = new VolumeUsage(checkParams); this.reservedInBytes = getReserved(b.conf); - this.usage = new VolumeUsage(checkParams, reservedInBytes); + this.usage.setReserved(reservedInBytes); } public long getCapacity() { 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 fe12d4be9ca2..2d21c8f8ad0d 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 @@ -30,10 +30,9 @@ public class VolumeUsage implements SpaceUsageSource { private final CachingSpaceUsageSource source; private boolean shutdownComplete; - private final long reservedInBytes; + private long reservedInBytes; - VolumeUsage(SpaceUsageCheckParams checkParams, long reservedInBytes) { - this.reservedInBytes = reservedInBytes; + VolumeUsage(SpaceUsageCheckParams checkParams) { source = new CachingSpaceUsageSource(checkParams); start(); // TODO should start only on demand } @@ -98,4 +97,8 @@ public synchronized void shutdown() { public void refreshNow() { source.refreshNow(); } + + public void setReserved(long reserved) { + this.reservedInBytes = reserved; + } } 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 new file mode 100644 index 000000000000..fe07bcedd7d6 --- /dev/null +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestReservedVolumeSpace.java @@ -0,0 +1,105 @@ +/** + * 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 org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.fs.MockSpaceUsageCheckFactory; +import org.apache.hadoop.hdds.scm.ScmConfigKeys; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.util.UUID; + +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_VOLUME_RESERVED; + +/** + * To test the reserved volume space. + */ +public class TestReservedVolumeSpace { + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + private static final String DATANODE_UUID = UUID.randomUUID().toString(); + private HddsVolume.Builder volumeBuilder; + + @Before + public void setup() throws Exception { + volumeBuilder = new HddsVolume.Builder(folder.getRoot().getPath()) + .datanodeUuid(DATANODE_UUID) + .usageCheckFactory(MockSpaceUsageCheckFactory.NONE); + } + + /** + * Test reserved capacity with respect to the percentage of actual capacity. + * @throws Exception + */ + @Test + public void testVolumeCapacityAfterReserve() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(HDDS_DATANODE_VOLUME_RESERVED, "0.3"); + HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); + //Reserving + float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED, -1); + + long volumeCapacity = hddsVolume.getCapacity(); + //Gets the actual total capacity + long totalCapacity = hddsVolume.getVolumeInfo() + .getUsageForTesting().getCapacity(); + long reservedCapacity = hddsVolume.getVolumeInfo().getReservedInBytes(); + //Volume Capacity with Reserved + long volumeCapacityReserved = totalCapacity - reservedCapacity; + + long reservedFromVolume = hddsVolume.getVolumeInfo().getReservedInBytes(); + long reservedCalculated = (long) Math.ceil(totalCapacity * percentage); + + Assert.assertEquals(reservedFromVolume, reservedCalculated); + Assert.assertEquals(volumeCapacity, volumeCapacityReserved); + } + + /** + * Either hdds.datanode.volume.reserved or hdds.datanode.dir.du.reserved + * should be set. But not both. + * @throws Exception + */ + @Test + public void testReservedToZeroWhenBothConfigSet() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(HDDS_DATANODE_VOLUME_RESERVED, "0.3"); + conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, + folder.getRoot() + ":500B"); + HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); + + long reservedFromVolume = hddsVolume.getVolumeInfo().getReservedInBytes(); + //When both the configs hdds.datanode.volume.reserved and + //hdds.datanode.dir.du.reserved are set. We set reserved to 0 + Assert.assertEquals(reservedFromVolume, 0); + } + + @Test + public void testReservedToZeroWhenBothConfigNotSet() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); + + long reservedFromVolume = hddsVolume.getVolumeInfo().getReservedInBytes(); + Assert.assertEquals(reservedFromVolume, 0); + } +} From 6542413c7bab56ddde9e1c470f3b7834f6dea204 Mon Sep 17 00:00:00 2001 From: Aswin Shakil Balasubramanian Date: Mon, 27 Jun 2022 22:49:23 -0700 Subject: [PATCH 2/4] Added suggestions. --- .../apache/hadoop/hdds/scm/ScmConfigKeys.java | 5 +- .../src/main/resources/ozone-default.xml | 2 +- .../hadoop/hdds/conf/ConfigurationSource.java | 12 ++++ .../container/common/volume/VolumeInfo.java | 61 ++++++++++--------- .../volume/TestReservedVolumeSpace.java | 10 +-- 5 files changed, 53 insertions(+), 37 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 fe5c94fcbaba..70dca81ce013 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 @@ -214,8 +214,9 @@ public final class ScmConfigKeys { public static final String HDDS_DATANODE_DIR_KEY = "hdds.datanode.dir"; public static final String HDDS_DATANODE_DIR_DU_RESERVED = "hdds.datanode.dir.du.reserved"; - public static final String HDDS_DATANODE_VOLUME_RESERVED = - "hdds.datanode.volume.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 String HDDS_REST_CSRF_ENABLED_KEY = "hdds.rest.rest-csrf.enabled"; public static final boolean HDDS_REST_CSRF_ENABLED_DEFAULT = false; diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index 69734451ff05..687b266f7b22 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -167,7 +167,7 @@ - hdds.datanode.volume.reserved + hdds.datanode.dir.du.reserved.percent OZONE, CONTAINER, STORAGE, MANAGEMENT Percentage of volume that should be reserved. This space is left free for other usage. diff --git a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java index 22c74ff325d9..a3c0b891b36a 100644 --- a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java +++ b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java @@ -143,6 +143,18 @@ default Map getPropsMatchPrefix(String keyPrefix) { return configMap; } + /** + * Checks if the property is set. + * @param key The property name. + * @return true if the value is set else false. + */ + default boolean isConfigured(String key) { + String configValue = get(key); + if (configValue != null) { + return true; + } + return false; + } /** * Create a Configuration object and inject the required configuration values. * diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java index ae8ff281490c..d6d68e0d20e0 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java @@ -33,7 +33,8 @@ import org.slf4j.LoggerFactory; import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED; -import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_VOLUME_RESERVED; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED_PERCENT; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT; /** * Stores information about a disk/volume. @@ -135,45 +136,45 @@ public VolumeInfo build() throws IOException { } private long getReserved(ConfigurationSource conf) { - final float defaultValue = Float.MAX_VALUE; - float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED, - defaultValue); - Collection reserveList = conf.getTrimmedStringCollection( - HDDS_DATANODE_DIR_DU_RESERVED); - //Both the configs are set. Log it and return 0 - if (reserveList.size() > 0 && percentage != defaultValue) { + if (conf.isConfigured(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT) + && conf.isConfigured(HDDS_DATANODE_DIR_DU_RESERVED)) { LOG.error("Both {} and {} are set. Set either one, not both. " + - "Setting reserved to 0.", HDDS_DATANODE_VOLUME_RESERVED, + "Setting reserved to 0.", HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, HDDS_DATANODE_DIR_DU_RESERVED); return 0; } - if (reserveList.size() > 0) { - for (String reserve : reserveList) { - String[] words = reserve.split(":"); - if (words.length < 2) { - LOG.error("Reserved space should config in pair, but current is {}", - reserve); - continue; - } - - if (words[0].trim().equals(rootDir)) { - try { - 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); - return 0; - } - } - } - } else if (percentage != defaultValue) { + 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(this.usage.getCapacity() * 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_VOLUME_RESERVED); + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT); + return 0; + } + + Collection reserveList = conf.getTrimmedStringCollection( + HDDS_DATANODE_DIR_DU_RESERVED); + for (String reserve : reserveList) { + String[] words = reserve.split(":"); + if (words.length < 2) { + LOG.error("Reserved space should config in pair, but current is {}", + reserve); + continue; + } + + if (words[0].trim().equals(rootDir)) { + try { + 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); + 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 fe07bcedd7d6..d5dd91d8ae33 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 @@ -29,7 +29,8 @@ import java.util.UUID; -import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_VOLUME_RESERVED; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED_PERCENT; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT; /** * To test the reserved volume space. @@ -55,10 +56,11 @@ public void setup() throws Exception { @Test public void testVolumeCapacityAfterReserve() throws Exception { OzoneConfiguration conf = new OzoneConfiguration(); - conf.set(HDDS_DATANODE_VOLUME_RESERVED, "0.3"); + conf.set(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, "0.3"); HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); //Reserving - float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED, -1); + float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); long volumeCapacity = hddsVolume.getCapacity(); //Gets the actual total capacity @@ -83,7 +85,7 @@ public void testVolumeCapacityAfterReserve() throws Exception { @Test public void testReservedToZeroWhenBothConfigSet() throws Exception { OzoneConfiguration conf = new OzoneConfiguration(); - conf.set(HDDS_DATANODE_VOLUME_RESERVED, "0.3"); + conf.set(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, "0.3"); conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, folder.getRoot() + ":500B"); HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); From b223db50e657497a65d61295757bb16ab9269de4 Mon Sep 17 00:00:00 2001 From: Aswin Shakil Balasubramanian Date: Wed, 29 Jun 2022 15:22:27 -0700 Subject: [PATCH 3/4] Changed config order and updated tests. --- .../container/common/volume/VolumeInfo.java | 39 ++++++++------ .../volume/TestReservedVolumeSpace.java | 54 ++++++++++++++++--- 2 files changed, 70 insertions(+), 23 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java index d6d68e0d20e0..eb19d44442a5 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java @@ -138,24 +138,14 @@ public VolumeInfo build() throws IOException { private long getReserved(ConfigurationSource conf) { if (conf.isConfigured(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT) && conf.isConfigured(HDDS_DATANODE_DIR_DU_RESERVED)) { - LOG.error("Both {} and {} are set. Set either one, not both. " + - "Setting reserved to 0.", HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, - HDDS_DATANODE_DIR_DU_RESERVED); - return 0; - } - - 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(this.usage.getCapacity() * 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); - return 0; + LOG.error("Both {} and {} are set. Set either one, not both. If the " + + "volume matches with volume parameter in former config, it is set " + + "as reserved space. If not it fall backs to the latter config.", + HDDS_DATANODE_DIR_DU_RESERVED, HDDS_DATANODE_DIR_DU_RESERVED_PERCENT); } + // 1. If hdds.datanode.dir.du.reserved is set for a volume then make it + // as the reserved bytes. Collection reserveList = conf.getTrimmedStringCollection( HDDS_DATANODE_DIR_DU_RESERVED); for (String reserve : reserveList) { @@ -171,12 +161,27 @@ private long getReserved(ConfigurationSource conf) { 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); + LOG.error("Failed to parse StorageSize: {} ,Setting reserved " + + "space to zero", words[1].trim(), e); return 0; } } } + // 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(this.usage.getCapacity() * 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 d5dd91d8ae33..471a9a750f02 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 @@ -39,6 +39,8 @@ public class TestReservedVolumeSpace { @Rule public TemporaryFolder folder = new TemporaryFolder(); + @Rule + public TemporaryFolder temp = new TemporaryFolder(); private static final String DATANODE_UUID = UUID.randomUUID().toString(); private HddsVolume.Builder volumeBuilder; @@ -78,12 +80,12 @@ public void testVolumeCapacityAfterReserve() throws Exception { } /** - * Either hdds.datanode.volume.reserved or hdds.datanode.dir.du.reserved - * should be set. But not both. + * When both configs are set, hdds.datanode.dir.du.reserved is set + * if the volume matches with volume in config parameter. * @throws Exception */ @Test - public void testReservedToZeroWhenBothConfigSet() throws Exception { + public void testReservedWhenBothConfigSet() throws Exception { OzoneConfiguration conf = new OzoneConfiguration(); conf.set(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, "0.3"); conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, @@ -91,9 +93,7 @@ public void testReservedToZeroWhenBothConfigSet() throws Exception { HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); long reservedFromVolume = hddsVolume.getVolumeInfo().getReservedInBytes(); - //When both the configs hdds.datanode.volume.reserved and - //hdds.datanode.dir.du.reserved are set. We set reserved to 0 - Assert.assertEquals(reservedFromVolume, 0); + Assert.assertEquals(reservedFromVolume, 500); } @Test @@ -104,4 +104,46 @@ public void testReservedToZeroWhenBothConfigNotSet() throws Exception { long reservedFromVolume = hddsVolume.getVolumeInfo().getReservedInBytes(); Assert.assertEquals(reservedFromVolume, 0); } + + @Test + public void testFallbackToPercentConfig() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, "0.3"); + //Setting config for different volume, hence fallback happens + conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, + temp.getRoot() + ":500B"); + HddsVolume hddsVolume = volumeBuilder.conf(conf).build(); + + long reservedFromVolume = hddsVolume.getVolumeInfo().getReservedInBytes(); + Assert.assertNotEquals(reservedFromVolume, 0); + + long totalCapacity = hddsVolume.getVolumeInfo() + .getUsageForTesting().getCapacity(); + float percentage = conf.getFloat(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, + HDDS_DATANODE_DIR_DU_RESERVED_PERCENT_DEFAULT); + long reservedCalculated = (long) Math.ceil(totalCapacity * percentage); + Assert.assertEquals(reservedFromVolume, reservedCalculated); + } + + @Test + public void testInvalidConfig() throws Exception { + OzoneConfiguration conf1 = new OzoneConfiguration(); + + // 500C doesn't match with any Storage Unit + conf1.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, + folder.getRoot() + ":500C"); + HddsVolume hddsVolume1 = volumeBuilder.conf(conf1).build(); + + long reservedFromVolume1 = hddsVolume1.getVolumeInfo().getReservedInBytes(); + Assert.assertEquals(reservedFromVolume1, 0); + + OzoneConfiguration conf2 = new OzoneConfiguration(); + + //Should be between 0-1. + conf2.set(HDDS_DATANODE_DIR_DU_RESERVED_PERCENT, "20"); + HddsVolume hddsVolume2 = volumeBuilder.conf(conf2).build(); + + long reservedFromVolume2 = hddsVolume2.getVolumeInfo().getReservedInBytes(); + Assert.assertEquals(reservedFromVolume2, 0); + } } From 11b91fed576c3603454b52d5120340fd1dbc192b Mon Sep 17 00:00:00 2001 From: Aswin Shakil Balasubramanian Date: Thu, 30 Jun 2022 10:56:08 -0700 Subject: [PATCH 4/4] Added suggestions. --- .../org/apache/hadoop/hdds/conf/ConfigurationSource.java | 6 +----- .../hadoop/ozone/container/common/volume/VolumeInfo.java | 5 ++--- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java index a3c0b891b36a..8c4ee7cd2f1a 100644 --- a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java +++ b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java @@ -149,11 +149,7 @@ default Map getPropsMatchPrefix(String keyPrefix) { * @return true if the value is set else false. */ default boolean isConfigured(String key) { - String configValue = get(key); - if (configValue != null) { - return true; - } - return false; + return get(key) != null; } /** * Create a Configuration object and inject the required configuration values. diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java index eb19d44442a5..6773b6ff6489 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java @@ -161,9 +161,8 @@ private long getReserved(ConfigurationSource conf) { StorageSize size = StorageSize.parse(words[1].trim()); return (long) size.getUnit().toBytes(size.getValue()); } catch (Exception e) { - LOG.error("Failed to parse StorageSize: {} ,Setting reserved " + - "space to zero", words[1].trim(), e); - return 0; + LOG.error("Failed to parse StorageSize: {}", words[1].trim(), e); + break; } } }