sarvekshayr commented on code in PR #11951: URL: https://github.com/apache/ozone/pull/11951#discussion_r4489124977
########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/storagepolicy/CheckStoragePolicySubCommand.java: ########## @@ -0,0 +1,238 @@ +/* + * 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.hdds.scm.cli.storagepolicy; + +import com.google.common.annotations.VisibleForTesting; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import org.apache.hadoop.fs.StorageType; +import org.apache.hadoop.hdds.cli.HddsVersionProvider; +import org.apache.hadoop.hdds.client.OzoneStoragePolicy; +import org.apache.hadoop.hdds.client.StorageTier; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.scm.cli.ScmSubcommand; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.container.ContainerReplicaInfo; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneKeyDetails; +import org.apache.hadoop.ozone.client.OzoneKeyLocation; +import org.apache.hadoop.ozone.shell.OzoneAddress; +import org.apache.hadoop.ozone.shell.keys.KeyUri; +import picocli.CommandLine; + +/** + * Handler of the command that checks whether the replicas of a key are on the + * storage type of the key's storage policy. + * + * <p>A replica matches when both its container storage type and the type of the volume it sits on + * equal the storage type of the policy's creation tier. A replica on the fallback tier does not match, + * and an unknown (null) type does not match. + */ [email protected]( + name = "check", + description = "Check whether the replicas of a key are on the storage type of its storage policy", + mixinStandardHelpOptions = true, + versionProvider = HddsVersionProvider.class) +public class CheckStoragePolicySubCommand extends ScmSubcommand { + + @CommandLine.Mixin + private KeyUri uri; + + @CommandLine.Option( + names = {"--show-replicas"}, + description = "Show the storage types of every replica of every container of the key.") + private boolean showReplicas; + + @Override + public void execute(ScmClient scmClient) throws IOException { + OzoneAddress address = uri.getValue(); + try (OzoneClient client = createOzoneClient(address)) { + OzoneKeyDetails keyDetails = client.getObjectStore() + .getVolume(address.getVolumeName()) + .getBucket(address.getBucketName()) + .getKey(address.getKeyName()); + System.out.println(buildReport(keyDetails, scmClient, showReplicas)); + } + } + + protected OzoneClient createOzoneClient(OzoneAddress address) throws IOException { + return address.createClient(getOzoneConf()); + } + + /** + * Checks the key and builds the text of the report. + * + * @param withReplicas true to add the container and replica details + */ + @VisibleForTesting Review Comment: @VisibleForTesting annotation is no longer used, see [HDDS-12725](https://issues.apache.org/jira/browse/HDDS-12725). ########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/storagepolicy/CheckStoragePolicySubCommand.java: ########## @@ -0,0 +1,238 @@ +/* + * 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.hdds.scm.cli.storagepolicy; + +import com.google.common.annotations.VisibleForTesting; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import org.apache.hadoop.fs.StorageType; +import org.apache.hadoop.hdds.cli.HddsVersionProvider; +import org.apache.hadoop.hdds.client.OzoneStoragePolicy; +import org.apache.hadoop.hdds.client.StorageTier; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.scm.cli.ScmSubcommand; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.container.ContainerReplicaInfo; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneKeyDetails; +import org.apache.hadoop.ozone.client.OzoneKeyLocation; +import org.apache.hadoop.ozone.shell.OzoneAddress; +import org.apache.hadoop.ozone.shell.keys.KeyUri; +import picocli.CommandLine; + +/** + * Handler of the command that checks whether the replicas of a key are on the + * storage type of the key's storage policy. + * + * <p>A replica matches when both its container storage type and the type of the volume it sits on + * equal the storage type of the policy's creation tier. A replica on the fallback tier does not match, + * and an unknown (null) type does not match. + */ [email protected]( + name = "check", + description = "Check whether the replicas of a key are on the storage type of its storage policy", + mixinStandardHelpOptions = true, + versionProvider = HddsVersionProvider.class) +public class CheckStoragePolicySubCommand extends ScmSubcommand { + + @CommandLine.Mixin + private KeyUri uri; + + @CommandLine.Option( + names = {"--show-replicas"}, + description = "Show the storage types of every replica of every container of the key.") + private boolean showReplicas; + + @Override + public void execute(ScmClient scmClient) throws IOException { + OzoneAddress address = uri.getValue(); + try (OzoneClient client = createOzoneClient(address)) { + OzoneKeyDetails keyDetails = client.getObjectStore() + .getVolume(address.getVolumeName()) + .getBucket(address.getBucketName()) + .getKey(address.getKeyName()); + System.out.println(buildReport(keyDetails, scmClient, showReplicas)); + } + } + + protected OzoneClient createOzoneClient(OzoneAddress address) throws IOException { + return address.createClient(getOzoneConf()); + } + + /** + * Checks the key and builds the text of the report. + * + * @param withReplicas true to add the container and replica details + */ + @VisibleForTesting + public String buildReport(OzoneKeyDetails keyDetails, ScmClient scmClient, boolean withReplicas) + throws IOException { + List<String> lines = new ArrayList<>(); + lines.add("Storage Policy for key '" + keyDetails.getVolumeName() + "/" + keyDetails.getBucketName() + "/" + + keyDetails.getName() + "':"); + + OzoneStoragePolicy policy = (OzoneStoragePolicy) keyDetails.getStoragePolicy(); + if (policy == null) { + lines.add(" Key Unset StoragePolicy"); Review Comment: Let's refine this message for better clarity. A couple of suggestions: - `Storage Policy: not set for this key` - `Storage Policy: none recorded for this key` Q: For keys created after this feature is introduced, will they always have a storage policy explicitly set, or are there scenarios where it could still remain unset/null? ########## hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/storagepolicy/TestCheckStoragePolicySubCommand.java: ########## @@ -0,0 +1,546 @@ +/* + * 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.hdds.scm.cli.storagepolicy; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.PrintStream; +import java.io.UnsupportedEncodingException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.hadoop.fs.StorageType; +import org.apache.hadoop.hdds.client.ECReplicationConfig; +import org.apache.hadoop.hdds.client.OzoneStoragePolicy; +import org.apache.hadoop.hdds.client.RatisReplicationConfig; +import org.apache.hadoop.hdds.client.ReplicationConfig; +import org.apache.hadoop.hdds.client.StoragePolicy; +import org.apache.hadoop.hdds.protocol.DatanodeDetails; +import org.apache.hadoop.hdds.protocol.MockDatanodeDetails; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.scm.cli.storagepolicy.CheckStoragePolicySubCommand.ContainerCheck; +import org.apache.hadoop.hdds.scm.cli.storagepolicy.CheckStoragePolicySubCommand.ReplicaCheck; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.container.ContainerReplicaInfo; +import org.apache.hadoop.ozone.client.ObjectStore; +import org.apache.hadoop.ozone.client.OzoneBucket; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneKeyDetails; +import org.apache.hadoop.ozone.client.OzoneKeyLocation; +import org.apache.hadoop.ozone.client.OzoneVolume; +import org.apache.hadoop.ozone.shell.OzoneAddress; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import picocli.CommandLine; + +/** + * Tests for {@link CheckStoragePolicySubCommand}. + * + * <p>Rule tests call {@code evaluate} and assert on the result objects. Layout tests call + * {@code buildReport} and assert on the text. SCM is a Mockito mock; no cluster is started. + */ +public class TestCheckStoragePolicySubCommand { + + private static final String VOLUME = "vol1"; + private static final String BUCKET = "bucket1"; + private static final String KEY = "key1"; + private static final String KEY_HEADER = "Storage Policy for key 'vol1/bucket1/key1':"; + // This module compiles with --release 8, so the Charset overloads of PrintStream and toString are unavailable. + private static final String ENCODING = StandardCharsets.UTF_8.name(); + private static final ReplicationConfig RATIS_THREE = + RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE); + + private final ByteArrayOutputStream outContent = new ByteArrayOutputStream(); + private final PrintStream originalOut = System.out; + + private ScmClient scmClient; + private CheckStoragePolicySubCommand cmd; + + @BeforeEach + public void setup() throws UnsupportedEncodingException { + scmClient = mock(ScmClient.class); + cmd = new CheckStoragePolicySubCommand(); + System.setOut(new PrintStream(outContent, false, ENCODING)); + } + + @AfterEach + public void tearDown() { + System.setOut(originalOut); + } + + /** + * Each policy is checked against its own creation tier. When both storageType and volumeStorageType of all three + * replicas equal that tier, the replicas, the container and the key all match. + */ + @ParameterizedTest + @CsvSource({"HOT,SSD", "WARM,DISK", "COLD,ARCHIVE"}) + public void testAllReplicasOnCreationTierMatch(OzoneStoragePolicy policy, StorageType expected) + throws IOException { + scmReturns(1L, replicas(1L, 3, expected, expected)); + + List<ContainerCheck> checks = cmd.evaluate(keyDetails(policy, RATIS_THREE, 1L), scmClient); + + assertThat(CheckStoragePolicySubCommand.keyMatches(checks)).isTrue(); + assertThat(checks).hasSize(1); + assertThat(checks.get(0).matches()).isTrue(); + assertThat(checks.get(0).getReplicaChecks()).hasSize(3).allMatch(ReplicaCheck::matches); + assertThat(checks.get(0).getReplicaChecks()) + .allSatisfy(r -> assertThat(r.getMismatchReasons()).isEmpty()); + } + + /** + * The container storage type is right but the replica sits on a volume of another type: NO, and the reason names + * volumeStorageType only. + */ + @Test + public void testWrongVolumeStorageTypeIsReported() throws IOException { Review Comment: Could we use `@ParameterizedTest` with a reasons column to cover all three - - testWrongVolumeStorageTypeIsReported is (SSD, DISK). - testBothTypesWrongReportsBothReasons is (DISK, ARCHIVE). - testReplicaOnFallbackTierDoesNotMatch is (DISK, DISK). -- 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]
