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 <175728472+Copilot@users.noreply.github.com>

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.

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

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

I don't think we can move the put inside the if block or avoid hasAtLeastOneGeoipProcessor . It's possible to remove a Geoip processor, in which case we need to set the value from true to false. It's also easier to reason about the logic this way.

Ah yeah you're right; I misread the old code. I think it's still worth doing getOrDefault instead of computeIfAbsent and I'd wrap the put inside a if (atLeastOneGeoipProcessor != newAtLeastOneGeoipProcessor), to save some unnecessary updates.

I'd prefer still using computeIfAbsent, there's a value always exists in the map for each project, it's just one put for every project, I don't think it's a huge problem. I added the conditional put though.

I don't understand what the value is of doing the compute? It's an extra put on the map without any value.

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

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

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

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?

It's intentional, IndexGraveyard is always added when a new project is created, this checks that IndexGraveyard is "changed".

Alright, could you put a comment here and below to mention that?

nielsbauman · 2025-06-19T16:17:10Z

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

+                this.taskIsBootstrappedByProject.computeIfAbsent(
+                    projectId,
+                    k -> new AtomicBoolean(hasAtLeastOneGeoipProcessor(projectMetadata))
+                );


I don't think these lines are correct. We're already setting taskIsBootstrapped above, right? Or am I missing something?

Good catch!

nielsbauman · 2025-06-19T16:23:18Z

...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) {


Ah yeah you're right; I misread the old code. I think it's still worth doing getOrDefault instead of computeIfAbsent and I'd wrap the put inside a if (atLeastOneGeoipProcessor != newAtLeastOneGeoipProcessor), to save some unnecessary updates.

nielsbauman · 2025-06-19T16:25: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));


Alright, could you put a comment here and below to mention that?

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 <175728472+Copilot@users.noreply.github.com>

[CI] Auto commit changes from spotless

a54d56b

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

Address comments

f5a5b86

samxbr requested a review from nielsbauman June 19, 2025 14:44

elasticsearchmachine and others added 2 commits June 19, 2025 14:55

[CI] Auto commit changes from spotless

735c88c

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

6d5be04

nielsbauman reviewed Jun 19, 2025

View reviewed changes

samxbr added 2 commits June 20, 2025 12:46

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

a6d155b

Address comments

734a6fa

samxbr requested a review from nielsbauman June 20, 2025 04:48

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!

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!

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!

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