1713: add multi-cluster IT testing class for Cross-Cluster support #319

acarbonetto · 2023-08-01T22:27:57Z

Description

Adds multiClusterSearch IT test task that creates a 3-node cluster and additional cross-cluster for CrossClusterSearchIT testing, with the Security plugin enabled.

This will bring IT test infrastructure more in-step with the release workflow to avoid security-plugin issues in the future.

Issues Resolved

opensearch-project#1713

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: acarbonetto <andrewc@bitquilltech.com>

codecov · 2023-08-01T22:48:46Z

Codecov Report

Merging #319 (c23ed04) into integ-add-security-plugin-it-1713 (d00dc4d) will not change coverage.
The diff coverage is n/a.

@@                         Coverage Diff                          @@
##             integ-add-security-plugin-it-1713     #319   +/-   ##
====================================================================
  Coverage                                97.42%   97.42%           
  Complexity                                4647     4647           
====================================================================
  Files                                      408      408           
  Lines                                    11526    11526           
  Branches                                   839      839           
====================================================================
  Hits                                     11229    11229           
  Misses                                     290      290           
  Partials                                     7        7

Flag	Coverage Δ
sql-engine	`97.42% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

MitchellGale · 2023-08-01T23:17:40Z

integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java

@@ -105,10 +105,10 @@ protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOE
  }

  // Modified from initClient in OpenSearchRestTestCase
-  public void initRemoteClient() throws IOException {
+  public void initRemoteClient(String clusterName) throws IOException {


Does this not reduce potential future reuse of this function?

The cluster name is defined in the gradle task. Sean hardcoded the name in a static variable, so this only used to work with the remoteCluster, and now it works with whatever the gradle task has named the cluster.
So... no. I think this is more reusable?

GumpacG

Can you also add a test that checks if https url works. I think default port when security enabled is 443.

GumpacG · 2023-08-02T14:59:31Z

.github/workflows/sql-test-workflow.yml

        ./gradlew :integ-test:integTest                   || echo "* Integration test failed" >> report.log
+        ./gradlew :integ-test:multiClusterSearch          || echo "* Multi-Cluster Search tests failed" >> report.log


Since this is for every push, can we run it after doctests? For development, doctests are more relevant to us than testing with security enabled.

I thought this was akin to Integration Testing, just against a different cluster. The tests run are integration tests.
I would categorize doctests are more API testing which seems more highlevel... but is just another form of Integration Test.
So... 🤷

Right it's more so when we push and we want to know if all integ test and doctest pass we would have to wait x time for mutlicluster IT to finish (spin up, run tests, and clean up) before getting our results. While the ITs added here are really only relevant for release testing.

This CI runs on demand only

If you want to run these tests in every push/PR, add it to sql-test-and-build-workflow.yml

GumpacG · 2023-08-02T15:00:02Z

integ-test/build.gradle

@@ -301,7 +304,7 @@ integTest {
    exclude 'org/opensearch/sql/jdbc/**'

    // Exclude this IT until running IT with security plugin enabled is ready
-    exclude 'org/opensearch/sql/ppl/CrossClusterSearchIT.class'
+    // exclude 'org/opensearch/sql/ppl/CrossClusterSearchIT.class'


Signed-off-by: acarbonetto <andrewc@bitquilltech.com>

Yury-Fridlyand · 2023-08-04T17:19:35Z

integ-test/build.gradle

@@ -301,7 +304,7 @@ integTest {
    exclude 'org/opensearch/sql/jdbc/**'

    // Exclude this IT until running IT with security plugin enabled is ready
-    exclude 'org/opensearch/sql/ppl/CrossClusterSearchIT.class'
+    // exclude 'org/opensearch/sql/ppl/CrossClusterSearchIT.class'


Yury-Fridlyand · 2023-08-04T17:34:23Z

.github/workflows/sql-test-workflow.yml

        ./gradlew :integ-test:integTest                   || echo "* Integration test failed" >> report.log
+        ./gradlew :integ-test:multiClusterSearch          || echo "* Multi-Cluster Search tests failed" >> report.log


If you want to run these tests in every push/PR, add it to sql-test-and-build-workflow.yml

Yury-Fridlyand · 2023-08-04T19:02:33Z

integ-test/src/test/java/org/opensearch/sql/multiClusterSearch/CrossClusterSearchIT.java

+import org.opensearch.client.ResponseException;
+import org.opensearch.sql.ppl.PPLIntegTestCase;
+
+public class CrossClusterSearchIT extends PPLIntegTestCase {


Probably add a comment describing difference with ppl/CrossClusterSearchIT

Yury-Fridlyand · 2023-08-04T19:03:04Z

integ-test/build.gradle

+    systemProperty "https", System.getProperty("https")
+    systemProperty "user", System.getProperty("user")
+    systemProperty "password", System.getProperty("password")


This is no-op, can be removed

Yury-Fridlyand · 2023-08-08T18:45:17Z

integ-test/build.gradle

+}
+
+testClusters {


Suggested change

}

testClusters {

1713: add multi-cluster IT testing class for Cross-Cluster support

e0a6f6d

Signed-off-by: acarbonetto <andrewc@bitquilltech.com>

acarbonetto requested review from MaxKsyunz, Yury-Fridlyand, forestmvey and GumpacG as code owners August 1, 2023 22:27

MitchellGale reviewed Aug 1, 2023

View reviewed changes

GumpacG reviewed Aug 2, 2023

View reviewed changes

matthewryanwells approved these changes Aug 2, 2023

View reviewed changes

MitchellGale approved these changes Aug 4, 2023

View reviewed changes

Add multiClusterSearch/CrossClusterSearchIT.java

c23ed04

Signed-off-by: acarbonetto <andrewc@bitquilltech.com>

Yury-Fridlyand approved these changes Aug 4, 2023

View reviewed changes

Yury-Fridlyand reviewed Aug 8, 2023

View reviewed changes

integ-test/build.gradle

Comment on lines +493 to +495

}

testClusters {

Copy link

Yury-Fridlyand Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

}

testClusters {

acarbonetto closed this Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1713: add multi-cluster IT testing class for Cross-Cluster support #319

1713: add multi-cluster IT testing class for Cross-Cluster support #319

acarbonetto commented Aug 1, 2023

codecov bot commented Aug 1, 2023 •

edited

Loading

MitchellGale Aug 1, 2023

acarbonetto Aug 2, 2023

GumpacG left a comment

GumpacG Aug 2, 2023

acarbonetto Aug 2, 2023

GumpacG Aug 2, 2023

Yury-Fridlyand Aug 2, 2023

Yury-Fridlyand Aug 4, 2023

GumpacG Aug 2, 2023

Yury-Fridlyand Aug 4, 2023

Yury-Fridlyand Aug 4, 2023

Yury-Fridlyand Aug 4, 2023

Yury-Fridlyand Aug 4, 2023

Yury-Fridlyand Aug 4, 2023

Yury-Fridlyand Aug 8, 2023

		./gradlew :integ-test:integTest \|\| echo "* Integration test failed" >> report.log
		./gradlew :integ-test:multiClusterSearch \|\| echo "* Multi-Cluster Search tests failed" >> report.log

1713: add multi-cluster IT testing class for Cross-Cluster support #319

1713: add multi-cluster IT testing class for Cross-Cluster support #319

Conversation

acarbonetto commented Aug 1, 2023

Description

Issues Resolved

Check List

codecov bot commented Aug 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GumpacG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 1, 2023 •

edited

Loading