Add backwards compatibility tests

[qreshi]

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.

[lezzago]

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

[codecov-commenter]

Codecov Report

Merging #199 (078b566) into main (7959450) will increase coverage by 0.02%.
The diff coverage is n/a.

@@             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              

Impacted Files	Coverage Δ
...ing/destination/client/DestinationEmailClient.java	72.50% <0.00%> (-5.00%)	⬇️
.../kotlin/org/opensearch/alerting/core/JobSweeper.kt	71.72% <0.00%> (-0.53%)	⬇️
...otlin/org/opensearch/alerting/alerts/AlertMover.kt	68.00% <0.00%> (ø)
...in/kotlin/org/opensearch/alerting/model/Monitor.kt	87.66% <0.00%> (+0.16%)	⬆️
...main/kotlin/org/opensearch/alerting/model/Alert.kt	82.74% <0.00%> (+0.26%)	⬆️
...ensearch/alerting/model/destination/Destination.kt	76.00% <0.00%> (+0.41%)	⬆️
...ain/kotlin/org/opensearch/alerting/AlertService.kt	79.02% <0.00%> (+0.48%)	⬆️
...rting/transport/TransportAcknowledgeAlertAction.kt	91.04% <0.00%> (+0.72%)	⬆️
...alerting/resthandler/RestAcknowledgeAlertAction.kt	90.00% <0.00%> (+3.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7959450...078b566. Read the comment docs.

[qreshi]

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]

Can we avoid sleep(...) ?

[qreshi]

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.

[qreshi]

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.

[skkosuri-amzn]

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

[qreshi]

Actually, since the bwcTestSuite executes the following scenarios:

1. mixedCluster - Hits the MIXED case
2. rollingUpgrade - Hits the MIXED and UPGRADED case
3. 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.

[skkosuri-amzn]

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

[qreshi]

@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.