[Extensions] Add DynamicActionRegistry to ActionModule by dbwiddis · Pull Request #6734 · opensearch-project/OpenSearch

dbwiddis · 2023-03-17T06:00:26Z

Companion PR: SDK #588. This PR must be merged first.

Description

Adds a DynamicActionRegistry class and instance field to the ActionModule.

TransportAction classes from OpenSearch and plugins are instantiated during Node bootstrap and injected as singletons.

Extensions have the need to proxy actions between extensions. For example, job scheduler needs to invoke actions on Anomaly Detector extension; Anomaly Detector may need to invoke actions on Alerting Extension; et. al. This capability is currently implemented with a single ProxyAction which relays these invocations blindly.

This PR adds a registry for Extensions to register and unregister classes which extend ActionType and TransportAction at times other than bootstrap.

The benefit of using a subclass of TransportAction is that the client.execute() invocation processes these actions through ActionFilters. The first such filter processed is in the security plugin and this provides a single point of contact for future application of security features.

Inspiration for some of these features: #4460 (but I did it my way)

Description of functionality:

On bootstrap, ActionModule creates an registry with two internal maps; one corresponds to the actions map obtained from the injector, which is immutable, and an empty registry for addition (and potentially removal) of extension actions
During Extension initialization, the getActions() extension point will be handled on OpenSearch by the ExtensionTransportActionsHandler which will register (in the registry) a custom subclass of the ActionType containing the fully qualified class name of the action on the Extension
- There is no current usage of the implemented "unregister" functionality, but the capability is provided for future "hot swap" functionality.
When one extension (or OpenSearch or a plugin) wants to invoke an action on an extension, that will be handled by the NodeClient's client.execute() path. This path uses associated ActionFilters and the potential for future checks and security/identity integration.
The execution of this TransportAction will send it to the Extension for further processing (TBD in Companion PR).

Issues Resolved

Fixes SDK #107.

Companion PR implements SDK #525.

SDK #525 could be implemented using the existing ProxyAction setup that this PR replaces, but I believe the benefits of funneling all requests through a single touch point is a significant benefit and combined these efforts.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
~~Commit changes are listed out in CHANGELOG.md file (See: Changelog)~~

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.

github-actions · 2023-03-17T06:09:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12578/
CommitID: eea1a21
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

peternied

Nice! I like the steps forward to consolidating all actions together. I've put in some notes about coupling and how the abstraction could work a little differently that I think could be more flexible.

server/src/main/java/org/opensearch/client/node/NodeClient.java

server/src/main/java/org/opensearch/action/ActionModule.java

github-actions · 2023-03-17T19:02:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12602/
CommitID: 7893511
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

cwperks · 2023-03-17T19:02:15Z

server/src/main/java/org/opensearch/extensions/action/ExtensionTransportActionsHandler.java

        }
-        actionsMap.putIfAbsent(action, extension);
+        // Register the action in the action module's extension actions map
+        dynamicActionRegistry.registerExtensionAction(new ExtensionAction(action, extension.getId()));


If the action requires parameters then how will then be sent to the extension?

The ExtensionAction here is just a subclass of ActionType which is an instance used as the key to a map. The value corresponding to this is an ExtensionTransportAction with the same name, that references the ExtensionActionRequest and ExtensionActionResponse classes.

That ExtensionActionRequest is how the details are communicated, with an action string (a fully qualified class name that the extension will match to one of its existing getAction() actions) and a byte array which will represent the parameters (serialized from the action's ActionRequest). Those will be serialized and deserialized consistently.... currently with Writeable but we'll probably switch to protobuf.

+1 for protobufs

server/src/main/java/org/opensearch/action/ActionModule.java

peternied

Thanks for the refactor 👍

server/src/main/java/org/opensearch/client/node/NodeClient.java

server/src/main/java/org/opensearch/action/ActionModule.java

owaiskazi19 · 2023-03-17T22:26:09Z

Overall looks good @dbwiddis. Small ask: would love to see a design diagram for the workflow in the SDK repository. Since we are touching the ActionModule here would be great to get some eyes from @reta @nknize

github-actions · 2023-03-17T22:55:35Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12611/
CommitID: 3829f15
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-17T23:03:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12612/
CommitID: 7c8061e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-18T00:17:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12616/
CommitID: b22dcad
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-18T02:29:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12628/
CommitID: a8f487a
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-18T03:21:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12629/
CommitID: b1f7982
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-18T04:00:02Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationAllocationIT.testSingleIndexShardAllocation

URL: https://build.ci.opensearch.org/job/gradle-check/12630/
CommitID: fe5f5ff
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov-commenter · 2023-03-18T04:00:49Z

Codecov Report

Merging #6734 (3abbeb2) into main (5f81930) will decrease coverage by 0.05%.
The diff coverage is 79.34%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6734      +/-   ##
============================================
- Coverage     70.74%   70.69%   -0.05%     
+ Complexity    59203    59195       -8     
============================================
  Files          4810     4809       -1     
  Lines        283487   283536      +49     
  Branches      40877    40885       +8     
============================================
- Hits         200543   200440     -103     
- Misses        66465    66624     +159     
+ Partials      16479    16472       -7

Impacted Files	Coverage Δ
...nsions/action/RegisterTransportActionsRequest.java	`85.00% <ø> (ø)`
...ns/action/TransportActionRequestFromExtension.java	`66.66% <0.00%> (-2.57%)`	⬇️
...ava/org/opensearch/test/client/NoOpNodeClient.java	`27.77% <ø> (ø)`
...sions/action/ExtensionTransportActionsHandler.java	`47.31% <57.14%> (-8.94%)`	⬇️
.../opensearch/extensions/action/ExtensionAction.java	`61.53% <61.53%> (ø)`
...in/java/org/opensearch/client/node/NodeClient.java	`71.42% <66.66%> (ø)`
.../main/java/org/opensearch/action/ActionModule.java	`97.83% <100.00%> (+0.73%)`	⬆️
...a/org/opensearch/extensions/ExtensionsManager.java	`45.45% <100.00%> (ø)`
...rch/extensions/action/ExtensionActionResponse.java	`100.00% <100.00%> (ø)`
...ch/extensions/action/ExtensionTransportAction.java	`37.50% <100.00%> (+37.50%)`	⬆️
... and 1 more

... and 483 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Daniel Widdis <widdis@gmail.com>

github-actions · 2023-03-24T03:41:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/12862/
CommitID: c9c753c

* Add dynamic action registry to ActionModule Signed-off-by: Daniel Widdis <widdis@gmail.com> * Update registration of transport actions Signed-off-by: Daniel Widdis <widdis@gmail.com> * Generate transport actions dynamically Signed-off-by: Daniel Widdis <widdis@gmail.com> * Refactor to combine registry internals Signed-off-by: Daniel Widdis <widdis@gmail.com> * Finally figured out the generics (or lack thereof) Signed-off-by: Daniel Widdis <widdis@gmail.com> * ExtensionProxyAction is dead! Long live ExtensionAction! Signed-off-by: Daniel Widdis <widdis@gmail.com> * Simplify ExtensionTransportActionHandler, fix compile issues Signed-off-by: Daniel Widdis <widdis@gmail.com> * Maybe tests will pass with this commit Signed-off-by: Daniel Widdis <widdis@gmail.com> * I guess you can't use null as a key in a map Signed-off-by: Daniel Widdis <widdis@gmail.com> * Lazy test setup, but this should finally work Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add Tests Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix TransportActionRequestFromExtension inheritance Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix return type for transport actions from extensions Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix ParametersInWrongOrderError and add some preemptive null handling Signed-off-by: Daniel Widdis <widdis@gmail.com> * NPE is not expected result if params are in correct order Signed-off-by: Daniel Widdis <widdis@gmail.com> * Remove redundant class and string parsing, add success boolean Signed-off-by: Daniel Widdis <widdis@gmail.com> * Last fix of params out of order. Working test case! Signed-off-by: Daniel Widdis <widdis@gmail.com> * Code worked, tests didn't. This is finally done (I think) Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add more detail to comments on immutable vs. dynamic maps Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add StreamInput getter to ExtensionActionResponse Signed-off-by: Daniel Widdis <widdis@gmail.com> * Generalize dynamic action registration Signed-off-by: Daniel Widdis <widdis@gmail.com> * Comment and naming fixes Signed-off-by: Daniel Widdis <widdis@gmail.com> * Register method renaming Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add generic type parameters Signed-off-by: Daniel Widdis <widdis@gmail.com> * Improve/simplify which parameter types get passed Signed-off-by: Daniel Widdis <widdis@gmail.com> * Revert removal of ProxyAction and changes to transport and requests Signed-off-by: Daniel Widdis <widdis@gmail.com> * Wrap ExtensionTransportResponse in a class denoting success Signed-off-by: Daniel Widdis <widdis@gmail.com> * Remove generic types as they are incompatible with Guice injection Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix response handling, it works (again) Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix up comments and remove debug logging Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> (cherry picked from commit 9febe10) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Add dynamic action registry to ActionModule * Update registration of transport actions * Generate transport actions dynamically * Refactor to combine registry internals * Finally figured out the generics (or lack thereof) * ExtensionProxyAction is dead! Long live ExtensionAction! * Simplify ExtensionTransportActionHandler, fix compile issues * Maybe tests will pass with this commit * I guess you can't use null as a key in a map * Lazy test setup, but this should finally work * Add Tests * Fix TransportActionRequestFromExtension inheritance * Fix return type for transport actions from extensions * Fix ParametersInWrongOrderError and add some preemptive null handling * NPE is not expected result if params are in correct order * Remove redundant class and string parsing, add success boolean * Last fix of params out of order. Working test case! * Code worked, tests didn't. This is finally done (I think) * Add more detail to comments on immutable vs. dynamic maps * Add StreamInput getter to ExtensionActionResponse * Generalize dynamic action registration * Comment and naming fixes * Register method renaming * Add generic type parameters * Improve/simplify which parameter types get passed * Revert removal of ProxyAction and changes to transport and requests * Wrap ExtensionTransportResponse in a class denoting success * Remove generic types as they are incompatible with Guice injection * Fix response handling, it works (again) * Fix up comments and remove debug logging --------- (cherry picked from commit 9febe10) Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…earch-project#6734) (opensearch-project#6829)" This reverts commit ac79a63.

…oject#6734) * Add dynamic action registry to ActionModule Signed-off-by: Daniel Widdis <widdis@gmail.com> * Update registration of transport actions Signed-off-by: Daniel Widdis <widdis@gmail.com> * Generate transport actions dynamically Signed-off-by: Daniel Widdis <widdis@gmail.com> * Refactor to combine registry internals Signed-off-by: Daniel Widdis <widdis@gmail.com> * Finally figured out the generics (or lack thereof) Signed-off-by: Daniel Widdis <widdis@gmail.com> * ExtensionProxyAction is dead! Long live ExtensionAction! Signed-off-by: Daniel Widdis <widdis@gmail.com> * Simplify ExtensionTransportActionHandler, fix compile issues Signed-off-by: Daniel Widdis <widdis@gmail.com> * Maybe tests will pass with this commit Signed-off-by: Daniel Widdis <widdis@gmail.com> * I guess you can't use null as a key in a map Signed-off-by: Daniel Widdis <widdis@gmail.com> * Lazy test setup, but this should finally work Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add Tests Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix TransportActionRequestFromExtension inheritance Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix return type for transport actions from extensions Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix ParametersInWrongOrderError and add some preemptive null handling Signed-off-by: Daniel Widdis <widdis@gmail.com> * NPE is not expected result if params are in correct order Signed-off-by: Daniel Widdis <widdis@gmail.com> * Remove redundant class and string parsing, add success boolean Signed-off-by: Daniel Widdis <widdis@gmail.com> * Last fix of params out of order. Working test case! Signed-off-by: Daniel Widdis <widdis@gmail.com> * Code worked, tests didn't. This is finally done (I think) Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add more detail to comments on immutable vs. dynamic maps Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add StreamInput getter to ExtensionActionResponse Signed-off-by: Daniel Widdis <widdis@gmail.com> * Generalize dynamic action registration Signed-off-by: Daniel Widdis <widdis@gmail.com> * Comment and naming fixes Signed-off-by: Daniel Widdis <widdis@gmail.com> * Register method renaming Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add generic type parameters Signed-off-by: Daniel Widdis <widdis@gmail.com> * Improve/simplify which parameter types get passed Signed-off-by: Daniel Widdis <widdis@gmail.com> * Revert removal of ProxyAction and changes to transport and requests Signed-off-by: Daniel Widdis <widdis@gmail.com> * Wrap ExtensionTransportResponse in a class denoting success Signed-off-by: Daniel Widdis <widdis@gmail.com> * Remove generic types as they are incompatible with Guice injection Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix response handling, it works (again) Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix up comments and remove debug logging Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Valentin Mitrofanov <mitrofmep@gmail.com>

dbwiddis mentioned this pull request Mar 17, 2023

[FEATURE] [DISCUSS] Implement Proxy Actions opensearch-project/opensearch-sdk-java#525

Closed

4 tasks

peternied reviewed Mar 17, 2023

View reviewed changes

server/src/main/java/org/opensearch/client/node/NodeClient.java Outdated Show resolved Hide resolved

server/src/main/java/org/opensearch/action/ActionModule.java Outdated Show resolved Hide resolved

cwperks reviewed Mar 17, 2023

View reviewed changes

peternied reviewed Mar 17, 2023

View reviewed changes

server/src/main/java/org/opensearch/action/ActionModule.java Outdated Show resolved Hide resolved

peternied reviewed Mar 17, 2023

View reviewed changes

owaiskazi19 reviewed Mar 17, 2023

View reviewed changes

server/src/main/java/org/opensearch/client/node/NodeClient.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Mar 17, 2023

View reviewed changes

server/src/main/java/org/opensearch/action/ActionModule.java Outdated Show resolved Hide resolved

dbwiddis force-pushed the dynamicActions branch from 3829f15 to 7c8061e Compare March 17, 2023 22:57

dbwiddis added the skip-changelog label Mar 18, 2023

dbwiddis marked this pull request as ready for review March 18, 2023 05:30

dbwiddis requested review from Bukhtawar, CEHENKLE, anasalkouz, andrross, dblock, gbbafna, kartg, kotwanikunal, reta and setiah as code owners March 18, 2023 05:30

dbwiddis added 15 commits March 23, 2023 20:11

Remove redundant class and string parsing, add success boolean

d9ca024

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Last fix of params out of order. Working test case!

27b6521

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Code worked, tests didn't. This is finally done (I think)

a979767

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Add more detail to comments on immutable vs. dynamic maps

b9afc50

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Add StreamInput getter to ExtensionActionResponse

46b5a83

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Generalize dynamic action registration

0834375

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Comment and naming fixes

b2fe38c

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Register method renaming

032ee0d

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Add generic type parameters

4a9690f

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Improve/simplify which parameter types get passed

c581e55

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Revert removal of ProxyAction and changes to transport and requests

a6f72e5

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Wrap ExtensionTransportResponse in a class denoting success

cd1630a

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Remove generic types as they are incompatible with Guice injection

56e37e8

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Fix response handling, it works (again)

c425be4

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Fix up comments and remove debug logging

c9c753c

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis force-pushed the dynamicActions branch from 0c43ea3 to c9c753c Compare March 24, 2023 03:11

ryanbogan approved these changes Mar 24, 2023

View reviewed changes

reta approved these changes Mar 24, 2023

View reviewed changes

ryanbogan merged commit 9febe10 into opensearch-project:main Mar 24, 2023

dbwiddis deleted the dynamicActions branch March 24, 2023 19:43

dbwiddis mentioned this pull request Mar 24, 2023

[Backport 1.x] Execute remote actions on another extension opensearch-project/opensearch-sdk-java#606

Merged

ryanbogan added the backport 2.x Backport to 2.x branch label Mar 24, 2023

opensearch-trigger-bot bot mentioned this pull request Mar 24, 2023

[Backport 2.x] [Extensions] Add DynamicActionRegistry to ActionModule #6829

Merged

joshpalis mentioned this pull request Mar 24, 2023

https://github.com/opensearch-project/job-scheduler/pull/351 opensearch-project/job-scheduler#349

Merged

5 tasks

Rishikesh1159 added a commit to Rishikesh1159/OpenSearch that referenced this pull request Mar 30, 2023

Revert "[Extensions] Add DynamicActionRegistry to ActionModule (opens…

306b8f5

…earch-project#6734) (opensearch-project#6829)" This reverts commit ac79a63.

owaiskazi19 mentioned this pull request May 22, 2023

Added @dbwiddis to OpenSearch maintainers #7665

Merged

6 tasks

Conversation

dbwiddis commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Mar 17, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 17, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

cwperks Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

dbwiddis Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

owaiskazi19 commented Mar 17, 2023

Uh oh!

github-actions bot commented Mar 17, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Mar 17, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Mar 18, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Mar 18, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Mar 18, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Mar 18, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 24, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dbwiddis commented Mar 17, 2023 •

edited

Loading

dbwiddis Mar 17, 2023 •

edited

Loading

codecov-commenter commented Mar 18, 2023 •

edited

Loading