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.

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

Re-enabled multi-node integration tests and Fix integration test set up #432

Merged
merged 12 commits into from
Jan 22, 2024

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented Jan 21, 2024

Description

This PR re-enables multi-node integration tests for JDK 21.0.1 and aims to resolve flaky integration tests.

Issues Resolved

Part of #409

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: Joshua Palis <[email protected]>
Copy link

codecov bot commented Jan 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (92d9108) 71.88% compared to head (81e94b4) 71.88%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #432   +/-   ##
=========================================
  Coverage     71.88%   71.88%           
  Complexity      620      620           
=========================================
  Files            78       78           
  Lines          3126     3126           
  Branches        236      236           
=========================================
  Hits           2247     2247           
  Misses          772      772           
  Partials        107      107           

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

…ng indices after each test run

Signed-off-by: Joshua Palis <[email protected]>
@joshpalis
Copy link
Member Author

joshpalis commented Jan 21, 2024

Security tests are failing due to the misconfiguration of the admin client when attempting to wipe all indices after each test run. The admin client incorrectly does not have the permissions to delete system indices. Will handle after non-security integration / multi-node tests pass consistently

org.opensearch.flowframework.rest.FlowFrameworkSecureRestApiIT > testSearchWorkflowWithReadAccess FAILED
    org.opensearch.client.ResponseException: method [DELETE], host [https://127.0.0.1:34647/], URI [/.plugins-flow-framework-state], status line [HTTP/2.0 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}

@joshpalis joshpalis changed the title Fix integration test set up Re-enabled multi-node integration tests and Fix integration test set up Jan 21, 2024
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

LGTM

@owaiskazi19 owaiskazi19 merged commit f039d7f into opensearch-project:main Jan 22, 2024
25 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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/flow-framework/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/flow-framework/backport-2.x
# Create a new branch
git switch --create backport/backport-432-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f039d7f802397c02bf3b1314a21a054738096d7a
# Push it to GitHub
git push --set-upstream origin backport/backport-432-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/flow-framework/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-432-to-2.x.

@owaiskazi19
Copy link
Member

@joshpalis this PR would need a manual backport.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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/flow-framework/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/flow-framework/backport-2.x
# Create a new branch
git switch --create backport/backport-432-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f039d7f802397c02bf3b1314a21a054738096d7a
# Push it to GitHub
git push --set-upstream origin backport/backport-432-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/flow-framework/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-432-to-2.x.

dbwiddis pushed a commit to dbwiddis/flow-framework that referenced this pull request Jan 22, 2024
(opensearch-project#432)

* Wiping system indices, moving ml config check to individual tests that
create a connector

Signed-off-by: Joshua Palis <[email protected]>

* spotless

Signed-off-by: Joshua Palis <[email protected]>

* Increasing ML config index timeout for multi-node case

Signed-off-by: Joshua Palis <[email protected]>

* Including flow framework config in wipeAllODFEIndices method

Signed-off-by: Joshua Palis <[email protected]>

* exluding model group index from wipe

Signed-off-by: Joshua Palis <[email protected]>

* testing wipe all indices with client

Signed-off-by: Joshua Palis <[email protected]>

* Reverting wipe method back to adminClient

Signed-off-by: Joshua Palis <[email protected]>

* removing deprovision from model tests, not necessary since we're
wiping indices after each test run

Signed-off-by: Joshua Palis <[email protected]>

* Wrapping testFailedUpdateWorkflow provision in ml config check

Signed-off-by: Joshua Palis <[email protected]>

* Fixing failing security tests

Signed-off-by: Joshua Palis <[email protected]>

* Testing model redeploy settings

Signed-off-by: Joshua Palis <[email protected]>

---------

Signed-off-by: Joshua Palis <[email protected]>
Co-authored-by: Owais Kazi <[email protected]>
joshpalis added a commit that referenced this pull request Jan 22, 2024
…ation test set up (#434)

Re-enabled multi-node integration tests and Fix integration test set up
(#432)

* Wiping system indices, moving ml config check to individual tests that
create a connector



* spotless



* Increasing ML config index timeout for multi-node case



* Including flow framework config in wipeAllODFEIndices method



* exluding model group index from wipe



* testing wipe all indices with client



* Reverting wipe method back to adminClient



* removing deprovision from model tests, not necessary since we're
wiping indices after each test run



* Wrapping testFailedUpdateWorkflow provision in ml config check



* Fixing failing security tests



* Testing model redeploy settings



---------

Signed-off-by: Joshua Palis <[email protected]>
Co-authored-by: Joshua Palis <[email protected]>
Co-authored-by: Owais Kazi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants