Add support of .opensearch-knn-model as system index to transport actions #847

martin-gaievski · 2023-04-07T04:30:40Z

Description

We need to stash thread context in transport actions in addition to ModelDao to address all integ tests failures like in log below. This change compliments previous PR #827

org.opensearch.client.ResponseException: method [POST], host [https://localhost:9200/], URI [/_plugins/_knn/models/test-model-id/_train], status line [HTTP/1.1 403 Forbidden]

    {"error":{"root_cause":[{"type":"security_exception","reason":"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]"}],"type":"security_exception","reason":"no permissions for [] and User [name=admin, backend_roles=[admin], requestedTenant=null]"},"status":403}

        at __randomizedtesting.SeedInfo.seed([99F6FF5E9B7971F9:4372F53206C75963]:0)

        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:375)

        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:345)

        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:320)

        at app//org.opensearch.knn.KNNRestTestCase.trainModel(KNNRestTestCase.java:1099)

        at app//org.opensearch.knn.KNNRestTestCase.ingestDataAndTrainModel(KNNRestTestCase.java:1229)

        at app//org.opensearch.knn.KNNRestTestCase.ingestDataAndTrainModel(KNNRestTestCase.java:1215)

        at app//org.opensearch.knn.plugin.action.RestDeleteModelHandlerIT.testDeleteTrainingModel(RestDeleteModelHandlerIT.java:95)

Issues Resolved

opensearch-project/security#2265

Check List

New functionality includes testing.
- All tests pass
Commits are signed as 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: Martin Gaievski <[email protected]>

codecov-commenter · 2023-04-07T04:47:57Z

Codecov Report

Merging #847 (b1cfaef) into main (427cd32) will increase coverage by 0.05%.
The diff coverage is 89.61%.

📣 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     #847      +/-   ##
============================================
+ Coverage     85.11%   85.17%   +0.05%     
- Complexity     1081     1086       +5     
============================================
  Files           151      152       +1     
  Lines          4394     4404      +10     
  Branches        390      393       +3     
============================================
+ Hits           3740     3751      +11     
+ Misses          477      474       -3     
- Partials        177      179       +2

Impacted Files	Coverage Δ
...n/plugin/transport/SearchModelTransportAction.java	`81.81% <71.42%> (+6.81%)`	⬆️
...plugin/transport/TrainingModelTransportAction.java	`86.11% <81.81%> (+1.26%)`	⬆️
...org/opensearch/knn/common/ThreadContextHelper.java	`83.33% <83.33%> (ø)`
...main/java/org/opensearch/knn/indices/ModelDao.java	`81.55% <91.89%> (-1.12%)`	⬇️
...n/plugin/transport/DeleteModelTransportAction.java	`100.00% <100.00%> (ø)`
.../knn/plugin/transport/GetModelTransportAction.java	`100.00% <100.00%> (ø)`
...in/transport/TrainingJobRouterTransportAction.java	`80.85% <100.00%> (+0.85%)`	⬆️

... and 1 file 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.

navneet1v · 2023-04-07T16:20:37Z

src/main/java/org/opensearch/knn/common/TaskRunner.java

+/**
+ * Class abstracts execution of runnable or function in specific context
+ */
+public class TaskRunner {


can we rename this class to ThreadContextHelper?

yes, this makes more sense. I could came up with a good name yesterday so took the straightforward one. I'll push a commit soon

Signed-off-by: Martin Gaievski <[email protected]>

navneet1v · 2023-04-07T16:56:22Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

@@ -219,21 +218,14 @@ public void create(ActionListener<CreateIndexResponse> actionListener) throws IO
            if (isCreated()) {
                return;
            }
-            runWithStashedThreadContext(() -> {


why we need to remove this from runWithStashedThreadContext?

we do context stashing at the transport action level, which is a caller for this method, so keeping this functionality here is redundant.

Thanks this make sense

navneet1v · 2023-04-07T16:56:35Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

-            final IndexRequestBuilder indexRequestBuilder = ModelDao.runWithStashedThreadContext(
-                () -> client.prepareIndex(MODEL_INDEX_NAME)
-            );
+            IndexRequestBuilder indexRequestBuilder = client.prepareIndex(MODEL_INDEX_NAME);


same as above.

we do context stashing at the transport action level, keeping this functionality here is redundant

Thanks this make sense

navneet1v · 2023-04-07T16:57:23Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

@@ -369,12 +361,12 @@ private ActionListener<IndexResponse> getUpdateModelMetadataListener(

        @SneakyThrows


sneakythrows are not supposed to be used in src files. it is mainly for tests. please remove this.

Signed-off-by: Martin Gaievski <[email protected]>

…ions (#847) * Add thread context stashing to transport actions Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 1b6fd48)

…ions (#847) (#849) * Add thread context stashing to transport actions Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 1b6fd48) Co-authored-by: Martin Gaievski <[email protected]>

…ions (#847) (#848) * Add thread context stashing to transport actions Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 1b6fd48) Co-authored-by: Martin Gaievski <[email protected]>

…port actions (opensearch-project#847)" This reverts commit 1b6fd48. Signed-off-by: John Mazanec <[email protected]>

…#859) Revert "Add support of .opensearch-knn-model as system index to transport actions (#847)" Makes minor fixes to stashed thread context for calls to the model system index. In general, only direct calls to the system index should be wrapped in the stashed context. Signed-off-by: John Mazanec <[email protected]>

…#859) Revert "Add support of .opensearch-knn-model as system index to transport actions (#847)" Makes minor fixes to stashed thread context for calls to the model system index. In general, only direct calls to the system index should be wrapped in the stashed context. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 11c6616)

…#863) Revert "Add support of .opensearch-knn-model as system index to transport actions (#847)" Makes minor fixes to stashed thread context for calls to the model system index. In general, only direct calls to the system index should be wrapped in the stashed context. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 11c6616)

…#862) Revert "Add support of .opensearch-knn-model as system index to transport actions (#847)" Makes minor fixes to stashed thread context for calls to the model system index. In general, only direct calls to the system index should be wrapped in the stashed context. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit 11c6616)

Add thread context stashing to transport actions

32dc7f8

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski force-pushed the use-thread-context-stashing-in-transport-actions branch from e9c7c1c to 32dc7f8 Compare April 7, 2023 04:31

martin-gaievski added Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x backport 2.7 labels Apr 7, 2023

martin-gaievski marked this pull request as ready for review April 7, 2023 05:19

martin-gaievski requested a review from a team April 7, 2023 05:19

navneet1v reviewed Apr 7, 2023

View reviewed changes

Rename TaskRunner to ThreadContextHelper

e4b3d4f

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski requested a review from navneet1v April 7, 2023 16:37

navneet1v reviewed Apr 7, 2023

View reviewed changes

navneet1v previously approved these changes Apr 7, 2023

View reviewed changes

Remove sneakythrows from non-test class

b1cfaef

Signed-off-by: Martin Gaievski <[email protected]>

martin-gaievski dismissed navneet1v’s stale review via b1cfaef April 7, 2023 17:52

martin-gaievski force-pushed the use-thread-context-stashing-in-transport-actions branch from 616e8e3 to b1cfaef Compare April 7, 2023 17:52

martin-gaievski requested a review from navneet1v April 7, 2023 18:17

naveentatikonda approved these changes Apr 7, 2023

View reviewed changes

navneet1v approved these changes Apr 7, 2023

View reviewed changes

martin-gaievski merged commit 1b6fd48 into opensearch-project:main Apr 7, 2023

This was referenced Apr 7, 2023

[Backport 2.x] Add support of .opensearch-knn-model as system index to transport actions #848

Merged

[Backport 2.7] Add support of .opensearch-knn-model as system index to transport actions #849

Merged

martin-gaievski mentioned this pull request Apr 7, 2023

Add github action for secure integ tests #836

Merged

3 tasks

jmazanec15 mentioned this pull request Apr 14, 2023

Switch stashing thread context back to only around system index calls #859

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of .opensearch-knn-model as system index to transport actions #847

Add support of .opensearch-knn-model as system index to transport actions #847

martin-gaievski commented Apr 7, 2023

codecov-commenter commented Apr 7, 2023 •

edited

Loading

navneet1v Apr 7, 2023

martin-gaievski Apr 7, 2023

navneet1v Apr 7, 2023

martin-gaievski Apr 7, 2023

navneet1v Apr 7, 2023

navneet1v Apr 7, 2023

martin-gaievski Apr 7, 2023

navneet1v Apr 7, 2023

navneet1v Apr 7, 2023

martin-gaievski Apr 7, 2023

		@@ -369,12 +361,12 @@ private ActionListener<IndexResponse> getUpdateModelMetadataListener(

		@SneakyThrows

Add support of .opensearch-knn-model as system index to transport actions #847

Add support of .opensearch-knn-model as system index to transport actions #847

Conversation

martin-gaievski commented Apr 7, 2023

Description

Issues Resolved

Check List

codecov-commenter commented Apr 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 7, 2023 •

edited

Loading