Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Skip delete .plugins-ml-config system index during integ test #1419

Merged
merged 2 commits into from
Jan 25, 2024
Merged

Skip delete .plugins-ml-config system index during integ test #1419

merged 2 commits into from
Jan 25, 2024

Conversation

junqiu-lei
Copy link
Member

@junqiu-lei junqiu-lei commented Jan 25, 2024 •
edited
Loading

Description

Skip delete .plugins-ml-config system index during integ test.

This PR helps to resolve the error during k-NN build security enabled integ test in release Jenkins concurrent-search-test CI:

org.opensearch.client.ResponseException: method [DELETE], host [https://localhost:9200/], URI [/.plugins-ml-config], 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 app//org.opensearch.client.RestClient.convertResponse(RestClient.java:376)

Test

With @peterzhuamazon 's help, the Jenkins’s CI build passed with this code change. Attached logs:
knn-junqiu-lei-skip-delete-ml-config-gradle.log
2.12.0-concurrent-settings-cluster.log

Issues Resolved

#1357, #1410, #1411, #1412

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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.

@codecov Codecov
Copy link

codecov bot commented Jan 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (89fc267) 84.92% compared to head (5b9e8a9) 84.88%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1419      +/-   ##
============================================
- Coverage     84.92%   84.88%   -0.04%     
  Complexity     1274     1274              
============================================
  Files           167      167              
  Lines          5186     5186              
  Branches        491      491              
============================================
- Hits           4404     4402       -2     
- Misses          573      575       +2     
  Partials        209      209

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peterzhuamazon peterzhuamazon mentioned this pull request Jan 25, 2024
Closed
@junqiu-lei junqiu-lei added the Maintenance Add support for new versions of OpenSearch/Dashboards from upstream label Jan 25, 2024
@junqiu-lei junqiu-lei marked this pull request as ready for review January 25, 2024 22:23
jmazanec15
jmazanec15 reviewed Jan 25, 2024
View reviewed changes
src/testFixtures/java/org/opensearch/knn/ODFERestTestCase.java Outdated
@@ -212,6 +213,7 @@ private boolean skipDeleteIndex(String indexName) {
return indexName == null
|| OPENDISTRO_SECURITY.equals(indexName)
|| IMMUTABLE_INDEX_PREFIXES.stream().anyMatch(indexName::startsWith)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add to immutable prefix list?

Copy link
Member Author

@junqiu-lei junqiu-lei Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmazanec15 Do you mean to add .plugins-ml as ML_PLUGIN_SYSTEM_INDEX_PREFIX for .plugins-ml-config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked on the index names in ml plugin CommonValue.java, all indexes prefix with .plugins-ml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

ryanbogan
ryanbogan previously approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@ryanbogan ryanbogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@junqiu-lei junqiu-lei merged commit 47728ce into opensearch-project:main Jan 25, 2024
52 checks passed
@junqiu-lei junqiu-lei deleted the skip-delete-ml-config branch January 25, 2024 23:50
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 25, 2024
@junqiu-lei @github-actions
Skip delete .plugins-ml-config system index during integ test (#1419)
f756402 
(cherry picked from commit 47728ce)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 25, 2024
Merged
junqiu-lei pushed a commit that referenced this pull request Jan 26, 2024
@opensearch-trigger-bot
Skip delete .plugins-ml-config system index during integ test (#1419) (…
f3d7d49 
…#1422)

(cherry picked from commit 47728ce)
Signed-off-by: Junqiu Lei <[email protected]>
Co-authored-by: Junqiu Lei <[email protected]>
@junqiu-lei
Copy link
Member Author

#1357 now successfully auto closed. #1410, #1411, #1412 still pending close, checked on the Jenkkins CI, they are failed for all plugins, assume it might need some fix at Jenkins CI level. cc @peterzhuamazon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-gaievski martin-gaievski martin-gaievski approved these changes

@jmazanec15 jmazanec15 jmazanec15 approved these changes

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@vibrantvarun vibrantvarun Awaiting requested review from vibrantvarun

@ryanbogan ryanbogan Awaiting requested review from ryanbogan ryanbogan is a code owner

Assignees
No one assigned
Labels
backport 2.x Maintenance Add support for new versions of OpenSearch/Dashboards from upstream skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@junqiu-lei @ryanbogan @jmazanec15 @vibrantvarun @martin-gaievski