Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding mixed cluster, rolling upgrade, restart upgrade bwc tests #158

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

VachaShah
Copy link
Contributor

@VachaShah VachaShah commented Aug 5, 2021

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

Description

Adding bwc tests and the supporting test framework for bwc. This is part of automating backwards compatibility support for the OpenSearch project. The bwc scenarios covered are:

  1. Mixed cluster
  2. Rolling Upgrade
  3. Restart Upgrade

To run the mixed cluster test: ./gradlew adBwcCluster#mixedClusterTask -DmixedCluster=123 -Dtests.security.manager=false

To run the rolling upgrade test: ./gradlew adBwcCluster#rollingUpgradeClusterTask -DrollingUpgradeCluster=123 -Dtests.security.manager=false

To run the restart upgrade test: ./gradlew adBwcCluster#fullRestartClusterTask -DfullRestartCluster=123 -Dtests.security.manager=false

These tests will be hooked up to CI as part of a separate PR.

Issues Resolved

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
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #158 (ef71fb1) into main (6843b29) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #158      +/-   ##
============================================
+ Coverage     70.30%   70.36%   +0.06%     
- Complexity     2970     2975       +5     
============================================
  Files           268      269       +1     
  Lines         14051    14056       +5     
  Branches       1409     1410       +1     
============================================
+ Hits           9878     9890      +12     
+ Misses         3554     3551       -3     
+ Partials        619      615       -4     
Flag Coverage Δ
plugin 70.36% <ø> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...pensearch/ad/settings/AnomalyDetectorSettings.java 100.00% <0.00%> (ø)
src/main/java/org/opensearch/ad/util/MathUtil.java 100.00% <0.00%> (ø)
...opensearch/ad/indices/AnomalyDetectionIndices.java 51.87% <0.00%> (+0.31%) ⬆️
src/main/java/org/opensearch/ad/MemoryTracker.java 77.38% <0.00%> (+1.13%) ⬆️
.../opensearch/ad/cluster/ADClusterEventListener.java 90.90% <0.00%> (+1.81%) ⬆️
...search/ad/transport/AnomalyDetectorJobRequest.java 100.00% <0.00%> (+13.51%) ⬆️

@VachaShah VachaShah marked this pull request as ready for review August 10, 2021 20:59
@@ -184,6 +185,12 @@ integTest {
}
}

if (System.getProperty("tests.rest.suite") == null) {
filter {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the tests are explicitly excluded in integTests, if BWC tests do not have any dependencies we should just let it run as part of the integtests which will be part of gradle build and will run in CI for every PR commit.

Ref: AD CI https://github.com/opensearch-project/anomaly-detection/blob/main/.github/workflows/CI.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are currently excluded from the integTest as they need some additional setup to be run in CI, if I include them with integTest right now without the changes, the workflow would fail. I figured that we could exclude currently and then hook them to CI (as part of integTest) as part of the next task. LMK what you think.

@@ -259,6 +266,97 @@ testClusters.integTest {
}
}

String bwcVersion = "1.13.0.0";
Copy link
Member

Choose a reason for hiding this comment

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

Lets test with last known latest release, I believe it was 1.13.2
Ref: https://opendistro.github.io/for-elasticsearch-docs/version-history/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Got it thank you.

testClusters {
"${baseName}" {
testDistribution = "ARCHIVE"
versions = ["7.10.2","1.0.0"]
Copy link
Member

Choose a reason for hiding this comment

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

We should have a way to define these versions as this is not scalable for each release.
We should open an issue for this, hopefully the plugin owners can help contribute it for us if not we can pick it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is temporary, we will definitely need a way to define versions for each release. I will open an issue for this.

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

This is awesome, just love this.
Dropped a few comments.

return new RegularFile() {
@Override
File getAsFile() {
return fileTree(bwcFilePath + "job-scheduler/" + bwcVersion).getSingleFile()
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment here.
We should be able to predictably get older releases of opensearch project plugins from an artifact repository.
Can you open an issue in opensearch-build to track this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build.gradle Outdated
}
}
}))
setting 'repositories.url.allowed_urls', 'http://snapshot.test*'
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is needed for snapshot tests and not needed for mixed cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

build.gradle Outdated
includeTestsMatching "org.opensearch.ad.bwc.*IT"
}
}
systemProperty 'tests.rest.suite', 'old_cluster'
Copy link
Member

Choose a reason for hiding this comment

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

rest.suite seems ambiguous, what do you think of renaming it to rest.bwcsuite?

Suggested change
systemProperty 'tests.rest.suite', 'old_cluster'
systemProperty 'tests.rest.bwcsuite', 'old_cluster'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! Renamed it to tests.rest.bwcsuite.

build.gradle Outdated
nonInputProperties.systemProperty('tests.clustername', "${-> testClusters."${baseName}".getName()}")
}

task "${baseName}#mixedClusterTask"(type: StandaloneRestIntegTestTask) {
Copy link
Member

Choose a reason for hiding this comment

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

Couple of comments:

  1. I see build.gradle is creeping up with lots of changes.
    Can all of this logic be abstracted in bwc.gradle and consumed here?
  2. Can you add comments to each of the steps, it would be helpful for other plugins owners to understand why these steps are required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I have created an issue for this Split the build.gradle to make it cleaner #167, will work on that.
  2. Added comments in the steps for all types of upgrades.

Signed-off-by: Vacha <vachshah@amazon.com>
@VachaShah VachaShah changed the title Adding mixed cluster bwc tests Adding mixed cluster, rolling upgrade, restart upgrade bwc tests Aug 12, 2021
Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Overall the changes look good to me.
I'll take another pass and would also want another reviewer from AD team since they own this code base.

build.gradle Outdated
})
]

//Creates a test cluster with 3 nodes of the old version.
Copy link
Member

Choose a reason for hiding this comment

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

super minor nit: space after comment escape string.

Suggested change
//Creates a test cluster with 3 nodes of the old version.
// Creates a test cluster with 3 nodes of the old version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

super minor nit: space after comment escape string.

Updated for all the comments.

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

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

The changes look good to me.
Thanks @VachaShah!

ohltyler pushed a commit to ohltyler/anomaly-detection-2 that referenced this pull request Sep 1, 2021
…nsearch-project#158)

* Adding mixed cluster bwc tests

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

* Adding anomaly detector assertions for bwc

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

* Adding rolling upgrade and full restart upgrade bwc tests

Signed-off-by: Vacha <vachshah@amazon.com>
ohltyler pushed a commit to ohltyler/anomaly-detection-2 that referenced this pull request Sep 1, 2021
…nsearch-project#158)

* Adding mixed cluster bwc tests

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

* Adding anomaly detector assertions for bwc

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

* Adding rolling upgrade and full restart upgrade bwc tests

Signed-off-by: Vacha <vachshah@amazon.com>
ohltyler pushed a commit that referenced this pull request Sep 1, 2021
* Adding mixed cluster bwc tests

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

* Adding anomaly detector assertions for bwc

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

* Adding rolling upgrade and full restart upgrade bwc tests

Signed-off-by: Vacha <vachshah@amazon.com>
peternied added a commit to peternied/security that referenced this pull request Mar 2, 2022
Using opensearch-project/anomaly-detection#158
as a template, created those changes as a seperate gradle file.

Had trouble with the dependency and import statements so reordered how
the buildscripts were setup.  Running into trouble with the applying the
'opensearch.testclusters' dependency because
org/gradle/jvm/toolchain/JavaInstallation was not found

Signed-off-by: Peter Nied <petern@amazon.com>
peternied added a commit to peternied/security that referenced this pull request Mar 7, 2022
Using opensearch-project/anomaly-detection#158
as a template, created those changes as a seperate gradle file.

Gradlew execution of tests has been implemented, but untested.  The
tests themselves have been started but do not have the actual security
plugin setup/validation logic that we would like to use.  Scan for TODO
within the files.

Signed-off-by: Peter Nied <petern@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants