You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "adoroszlai (via GitHub)" <gi...@apache.org> on 2025/02/19 13:15:55 UTC
[PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
adoroszlai opened a new pull request, #7932:
URL: https://github.com/apache/ozone/pull/7932
## What changes were proposed in this pull request?
- Convert some HDDS integration tests to use shared cluster.
- Extract safe test case from `TestStorageContainerManagerHA` into a separate class, to be used with shared cluster. Some trivial code cleanup in this test class, to avoid duplicating the problems.
- Move safe test cases from `TestQueryNode` to `TestContainerOperations` (which also tests `ScmClient`).
- Remove flaky tag from `TestDecommissionAndMaintenance`, passed 10x10 runs twice ([1](https://github.com/adoroszlai/ozone/actions/runs/13352899519), [2](https://github.com/adoroszlai/ozone/actions/runs/13358224366)). This does not result in speed-up, but reduces run time of _integration (flaky)_, the slowest check currently.
https://issues.apache.org/jira/browse/HDDS-12349
## How was this patch tested?
Before:
```
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 158.4 s - in org.apache.hadoop.hdds.scm.TestStorageContainerManagerHA
Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 35.973 s - in org.apache.hadoop.hdds.scm.TestXceiverClientManager
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 68.415 s - in org.apache.hadoop.hdds.scm.container.metrics.TestSCMContainerManagerMetrics
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 110.01 s - in org.apache.hadoop.hdds.scm.node.TestQueryNode
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 37.104 s - in org.apache.hadoop.ozone.TestContainerOperations
```
After:
```
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.832 s - in org.apache.ozone.test.HATests$StorageContainerManagerHAWithAllRunning
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 111.809 s - in org.apache.hadoop.hdds.scm.TestStorageContainerManagerHA
Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.607 s - in org.apache.ozone.test.NonHATests$XceiverClientManager
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.82 s - in org.apache.ozone.test.NonHATests$SCMContainerManagerMetrics
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 44.172 s - in org.apache.hadoop.hdds.scm.node.TestQueryNode
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.226 s - in org.apache.ozone.test.NonHATests$ContainerOperations
```
https://github.com/adoroszlai/ozone/actions/runs/13411975449
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #7932:
URL: https://github.com/apache/ozone/pull/7932#issuecomment-2670739606
Thanks @nandakumar131, @sodonnel for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #7932:
URL: https://github.com/apache/ozone/pull/7932#issuecomment-2670702814
Thanks @nandakumar131 for the comments, created HDDS-12387.
I try to make minimal changes as part of these "runtime" changes to make them easier to review. Most of the changes are usually in setup/cleanup. Test logic needs to be changed when it assumes a fresh cluster.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 merged PR #7932:
URL: https://github.com/apache/ozone/pull/7932
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #7932:
URL: https://github.com/apache/ozone/pull/7932#issuecomment-2670722008
Thanks @adoroszlai for the contribution and @sodonnel for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #7932:
URL: https://github.com/apache/ozone/pull/7932#discussion_r1962970250
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java:
##########
@@ -199,35 +193,83 @@ public void testDatanodeUsageInfoCompatibility() throws IOException {
}
@Test
- public void testDatanodeUsageInfoContainerCount() throws IOException {
- List<DatanodeDetails> dnList = cluster.getStorageContainerManager()
+ public void testDatanodeUsageInfoContainerCount() throws Exception {
+ List<DatanodeDetails> dnList = cluster().getStorageContainerManager()
.getScmNodeManager()
.getAllNodes();
for (DatanodeDetails dn : dnList) {
+ final int expected = cluster().getStorageContainerManager().getScmNodeManager().getContainers(dn).size();
+
List<HddsProtos.DatanodeUsageInfoProto> usageInfoList =
storageClient.getDatanodeUsageInfo(
dn.getIpAddress(), dn.getUuidString());
assertEquals(1, usageInfoList.size());
- assertEquals(0, usageInfoList.get(0).getContainerCount());
+ assertEquals(expected, usageInfoList.get(0).getContainerCount());
}
+ }
- storageClient.createContainer(HddsProtos
- .ReplicationType.STAND_ALONE, HddsProtos.ReplicationFactor
- .ONE, OzoneConsts.OZONE);
+ @Test
+ public void testHealthyNodesCount() throws Exception {
+ List<HddsProtos.Node> nodes = storageClient.queryNode(null, HEALTHY,
+ HddsProtos.QueryScope.CLUSTER, "");
+ assertEquals(cluster().getHddsDatanodes().size(), nodes.size(), "Expected live nodes");
+ }
- int[] totalContainerCount = new int[2];
- for (DatanodeDetails dn : dnList) {
- List<HddsProtos.DatanodeUsageInfoProto> usageInfoList =
- storageClient.getDatanodeUsageInfo(
- dn.getIpAddress(), dn.getUuidString());
+ @Test
+ public void testNodeOperationalStates() throws Exception {
+ StorageContainerManager scm = cluster().getStorageContainerManager();
+ NodeManager nm = scm.getScmNodeManager();
+ final int numOfDatanodes = nm.getAllNodes().size();
- assertEquals(1, usageInfoList.size());
- assertThat(usageInfoList.get(0).getContainerCount()).isGreaterThanOrEqualTo(0).isLessThanOrEqualTo(1);
- totalContainerCount[(int)usageInfoList.get(0).getContainerCount()]++;
+ // Set one node to be something other than IN_SERVICE
+ DatanodeDetails node = nm.getAllNodes().get(0);
+ nm.setNodeOperationalState(node, DECOMMISSIONING);
+
+ // All nodes should be returned as they are all in service
+ int nodeCount = storageClient.queryNode(IN_SERVICE, HEALTHY,
+ HddsProtos.QueryScope.CLUSTER, "").size();
+ assertEquals(numOfDatanodes - 1, nodeCount);
+
+ // null acts as wildcard for opState
+ nodeCount = storageClient.queryNode(null, HEALTHY,
+ HddsProtos.QueryScope.CLUSTER, "").size();
+ assertEquals(numOfDatanodes, nodeCount);
+
+ // null acts as wildcard for nodeState
+ nodeCount = storageClient.queryNode(IN_SERVICE, null,
+ HddsProtos.QueryScope.CLUSTER, "").size();
+ assertEquals(numOfDatanodes - 1, nodeCount);
+
+ // Both null - should return all nodes
+ nodeCount = storageClient.queryNode(null, null,
+ HddsProtos.QueryScope.CLUSTER, "").size();
+ assertEquals(numOfDatanodes, nodeCount);
+
+ // No node should be returned
+ nodeCount = storageClient.queryNode(IN_MAINTENANCE, HEALTHY,
+ HddsProtos.QueryScope.CLUSTER, "").size();
+ assertEquals(0, nodeCount);
+
+ // Test all operational states by looping over them all and setting the
+ // state manually.
+ node = nm.getAllNodes().get(0);
+ HddsProtos.NodeOperationalState originalState = nm.getNodeStatus(node).getOperationalState();
+ try {
+ for (HddsProtos.NodeOperationalState s :
+ HddsProtos.NodeOperationalState.values()) {
+ nm.setNodeOperationalState(node, s);
+ nodeCount = storageClient.queryNode(s, HEALTHY,
+ HddsProtos.QueryScope.CLUSTER, "").size();
+ if (s == IN_SERVICE) {
+ assertEquals(5, nodeCount);
Review Comment:
Can we use `cluster().getHddsDatanodes().size()` instead of hardcoded value `5`?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/ozone/test/NonHATests.java:
##########
@@ -50,6 +51,14 @@ public MiniOzoneCluster cluster() {
}
}
+ @Nested
+ class ContainerOperations extends TestContainerOperations {
Review Comment:
Nit: Can we have the fully qualified name to be consistent with other nested classes? or use import for all the test classes?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java:
##########
@@ -27,68 +31,60 @@
import java.io.IOException;
import java.util.List;
-import java.util.concurrent.TimeUnit;
import org.apache.commons.lang3.RandomStringUtils;
import org.apache.commons.lang3.RandomUtils;
import org.apache.hadoop.hdds.client.BlockID;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.DatanodeDetails;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
-import org.apache.hadoop.hdds.scm.PlacementPolicy;
-import org.apache.hadoop.hdds.scm.ScmConfigKeys;
-import org.apache.hadoop.hdds.scm.XceiverClientManager;
-import org.apache.hadoop.hdds.scm.XceiverClientSpi;
import org.apache.hadoop.hdds.scm.cli.ContainerOperationClient;
import org.apache.hadoop.hdds.scm.client.ScmClient;
import org.apache.hadoop.hdds.scm.container.common.helpers.ContainerWithPipeline;
-import org.apache.hadoop.hdds.scm.container.placement.algorithms.SCMContainerPlacementCapacity;
import org.apache.hadoop.hdds.scm.ha.SCMHAUtils;
+import org.apache.hadoop.hdds.scm.node.NodeManager;
import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
import org.apache.hadoop.hdds.scm.pipeline.PipelineNotFoundException;
import org.apache.hadoop.hdds.scm.protocolPB.StorageContainerLocationProtocolClientSideTranslatorPB;
+import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
import org.apache.hadoop.hdds.scm.storage.ContainerProtocolCalls;
-import org.apache.hadoop.io.IOUtils;
+import org.apache.hadoop.hdds.utils.IOUtils;
+import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.container.ContainerTestHelper;
+import org.apache.ozone.test.NonHATests;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.Timeout;
/**
* This class tests container operations (TODO currently only supports create)
* from cblock clients.
*/
-@Timeout(value = 300, unit = TimeUnit.SECONDS)
-public class TestContainerOperations {
- private static ScmClient storageClient;
- private static MiniOzoneCluster cluster;
- private static OzoneConfiguration ozoneConf;
- private static StorageContainerLocationProtocolClientSideTranslatorPB
- storageContainerLocationClient;
- private static XceiverClientManager xceiverClientManager;
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+@Timeout(300)
+public abstract class TestContainerOperations implements NonHATests.TestCase {
+
+ private static final int CONTAINER_LIST_LIMIT = 1;
+
+ private ScmClient storageClient;
+ private StorageContainerLocationProtocolClientSideTranslatorPB storageContainerLocationClient;
+ private XceiverClientManager xceiverClientManager;
@BeforeAll
- public static void setup() throws Exception {
- ozoneConf = new OzoneConfiguration();
- ozoneConf.setClass(ScmConfigKeys.OZONE_SCM_CONTAINER_PLACEMENT_IMPL_KEY,
- SCMContainerPlacementCapacity.class, PlacementPolicy.class);
- ozoneConf.set(ScmConfigKeys.OZONE_SCM_CONTAINER_LIST_MAX_COUNT, "1");
- cluster = MiniOzoneCluster.newBuilder(ozoneConf).setNumDatanodes(3).build();
- storageClient = new ContainerOperationClient(ozoneConf);
- cluster.waitForClusterToBeReady();
- storageContainerLocationClient =
- cluster.getStorageContainerLocationClient();
- xceiverClientManager = new XceiverClientManager(ozoneConf);
+ void setup() throws Exception {
+ OzoneConfiguration clientConf = new OzoneConfiguration(cluster().getConf());
+ clientConf.setInt(ScmConfigKeys.OZONE_SCM_CONTAINER_LIST_MAX_COUNT, CONTAINER_LIST_LIMIT);
+ storageClient = new ContainerOperationClient(clientConf);
+ storageContainerLocationClient = cluster().getStorageContainerLocationClient();
Review Comment:
Can we use `storageClient` in place of `storageContainerLocationClient` and remove `storageContainerLocationClient` altogether in the test?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java:
##########
@@ -199,35 +193,83 @@ public void testDatanodeUsageInfoCompatibility() throws IOException {
}
@Test
- public void testDatanodeUsageInfoContainerCount() throws IOException {
- List<DatanodeDetails> dnList = cluster.getStorageContainerManager()
+ public void testDatanodeUsageInfoContainerCount() throws Exception {
+ List<DatanodeDetails> dnList = cluster().getStorageContainerManager()
.getScmNodeManager()
.getAllNodes();
for (DatanodeDetails dn : dnList) {
+ final int expected = cluster().getStorageContainerManager().getScmNodeManager().getContainers(dn).size();
+
List<HddsProtos.DatanodeUsageInfoProto> usageInfoList =
storageClient.getDatanodeUsageInfo(
dn.getIpAddress(), dn.getUuidString());
assertEquals(1, usageInfoList.size());
- assertEquals(0, usageInfoList.get(0).getContainerCount());
+ assertEquals(expected, usageInfoList.get(0).getContainerCount());
}
+ }
- storageClient.createContainer(HddsProtos
- .ReplicationType.STAND_ALONE, HddsProtos.ReplicationFactor
- .ONE, OzoneConsts.OZONE);
+ @Test
+ public void testHealthyNodesCount() throws Exception {
+ List<HddsProtos.Node> nodes = storageClient.queryNode(null, HEALTHY,
+ HddsProtos.QueryScope.CLUSTER, "");
+ assertEquals(cluster().getHddsDatanodes().size(), nodes.size(), "Expected live nodes");
+ }
- int[] totalContainerCount = new int[2];
- for (DatanodeDetails dn : dnList) {
- List<HddsProtos.DatanodeUsageInfoProto> usageInfoList =
- storageClient.getDatanodeUsageInfo(
- dn.getIpAddress(), dn.getUuidString());
+ @Test
+ public void testNodeOperationalStates() throws Exception {
+ StorageContainerManager scm = cluster().getStorageContainerManager();
+ NodeManager nm = scm.getScmNodeManager();
+ final int numOfDatanodes = nm.getAllNodes().size();
- assertEquals(1, usageInfoList.size());
- assertThat(usageInfoList.get(0).getContainerCount()).isGreaterThanOrEqualTo(0).isLessThanOrEqualTo(1);
- totalContainerCount[(int)usageInfoList.get(0).getContainerCount()]++;
+ // Set one node to be something other than IN_SERVICE
+ DatanodeDetails node = nm.getAllNodes().get(0);
+ nm.setNodeOperationalState(node, DECOMMISSIONING);
+
+ // All nodes should be returned as they are all in service
Review Comment:
This comment seems incorrect, one of the node is in `DECOMMISSIONING` state. The code is correct in asserting `numOfdatanodes - 1`, just the comment is misleading.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7932:
URL: https://github.com/apache/ozone/pull/7932#discussion_r1962997477
##########
hadoop-ozone/integration-test/src/test/java/org/apache/ozone/test/NonHATests.java:
##########
@@ -50,6 +51,14 @@ public MiniOzoneCluster cluster() {
}
}
+ @Nested
+ class ContainerOperations extends TestContainerOperations {
Review Comment:
I plan to add imports eventually, but using fully-qualified names reduces conflicts while working on multiple tasks in parallel.
In this case, full name was over checkstyle's line length limit, so I added the import.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12349. Speed up some HDDS integration tests [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #7932:
URL: https://github.com/apache/ozone/pull/7932#issuecomment-2670720876
@adoroszlai makes sense! Thanks for creating the jira.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org