ChenSammi commented on code in PR #9587: URL: https://github.com/apache/ozone/pull/9587#discussion_r2664057539
########## hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDefaultVolumeChoosingPolicy.java: ########## @@ -0,0 +1,500 @@ +/* + * 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.diskbalancer; + +import static org.apache.hadoop.ozone.container.diskbalancer.DiskBalancerVolumeCalculation.getVolumeUsages; +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 java.io.IOException; +import java.nio.file.Path; +import java.time.Duration; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.locks.ReentrantLock; +import java.util.stream.Stream; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.fs.MockSpaceUsageCheckFactory; +import org.apache.hadoop.hdds.fs.MockSpaceUsageSource; +import org.apache.hadoop.hdds.fs.SpaceUsageCheckFactory; +import org.apache.hadoop.hdds.fs.SpaceUsagePersistence; +import org.apache.hadoop.hdds.fs.SpaceUsageSource; +import org.apache.hadoop.ozone.container.common.volume.HddsVolume; +import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet; +import org.apache.hadoop.ozone.container.common.volume.StorageVolume; +import org.apache.hadoop.ozone.container.diskbalancer.DiskBalancerVolumeCalculation.VolumeFixedUsage; +import org.apache.hadoop.ozone.container.diskbalancer.policy.DefaultVolumeChoosingPolicy; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Unit tests for DefaultVolumeChoosingPolicy. + */ +public class TestDefaultVolumeChoosingPolicy { + + @TempDir + private Path baseDir; + + private static final long MB = 1024L * 1024L; + private static final long VOLUME_CAPACITY = 2500L * MB; // 2500MB - same for all volumes + private static final long DEFAULT_CONTAINER_SIZE = 100L * MB; // 100MB + private DefaultVolumeChoosingPolicy policy; + private MutableVolumeSet volumeSet; + private String datanodeUuid; + private Map<HddsVolume, Long> deltaMap; + + @BeforeEach + public void setup() { + datanodeUuid = UUID.randomUUID().toString(); + policy = new DefaultVolumeChoosingPolicy(new ReentrantLock()); + deltaMap = new HashMap<>(); + } + + /** + * Test case data structure for volume configuration. + */ + public static class VolumeTestConfig { + private final String name; + private final double utilization; + private final Long customCapacity; // null means use default VOLUME_CAPACITY + + public VolumeTestConfig(String name, double utilization) { + this.name = name; + this.utilization = utilization; + this.customCapacity = null; + } + + public VolumeTestConfig(String name, double utilization, long customCapacity) { + this.name = name; + this.utilization = utilization; + this.customCapacity = customCapacity; + } + + public String getName() { + return name; + } + + public double getUtilization() { + return utilization; + } + + public Long getCustomCapacity() { + return customCapacity; + } + } + + /** + * Test scenario configuration. + */ + public static class TestScenario { + private final String name; + private final List<VolumeTestConfig> volumes; + private final double thresholdPercentage; + private final long containerSize; + private final boolean shouldFindPair; + private final String expectedSourceDisk; + private final String expectedDestinationDisk; + + public TestScenario(String name, List<VolumeTestConfig> volumes, double thresholdPercentage, + long containerSize, boolean shouldFindPair, + String expectedSourceDisk, String expectedDestinationDisk) { + this.name = name; + this.volumes = volumes; + this.thresholdPercentage = thresholdPercentage; + this.containerSize = containerSize; + this.shouldFindPair = shouldFindPair; + this.expectedSourceDisk = expectedSourceDisk; + this.expectedDestinationDisk = expectedDestinationDisk; + } + + public String getName() { + return name; + } + + public List<VolumeTestConfig> getVolumes() { + return volumes; + } + + public double getThresholdPercentage() { + return thresholdPercentage; + } + + public long getContainerSize() { + return containerSize; + } + + public boolean shouldFindPair() { + return shouldFindPair; + } + + public String getExpectedSourceDisk() { + return expectedSourceDisk; + } + + public String getExpectedDestinationDisk() { + return expectedDestinationDisk; + } + + @Override + public String toString() { + return name; + } + } + + /** + * Creates a volume with specific utilization and capacity. + * + * @param name Volume name/identifier + * @param utilization Utilization as a double (0.0 to 1.0, representing percentage) + * @param capacity capacity for the volume + * @return HddsVolume with specified utilization and capacity + */ + private HddsVolume createVolume(String name, double utilization, long capacity) + throws IOException { + long usedSpace = (long) (capacity * utilization); + Path volumePath = baseDir.resolve(name); + + // Create a configuration without reserved space to avoid capacity adjustments + OzoneConfiguration volumeConf = new OzoneConfiguration(); + volumeConf.setFloat("hdds.datanode.dir.du.reserved.percent", 0.0f); + + SpaceUsageSource source = MockSpaceUsageSource.fixed(capacity, + capacity - usedSpace); + SpaceUsageCheckFactory factory = MockSpaceUsageCheckFactory.of( + source, Duration.ZERO, SpaceUsagePersistence.None.INSTANCE); + + HddsVolume volume = new HddsVolume.Builder(volumePath.toString()) + .conf(volumeConf) + .usageCheckFactory(factory) + .build(); + return volume; + } + + /** + * Creates volumes from test configuration. + * + * @param configs List of volume configurations + * @return List of created HddsVolumes + */ + private List<HddsVolume> createVolumes(List<VolumeTestConfig> configs) + throws IOException { + List<HddsVolume> volumes = new ArrayList<>(); + for (VolumeTestConfig config : configs) { + long capacity = config.getCustomCapacity() != null ? + config.getCustomCapacity() : VOLUME_CAPACITY; + HddsVolume volume = createVolume(config.getName(), config.getUtilization(), capacity); + volumes.add(volume); + } + return volumes; + } + + /** + * Sets up volume set with given volumes. + * + * @param volumes List of volumes to add to volume set + */ + private void setupVolumeSet(List<HddsVolume> volumes) throws IOException { + // Create volume set without any default volumes from configuration + // Use a clean configuration to avoid loading default volumes + OzoneConfiguration testConf = new OzoneConfiguration(); + // Explicitly set HDDS_DATANODE_DIR_KEY to empty to prevent default volumes + testConf.set("hdds.datanode.dir.key", ""); + volumeSet = new MutableVolumeSet(datanodeUuid, testConf, null, + StorageVolume.VolumeType.DATA_VOLUME, null); + + // Use setVolumeMapForTesting to set only our test volumes + // This ensures no default volumes from configuration are included + Map<String, StorageVolume> volumeMap = new HashMap<>(); + for (HddsVolume volume : volumes) { + volumeMap.put(volume.getStorageDir().getAbsolutePath(), volume); + } + volumeSet.setVolumeMapForTesting(volumeMap); + } + + /** + * Helper method to get disk name from volume. + */ + private String getVolumeName(HddsVolume volume, Map<String, HddsVolume> diskNameToVolume) { + for (Map.Entry<String, HddsVolume> entry : diskNameToVolume.entrySet()) { + if (entry.getValue().equals(volume)) { + return entry.getKey(); + } + } + return "unknown"; + } + + /** + * Generic test method that can be reused for different scenarios. + * + * @param scenario Test scenario configuration + */ + @ParameterizedTest(name = "{0}") + @MethodSource("testScenarios") + public void testVolumeChoosingPolicy(TestScenario scenario) + throws IOException { + // Create volumes from configuration + List<HddsVolume> volumes = createVolumes(scenario.getVolumes()); + setupVolumeSet(volumes); + + // Create a map of disk names to volumes for verification + Map<String, HddsVolume> diskNameToVolume = new HashMap<>(); + for (int i = 0; i < scenario.getVolumes().size(); i++) { + VolumeTestConfig config = scenario.getVolumes().get(i); + diskNameToVolume.put(config.getName(), volumes.get(i)); + } + + // Get volume usages for verification + List<VolumeFixedUsage> volumeUsages = getVolumeUsages(volumeSet, deltaMap); + + // Try to find a valid source-destination pair + Pair<HddsVolume, HddsVolume> result = policy.chooseVolume(volumeSet, + scenario.getThresholdPercentage(), deltaMap, scenario.getContainerSize()); + + if (scenario.shouldFindPair()) { + assertNotNull(result); + assertNotNull(result.getLeft()); + assertNotNull(result.getRight()); + + // Verify source is the expected disk + if (scenario.getExpectedSourceDisk() != null) { + HddsVolume expectedSource = diskNameToVolume.get(scenario.getExpectedSourceDisk()); + assertNotNull(expectedSource, "Expected source disk not found: " + scenario.getExpectedSourceDisk()); + assertTrue(result.getLeft().equals(expectedSource), + "Expected source disk: " + scenario.getExpectedSourceDisk() + + ", but got: " + getVolumeName(result.getLeft(), diskNameToVolume)); + } + + // Verify destination is the expected disk (or one of the valid options) + if (scenario.getExpectedDestinationDisk() != null) { + HddsVolume expectedDest = diskNameToVolume.get(scenario.getExpectedDestinationDisk()); + assertNotNull(expectedDest, "Expected destination disk not found: " + scenario.getExpectedDestinationDisk()); + assertTrue(result.getRight().equals(expectedDest), + "Expected destination disk: " + scenario.getExpectedDestinationDisk() + + ", but got: " + getVolumeName(result.getRight(), diskNameToVolume)); + } + + // Verify source is the highest utilization volume Review Comment: From the code, it cannot tell that the selected source and dest volume has the highest, or lowest utilization. Maybe you can reorder the volume list order of TestScenario, so that the first volume has the highest utilization, and last volume has the lowest utilization, or vice versa, to make sure the fixed index volume can be used to do the compare assertion. -- 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]
