Proposal for Possible ways to run the search test suite with concurrent search enabled #8673
Comments
|
Hi @neetikasinghal, I'm not sure what this issue is asking. Is this a question or bug or proposal for a feature? The linked issue implies you'll be creating/modifying tests, but I'm not sure. Can you clarify in your description what you are asking for? I expect the feature template can be helpful with questions that frame the issue. |
neetikasinghal
changed the title
Thanks for the ideas @neetikasinghal! Just for clarification, are you suggesting adding another gradle task for this? Additionally, what's the scope of tests we want to do this for? Taking a step back though, I definitely think our solution should fall under the "2. Full coverage of the tests", randomization as a form of test coverage is not a good practice (this is even called out in 1. Randomization of the tests). From the developer perspective, if I am working on a bug fix or improvement specific to concurrent segment search, I want to be able to have test coverage of my fix 100% of the times that I run the tests. |
jed326
added
:test
|
Regarding randomization, as @jed326 described it is not appropriate to use randomization for coverage. However, there are many test cases that invoke a search API as a part of the test scenario that aren't specifically testing search. Just randomly picking one test as an example, this integration test of the cluster block API invokes the search API to assert a document count. How the search is done is not important to that test, so in such cases randomly selecting whether to use concurrent search is a valid approach and can help find bugs with interactions between features that should be independent. @jed326 Asks some good questions above. When you get into the details of how you select the tests you want to run in "option 4", I think it might end up looking a lot like "option 2". I also agree that whatever the mechanism we need to ensure we have full coverage. |
|
I agree on the randomization approach not to be the best mechanism. @andrross for the scenario you described if we want to test the cluster block API both with concurrent vs non concurrent semantics (which we should), then I think having explicit run will be better. For example: With randomization, let's say there is a bug in concurrent semantics found via that test. Then the test will be flaky. Once the issue gets fixed we will need to ensure that the test again runs all the time with concurrent semantics. So I think for such tests as well running it explicitly with concurrent semantics probably will help. Pro with option 4 is that it will avoid the chance to miss any new or existing search ITs with concurrent segment path. Since it is going to be another step in PR workflow which will run in parallel, atleast the test time there will not be double. I am not completely sure but probably majority of the tests will fall under category to run with concurrent segment search enabled as well, so starting simple to just run all may be fine. We can later build some mechanism to ignore tests suite which are not needed to be run with concurrent search to improve on the test time. With option 2, the issue is looking into each and every test and then adding support for those in concurrent path seems error prone to me. There are chances of missing tests with that approach. Also we will need mechanism to have all the external plugins implementations to also update their tests to use the new search base class IT. With option 4, plugins needs to set the env variable which will take care of running it with concurrent segment search with existing base class.
Good callout, we should see on how to add Yaml tests and any other test. I guess any test which is running by creating a OpenSearch cluster can be a potential candidate here. Like ones extending |
@jed326 thanks for your feedback, yes I was implying all the tests that have to be tested be added in a gradle task. |
@andrross thanks for the feedback, I agree with you, @jed326 and @sohami that we definitely need full coverage of the tests, randomization will miss out on the coverage leading to unknown bugs that would pop up later. |
@sohami thanks for the feedback. Since there will be a new gradle task to run the tests with concurrent search and it will be different from what gradle check already runs, if we want to run all the tests as part of this gradle task, the time to run these tests I think will be double than what is needed to run the tests currently. The IT tests run in parallel as part of the gradle check. I need to check if there is a way to parallelize the different gradle tasks within the same run of gradle check. |
I am not sure if I understood this, since I am assuming in the PR workflow we can run multiple tasks in parallel (like gradle check and this new task with new sets of test). So if both tasks runs in parallel it should be able to complete around same time.
Running IT tests in parallel will mostly be a My main concern with approach 2 is figuring out and updating all the tests related to search. Then also same update will be needed in all the plugins to use this new base class. Once we get enough stable runtime with concurrent search mechanism (over the releases), we can make the concurrent test run as part of mainstream run and control the concurrency (sequential vs concurrent path) via slice count (=1 vs > 1) and don't need to run tests with concurrent search disabled. So that time this new IT base class will become irrelevant and again we have to perform the cleanup. |
I was thinking to run tests as part of the gradle check as the tests need the build to be done before running the tests else the build has to be done again as part of a new workflow. I think it would be better if we can run the tests as part of gradle check only.
If we control the concurrency via the slice count, wouldn't that mean that it would go to an either/or case where we either run concurrent search or run sequential run, it wouldn't run both which means that we wouldn't have full coverage? |
I think this is not bad as an approach, we could push it further and actually generalized it over the feature flags (and may be their combinations later on), for example |
|
Thanks @neetikasinghal for putting up this for discussion. I am currently working on similar issue #8965 with remote store. I am trying to run entire Integ test suite with remote store enabled. I was planning to use system property to enabled remote store on |
thanks @reta for your feedback, as of now we are trying to figure out the number of test classes or the number of tests that would need to be added specifically for concurrent search. Once, we have that number, I think then we will be in a better situation to decide if going with approach 2 is feasible. |
@Rishikesh1159 to solve this problem temporarily, we are running periodic integration tests for concurrent search with feature flag enabled via github action in order to monitor the tests. You can look into it if you are interested: link |
@reta the parameterization can only work for the dynamic settings, so the base class can have a combination of dynamic settings that needs the parameterization. This won't work for the parameterization of the feature flags as for an IT test the cluster is launched with either the feature flag on or off and it cannot be changed at run time. You can refer to my PoC here. |
|
Also, we checked that there are around 191/540 IT classes that are touching the search use-cases and needs testing with concurrent search use-case. Given these numbers @reta @andrross @sohami what are your thoughts on the approach that we should go with?
|
I didn't get that, sorry. The feature flags are not dynamic - you cannot change them at runtime, the settings are (some, to be precise). When using the parametrized tests, the cluster is (re)constructed with the feature flags and than settings could be changed later on if needed. |
|
@neetikasinghal Given there are ~200 tests that needs to be changed and probably the approach can be useful for other features as well, I think this will be one time effort to change these tests. Also thinking more about extra step in workflow, there are chances of missed runs with features on/off in other workflows which are running the tests or will need similar change. Putting it in the test code itself will not have that caveat and all the workflow running the tests will not have to be updated and same holds true for developers running tests locally. I am leaning towards option 2 at this point. nit: Probably would be better to put the list of tests in a separate file (.txt) and merge in some branch of your github repo and share the link here.
@reta with IT tests being parameterized that doesn't seem to be the case. It sets the cluster only once and runs the test with different parameter for that cluster. This may be because in multiple tests the cluster is just set at TestSuite level not at per test level (probably to reduce the test time) |
Thanks @sohami , I assume you mean usage @OpenSearchIntegTestCase.ClusterScope (& co), that could totally be the case that this annotation does not play well with parametrized tests (since we've never needed that support), but I am pretty sure we could address that (I am by no means pushing towards using parametrized tests, while we discussing the solution in principle - any shortcoming could be addressed if we found that is the way to go). What I like about parametrized tests in this particular context - each test suite starts with clean state (it should be with caveats above), and feature flags fit perfect into that. |
|
@neetikasinghal just to reconfirm
If yes, it is also reasonable approach to try out in my opinion, no objections |
|
thanks @reta, just calling out again that this is going to need changes in 200+ test classes to extend a new base class and initialize the constructor too. |
Once concurrent search goes GA, there needs to be a way to run all the search related tests with concurrent search enabled.
Here are some options for that:
1. Randomization of the tests
We can achieve this by randomizing the cluster setting for the concurrent search in the base IT search classes and running the tests.
Pros
i. Simple and straightforward to implement
ii. The run time of the tests will remain the same
iii. Most of the other teams like segrep are following the same approach
Cons
i. It doesn’t give full coverage of the tests for concurrent search
ii. We might miss testing some scenarios which might fail later
2. Full coverage of the tests
Full coverage of the tests using concurrent search can be achieved by using the following options:
1. Create another class IT class for every class that needs to be run with concurrent search and set the cluster setting for concurrent search as true.
[Con] This would create a lot of classes making it not a solution to opt for.
2. Create a sub-class ConcurrentSearchIntegTestCase of OpenSearchIntegTestCase that has a constructor to work with the @ParametersFactory to create parameters’ values of the concurrent search cluster setting as true/false. Change all the Search ITs to extend the new base class ConcurrentSearchIntegTestCase and initialize the base class constructor. We need to change the default value of the dynamic value of the cluster setting as false and also make code changes to rely on the cluster setting rather than the feature flag as of now. A Sample code change for an IT class would look like: 5f7cd40
[Con] This would also require making changes to many test classes, this is as good as parameterizing all the test classes that need parameterization.
3. Junit has a runner for Parameterized class that can do the initialization of the parameters without the need of a constructor unlike ParametersFactory, ref: link.
[Con] This approach doesn’t work as the existing OpenSearch tests run with RandomizedRunner class defined at the test framework of lucene: ref.
4. [Recommended] We can have a system property that can enable the cluster setting for concurrent search and run the tests again with this system property.
[Con] This will double up the time taken currently to run the tests as part of the gradle check. We can run a subset of tests that concern only concurrent search use case to reduce the time take.
[Pro] This is simple to implement
Relates #7440
The text was updated successfully, but these errors were encountered: