From 3c95a3824309a449f55da20d522e78020f9fe952 Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Wed, 23 Oct 2024 15:09:30 -0500 Subject: [PATCH 1/4] Fixing DatabaseNodeServiceIT testNonGzippedDatabase and testGzippedDatabase race condition --- .../elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java | 5 +++++ .../elasticsearch/ingest/geoip/DatabaseNodeService.java | 8 ++++++++ muted-tests.yml | 6 ------ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index 786f091e0c024..0ffe98cd93b25 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -50,6 +50,11 @@ public void testNonGzippedDatabase() throws Exception { String databaseName = randomAlphaOfLength(20) + "-" + databaseFileName; byte[] mmdbBytes = getBytesForFile(databaseFileName); final DatabaseNodeService databaseNodeService = internalCluster().getInstance(DatabaseNodeService.class); + /* + * If DatabaseNodeService::checkDatabases runs it will sometimes (rarely) remove the database we are using in this test while we + * are trying to assert things about it. So we disable checkDatabase. + */ + databaseNodeService.doNotCheckDatabases = true; assertNull(databaseNodeService.getDatabase(databaseName)); int numChunks = indexData(databaseName, mmdbBytes); retrieveDatabase(databaseNodeService, databaseName, mmdbBytes, numChunks); diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java index 940231b12c894..530dcf54f35da 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java @@ -105,6 +105,11 @@ public final class DatabaseNodeService implements IpDatabaseProvider { private IngestService ingestService; private final ConcurrentMap databases = new ConcurrentHashMap<>(); + /* + * This is meant to be used by tests to disable the checkDatabases() method. That method can run at unexpected times, and removes + * any databases it considers stale (which might break tests). + */ + boolean doNotCheckDatabases = false; DatabaseNodeService( Environment environment, @@ -258,6 +263,9 @@ public void close() throws IOException { } void checkDatabases(ClusterState state) { + if (doNotCheckDatabases) { + return; + } if (state.blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK)) { return; } diff --git a/muted-tests.yml b/muted-tests.yml index bd0145611237b..084ece59789de 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -191,12 +191,6 @@ tests: - class: org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT method: test {categorize.Categorize SYNC} issue: https://github.com/elastic/elasticsearch/issues/113722 -- class: org.elasticsearch.ingest.geoip.DatabaseNodeServiceIT - method: testNonGzippedDatabase - issue: https://github.com/elastic/elasticsearch/issues/113821 -- class: org.elasticsearch.ingest.geoip.DatabaseNodeServiceIT - method: testGzippedDatabase - issue: https://github.com/elastic/elasticsearch/issues/113752 - class: org.elasticsearch.threadpool.SimpleThreadPoolIT method: testThreadPoolMetrics issue: https://github.com/elastic/elasticsearch/issues/108320 From ba4c73705af2bfecce5535c9d3a4f74cb9114a90 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 25 Oct 2024 14:33:42 -0400 Subject: [PATCH 2/4] Fix a typo --- .../org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index 0ffe98cd93b25..a093e7dcd9d2a 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -46,7 +46,7 @@ public class DatabaseNodeServiceIT extends AbstractGeoIpIT { public void testNonGzippedDatabase() throws Exception { String databaseType = "GeoLite2-Country"; String databaseFileName = databaseType + ".mmdb"; - // making the dabase name unique so we know we're not using another one: + // making the database name unique so we know we're not using another one: String databaseName = randomAlphaOfLength(20) + "-" + databaseFileName; byte[] mmdbBytes = getBytesForFile(databaseFileName); final DatabaseNodeService databaseNodeService = internalCluster().getInstance(DatabaseNodeService.class); @@ -69,7 +69,7 @@ public void testNonGzippedDatabase() throws Exception { public void testGzippedDatabase() throws Exception { String databaseType = "GeoLite2-Country"; String databaseFileName = databaseType + ".mmdb"; - // making the dabase name unique so we know we're not using another one: + // making the database name unique so we know we're not using another one: String databaseName = randomAlphaOfLength(20) + "-" + databaseFileName; byte[] mmdbBytes = getBytesForFile(databaseFileName); byte[] gzipBytes = gzipFileBytes(databaseName, mmdbBytes); From 356f4bc7d13ff72cfc05e74c7b69550ff47a7fcd Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 25 Oct 2024 14:52:09 -0400 Subject: [PATCH 3/4] Move the assertBusy to the whole stanza --- .../ingest/geoip/DatabaseNodeServiceIT.java | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java index a093e7dcd9d2a..7331afdbf585a 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/DatabaseNodeServiceIT.java @@ -50,16 +50,17 @@ public void testNonGzippedDatabase() throws Exception { String databaseName = randomAlphaOfLength(20) + "-" + databaseFileName; byte[] mmdbBytes = getBytesForFile(databaseFileName); final DatabaseNodeService databaseNodeService = internalCluster().getInstance(DatabaseNodeService.class); + assertNull(databaseNodeService.getDatabase(databaseName)); + int numChunks = indexData(databaseName, mmdbBytes); /* * If DatabaseNodeService::checkDatabases runs it will sometimes (rarely) remove the database we are using in this test while we - * are trying to assert things about it. So we disable checkDatabase. + * are trying to assert things about it. So if it does then we 'just' try again. */ - databaseNodeService.doNotCheckDatabases = true; - assertNull(databaseNodeService.getDatabase(databaseName)); - int numChunks = indexData(databaseName, mmdbBytes); - retrieveDatabase(databaseNodeService, databaseName, mmdbBytes, numChunks); - assertBusy(() -> assertNotNull(databaseNodeService.getDatabase(databaseName))); - assertValidDatabase(databaseNodeService, databaseName, databaseType); + assertBusy(() -> { + retrieveDatabase(databaseNodeService, databaseName, mmdbBytes, numChunks); + assertNotNull(databaseNodeService.getDatabase(databaseName)); + assertValidDatabase(databaseNodeService, databaseName, databaseType); + }); } /* @@ -76,9 +77,15 @@ public void testGzippedDatabase() throws Exception { final DatabaseNodeService databaseNodeService = internalCluster().getInstance(DatabaseNodeService.class); assertNull(databaseNodeService.getDatabase(databaseName)); int numChunks = indexData(databaseName, gzipBytes); - retrieveDatabase(databaseNodeService, databaseName, gzipBytes, numChunks); - assertBusy(() -> assertNotNull(databaseNodeService.getDatabase(databaseName))); - assertValidDatabase(databaseNodeService, databaseName, databaseType); + /* + * If DatabaseNodeService::checkDatabases runs it will sometimes (rarely) remove the database we are using in this test while we + * are trying to assert things about it. So if it does then we 'just' try again. + */ + assertBusy(() -> { + retrieveDatabase(databaseNodeService, databaseName, gzipBytes, numChunks); + assertNotNull(databaseNodeService.getDatabase(databaseName)); + assertValidDatabase(databaseNodeService, databaseName, databaseType); + }); } /* From 187ebe25ec20ba4b31b5700e73a5c2b5167b708d Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Fri, 25 Oct 2024 14:55:11 -0400 Subject: [PATCH 4/4] There's no need to modify the src anymore --- .../elasticsearch/ingest/geoip/DatabaseNodeService.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java index 530dcf54f35da..940231b12c894 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseNodeService.java @@ -105,11 +105,6 @@ public final class DatabaseNodeService implements IpDatabaseProvider { private IngestService ingestService; private final ConcurrentMap databases = new ConcurrentHashMap<>(); - /* - * This is meant to be used by tests to disable the checkDatabases() method. That method can run at unexpected times, and removes - * any databases it considers stale (which might break tests). - */ - boolean doNotCheckDatabases = false; DatabaseNodeService( Environment environment, @@ -263,9 +258,6 @@ public void close() throws IOException { } void checkDatabases(ClusterState state) { - if (doNotCheckDatabases) { - return; - } if (state.blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK)) { return; }