-
Notifications
You must be signed in to change notification settings - Fork 94
Support multi node integ tests #1320
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
base: main
Are you sure you want to change the base?
Support multi node integ tests #1320
Conversation
c4578a3
to
e4cb9cf
Compare
Gradle check is failing. |
@owaiskazi19 Can you update the change log? |
Unrelated test is failing on main as well
|
0fd5b66
to
0b70dac
Compare
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.
Looks good to me! Thanks for the PR @owaiskazi19 !
Not have much context on the current flakey tests. Can @vibrantvarun help verify? |
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.
LGTM, thank you! @owaiskazi19
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1320 +/- ##
===========================================
+ Coverage 0 80.08% +80.08%
- Complexity 0 2185 +2185
===========================================
Files 0 159 +159
Lines 0 8326 +8326
Branches 0 1346 +1346
===========================================
+ Hits 0 6668 +6668
- Misses 0 1139 +1139
- Partials 0 519 +519 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks for the PR. I have one more request—could you set the default number of shards to 3 for the entire test? This will ensure the test properly exercises the multi-node scenario. |
.github/workflows/test_security.yml
Outdated
@@ -52,3 +52,39 @@ jobs: | |||
run: | | |||
chown -R 1000:1000 `pwd` | |||
su `id -un 1000` -c "whoami && java -version && ./gradlew integTest -Dsecurity.enabled=true" | |||
|
|||
multi-node-integ-test-with-security-linux: |
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.
why this is added only for secure cluster only, I think we need it for general non-secure mode at the first place
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.
As we are running integ tests for only secured mode https://github.com/opensearch-project/neural-search/blob/main/.github/workflows/test_security.yml#L54
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.
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.
Did coupe of improvements in the CI overall. Please see the description #1320 (comment)
I'm not sure if you mean to move or copy the entire test suite for a multi-node setup. I think we need a one node tests, we had multiple cases in the past where issue appears only in a single shard/node setup. I suggest we have both single and multi node setup as part of CI |
c662df4
to
a832c99
Compare
Signed-off-by: Owais <[email protected]>
a832c99
to
5e12c09
Compare
Signed-off-by: Owais <[email protected]>
0188474
to
6c1b43a
Compare
I thought we are sharing test cases between single node and multi node? I that case, setting shard number to be 3 even for single node won't have any harm? |
We can only change number of nodes from the outside of test logic. If we have whole test suite running on both single node and multiple nodes that's fine, we have more coverage. Where I do have concerns is if we remove tests that run on a single node and will run everything on a multiple nodes. At first glance this should lead to a yellow cluster state if we have only one shard without replicas. |
This is true for single node as well. Cluster health will be yellow if there is no replica even with single node. |
b512144
to
7a5fda6
Compare
Yes, you're correct, this will be same for a single node cluster, then it should be fine to run all tests on a multi node cluster |
527203d
to
7949a8f
Compare
Signed-off-by: Owais <[email protected]>
7949a8f
to
d4195b8
Compare
Signed-off-by: Owais <[email protected]>
Signed-off-by: Owais <[email protected]>
Signed-off-by: Owais <[email protected]>
Description
Support multi node integ tests.
Improvements done for CI in this PR:
Related Issues
Resolves ##1307
Check List
--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.