Add bwc tests to run with CI #181

VachaShah · 2021-08-17T06:36:27Z

Description

Adding bwc tests introduced in #158 to run with CI for anomaly-detection. Also adding the test run commands in the developer guide.

Issues Resolved

opensearch-build#222

Check List

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: Vacha <vachshah@amazon.com>

codecov-commenter · 2021-08-17T06:55:12Z

Codecov Report

Merging #181 (8bf54ca) into main (878918e) will decrease coverage by 0.40%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main     #181      +/-   ##
============================================
- Coverage     67.66%   67.25%   -0.41%     
- Complexity     2885     2896      +11     
============================================
  Files           270      270              
  Lines         14104    14241     +137     
  Branches       1418     1428      +10     
============================================
+ Hits           9543     9578      +35     
- Misses         3973     4070      +97     
- Partials        588      593       +5

Flag	Coverage Δ
plugin	`67.25% <ø> (-0.41%)`	⬇️

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

Impacted Files	Coverage Δ
...ava/org/opensearch/ad/task/ADTaskCacheManager.java	`80.88% <0.00%> (-3.30%)`	⬇️
...ava/org/opensearch/ad/task/ADHCBatchTaskCache.java	`59.45% <0.00%> (-1.97%)`	⬇️
...java/org/opensearch/ad/task/ADBatchTaskRunner.java	`25.57% <0.00%> (-1.36%)`	⬇️
...c/main/java/org/opensearch/ad/util/ParseUtils.java	`62.13% <0.00%> (-1.31%)`	⬇️
.../java/org/opensearch/ad/model/AnomalyDetector.java	`96.13% <0.00%> (-0.47%)`	⬇️
src/main/java/org/opensearch/ad/model/Entity.java	`72.89% <0.00%> (-0.26%)`	⬇️
src/main/java/org/opensearch/ad/model/ADTask.java	`88.94% <0.00%> (-0.23%)`	⬇️
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java	`94.73% <0.00%> (ø)`
...java/org/opensearch/ad/feature/FeatureManager.java	`93.91% <0.00%> (ø)`
...pensearch/ad/settings/AnomalyDetectorSettings.java	`100.00% <0.00%> (ø)`
... and 8 more

saratvemulapalli

Couple of questions, most of the changes look good to me.

saratvemulapalli · 2021-08-17T16:31:21Z

.github/workflows/CI.yml

+          ./gradlew assemble -Dopensearch.version=1.1.0-SNAPSHOT
+          echo "Creating ./src/test/resources/org/opensearch/ad/bwc/anomaly-detection/1.1.0.0-SNAPSHOT ..."
+          mkdir -p ./src/test/resources/org/opensearch/ad/bwc/anomaly-detection/1.1.0.0-SNAPSHOT
+          pwd


nit: We should be able to remove this

Suggested change

pwd

Removed it.

saratvemulapalli · 2021-08-17T17:09:47Z

.github/workflows/CI.yml

+        run: |
+          ./gradlew assemble -Dopensearch.version=1.1.0-SNAPSHOT
+          echo "Creating ./src/test/resources/org/opensearch/ad/bwc/anomaly-detection/1.1.0.0-SNAPSHOT ..."
+          mkdir -p ./src/test/resources/org/opensearch/ad/bwc/anomaly-detection/1.1.0.0-SNAPSHOT


Do we need this?
Gradle plugin automatically can pick up the plugin from the build directory.
Take a look at: https://github.com/opensearch-project/anomaly-detection/blob/main/build.gradle#L235

This is needed since the upgradeNodeAndPluginToNextVersion method is designed to take in a list of Provider<RegularFile> for installing plugins on the node during the upgrade. The plugin method is separate which has two implementations, one for taking in a file and one for taking in the path which is used for the node.

Got it. That makes sense. I think it worth overloading that function to have another method to take care of this.
We should avoid as much as possible manually copying artifacts for tests.

We can goahead with this change, but can you also make a change in OpenSearch to add this functionality?

So this method has the file list as input since the job-scheduler artifact would be a zip file and since AD has a dependency on that, we have to supply it at the same time so that the newer version plugins can be added when the node is upgraded. The new functionality can be useful for plugins that don't have any other plugins as dependency.

saratvemulapalli · 2021-08-17T17:13:44Z

.github/workflows/CI.yml

+      - name: Mixed cluster backwards compatibility tests
+        run: |
+          echo "Running mixed cluster backwards compatibility tests ..."
+          ./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=123 -Dtests.security.manager=false


I understand we are using mixedCluster variable to run some specific tests.
Can we use a boolean flag instead of an arbitrary number like 123?

Suggested change

./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=123 -Dtests.security.manager=false

./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=true -Dtests.security.manager=false

Makes sense, updated it to true for the flag.

saratvemulapalli · 2021-08-17T17:16:24Z

.github/workflows/CI.yml

+          echo "Running mixed cluster backwards compatibility tests ..."
+          ./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=123 -Dtests.security.manager=false
+
+      - name: Rolling upgrade backwards compatibility tests


This is nice to separate out dedicated tests but is there a way to run all BWC tests with a single command?

There is currently no task to run that, I can add one that runs all three of them together.

Yeah definitely, I think its worth adding that task and will help us when we would want to publish nightly builds and run these tests in one go.

Definitely in agreement, I will work on adding a single task which can be invoked to run the bwc tests after this PR.

Signed-off-by: Vacha <vachshah@amazon.com>

saratvemulapalli

Thanks @VachaShah for this change!
This is great.

I would like to get AD team to also take a look.
@ylwu-amzn can you review this change?

ylwu-amzn · 2021-08-17T22:10:43Z

DEVELOPER_GUIDE.md

@@ -41,6 +41,9 @@ Currently we just put RCF jar in lib as dependency. Plan to publish to Maven and
 4. ` ./gradlew :integTest --tests="**.test execute foo"` runs a single integration test class or method
 5. `./gradlew integTest -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername="docker-cluster" -Dhttps=true -Duser=admin -Dpassword=admin` launches integration tests against a local cluster and run tests with security
 6. `./gradlew spotlessApply` formats code. And/or import formatting rules in `.eclipseformat.xml` with IDE.
+7. `./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=true -Dtests.security.manager=false` launches a cluster with three nodes of bwc version of OpenSearch with anomaly-detection and job-scheduler and tests backwards compatibility by upgrading one of the nodes with the current version of OpenSearch with anomaly-detection and job-scheduler creating a mixed cluster.


Will the three nodes run three different OpenSearch versions? Or 1 node runs old version, other 2 nodes run new version?

The old version cluster will have 3 nodes of the old (same) version of OpenSearch and then one node will be upgraded to current version (for example 1.1) along with anomaly-detection and job-scheduler upgrade to 1.1 on that node. 2 other nodes will still be on the old version, this creates a mixed cluster.

ylwu-amzn · 2021-08-17T22:13:11Z

DEVELOPER_GUIDE.md

@@ -41,6 +41,9 @@ Currently we just put RCF jar in lib as dependency. Plan to publish to Maven and
 4. ` ./gradlew :integTest --tests="**.test execute foo"` runs a single integration test class or method
 5. `./gradlew integTest -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername="docker-cluster" -Dhttps=true -Duser=admin -Dpassword=admin` launches integration tests against a local cluster and run tests with security
 6. `./gradlew spotlessApply` formats code. And/or import formatting rules in `.eclipseformat.xml` with IDE.
+7. `./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=true -Dtests.security.manager=false` launches a cluster with three nodes of bwc version of OpenSearch with anomaly-detection and job-scheduler and tests backwards compatibility by upgrading one of the nodes with the current version of OpenSearch with anomaly-detection and job-scheduler creating a mixed cluster.
+8. `./gradlew adBwcCluster#rollingUpgradeClusterTask -DrollingUpgradeCluster=true -Dtests.security.manager=false` launches a cluster with three nodes of bwc version of OpenSearch with anomaly-detection and job-scheduler and tests backwards compatibility by performing rolling upgrade of each node with the current version of OpenSearch with anomaly-detection and job-scheduler.
+9. `./gradlew adBwcCluster#fullRestartClusterTask -DfullRestartCluster=true -Dtests.security.manager=false` launches a cluster with three nodes of bwc version of OpenSearch with anomaly-detection and job-scheduler and tests backwards compatibility by performing a full restart on the cluster upgrading all the nodes with the current version of OpenSearch with anomaly-detection and job-scheduler.


Do we have some historical data of old version ready before full restart ? For AD, that will be create some detectors and run it to generate some AD result; then fully restart and test.

Same question for other 2 BWC tests: mixed cluster and rolling upgrade.

AD can definitely add more assertions to the tests according to the various scenarios.

ylwu-amzn

LGTM. Thanks for the change!

* Adding support for BWC tests to run in CI Signed-off-by: Vacha <vachshah@amazon.com> * Adding bwc test commands in developer guide Signed-off-by: Vacha <vachshah@amazon.com>

Adding support for BWC tests to run in CI

892d4cc

Signed-off-by: Vacha <vachshah@amazon.com>

VachaShah changed the title ~~Add bwc to ci~~ Add bwc tests to run with CI Aug 17, 2021

VachaShah requested a review from saratvemulapalli August 17, 2021 06:38

saratvemulapalli reviewed Aug 17, 2021

View reviewed changes

saratvemulapalli requested a review from ylwu-amzn August 17, 2021 19:58

Adding bwc test commands in developer guide

8bf54ca

Signed-off-by: Vacha <vachshah@amazon.com>

VachaShah force-pushed the add-bwc-to-ci branch from 0613533 to 8bf54ca Compare August 17, 2021 21:11

VachaShah requested a review from saratvemulapalli August 17, 2021 21:40

saratvemulapalli approved these changes Aug 17, 2021

View reviewed changes

ylwu-amzn reviewed Aug 17, 2021

View reviewed changes

VachaShah requested a review from ylwu-amzn August 17, 2021 22:16

ylwu-amzn approved these changes Aug 17, 2021

View reviewed changes

VachaShah merged commit 0ccbb0c into opensearch-project:main Aug 17, 2021

VachaShah deleted the add-bwc-to-ci branch August 19, 2021 22:16

VachaShah mentioned this pull request Aug 23, 2021

Add backwards compatibility tests for automation #188

Closed

ohltyler pushed a commit that referenced this pull request Sep 1, 2021

Add bwc tests to run with CI (#181)

30ab325

* Adding support for BWC tests to run in CI Signed-off-by: Vacha <vachshah@amazon.com> * Adding bwc test commands in developer guide Signed-off-by: Vacha <vachshah@amazon.com>

ohltyler added the infra Changes to infrastructure, testing, CI/CD, pipelines, etc. label Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bwc tests to run with CI #181

Add bwc tests to run with CI #181

VachaShah commented Aug 17, 2021

codecov-commenter commented Aug 17, 2021 •

edited

Loading

saratvemulapalli left a comment

saratvemulapalli Aug 17, 2021 •

edited

Loading

VachaShah Aug 17, 2021

saratvemulapalli Aug 17, 2021

VachaShah Aug 17, 2021

saratvemulapalli Aug 17, 2021

VachaShah Aug 17, 2021

saratvemulapalli Aug 17, 2021

VachaShah Aug 17, 2021

saratvemulapalli Aug 17, 2021

VachaShah Aug 17, 2021

saratvemulapalli Aug 17, 2021

VachaShah Aug 17, 2021

saratvemulapalli left a comment

ylwu-amzn Aug 17, 2021

VachaShah Aug 17, 2021

ylwu-amzn Aug 17, 2021

VachaShah Aug 17, 2021

ylwu-amzn left a comment

	./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=123 -Dtests.security.manager=false
	./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=true -Dtests.security.manager=false

Add bwc tests to run with CI #181

Add bwc tests to run with CI #181

Conversation

VachaShah commented Aug 17, 2021

Description

Issues Resolved

Check List

codecov-commenter commented Aug 17, 2021 • edited Loading

Codecov Report

saratvemulapalli left a comment

Choose a reason for hiding this comment

saratvemulapalli Aug 17, 2021 • edited Loading

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

saratvemulapalli 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

ylwu-amzn left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 17, 2021 •

edited

Loading

saratvemulapalli Aug 17, 2021 •

edited

Loading