Make GeoIp downloader multi-project aware #128282

samxbr · 2025-05-22T01:21:50Z

This change makes the GeoIp persistent task executor/downloader multi-project aware.

the database downloader persistent task will be at the project level, meaning there will be a downloader instance per project
persistent task id is prefixed with project id, namely <project-id>/geoip-downloader for cluster in MP mode

To keep the size of PR review friendly, this PR only focus on the downloading part of GeoIP database, there will be more changes coming in separate PRs to make GeoIP multi-project aware in general.

…oip/GeoIpDownloader.java Co-authored-by: Copilot <[email protected]>

elasticsearchmachine · 2025-06-08T11:26:24Z

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

Copilot

Pull Request Overview

This PR refactors the GeoIP downloader to be aware of multiple projects by scoping persistent tasks, routing table lookups, and state updates per project.

Introduces project-scoped task IDs and a ProjectResolver to manage downloaders per project.
Extends ClusterChangedEvent, task executor, downloader, transport actions, and tests to pass and respect ProjectId.
Adds a multi-project integration test and updates existing unit tests for project context.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java	Added `testChangedCustomProjectMetadataSet` to verify per-project custom metadata changes.
server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java	Added `changedCustomProjectMetadataSet(ProjectId)` for per-project metadata diffs.
muted-tests.yml	Unmuted old ingest-geoip client YAML tests.
modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTests.java	Updated unit tests to initialize and pass `ProjectId` and `ProjectResolver`.
modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutorTests.java	Replaced cluster-state APIs in tests with project-scoped builders.
modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceTests.java	Extended `createClusterState` helpers to accept `ProjectId`.
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpStatsTransportAction.java	Injected `ProjectResolver` and fetch task per project.
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java	Wired `ProjectResolver` into the downloader executor.
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java	Refactored to maintain a map of downloaders per project, use per-project settings, and handle cluster changes.
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java	Made downloader project-aware in index lookups, logging, and boundary checks.
modules/ingest-geoip/qa/multi-project/src/javaRestTest/java/geoip/GeoIpMultiProjectIT.java	New REST integration test for multi-project GeoIP download behavior.
modules/ingest-geoip/qa/multi-project/build.gradle	Added the multi-project QA module to build and test.

Comments suppressed due to low confidence (1)

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java:239

There are no unit tests covering the multi‐project clusterChanged logic for starting and stopping tasks per project. Add tests to verify behavior when settings change, pipelines are added/removed, and projects are deleted.

if (event.metadataChanged() == false) {

Copilot · 2025-06-08T11:27:26Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

-                GeoIpDownloader currentDownloader = getCurrentTask();
-                if (currentDownloader != null) {
-                    currentDownloader.requestReschedule();
+        for (var projectMetadataEntry : event.state().metadata().projects().entrySet()) {


The clusterChanged method only iterates over newly present projects, so removed projects never trigger task removal. Consider also iterating previousState.metadata().projects() to stop tasks for projects that were deleted.

Not sure if this is valid. To delete a project, the related persistent tasks ought to be cancelled before the project metadata is deleted from the cluster state, otherwise all sorts of things can go wrong. Project deletion should probably be handled separately.

the related persistent tasks ought to be cancelled before the project metadata is deleted from the cluster state, otherwise all sorts of things can go wrong

Yep, I agree. Project deletion is currently still being discussed, so we can leave this for now. Can you put a @FixForMultiProject with a comment to remind us that we'll want to have some safeguards in place for project deletion? I'm sure we'll want to have safeguards, but I'm not sure yet what we should do as a safeguard, hence the annotation reminder.

Should the safeguard be at the place where project deletion is handled (probably non-existing yet) instead instead of here? Perhaps it's better to track this safeguarding in the work for project deletion in general. I added a comment here as a reminder.

I'd prefer that we put the annotation here as well. That way we're sure it won't be forgotten.

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

muted-tests.yml

nielsbauman

Thanks for working on this, Sam! And thanks for splitting the changes up into a very comfortable size :)

Can you also update the title and/or final squashed commit message to be a bit more specific?
Did you try running the GeoIP YAML tests in MP mode too? Or do those require some more work?

modules/ingest-geoip/qa/multi-project/src/javaRestTest/java/geoip/GeoIpMultiProjectIT.java

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

nielsbauman · 2025-06-09T13:28:45Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

-                GeoIpDownloader currentDownloader = getCurrentTask();
-                if (currentDownloader != null) {
-                    currentDownloader.requestReschedule();
+        for (var projectMetadataEntry : event.state().metadata().projects().entrySet()) {


the related persistent tasks ought to be cancelled before the project metadata is deleted from the cluster state, otherwise all sorts of things can go wrong

Yep, I agree. Project deletion is currently still being discussed, so we can leave this for now. Can you put a @FixForMultiProject with a comment to remind us that we'll want to have some safeguards in place for project deletion? I'm sure we'll want to have safeguards, but I'm not sure yet what we should do as a safeguard, hence the annotation reminder.

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java

settings.gradle

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

samxbr · 2025-06-10T09:27:06Z

Can you also update the title and/or final squashed commit message to be a bit more specific?

Updated the title, and will for sure squash the commits before merging

Did you try running the GeoIP YAML tests in MP mode too? Or do those require some more work?

The GeoIP yaml tests requires more work in MP, which is loading the databases from system index to ingest node. To limit the size of this PR, I will include that part in another PR

nielsbauman · 2025-06-18T13:44:41Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

        if (clusterService.state().nodes().isLocalNodeElectedMaster() == false) {
            // we should only start/stop task from single node, master is the best as it will go through it anyway
            return;
        }
        if (enabled) {
-            startTask(() -> {});
+            startTask(projectResolver.getProjectId(), () -> {});


This only works because the project resolver currently still returns a fallback project ID (which will be removed in the future). This will need to be updated in the future anyway as well (when these settings are made project-aware), but I'd still prefer that we just hard-code ProjectId.DEFAULT instead. That way we at least don't rely on temporary behavior. The same goes for the methods/settings below.

nielsbauman · 2025-06-18T13:48:05Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

-                GeoIpDownloader currentDownloader = getCurrentTask();
-                if (currentDownloader != null) {
-                    currentDownloader.requestReschedule();
+        for (var projectMetadataEntry : event.state().metadata().projects().entrySet()) {


I'd prefer that we put the annotation here as well. That way we're sure it won't be forgotten.

nielsbauman · 2025-06-18T13:49:45Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

+        for (var projectMetadataEntry : event.state().metadata().projects().entrySet()) {
+            ProjectId projectId = projectMetadataEntry.getKey();
+            ProjectMetadata projectMetadata = projectMetadataEntry.getValue();


Suggested change

for (var projectMetadataEntry : event.state().metadata().projects().entrySet()) {

ProjectId projectId = projectMetadataEntry.getKey();

ProjectMetadata projectMetadata = projectMetadataEntry.getValue();

for (var projectMetadata : event.state().metadata().projects().values()) {

ProjectId projectId = projectMetadata.id();

nielsbauman · 2025-06-18T14:03:04Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

+            boolean taskIsBootstrapped = taskIsBootstrappedByProject.computeIfAbsent(projectId, k -> false);
+            if (taskIsBootstrapped != true) {
+                taskIsBootstrappedByProject.put(projectId, true);
+                this.taskIsBootstrappedByProject.computeIfAbsent(projectId, k -> hasAtLeastOneGeoipProcessor(projectMetadata));


Ah, I missed that taskIsBootstrappedByProject is used async in startTask and stopTask. The current code isn't 100% airtight. I was checking on ConcurrentHashMap if there's anything we can use, but I don't think so. ConcurrentHashMap#replace is close, but it doesn't account for unset/null values... So, I think we have to go back to the AtomicBoolean inside the map again, sorry about that. I think the other maps are OK like this.

nielsbauman · 2025-06-18T14:13:00Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

+                var atLeastOneGeoipProcessor = atLeastOneGeoipProcessorByProject.computeIfAbsent(projectId, k -> false);
+                boolean newAtLeastOneGeoipProcessor = hasAtLeastOneGeoipProcessor(projectMetadata);
+
+                if (newAtLeastOneGeoipProcessor && atLeastOneGeoipProcessor == false) {


Suggested change

var atLeastOneGeoipProcessor = atLeastOneGeoipProcessorByProject.computeIfAbsent(projectId, k -> false);

boolean newAtLeastOneGeoipProcessor = hasAtLeastOneGeoipProcessor(projectMetadata);

if (newAtLeastOneGeoipProcessor && atLeastOneGeoipProcessor == false) {

var atLeastOneGeoipProcessor = atLeastOneGeoipProcessorByProject.getOrDefault(projectId, false);

if (atLeastOneGeoipProcessor == false && hasAtLeastOneGeoipProcessor(projectMetadata)) {

and move the put below to inside the if-block.

doing getOrDefault instead of computIfAbsent avoids the redundant new value

performing the (relatively expensive check) as the second condition allows us to skip it if we already set this value to true once before (as it'll never get set to false, we only remove the entry)

we don't need to put false on every run

nielsbauman · 2025-06-18T14:14:52Z

...s/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java

+                    .projectState(projectId)
                    .metadata()


Suggested change

.projectState(projectId)

.metadata()

.metadata()

.getProject(projectId)

No need to convert to ProjectState here.

nielsbauman · 2025-06-18T14:16:31Z

...est-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutorTests.java

@@ -287,4 +281,14 @@ private ClusterState clusterStateWithIndex(Consumer<Settings.Builder> consumer,
            .build();
        return ClusterState.builder(ClusterName.DEFAULT).putProjectMetadata(project).build();


I think we can remove the clusterStateWithIndex method, right?

nielsbauman · 2025-06-18T20:11:24Z

server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java

+        Set<String> changed = new HashSet<>();
+        ProjectMetadata project = state.metadata().projects().get(projectId);
+        ProjectMetadata previousProject = previousState.metadata().projects().get(projectId);
+        if (previousProject != null && project != null) {
+            changed.addAll(changedCustoms(project.customs(), previousProject.customs()));
+        } else if (previousProject != null) {
+            changed.addAll(previousProject.customs().keySet());
+        } else if (project != null) {
+            changed.addAll(project.customs().keySet());
+        }
+        return changed.contains(customMetadataType);


This still computes the entire Set unnecessarily. What do you think about something like this:

Suggested change

Set<String> changed = new HashSet<>();

ProjectMetadata project = state.metadata().projects().get(projectId);

ProjectMetadata previousProject = previousState.metadata().projects().get(projectId);

if (previousProject != null && project != null) {

changed.addAll(changedCustoms(project.customs(), previousProject.customs()));

} else if (previousProject != null) {

changed.addAll(previousProject.customs().keySet());

} else if (project != null) {

changed.addAll(project.customs().keySet());

}

return changed.contains(customMetadataType);

ProjectMetadata previousProject = previousState.metadata().projects().get(projectId);

ProjectMetadata project = state.metadata().projects().get(projectId);

Object previousValue = previousProject == null ? null : previousProject.customs().get(customMetadataType);

Object value = project == null ? null : project.customs().get(customMetadataType);

return Objects.equals(previousValue, value) == false;

nielsbauman · 2025-06-18T20:29:36Z

server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java

+                .putProjectMetadata(ProjectMetadata.builder(project2).build())
+                .build();
+            ClusterChangedEvent event = new ClusterChangedEvent("_na_", newState, originalState);
+            assertTrue(event.customMetadataChanged(project2.id(), IndexGraveyard.TYPE));


Is this IndexGraveyard line a leftover, or did you put it there intentionally?

Make GeoIp multi-project aware

49adbff

elasticsearchmachine added the v9.1.0 label May 22, 2025

samxbr and others added 24 commits May 21, 2025 22:25

Add project resolver in plugin

6ce72b7

Update downloader

22cc67c

[CI] Auto commit changes from spotless

48ef756

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

e68c0ad

Add projectId context

6bacb71

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

bf65981

[CI] Auto commit changes from spotless

42c863f

remove yaml tests

b82df2f

Fix bug for atLeastOneGeoipProcessorByProject

6d425a4

Temporarily unmute geoip yaml tests

54e7975

Add unit tests for changedCustomProjectMetadataSet

f5ece95

[CI] Auto commit changes from spotless

b561598

WIP: make GeoIp search project aware

2ddbdea

[CI] Auto commit changes from spotless

64b7145

WIP changes

c244087

[CI] Auto commit changes from spotless

45d982e

Revert GeoIp search to reduce size of change

efb99d3

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

2382456

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

bcf458b

Add java rest test

612651a

remove internal cluster test

2789e08

Update settings.gradle

789399e

remove MP rest test change

2c384aa

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

0552403

samxbr requested a review from Copilot June 8, 2025 09:38

This comment was marked as outdated.

Sign in to view

samxbr and others added 2 commits June 8, 2025 17:40

Update modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/ge…

77ab9f0

…oip/GeoIpDownloader.java Co-authored-by: Copilot <[email protected]>

[CI] Auto commit changes from spotless

a54d56b

samxbr requested a review from Copilot June 8, 2025 11:25

samxbr marked this pull request as ready for review June 8, 2025 11:26

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

Copilot AI reviewed Jun 8, 2025

View reviewed changes

samxbr commented Jun 8, 2025

View reviewed changes

muted-tests.yml Outdated Show resolved Hide resolved

nielsbauman reviewed Jun 9, 2025

View reviewed changes

samxbr changed the title ~~Make GeoIp multi-project aware~~ Make GeoIp downloader multi-project aware Jun 10, 2025

samxbr mentioned this pull request Jun 10, 2025

Unmute IngestGeoIpClientYamlTestSuiteIT #129178

Merged

samxbr and others added 2 commits June 10, 2025 20:02

Remove atomic and test change

5092830

[CI] Auto commit changes from spotless

f3dc2f8

samxbr mentioned this pull request Jun 11, 2025

[CI] IngestGeoIpClientYamlTestSuiteIT class failing #111497

Closed

samxbr and others added 12 commits June 11, 2025 11:59

Remove AtomicBoolean and update customMetadataChanged

8fb68bd

revert settings.gradle

d82724e

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

d0721e6

[CI] Auto commit changes from spotless

4e2eefa

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

43d5490

Replace executeOnProject with explicit project methods

29936f5

Add assert

0974d41

remove constructor param

3690cd9

remove downloader project resolver

303f4f6

Fix unit tests

22db3b8

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

1140e18

Add update project

ba8e9a1

samxbr requested a review from nielsbauman June 17, 2025 16:10

samxbr added 2 commits June 18, 2025 15:29

Mock updateProjectPersistentTaskState

59c0286

Add project id to mock client

69240ed

nielsbauman reviewed Jun 18, 2025

View reviewed changes

		@@ -287,4 +281,14 @@ private ClusterState clusterStateWithIndex(Consumer<Settings.Builder> consumer,
		.build();
		return ClusterState.builder(ClusterName.DEFAULT).putProjectMetadata(project).build();

Make GeoIp downloader multi-project aware #128282

Are you sure you want to change the base?

Make GeoIp downloader multi-project aware #128282

Conversation

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

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

elasticsearchmachine commented Jun 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 8, 2025

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!

nielsbauman 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samxbr commented Jun 10, 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!

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!

samxbr commented May 22, 2025 •

edited

Loading