Run GeoIp YAML tests in multi-project cluster and fix bug discovered by tests #131521

samxbr · 2025-07-18T13:07:47Z

This change runs the GeoIP YAML REST tests in multi-project cluster, it also fixes a bug in DatabaseNodeService discovered by running the YAML tests in MP mode.

Sample test run

Copilot

Pull Request Overview

This PR enables GeoIP YAML REST tests to run in multi-project clusters and fixes a bug in the DatabaseNodeService class. The changes include refactoring the database checking logic to support multi-project environments and adding new test infrastructure.

Refactored DatabaseNodeService.checkDatabases() to extract project-specific logic into a separate method
Added new YAML test suite for multi-project GeoIP functionality
Fixed package declaration in existing Java test class
Updated build configuration to support YAML REST tests

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
DatabaseNodeService.java	Refactored database checking logic to support multi-project clusters by extracting project-specific logic
IngestGeoIpClientMultiProjectYamlTestSuiteIT.java	Added new YAML test suite for multi-project GeoIP functionality
GeoIpMultiProjectIT.java	Fixed package declaration and removed redundant imports
build.gradle	Added YAML REST test plugin configuration and dependencies

modules/ingest-geoip/qa/multi-project/build.gradle

elasticsearchmachine · 2025-07-18T23:10:54Z

Pinging @elastic/es-data-management (Team:Data Management)

PeteGillinElastic · 2025-07-24T20:02:39Z

...stTest/java/org/elasticsearch/ingest/geoip/IngestGeoIpClientMultiProjectYamlTestSuiteIT.java

+                containsInAnyOrder("GeoLite2-City.mmdb", "GeoLite2-Country.mmdb", "GeoLite2-ASN.mmdb", "MyCustomGeoLite2-City.mmdb")
+            );
+        }, 10, TimeUnit.SECONDS);
+    }


Is this the same as the method in IngestGeoIpClientYamlTestSuiteIT? Do you think it's worth commoning them up somehow? It seems like quite a lot of code to duplicate — if we did something to how this works that meant we had to do a subtle fix, it would be a pain to have to do it twice.

That's a good point. But I was struggling to get methods from IngestGeoIpClientYamlTestSuiteIT imported here. Do you know how to do it? I am still trying to get it working.

Hmm, so I guess you might be able to declare a javaRestTestImplementation on the module where the other IT class is declared. I'm not sure if having test modules that depend on other test modules is allowed or not — most places I've worked before certainly wouldn't consider it best practice. I guess the other option would be to put it somewhere that both IT classes already have access to, e.g. GeoIpHttpFixture or something... but it kind of doesn't feel like it belongs there either.

I dunno, maybe this isn't worth the effort WDYT?

Turns out we have this elasticsearch.internal-test-artifactgradle plugin that does just what we want. Having test module dependent on another test module feels kind of ok here to me since we are running a MP version of the same tests.

PeteGillinElastic · 2025-07-24T20:03:09Z

...stTest/java/org/elasticsearch/ingest/geoip/IngestGeoIpClientMultiProjectYamlTestSuiteIT.java

+        Request putPipelineRequest = new Request("PUT", "/_ingest/pipeline/pipeline-with-geoip");
+        putPipelineRequest.setEntity(new ByteArrayEntity(bytes.array(), ContentType.APPLICATION_JSON));
+        client().performRequest(putPipelineRequest);
+    }


Same comment about commoning up.

Reused code from IngestGeoIpClientYamlTestSuiteIT

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java

This reverts commit 5f651a3.

nielsbauman · 2025-07-28T18:46:38Z

...stTest/java/org/elasticsearch/ingest/geoip/IngestGeoIpClientMultiProjectYamlTestSuiteIT.java

+import static org.elasticsearch.ingest.geoip.IngestGeoIpClientYamlTestSuiteIT.assertDatabasesLoaded;
+import static org.elasticsearch.ingest.geoip.IngestGeoIpClientYamlTestSuiteIT.putGeoipPipeline;
+
+public class IngestGeoIpClientMultiProjectYamlTestSuiteIT extends MultipleProjectsClientYamlSuiteTestCase {


I'm probably missing something, but I'm having trouble identifying why we need this dedicated YAML test suite. Can you explain why we can't just run the regular YAML suite in MP mode (i.e. tests.multi_project.enabled=true)?

I thought we'd want to run the yaml tests in both MP enabled and disabled mode, is there a way to do that with the existing test suite?

I thought we'd want to run the yaml tests in both MP enabled and disabled mode

100% agree on that

is there a way to do that with the existing test suite?

Once ES-12094 gets implemented, that should be trivial. However, I'm not familiar enough with Gradle to know if there's a simple way to do that right now. Perhaps you can ask in #es-delivery - mainly for an answer to this question and potentially also a status update on the ticket itself.

Thanks for pointing the issue. Until that is implemented, I think keeping the MP YAML tests in qa/multi-project Gradle project together with other MP Java rest tests is more organized.

Also asked in the linked Slack channel to see if there's better way of doing this now.

Thanks for asking in the channel. If we don't find a better way to do it, I think I'd be more in favor of not running the MP YAML tests in the CI yet. I don't think these changes are that small and especially if we're combining them with the fix itself, it'll be annoying to revert.

I am not sure, leaving the MP YAML tests out of CI seems missing test coverage, considering the amount of code change have been made to make GeoIP project-aware (especially there's already a bug found from these tests). Although MP is not in use yet, so probably not a huge due if there's bug.
Why do we want to revert? If these tests fail in CI then they should be muted, and we can fix them instead of reverting.

I agree that we're missing test coverage if we don't run this GeoIP YAML suite in MP mode, but there are other suites that we're not covering yet either, and, most importantly, we're not close to running any of this in production. So, I think we can live without coverage a little longer.

Why do we want to revert?

Once the Jira ticket I linked gets implemented, all these changes become redundant, and we can just do something like

tasks.register('multiProjectYamlTest') { }

or whatever the gradle lines will look like to make a YAML test suite run in MP mode as well.

Once the Jira ticket I linked gets implemented, all these changes become redundant

That's true, I have added a FixForMultiProject as reminder. For this specific PR, I think there's value including the tests suite as it validates the bug has been fixed. We can always remove it after the linked ticket is implemented.

Thanks for adding the annotation. I still don't really think it's worth merging these changes, but I also don't think it's worth discussing this any longer.

Thanks Niels, really appreciate your thought on this. I also don't think it's worth spending more time discussing on this. I agree that this YAML test change is not effortless, we can defer making similar changes for other modules until ES-12094 is implemented.

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java

PeteGillinElastic

Just a couple of things to add to Niels's points — I will leave the discussion on testing to you as you seem to have more knowledge there.

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java

nielsbauman · 2025-07-31T09:32:28Z

...stTest/java/org/elasticsearch/ingest/geoip/IngestGeoIpClientMultiProjectYamlTestSuiteIT.java

+import static org.elasticsearch.ingest.geoip.IngestGeoIpClientYamlTestSuiteIT.assertDatabasesLoaded;
+import static org.elasticsearch.ingest.geoip.IngestGeoIpClientYamlTestSuiteIT.putGeoipPipeline;
+
+public class IngestGeoIpClientMultiProjectYamlTestSuiteIT extends MultipleProjectsClientYamlSuiteTestCase {


I agree that we're missing test coverage if we don't run this GeoIP YAML suite in MP mode, but there are other suites that we're not covering yet either, and, most importantly, we're not close to running any of this in production. So, I think we can live without coverage a little longer.

Why do we want to revert?

Once the Jira ticket I linked gets implemented, all these changes become redundant, and we can just do something like

tasks.register('multiProjectYamlTest') { }

or whatever the gradle lines will look like to make a YAML test suite run in MP mode as well.

nielsbauman · 2025-07-31T19:46:30Z

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java

+            IndexRoutingTable databasesIndexRT = projectState.routingTable().index(databasesIndex);
+            if (databasesIndexRT == null || databasesIndexRT.allPrimaryShardsActive() == false) {
+                logger.trace(
+                    "Not checking databases because geoip databases index does not have all active primary shards for" + " project [{}]",


Nit:

Suggested change

"Not checking databases because geoip databases index does not have all active primary shards for" + " project [{}]",

"Not checking databases because geoip databases index does not have all active primary shards for project [{}]",

nielsbauman

Great catch on the bug fix!

…cking * upstream/main: (166 commits) Reduce inactive sink interval in VectorSimilarityFunctionsIT (elastic#132288) ESQL: Allow agg tests to process many columns (elastic#132358) Update analysis-lowercase-tokenfilter.md (elastic#132359) Add Sparse Vector Index Options Settings to Semantic Text Field (elastic#131058) Collect node thread pool usage for shard balancing (elastic#131480) Add tasks to validate new style transport versions (elastic#131782) Mute org.elasticsearch.search.routing.SearchReplicaSelectionIT testNodeSelection elastic#132354 Mute org.elasticsearch.xpack.esql.action.CrossClusterAsyncQueryIT testBadAsyncId elastic#132353 Fixes DenseVectorFieldIndexTypeUpdateIT release tests (elastic#132346) Fix testCloseOrReallocateDuringPartialSnapshot (elastic#132049) (Doc) ILM Force Merge not on HDD and happens on hosting node not current phase tier (elastic#130280) Run GeoIp YAML tests in multi-project cluster and fix bug discovered by tests (elastic#131521) Unmutes elastic#132111, seems a transient, non reproducible issue (elastic#132253) Mute org.elasticsearch.search.suggest.phrase.PhraseSuggesterIT testPhraseSuggestionWithNgramOnlyAnalyzerThrowsException elastic#132347 Add AI21 support to Inference Plugin (elastic#131238) OpenJDK EA builds should use https instead of http (elastic#132297) ESQL: Normalize timeseries aggs slightly (elastic#132284) Avoid internal server error on suggester ngram bad request (elastic#132321) [ES|QL] Rerank operator improvements (elastic#132318) Mute org.elasticsearch.xpack.logsdb.qa.LogsDbVersusReindexedLogsDbChallengeRestIT testTermsQuery elastic#132337 ...

Run GeoIp yaml tests in multi-project cluster

17b4038

samxbr added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Jul 18, 2025

elasticsearchmachine added the v9.2.0 label Jul 18, 2025

samxbr changed the title ~~Run GeoIp yaml tests in multi-project cluster~~ Run GeoIp YAML tests in multi-project cluster Jul 18, 2025

[CI] Auto commit changes from spotless

1f9ab3c

samxbr requested a review from Copilot July 18, 2025 15:07

Copilot AI reviewed Jul 18, 2025

View reviewed changes

modules/ingest-geoip/qa/multi-project/build.gradle Outdated Show resolved Hide resolved

samxbr added 2 commits July 18, 2025 16:42

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

17a144c

minor

e3a9314

samxbr marked this pull request as ready for review July 18, 2025 23:10

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Jul 18, 2025

samxbr requested a review from PeteGillinElastic July 18, 2025 23:11

PeteGillinElastic reviewed Jul 24, 2025

View reviewed changes

samxbr added 9 commits July 25, 2025 12:14

Use ProjectState

4ed662e

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

20955aa

share yaml test code WIP

5f651a3

use projectState.cluster()

d4a08b3

Revert "share yaml test code WIP"

92935c8

This reverts commit 5f651a3.

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

348494c

share test code

ee11eba

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

9accd8e

remove unncessary plugin

0fdf1b0

nielsbauman reviewed Jul 28, 2025

View reviewed changes

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java Outdated Show resolved Hide resolved

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java Show resolved Hide resolved

samxbr changed the title ~~Run GeoIp YAML tests in multi-project cluster~~ Run GeoIp YAML tests in multi-project cluster and fix bug discovered by tests Jul 28, 2025

samxbr added 2 commits July 28, 2025 15:55

remove clusterState

d2c338c

fix

c611574

samxbr requested a review from PeteGillinElastic July 28, 2025 22:39

samxbr requested a review from nielsbauman July 28, 2025 22:39

PeteGillinElastic reviewed Jul 29, 2025

View reviewed changes

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java Outdated Show resolved Hide resolved

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java Outdated Show resolved Hide resolved

samxbr and others added 4 commits July 29, 2025 14:04

Add FixForMultiProject and other fix

31fca85

[CI] Auto commit changes from spotless

4953636

fix doc

892d97d

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

e281e0a

nielsbauman reviewed Jul 31, 2025

View reviewed changes

samxbr and others added 5 commits July 31, 2025 12:49

Remove NotMultiProjectCapable

24cd03c

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

14f0284

[CI] Auto commit changes from spotless

1287fdc

Add reminder to remove test

743602e

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

7089827

nielsbauman reviewed Jul 31, 2025

View reviewed changes

nielsbauman approved these changes Jul 31, 2025

View reviewed changes

samxbr added 3 commits July 31, 2025 16:30

comment

f02df24

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

ce80bbb

Merge branch 'main' into feature/multi-project/geoip-yaml-tests

d0e8e31

samxbr merged commit cc62555 into elastic:main Aug 1, 2025
33 checks passed

samxbr deleted the feature/multi-project/geoip-yaml-tests branch August 1, 2025 16:56

	"Not checking databases because geoip databases index does not have all active primary shards for" + " project [{}]",
	"Not checking databases because geoip databases index does not have all active primary shards for project [{}]",

Run GeoIp YAML tests in multi-project cluster and fix bug discovered by tests #131521

Run GeoIp YAML tests in multi-project cluster and fix bug discovered by tests #131521

Uh oh!

Conversation

samxbr commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PeteGillinElastic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samxbr commented Jul 18, 2025 •

edited

Loading