Add backwards compatibility tests by qreshi · Pull Request #199 · opensearch-project/alerting

qreshi · 2021-10-08T20:30:09Z

Issue #, if available: #147

Description of changes:

Adds backwards compatibility tests for mixed cluster, rolling upgrade and full restart scenarios
Updates DEVELOPER_GUIDE to add instructions for running bwc tests
Adds bwc tests to the multi-node GitHub Actions tests workflow

A couple improvements that can probably be made in future iterations:

A hardcoded Monitor in the legacy format (ODFE 1.13.1) is being used for the request on the old version, this is because the toXContent method that is being used in the current version of the plugin test is not forwards compatible, so we would need a workaround or some legacy toXContent method for Monitor if we want to use the randomMonitor() helper method for creating the Monitor request
We can manually update the Monitor execution time to artificially fast forward time when trying to execute the job again instead of sleeping since it inflate the test execution time
[DONE] We can generate the current version of the artifact dynamically and place it in the expected file path when running the bwc tests so we don't have to place an artifact there each time we move the versions along (ex. the 1.1 Alerting version being used is under src/test/resources/bwc/alerting/1.1.0.0-SNAPSHOT/opensearch-alerting-1.1.0.0-SNAPSHOT.zip)

CheckList:
[x] 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: Mohammad Qureshi <qreshi@amazon.com>

…tions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

lezzago · 2021-10-08T22:12:37Z

Is there a reason we are using the snapshot version of alerting 1.1 zip?

… manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

codecov-commenter · 2021-10-09T01:05:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.86%. Comparing base (7959450) to head (078b566).
⚠️ Report is 368 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #199      +/-   ##
============================================
+ Coverage     78.83%   78.86%   +0.02%     
+ Complexity      215      214       -1     
============================================
  Files           172      172              
  Lines          6951     6964      +13     
  Branches        905      908       +3     
============================================
+ Hits           5480     5492      +12     
- Misses          986      987       +1     
  Partials        485      485

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

qreshi · 2021-10-11T23:55:33Z

Is there a reason we are using the snapshot version of alerting 1.1 zip?

I updated the newest artifact to be generated automatically but I think it will always be the snapshot since that's the default for most recent point-in-time artifact being built.

skkosuri-amzn · 2021-10-12T02:27:27Z

alerting/src/test/kotlin/org/opensearch/alerting/bwc/AlertingBackwardsCompatibilityIT.kt

+                    // on time
+                    // TODO: Should probably change the next execution time of the Monitor manually instead since this inflates
+                    //  the test execution by a lot
+                    Thread.sleep(60000)


Can we avoid sleep(...) ?

I had originally planned on adding a utility method like what ISM had added:

https://github.com/opensearch-project/index-management/blob/ab122799283a58bb3f5c6f6d8576c7ba5875f13b/src/test/kotlin/org/opensearch/indexmanagement/indexstatemanagement/IndexStateManagementRestTestCase.kt#L382

As well as a waitFor wrapper so we could add assertions that could wait for the Monitor to execute and confirm state afterwards:

https://github.com/opensearch-project/index-management/blob/ab122799283a58bb3f5c6f6d8576c7ba5875f13b/src/test/kotlin/org/opensearch/indexmanagement/TestHelpers.kt#L133-L149

However, it looks like the JobScheduler implementation of the schedule information stores things like start_time which is convenient to modify for the particular scheduled job you'd like to speed up in test scenarios:

https://github.com/opensearch-project/job-scheduler/blob/main/spi/src/main/java/org/opensearch/jobscheduler/spi/schedule/IntervalSchedule.java#L54

Alerting's scheduled job implementation does not do this and instead uses enabled_time to resolve the next expected execution time only if the expectedPreviousExecutionTime is not set, otherwise it uses that:

alerting/core/src/main/kotlin/org/opensearch/alerting/core/model/Schedule.kt

Line 332 in e0e41df

val expectedPreviousExecutionTimeEpochMillis = (expectedPreviousExecutionTime ?: enabledTime).toEpochMilli()

The expectedPreviousExecutionTime is stored in scheduledJobIdToInfo which is part of a concurrent hashmap which is stored in memory as far as I'm aware:

alerting/core/src/main/kotlin/org/opensearch/alerting/core/schedule/JobScheduler.kt

Line 52 in e0e41df

private val scheduledJobIdToInfo = ConcurrentHashMap<String, ScheduledJobInfo>()

This would make modification of an already running Monitor to execute sooner less trivial than expected. I'd like to leave it out of scope of this particular PR for now and have it be a possible follow-up for a test refactor.

Hmm, looks like these tests do get run automatically as part of ./gradlew build so this would impact build times when running all tests. I was hoping to exclude them unless running something like ./gradlew bwcTestSuite to avoid the overhead for the time being but ./gradlew build will likely pick up all tests by nature.

Thanks for diving deep into this. Currently gradlew build takes around 4mins, this sleep would add one more min.

Actually, since the bwcTestSuite executes the following scenarios:

mixedCluster - Hits the MIXED case

rollingUpgrade - Hits the MIXED and UPGRADED case

fullRestart - Hits the UPGRADED case

We'd be adding 4 minutes of sleep across all the tests since both the MIXED and UPGRADED cases are adding a sleep of 1 minute before verifying.

alerting/src/test/kotlin/org/opensearch/alerting/bwc/AlertingBackwardsCompatibilityIT.kt

.github/workflows/multi-node-test-workflow.yml

alerting/build.gradle

Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

skkosuri-amzn · 2021-11-08T19:59:33Z

@qreshi Please confirm if ./grawdlew build -x bwcTestSuite excludes the bwc tests.

qreshi · 2021-11-08T21:49:53Z

@qreshi Please confirm if ./grawdlew build -x bwcTestSuite excludes the bwc tests.

I checked and it looks like it's not excluding the dependent tasks which run the BWC tests. This could be because they're defined as StandaloneRestIntegTestTask. We might have to exclude all the tasks explicitly.

* Initial commit for BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update bwc test to check Monitor stats and add bwc tests to GitHub Actions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Use current version plugin bundle from build for bwc tests instead of manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update mockito-core dependency to 3.12.4 to prevent conflict Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Remove disabling security manager flag when running BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

* Updates alerting version to 1.2 (#192) * Updates alerting version to 1.2 * Adds snapshot repo to the repository file Signed-off-by: Clay Downs <downsrob@amazon.com> * Update build to use public Maven repo (#184) Signed-off-by: Abbas Hussain <abbas_10690@yahoo.com> * Publish notification JARs checksums. (#196) * Publish notification JARs checksums. Signed-off-by: dblock <dblock@dblock.org> * Remove sonatype staging. Signed-off-by: dblock <dblock@dblock.org> * Updates testCompile mockito version to match OpenSearch changes (#204) Signed-off-by: Clay Downs <downsrob@amazon.com> * Update maven publication to include cksums. (#224) This change adds a task to publish to a local staging repo under build/ that includes cksums. It also updates build.sh to use this new task and copy the contents of the staging repo to the output directory. The maven publish plugin will not include these cksums when publishing to maven local but will when published to a separate folder. Signed-off-by: Marc Handalian <handalm@amazon.com> * Add release notes for 1.2.0.0 release (#225) * Create opensearch-alerting.release-notes-1.2.0.0.md Signed-off-by: Annie Lee <leeyun@amazon.com> * Update opensearch-alerting.release-notes-1.2.0.0.md * Update opensearch-alerting.release-notes-1.2.0.0.md * Add backwards compatibility tests (#199) * Initial commit for BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update bwc test to check Monitor stats and add bwc tests to GitHub Actions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Use current version plugin bundle from build for bwc tests instead of manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update mockito-core dependency to 3.12.4 to prevent conflict Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Remove disabling security manager flag when running BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> Co-authored-by: Clay Downs <89109232+downsrob@users.noreply.github.com> Co-authored-by: Abbas Hussain <abbashus@amazon.com> Co-authored-by: Daniel Doubrovkine (dB.) <dblock@dblock.org> Co-authored-by: Marc Handalian <handalm@amazon.com> Co-authored-by: Annie Lee <71157062+leeyun-amzn@users.noreply.github.com> Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

* Cherry-pick commits to 1.x (#227) * Update copyright notice (#222) Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Admin Users must be able to access all monitors #139 (#220) * Admin Users must be able to access all monitors #139 * Refactored Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com> * Updates alerting version to 1.2 (#192) * Updates alerting version to 1.2 * Adds snapshot repo to the repository file Signed-off-by: Clay Downs <downsrob@amazon.com> * Update build to use public Maven repo (#184) Signed-off-by: Abbas Hussain <abbas_10690@yahoo.com> * Publish notification JARs checksums. (#196) * Publish notification JARs checksums. Signed-off-by: dblock <dblock@dblock.org> * Remove sonatype staging. Signed-off-by: dblock <dblock@dblock.org> * Updates testCompile mockito version to match OpenSearch changes (#204) Signed-off-by: Clay Downs <downsrob@amazon.com> * Update maven publication to include cksums. (#224) This change adds a task to publish to a local staging repo under build/ that includes cksums. It also updates build.sh to use this new task and copy the contents of the staging repo to the output directory. The maven publish plugin will not include these cksums when publishing to maven local but will when published to a separate folder. Signed-off-by: Marc Handalian <handalm@amazon.com> * Add release notes for 1.2.0.0 release (#225) * Create opensearch-alerting.release-notes-1.2.0.0.md Signed-off-by: Annie Lee <leeyun@amazon.com> * Update opensearch-alerting.release-notes-1.2.0.0.md * Update opensearch-alerting.release-notes-1.2.0.0.md * Add backwards compatibility tests (#199) * Initial commit for BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update bwc test to check Monitor stats and add bwc tests to GitHub Actions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Use current version plugin bundle from build for bwc tests instead of manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update mockito-core dependency to 3.12.4 to prevent conflict Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Remove disabling security manager flag when running BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com> Co-authored-by: Clay Downs <89109232+downsrob@users.noreply.github.com> Co-authored-by: Abbas Hussain <abbashus@amazon.com> Co-authored-by: Daniel Doubrovkine (dB.) <dblock@dblock.org> Co-authored-by: Marc Handalian <handalm@amazon.com> Co-authored-by: Annie Lee <71157062+leeyun-amzn@users.noreply.github.com>

* Updates alerting version to 1.2 (opensearch-project#192) * Updates alerting version to 1.2 * Adds snapshot repo to the repository file Signed-off-by: Clay Downs <downsrob@amazon.com> * Update build to use public Maven repo (opensearch-project#184) Signed-off-by: Abbas Hussain <abbas_10690@yahoo.com> * Publish notification JARs checksums. (opensearch-project#196) * Publish notification JARs checksums. Signed-off-by: dblock <dblock@dblock.org> * Remove sonatype staging. Signed-off-by: dblock <dblock@dblock.org> * Updates testCompile mockito version to match OpenSearch changes (opensearch-project#204) Signed-off-by: Clay Downs <downsrob@amazon.com> * Update maven publication to include cksums. (opensearch-project#224) This change adds a task to publish to a local staging repo under build/ that includes cksums. It also updates build.sh to use this new task and copy the contents of the staging repo to the output directory. The maven publish plugin will not include these cksums when publishing to maven local but will when published to a separate folder. Signed-off-by: Marc Handalian <handalm@amazon.com> * Add release notes for 1.2.0.0 release (opensearch-project#225) * Create opensearch-alerting.release-notes-1.2.0.0.md Signed-off-by: Annie Lee <leeyun@amazon.com> * Update opensearch-alerting.release-notes-1.2.0.0.md * Update opensearch-alerting.release-notes-1.2.0.0.md * Add backwards compatibility tests (opensearch-project#199) * Initial commit for BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update bwc test to check Monitor stats and add bwc tests to GitHub Actions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Use current version plugin bundle from build for bwc tests instead of manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update mockito-core dependency to 3.12.4 to prevent conflict Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Remove disabling security manager flag when running BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> Co-authored-by: Clay Downs <89109232+downsrob@users.noreply.github.com> Co-authored-by: Abbas Hussain <abbashus@amazon.com> Co-authored-by: Daniel Doubrovkine (dB.) <dblock@dblock.org> Co-authored-by: Marc Handalian <handalm@amazon.com> Co-authored-by: Annie Lee <71157062+leeyun-amzn@users.noreply.github.com> Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Signed-off-by: AWSHurneyt <hurneyt@amazon.com>

qreshi added 2 commits October 8, 2021 13:18

Initial commit for BWC tests

c2d1173

Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

Update bwc test to check Monitor stats and add bwc tests to GitHub Ac…

42b4cf5

…tions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

qreshi added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Oct 8, 2021

qreshi requested review from lezzago and skkosuri-amzn October 8, 2021 20:30

Use current version plugin bundle from build for bwc tests instead of…

254fa98

… manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

qreshi added the backwards-compatibility label Oct 11, 2021

skkosuri-amzn reviewed Oct 12, 2021

View reviewed changes

alerting/src/test/kotlin/org/opensearch/alerting/bwc/AlertingBackwardsCompatibilityIT.kt Show resolved Hide resolved

skkosuri-amzn reviewed Oct 12, 2021

View reviewed changes

alerting/src/test/kotlin/org/opensearch/alerting/bwc/AlertingBackwardsCompatibilityIT.kt Show resolved Hide resolved

skkosuri-amzn reviewed Oct 12, 2021

View reviewed changes

.github/workflows/multi-node-test-workflow.yml Outdated Show resolved Hide resolved

lezzago reviewed Oct 12, 2021

View reviewed changes

alerting/build.gradle Show resolved Hide resolved

qreshi added 2 commits November 2, 2021 13:46

Update mockito-core dependency to 3.12.4 to prevent conflict

08b5dc5

Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

Remove disabling security manager flag when running BWC tests

078b566

Signed-off-by: Mohammad Qureshi <qreshi@amazon.com>

skkosuri-amzn approved these changes Nov 8, 2021

View reviewed changes

lezzago approved these changes Nov 8, 2021

View reviewed changes

qreshi merged commit 11aafbe into opensearch-project:main Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backwards compatibility tests#199

Add backwards compatibility tests#199
qreshi merged 5 commits intoopensearch-project:mainfrom
qreshi:bwc-tests

qreshi commented Oct 8, 2021 •

edited

Loading

Uh oh!

lezzago commented Oct 8, 2021

Uh oh!

codecov-commenter commented Oct 9, 2021 •

edited by codecov bot

Loading

Uh oh!

qreshi commented Oct 11, 2021

Uh oh!

skkosuri-amzn Oct 12, 2021

Uh oh!

qreshi Nov 1, 2021

Uh oh!

qreshi Nov 1, 2021

Uh oh!

skkosuri-amzn Nov 2, 2021

Uh oh!

qreshi Nov 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skkosuri-amzn commented Nov 8, 2021 •

edited

Loading

Uh oh!

qreshi commented Nov 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

qreshi commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lezzago commented Oct 8, 2021

Uh oh!

codecov-commenter commented Oct 9, 2021 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qreshi commented Oct 11, 2021

Uh oh!

skkosuri-amzn Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

qreshi Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

qreshi Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

skkosuri-amzn Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

qreshi Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skkosuri-amzn commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qreshi commented Nov 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qreshi commented Oct 8, 2021 •

edited

Loading

codecov-commenter commented Oct 9, 2021 •

edited by codecov bot

Loading

skkosuri-amzn commented Nov 8, 2021 •

edited

Loading