Allow REST tests to run in MP mode

[nielsbauman]

Mainly moves project setup from MultipleProjectsClientYamlSuiteTestCase to
ESRestTestCase. This allows both Java REST tests and YAML tests to be run
in MP mode. This PR makes use of a simple system property to toggle the behavior.
Future work will add the required gradle changes to be able to run any REST test in
MP mode more easily.

[nielsbauman]

At some point in the future, we won't need this class anymore. But until we make the required gradle and CI changes, we still need these classes to run MP suites.

[pxsalehi]

A question for my understanding, it is a bit unclear to me what we're going for here. Do we want to run all rest yaml tests or all rest ITs (or both) to run by default in MP-mode, i.e., with more than one project in the cluster? And if so, we want that to be the only mode of running those rest yaml/ITs or in parallel to them?

[ywangd]

Rene suggested to use a separate gradle task, e.g. javaRestTestMp, for multi-project REST tests in the Slack thread. I wonder whether we should start with that instead of trying to improving the current system property based approach. What I had in mind is that the tests will begin with a dedicated Gradle task which configures the test clusters with MP related settings similar to how CoreServerlessLocalClusterSpecBuilder works. This also makes sure it works only for the new Java test clusters. ESRestTestCase will not need to check the system property but instead tells the difference by calling a new method similar to the existing getTestRestCluster() method. I also think adding/removing extra projects could be using either the APIs or file-based settings based on the test cluster's variant, e.g. whether it is serverless or stateful, something that can probably be exposed via the new method as well.

I am no Gradle expert. So it might be that not all of the above is feasible. It is also possible that the separate Gradle task internally uses system property as well. But overall I still feel it is better to start with the new Gradle task so that we have the overall framework in place first before optimizing the downstream usages.

[nielsbauman]

Thanks for your input, @ywangd! I indeed assumed the gradle task would make use of a system property (or something similar) but my knowledge of gradle is very limited - I also didn't know CoreServerlessLocalClusterSpecBuilder was a thing. It indeed sounds like that spec builder thing is something that would fit well here too. @breskeby could you share your insights on what would be a good approach here?

[breskeby]

The seperate gradle test task would just be a thin layer on top and pass a system property to the test suite to enable the MP mode. the benefit of having that done in a dedicated task (e.g. javaRestTestMP) would be that

1. we have a clear test history indicating the mode we ran the tests with
2. facilitate better caching of the test suites (gradle relies on inputs not changing for figuring out if a test task can be skipped or not)
3. better ux from the IDE as developers can select which test task to actually run without fiddling with system properties in the commandline or IDEA run test configuration dialog

For having that dedicated gradle task added I would want to have this work merged first: #126341

But with this PR here you should be already able to run MP only suites that we also wanna support afair (test subprojects that contain only MP related tests and running in non mp mode wouldn't make any sense)

[ywangd]

The dedicated Gradle task itself, as an entry point, can be kept simple. I was hoping there could be an additional shared Gradle logic so that enabling MP mode is handled outside of ESRestTestCase (similar to how CoreServerlessLocalClusterSpecBuilder sets up a cluster so that stateful tests can run in the serverless mode). The beneifit is that any REST tests (existing or future) with the new Java test cluster can run in the MP mode without any change to the test class itself. Otherwise, we will need to update each Java test cluster builder to check the sys property and enabling MP. All the setup work such as adding/removing projects (with some auxiliary changes from the test cluster) can be done within ESRestTestCase. But enabling MP itself needs to happen outside because it would too late to try it in ESRestTestCase.

Does it sound plausible?

[nielsbauman]

@breskeby do you see any reason not to try to move the enabling of MP mode to a ClusterSpecBuilder? I can give that a try and see if I can get that to work.

[elasticsearchmachine]

Pinging @elastic/es-delivery (Team:Delivery)

[nielsbauman]

I've just marked this PR as "ready for review". I added a MultiProjectEnabledClusterConfigProvider in bf26c1b. That's used in the DefaultLocalClusterSpecBuilder and configures the cluster setting based on the system property - it's based on the FipsEnabledClusterConfigProvider.

@ywangd I don't know if this is exactly what you had in mind, but it still requires a system property to make things work. Since that's the same approach we use for FIPS, I figured there's at least some precedent. What do you think?

[ywangd]

Thanks for working on this. The addition of MultiProjectEnabledClusterConfigProvider looks good to me. It is simpler than a complete new specBuilder. It's fine to depend a system propery. The system property may come from the dedicate Gradle task in the future. For now, it is OK to explicitly specify it. I'd prefer logics in ESRestTestCase do not depend on it though because it can be queried from the cluster itself, either through the testCluster interface or REST requests/responses with the actual cluster.

I suggest we make this PR focus on changes to the TestCluster and the MP yaml tests (simply by passing the new system property as a test to show it works). It is easier to review especially since we need Rene's approval on test cluster changes.

I prefer leaving changes to ESRestTestCase to a separate PR because I'd like to see it handles more generally between cluster with test-mp plugin and the serverless-mp plugin. It's better iterated with its own PR.

We can also leave the MP-only tests to a separate PR. What I have in mind is that they should be placed under a javaRestTestMp folder which automatically triggers the new Gradle task that specifies the system property for us. Existing tests in javaRestTest folder will see two Gradle tasks, one as is and a new opt-in one with MP enabled.

This PR is still linked with the serverless PR. I think we should unlink them. The serverless side can use a similiar configProvider for ServerlessLocalClusterSpecBuilder. I think a separate PR for this change (plus using it for the serverless side's MP yaml tests) would be preferable as well.

[nielsbauman]

I'd prefer logics in ESRestTestCase do not depend on it [the system property] though because it can be queried from the cluster itself, either through the testCluster interface or REST requests/responses with the actual cluster.

How would that work exactly? You mentioned ESRestTestCase#getTestRestCluster() before, but that just returns a string from a system property by default. In ESIntegTestCase you have a handle to the cluster object, but in the REST tests you don't. You have a handle to a cluster spec builder when you create the cluster in the test class (i.e. not ESRestTestCase). Were you suggesting adding a new method to ESRestTestCase that subclasses have to override in order to enable MP mode (just like they override getRestTestCluster() now)? That would still require changes in every subclass to allow them to run in MP mode.

I prefer leaving changes to ESRestTestCase to a separate PR because I'd like to see it handles more generally between cluster with test-mp plugin and the serverless-mp plugin.

I'm not sure I follow why you want to split up this PR. My reason for combining things this way is that I eliminated all duplicate configuration, and make everything configure MP tests in the same way. That way, it's more clear in next steps what we need to change (and where). I like your suggestion of javaRestTestMp but I see that as a next step, not something that necessarily has to happen along with the MP-only test changes I'm doing here. My preference is to make use of the new configuration I introduce here and align all usages to configure MP tests in a consistent way.

With my current changes, we need to make some changes in serverless too, to align the way MP mode is enabled (otherwise tests fail). I agree that creating a dedicated config provider in serverless should be done in a follow-up PR.

[nielsbauman]

@breskeby could you have a look at this when you have time?

Yang and I discussed this over Zoom today and we're in agreement with the current approach. There are some next steps to this of course, but we'll start tackling them after this PR.

I just tested (again) and with the changes in this PR I am able to run java REST tests in MP mode simply by passing the system property -Dtests.multi_project.enabled=true 🎉.

[ywangd]

LGTM

Please want to get approval from @breskeby as well. Thanks!

[ywangd]

Nit: This is not necessary since the distribution is DEFAULT, i.e. all modules are included.

[breskeby]

do we pick DEFAULT here purposly? or would INTEG_TEST be enough?

[nielsbauman]

This configuration is copied from XPackRestIT. Since this test suite is a collection of a lot of different x-pack features (I think we still want to split those tests up, but that's unrelated to MP), these tests will need most x-pack modules. I don't think we'd save a lot by converting this to INTEG_TEST.

[breskeby]

Lgtm in general. nothing from blocking. left just a few questions regarding DEFAULT distro vs. INTEG_TEST

[breskeby]

Why was that changed? Using DEFAULT is build wise quite more expensive to run as it requires all plugins and modules to be build as part of this. In general we should aim to use the INTEG_TEST distribution where possible. If there's any convenience we can add to make that easier, lets do that.

[nielsbauman]

Tbh, I mainly removed it because of the TODO and the tests passed. I'll revert the changes and remove the comments.

The DistributionType names are unintuitive to me. I asked about this in #es-elivery a few days ago so now I understand what the difference is why we actually want INTEG_TEST. I think a first step to make people move away from DEFAULT to INTEG_TEST is to rename the values. DEFAULT could maybe be ALL_MODULES with a JavaDoc explaining why you shouldn't use that. Then INTEG_TEST could maybe be DEFAULT with a JavaDoc explaining that you need to add modules yourself (plus an explanation why we want people to use the latter). These are just suggestions, my point is that not wanting people to use something called DEFAULT without any docs (except for an email that people like me might have missed) is a bit confusing.