-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Support for bwc tests for plugins #1051
Support for bwc tests for plugins #1051
Conversation
Signed-off-by: Vacha <vachshah@amazon.com>
✅ DCO Check Passed e616879 |
✅ Gradle Wrapper Validation success e616879 |
✅ Gradle Precommit success e616879 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped a few comments.
How do we test this new functionality? Can you add unit/integ tests ?
@@ -404,6 +409,19 @@ public void nextNodeToNextVersion() { | |||
node.start(); | |||
} | |||
|
|||
public void upgradeNodeAndPluginToNextVersion(List<Provider<RegularFile>> plugins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice. It works for rolling upgrade/mixed cluster cases where we upgrade node by node.
For full cluster restarts/restart upgrades we need another function to handle it for plugins.
Take a look at:
public void goToNextVersion()
Minor nit: I wish we didn't duplicate it but I dont see another way too :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that! I will add that method as well since we are adding bwc support. For the duplication, yeah I had to create a new method since the plugins have to be added before starting the node and this would otherwise affect OpenSearch bwc tests :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for full cluster restarts/restart upgrades. I have also tried to reduce duplication where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, we should be able to leverage restart cluster infra for snapshot upgrades. Essentially its taking snapshot one old version, store it in a repo, restart the cluster to new version and restore the snapshot.
Lets open an issue to take care fo snapshot upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, opened an issue for tracking snapshot upgrade tests #1088.
@@ -186,6 +186,11 @@ public void plugin(String pluginProjectPath) { | |||
nodes.all(each -> each.plugin(pluginProjectPath)); | |||
} | |||
|
|||
@Override | |||
public void upgradePlugin(List<Provider<RegularFile>> plugins) { | |||
nodes.all(each -> each.upgradePlugin(plugins)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the only valid case for this function is during restart upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had to override it since it is added in interface TestClusterConfiguration
.
Currently, there looks to be no tests specifically for this class, they are directly used in the bwc testing in OpenSearch. I will see if I can find a place for them or start adding tests for it. |
Signed-off-by: Vacha <vachshah@amazon.com>
✅ Gradle Wrapper Validation success 7216b78 |
✅ DCO Check Passed 7216b78 |
✅ Gradle Precommit success 7216b78 |
For the tests for OpenSearchCluster, it looks like it might take some time to setup, we could create an issue for it and revisit it next week, the methods in OpenSearchCluster are all used in bwc tests. WDYT?
I have created an issue #1086 for adding the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, plugins would love it and great work!
Thanks a lot @VachaShah !!
@@ -404,6 +409,19 @@ public void nextNodeToNextVersion() { | |||
node.start(); | |||
} | |||
|
|||
public void upgradeNodeAndPluginToNextVersion(List<Provider<RegularFile>> plugins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, we should be able to leverage restart cluster infra for snapshot upgrades. Essentially its taking snapshot one old version, store it in a repo, restart the cluster to new version and restore the snapshot.
Lets open an issue to take care fo snapshot upgrades.
nodes.all(OpenSearchNode::goToNextVersion); | ||
upgradePlugin(plugins); | ||
start(); | ||
writeUnicastHostsFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: Do we need to writeUnicastHostFiles
?
Unicast is used for discovery of cluster nodes, if the cluster has the same ports already published and written to unicast_host.txt
and upgrading the nodes the new version, wouldnt the framework start the new version of OpenSearch with the same ports?
start gradle check |
Thank you @saratvemulapalli ! |
@VachaShah we should backport this PR to 1.x. |
Sure, I will raise a PR for backporting it. |
* Support for bwc tests for plugins Signed-off-by: Vacha <vachshah@amazon.com> * Adding support for restart upgrades for plugins bwc Signed-off-by: Vacha <vachshah@amazon.com>
Signed-off-by: Vacha vachshah@amazon.com
Description
This PR adds supporting logic for implementing bwc tests in plugins. This are being used in PR anomaly-detection#158.
Issues Resolved
As part of #1002
Check List
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.