errose28 commented on code in PR #6219:
URL: https://github.com/apache/ozone/pull/6219#discussion_r1490233387
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/container/ClusterContainersUtil.java:
##########
Review Comment:
Maybe we can put this in
`hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/utils` to
avoid having another package. I can't find an existing util class that will
work with the mini ozone cluster dependency though, so from what I can see the
new class is necessary.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java:
##########
@@ -250,6 +251,12 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, TermIn
omBucketInfo.getDefaultReplicationConfig(),
ozoneManager);
+ if (omBucketInfo.getEncryptionKeyInfo() != null &&
+ !keyArgs.hasFileEncryptionInfo()) {
+ throw new OMException("Attempting to create unencrypted file " +
+ keyName + " in encrypted bucket " + bucketName, INTERNAL_ERROR);
+ }
Review Comment:
This is a good check, do we need this check in other key create methods too?
Unfortunately there's so many different create and commit classes it might be
kind of a pain to add it everywhere, even if it's a util method in
`OMKeyRequest`. Might be worth it to prevent serious bugs though.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/container/ClusterContainersUtil.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.statemachine.container;
+
+import com.google.common.base.Preconditions;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.ozone.HddsDatanodeService;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.client.OzoneKey;
+import org.apache.hadoop.ozone.client.OzoneKeyDetails;
+import org.apache.hadoop.ozone.container.common.helpers.BlockData;
+import org.apache.hadoop.ozone.container.common.interfaces.BlockIterator;
+import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.common.interfaces.DBHandle;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils;
+import
org.apache.hadoop.ozone.container.keyvalue.helpers.KeyValueContainerLocationUtil;
+import org.apache.ratis.io.CorruptedFileException;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.Charset;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * Utility method to manipulate/inspect container data on disk in a mini
cluster.
+ */
+public final class ClusterContainersUtil {
+ private ClusterContainersUtil() {
+ }
+
+ /**
+ *
+ *
+ * @param cluster a mini ozone cluster object.
+ * @param container a container object.
+ * @param key an OzoneKey object.
+ * @return the location of the chunk file.
+ * @throws IOException
+ */
+ public static File getChunksLocationPath(MiniOzoneCluster cluster, Container
container, OzoneKey key)
+ throws IOException {
+ Preconditions.checkArgument(key instanceof OzoneKeyDetails);
+ long containerID = ((OzoneKeyDetails) key).getOzoneKeyLocations().get(0)
+ .getContainerID();
+ long localID = ((OzoneKeyDetails) key).getOzoneKeyLocations().get(0)
+ .getLocalID();
+ // From the containerData, get the block iterator for all the blocks in
+ // the container.
+ KeyValueContainerData containerData =
+ (KeyValueContainerData) container.getContainerData();
+ try (DBHandle db = BlockUtils.getDB(containerData, cluster.getConf());
+ BlockIterator<BlockData> keyValueBlockIterator =
+ db.getStore().getBlockIterator(containerID)) {
+ // Find the block corresponding to the key we put. We use the localID of
+ // the BlockData to identify out key.
+ BlockData blockData = null;
+ while (keyValueBlockIterator.hasNext()) {
+ blockData = keyValueBlockIterator.nextBlock();
+ if (blockData.getBlockID().getLocalID() == localID) {
+ break;
+ }
+ }
+ assertNotNull(blockData, "Block not found");
+
+ // Get the location of the chunk file
+ String containreBaseDir =
+ container.getContainerData().getVolume().getHddsRootDir().getPath();
+ File chunksLocationPath = KeyValueContainerLocationUtil
+ .getChunksLocationPath(containreBaseDir, cluster.getClusterId(),
containerID);
+ return chunksLocationPath;
+ }
+ }
+
+ /**
+ * Corrupt the chunk backing the key in a mini cluster.
+ * @param cluster a mini ozone cluster object.
+ * @param container a container object.
+ * @param key an OzoneKey object.
+ * @throws IOException
+ */
+ public static void corruptData(MiniOzoneCluster cluster, Container
container, OzoneKey key)
+ throws IOException {
+ File chunksLocationPath = getChunksLocationPath(cluster, container, key);
+ byte[] corruptData = "corrupted data".getBytes(UTF_8);
+ // Corrupt the contents of chunk files
+ for (File file : FileUtils.listFiles(chunksLocationPath, null, false)) {
+ FileUtils.writeByteArrayToFile(file, corruptData);
+ }
+ }
+
+ /**
+ * Inspect and verify if chunk backing the key in a mini cluster is the same
as the string.
+ * @param cluster a mini ozone cluster object.
+ * @param container a container object.
+ * @param key an OzoneKey object.
+ * @throws IOException
+ */
+ public static void verifyOnDiskData(MiniOzoneCluster cluster, Container
container, OzoneKey key, String data)
+ throws IOException {
+ File chunksLocationPath = getChunksLocationPath(cluster, container, key);
+ for (File file : FileUtils.listFiles(chunksLocationPath, null, false)) {
+ String chunkOnDisk = FileUtils.readFileToString(file,
Charset.defaultCharset());
+ if (!data.equals(chunkOnDisk)) {
+ throw new CorruptedFileException(file, " does not match the source.");
Review Comment:
Can we just make this return a boolean if the data does not match? Current
usage in `TestOzoneAtRestEncryption` is kind of confusing since it appears to
be checking for "corruption" when actually it just wants the data on disk to be
different than the input.
--
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]
