Register IngestGeoIpMetadata as a NamedXContent

[joegallo]

If you upgrade Elasticsearch via a full cluster restart, then the DatabaseConfigurationMetadata entries from the cluster state get cleared out. This is easiest to reproduce by putting a single node cluster through an upgrade OR a restart. In either of those cases, we restore the cluster state by reading the x-content from disk (rather than serializing it over the network), so we're exercising an entirely different path than in the case of the ordinary operations of a cluster, or the case of rolling (node-by-node) upgrades.

---

I renamed the existing full-cluster-restart test to geoip-reindexed to better capture the purpose of the test, and I added a new full-cluster-restart test that would have caught wrong behavior here (note: it really doesn't do anything more than that, though, but I'd argue that's enough for right now).

As a matter of inside baseball, the previous paragraph means that you'd almost certainly be better off reviewing this PR commit by commit rather than reading the overall diff of the PR. But do as you'd like.

[elasticsearchmachine]

Hi @joegallo, I've created a changelog YAML for you.

[DaveCTurner]

In terms of tests, I would expect ./gradlew :modules:ingest-geoip:qa:full-cluster-restart:check to pick this up. It seems that it doesn't, which indicates maybe this test needs to be expanded.

Also when I ran that task I saw that it only attempts an upgrade from v8.19.0. If it's trying to test BwC then it should be picking up all compatible versions right? And it should definitely be doing a full-cluster-restart without an upgrade too.

[DaveCTurner]

By "pick this up" I mean I would expect some assertion somewhere to trip, if we wrote out some NamedXContent to the cluster state and then couldn't read it back in again. But then I see that we just quietly ignore that kind of issue even in tests:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

Lines 2684 to 2686 in 2ee7ca4

	} catch (NamedObjectNotFoundException ex) {
	logger.warn("Skipping unknown custom object with type {}", currentFieldName);
	parser.skipChildren();

[DaveCTurner]

FTR we don't hit that lenient code in ./gradlew :modules:ingest-geoip:qa:full-cluster-restart:check anyway, in the sense that this task still passes even with the following:

diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java
index 35e853cdd55a..bc7e08fd76eb 100644
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java
@@ -2682,6 +2682,7 @@ public class Metadata implements Iterable<IndexMetadata>, Diffable<Metadata>, Ch
                             Custom custom = parser.namedObject(Custom.class, currentFieldName, null);
                             builder.putCustom(custom.getWriteableName(), custom);
                         } catch (NamedObjectNotFoundException ex) {
+                            assert false : ex;
                             logger.warn("Skipping unknown custom object with type {}", currentFieldName);
                             parser.skipChildren();
                         }

So there's at least three test problems here:

1. We don't actually run full-cluster-restart tests to cover very many relevant scenarios.
2. Even in the ones we do cover, we don't try and insert the problematic object in the cluster state first.
3. Even if we did insert such a problematic object, the tests wouldn't automatically fail when encountering a missing NamedXContent registration.

[joegallo]

I happen to know the history here -- I wrote the FullClusterRestartIT three years ago in #85792 specifically to cover the case of upgrading and using the system indices migration feature, that's why it covers such a limited number of upgrade-from versions. Reading between the lines of what I know now from this conversation, that test is poorly named -- we don't have (and never have had) a true FullClusterRestartIT for the geoip code.

I'll rename the test to better reflect its true purpose, and write a new FullClusterRestartIT that would have failed without the fix on this PR.

I'll add the assert you mentioned, too. I'd already found that line of code and had a big loop of printlns there to figure out why it was claiming that there wasn't any way to handle "ingest_geoip" when I'd clearly registered all the relevant NamedWriteables (I'm in the future, too, so I understand that this thinking was not right, although I can't actually explain confidently why it isn't right).

However, I think we should go one more: I'm curious what it would look like to add an assert that makes sure that a cluster state that we write to disk is deserializable from disk (immediately after writing). This would have failed on the code prior to this PR, and it wouldn't have required a test that we don't have, and that, indeed, I didn't realize I had to write.

[DaveCTurner]

However, I think we should go one more: I'm curious what it would look like to add an assert that makes sure that a cluster state that we write to disk is deserializable from disk (immediately after writing).

We already have this, see org.elasticsearch.gateway.PersistedClusterStateService#getAssertOnCommit. The trouble is that the state in question is deserializable (i.e. doesn't throw an exception) but not faithfully so.

[elasticsearchmachine]

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

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	8.18	Commit could not be cherrypicked due to conflicts
❌	8.x	Commit could not be cherrypicked due to conflicts
✅	9.0
❌	8.16	Commit could not be cherrypicked due to conflicts
❌	8.17	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123079

[joegallo]

Manual backports are up.