-
Notifications
You must be signed in to change notification settings - Fork 890
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
[Decouple] Add new core service to expose functionality to verify plugin compatibility with OpenSearch plugins #4710
[Decouple] Add new core service to expose functionality to verify plugin compatibility with OpenSearch plugins #4710
Conversation
d268eff
to
6a6a398
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4710 +/- ##
==========================================
+ Coverage 66.96% 66.98% +0.01%
==========================================
Files 3291 3293 +2
Lines 63249 63281 +32
Branches 10057 10061 +4
==========================================
+ Hits 42355 42386 +31
- Misses 18453 18499 +46
+ Partials 2441 2396 -45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6a6a398
to
8133a51
Compare
8133a51
to
42d93a3
Compare
42d93a3
to
f341b31
Compare
f341b31
to
7cfc13e
Compare
|
||
The cross compatibility service can be used by plugins to: | ||
|
||
* Disable themselves if they are not compatible with the installed OpenSearch 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.
In the above example, can you please add a condition saying if mustHavePlug
is not compatble and show how I can disable my plugin?
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! In current world plugins can use core/capabilities
API to configure whether to enable/disable plugin. I've included the example.
src/core/server/cross_compatibility/cross_compatibility_service.ts
Outdated
Show resolved
Hide resolved
opensearchInstalledPlugins.forEach((obj) => { | ||
if (obj.component === depPluginName && obj.version) { | ||
installedPluginVersions.add(obj.version); | ||
if (semver.satisfies(semver.coerce(obj.version)!.version, depPluginVersionRange)) { |
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.
FYI, I was going to release a PEP 440 alternate to semver
but I found @renovatebot/pep440
on NPM which I have not used. If that library works, it would be better solution to semver.coerce
and if it doesn't, I would release my alternative and we can use it here. If you have the time, can you check if @renovatebot/pep440
works?
} catch (error) { | ||
this.log.warn( | ||
`Cat API call to OpenSearch to get list of plugins installed on the cluster has failed: ${error}` | ||
await this.crossCompatibilityService.checkPluginVersionCompatibility( |
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.
So the health check is not doing anything here other than recording the compatibility for itself. What are the plans to notify the OSD plugins if one of their deps becomes incompatible?
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.
Yes that's correct. To notify the OSD plugins if one of their deps becomes incompatible during runtime needs a hook to be identified to embed this new service when engine state changes. For now, plugins can call the cross compatibility service API on their own during runtime to get to know the compatibility state.
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.
Let's create an issue for an enhancement which will have plugins possibly expose a function that we call when compatibility changes.
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.
7cfc13e
to
2e0179d
Compare
9a4e15a
to
32ad48a
Compare
32ad48a
to
775c2b3
Compare
return results; | ||
} | ||
|
||
private async isVersionCompatibleOSPluginInstalled( |
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.
The async signature seems not used in this case, could we remove the async
and make the call to this method without await? As I thought there is a async call in every for of
loop in the code above when there is an await
.
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.
Hi @SuZhou-Joe Thanks for your review. Yeah I don't think I need async here and in another public function. Removed the unused async usages.
src/core/server/cross_compatibility/cross_compatibility_service.ts
Outdated
Show resolved
Hide resolved
src/core/server/cross_compatibility/cross_compatibility_service.ts
Outdated
Show resolved
Hide resolved
775c2b3
to
6c17b6b
Compare
6c17b6b
to
16be1b8
Compare
16be1b8
to
7360e2b
Compare
@mana, this has conflicts to resolve. Are you still actively working, or should it move to draft status? |
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 will be really helpful
…lugin and verify if it is installed on the cluster Signed-off-by: Manasvini B Suryanarayana <[email protected]>
…bility with OpenSearch plugins Signed-off-by: Manasvini B Suryanarayana <[email protected]>
Signed-off-by: Manasvini B Suryanarayana <[email protected]>
7360e2b
to
9c90d18
Compare
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-4710-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cf4f9296c0f3f5dfa96df59f6efada868e7cdffe
# Push it to GitHub
git push --set-upstream origin backport/backport-4710-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x Then, create a pull request where the |
…gin compatibility with OpenSearch plugins (opensearch-project#4710) * Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster Signed-off-by: Manasvini B Suryanarayana <[email protected]> * Add new core service to expose functionality to verify plugin compatibility with OpenSearch plugins Signed-off-by: Manasvini B Suryanarayana <[email protected]> * Add readme doc for cross compatibility service Signed-off-by: Manasvini B Suryanarayana <[email protected]> --------- Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit cf4f929)
…gin compatibility with OpenSearch plugins (opensearch-project#4710) * Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster Signed-off-by: Manasvini B Suryanarayana <[email protected]> * Add new core service to expose functionality to verify plugin compatibility with OpenSearch plugins Signed-off-by: Manasvini B Suryanarayana <[email protected]> * Add readme doc for cross compatibility service Signed-off-by: Manasvini B Suryanarayana <[email protected]> --------- Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit cf4f929)
…gin compatibility with OpenSearch plugins (#4710) (#5503) * Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster * Add new core service to expose functionality to verify plugin compatibility with OpenSearch plugins * Add readme doc for cross compatibility service --------- Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit cf4f929)
Description
cross_compatibility_service
which export feature that plugins can extend to decide what they want to do if their OpenSearch plugin counterpart is not installed or has incompatible version.Issues Resolved
#4903
https://github.com/orgs/opensearch-project/projects/63/views/27?pane=issue&itemId=34093291
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr