From 34e9c9a7562817fb7b4ddbe181133aac0638c3e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Mon, 31 May 2021 13:10:44 +0200 Subject: [PATCH 1/4] Skip check of initial indices if they have been rolled-over already We are getting list of initial indices without write alias to operate on later (i.e. we create write alias for them). There can be, however, a valid case when initial index had the write alias but after the rollover a new index has been created and the write alias was pointed at this new index. The situation looks like this: app-foo-000001 app-foo-000002 <- app-foo-write In this case we need to skip the initial index without write alias. It is valid case and we shall not try to create write alias for it. This was causing unnecessary exception being thrown out. --- .../openshift/OpenshiftIndicesUtil.java | 8 ++-- .../openshift/OpenshiftIngestPlugin.java | 38 ++++++++++++------- .../openshift/OpenshiftIndicesUtilTests.java | 4 ++ .../test/ingest/20_define_pipeline.yml | 4 +- .../rest-api-spec/test/ingest/30_rollover.yml | 4 +- 5 files changed, 39 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java index d5785a9..0ce42f6 100644 --- a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java +++ b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java @@ -28,21 +28,21 @@ public abstract class OpenshiftIndicesUtil { /** - * Replaces ending "-write" with "-00001". + * Does trim() and replaces "-write" suffix with "-00001". * @param aliasName assume write-alias * @return initial index name */ public static String generateInitialIndexName(final String aliasName) { - return aliasName.replaceAll("-write$", "-000001"); + return aliasName.trim().replaceAll("-write$", "-000001"); } /** - * Replaces ending "-00001" with "-write". + * Does trim() and replaces "-00001" suffix with "-write". * @param index assume initialIndex * @return write-alias */ public static String generateWriteAliasName(final String index) { - return index.replaceAll("-000001$", "-write"); + return index.trim().replaceAll("-000001$", "-write"); } public static boolean isInitialIndex(final String index) { diff --git a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java index ac01670..c12f765 100644 --- a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java +++ b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java @@ -24,6 +24,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest; import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest.AliasActions; import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequestBuilder; import org.elasticsearch.action.support.master.AcknowledgedResponse; @@ -102,6 +103,7 @@ public void clusterChanged(ClusterChangedEvent event) { synchronized (this) { if (eventState.version() > clusterStateVersion) { latestAliasAndIndicesLookup = eventState.metaData().getAliasAndIndexLookup(); + logger.trace("New version of metadata: {} > {}", eventState.version(), clusterStateVersion); clusterStateVersion = eventState.version(); } } @@ -116,6 +118,7 @@ Any modification of index metadata (including adding index aliases) is done on t */ if (event.localNodeMaster()) { + logger.trace("Master node is handling new metadata"); List indices = getInitialIndicesWithoutWriteAlias(latestAliasAndIndicesLookup); if (!indices.isEmpty()) { IndicesAliasesRequestBuilder iarb = client.admin().indices().prepareAliases(); @@ -123,26 +126,35 @@ Any modification of index metadata (including adding index aliases) is done on t for (String index: indices) { String writeAlias = generateWriteAliasName(index); - if (!writeAlias.trim().isEmpty()) { + // Initial indices that were already rolled-over will not have write alias. We need to skip them. + // In other words the writeAlias already exists (perhaps pointed to "-000002" index or older). + if (!writeAlias.isEmpty() && !latestAliasAndIndicesLookup.containsKey(writeAlias)) { iarb.addAliasAction(AliasActions.add() .index(index) .alias(writeAlias) .writeIndex(true)); + logger.trace("Prepare new index alias {} request for index {}", writeAlias, index); } } - client.admin().indices().aliases(iarb.request(), new ActionListener() { - @Override - public void onResponse(AcknowledgedResponse acknowledgedResponse) { - logger.debug("Write aliases added for the following indices: {}", indices); - } - - @Override - public void onFailure(Exception e) { - // TODO[lvlcek]: If there is an exception like "Alias already exists" then we need to ignore it - logger.info("Error occurred when adding write aliases for the following indices: {}. {}", indices, e); - } - }); + IndicesAliasesRequest iar = iarb.request(); + if (!iar.getAliasActions().isEmpty()) { + client.admin().indices().aliases(iar, new ActionListener() { + @Override + public void onResponse(AcknowledgedResponse acknowledgedResponse) { + logger.debug("Write aliases added for the following indices: {}", indices); + } + + @Override + public void onFailure(Exception e) { + // TODO[lvlcek]: If there is an exception like "Alias already exists" then we can ignore it. + // However, if the index create request fails because the client does not have appropriate credentials + // or any other serious reason then we need to escalate it. + logger.warn("Error occurred when adding write aliases for the following indices: {}. {}", indices, e); + } + }); + logger.trace("Request to create new index aliases created"); + } } } } diff --git a/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java b/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java index c83e6c0..9b14fa8 100644 --- a/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java +++ b/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java @@ -39,12 +39,16 @@ public void testGenerateInitialIndexName() { assertEquals("app-000001", generateInitialIndexName("app-write")); assertEquals("alias-write-000001", generateInitialIndexName("alias-write-write")); assertEquals("-000001", generateInitialIndexName("-write")); + + assertEquals("app-000001", generateInitialIndexName(" app-write ")); } public void testGenerateWriteAliasName() { assertEquals("app-write", generateWriteAliasName("app-000001")); assertEquals("foo-write-write", generateWriteAliasName("foo-write-000001")); assertEquals("-write", generateWriteAliasName("-000001")); + + assertEquals("app-write", generateWriteAliasName(" app-000001 ")); } public void testDataModelPrefix() { diff --git a/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml b/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml index 97c4ccc..582a1e2 100644 --- a/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml +++ b/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml @@ -6,7 +6,9 @@ cluster.put_settings: body: transient: - logger.org.elasticsearch.ingest.openshift: "TRACE" + logger: + org.elasticsearch.ingest.openshift: "TRACE" + org.elasticsearch.action: "DEBUG" flat_settings: true - match: { acknowledged: true } diff --git a/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml b/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml index c719593..b935b53 100644 --- a/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml +++ b/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml @@ -6,7 +6,9 @@ cluster.put_settings: body: transient: - logger.org.elasticsearch.ingest.openshift: "TRACE" + logger: + org.elasticsearch.ingest.openshift: "TRACE" + org.elasticsearch.action: "DEBUG" flat_settings: true - match: { acknowledged: true } From 6dd5b26ecf6a6c51c5d61527574f52aa66e8fd84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Mon, 7 Jun 2021 11:43:54 +0200 Subject: [PATCH 2/4] Add action.auto_create_index setting Intorduce production action.auto_create_index settings into our tests to prove this is not blocking creating index aliases. --- .../resources/rest-api-spec/test/ingest/20_define_pipeline.yml | 1 + src/test/resources/rest-api-spec/test/ingest/30_rollover.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml b/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml index 582a1e2..def9601 100644 --- a/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml +++ b/src/test/resources/rest-api-spec/test/ingest/20_define_pipeline.yml @@ -6,6 +6,7 @@ cluster.put_settings: body: transient: + action.auto_create_index: "-*-write,+*" logger: org.elasticsearch.ingest.openshift: "TRACE" org.elasticsearch.action: "DEBUG" diff --git a/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml b/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml index b935b53..d6bd823 100644 --- a/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml +++ b/src/test/resources/rest-api-spec/test/ingest/30_rollover.yml @@ -6,6 +6,7 @@ cluster.put_settings: body: transient: + action.auto_create_index: "-*-write,+*" logger: org.elasticsearch.ingest.openshift: "TRACE" org.elasticsearch.action: "DEBUG" From 4d6ea495a70ce91fb608c247c4917a69694390e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Wed, 9 Jun 2021 15:27:39 +0200 Subject: [PATCH 3/4] Fix NPE on AliasMetaData check Extended AliasMetaData (such as writeAlias) need to be checked for null value before it is accessed. --- .../openshift/OpenshiftIndicesUtil.java | 4 +++- .../openshift/OpenshiftIndicesUtilTests.java | 22 +++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java index 0ce42f6..c3dd1fd 100644 --- a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java +++ b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtil.java @@ -74,7 +74,9 @@ public static List getInitialIndicesWithoutWriteAlias(final Map md: imd.getAliases().values()) { - if (md.value.writeIndex()) { + // Alias metadata can be undefined because the Builder does not require all MetaData to + // be specified. See . + if (md.value.writeIndex() != null && md.value.writeIndex()) { return false; } } diff --git a/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java b/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java index 9b14fa8..9a27aea 100644 --- a/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java +++ b/src/test/java/org/elasticsearch/ingest/openshift/OpenshiftIndicesUtilTests.java @@ -94,10 +94,17 @@ public void testIndicesWithoutWriteIndex() { im.put("app-index3-000003", new AliasOrIndex.Index(createIndexMetaData("app-index3-000003", new AliasInfo("app-alias1", true)))); + // We also test index with alias that has no metadata, just the name. + // Aliases without any additional metadata can lead to NPE when client is iterating over them and does + // not check nullity of metadata when accessing it. + im.put("app-index4-000001", new AliasOrIndex.Index(createIndexMetaData("app-index4-000001", + new AliasInfo("app-alias1")))); + indices = OpenshiftIndicesUtil.getInitialIndicesWithoutWriteAlias(im); - assertEquals(indices.size(), 1); + assertEquals(indices.size(), 2); assertTrue(indices.contains("app-index1-000001")); + assertTrue(indices.contains("app-index4-000001")); } private AliasMetaData createAliasMetaData(String alias) { @@ -114,15 +121,22 @@ private IndexMetaData createIndexMetaData(String index, AliasInfo ... alias) { // fails: Throwable #1: java.lang.IllegalArgumentException: must specify numberOfShards for index [___] for (AliasInfo ai : alias) { - imBuilder.putAlias(AliasMetaData.builder(ai.alias).writeIndex(ai.writeAlias).build()); + AliasMetaData.Builder amdt = AliasMetaData.builder(ai.alias); + if (ai.writeAlias != null) { + amdt.writeIndex(ai.writeAlias); + } + imBuilder.putAlias(amdt.build()); } return imBuilder.build(); } private class AliasInfo { - public final String alias; - public final Boolean writeAlias; + private String alias; + private Boolean writeAlias; + AliasInfo(String alias) { + this.alias = alias; + } AliasInfo(String alias, Boolean writeAlias) { this.alias = alias; this.writeAlias = writeAlias; From b7f18c314d2e38a00df1bfbe19c608e24678b044 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Wed, 9 Jun 2021 20:21:16 +0200 Subject: [PATCH 4/4] Clean-up the logging Removing some of the logs that we used to troubleshoot issues. --- gradle.properties | 2 +- .../ingest/openshift/OpenshiftIndexProcessor.java | 3 --- .../ingest/openshift/OpenshiftIngestPlugin.java | 5 +---- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/gradle.properties b/gradle.properties index 0d30e09..f67684a 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,2 +1,2 @@ group = org.elasticsearch.plugin.ingest -version = 6.8.1.0-SNAPSHOT \ No newline at end of file +version = 6.8.1.0 \ No newline at end of file diff --git a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndexProcessor.java b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndexProcessor.java index 596cc1c..538aba2 100644 --- a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndexProcessor.java +++ b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIndexProcessor.java @@ -16,8 +16,6 @@ package org.elasticsearch.ingest.openshift; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateListener; import org.elasticsearch.cluster.metadata.AliasOrIndex; @@ -42,7 +40,6 @@ * Every instance of this plugin keeps its own lookup table of latest indices/aliases. */ public final class OpenshiftIndexProcessor extends AbstractProcessor { - private static final Logger logger = LogManager.getLogger(OpenshiftIndexProcessor.class); public static final String TYPE = "openshift-ingestion-processor"; diff --git a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java index c12f765..4b99a87 100644 --- a/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java +++ b/src/main/java/org/elasticsearch/ingest/openshift/OpenshiftIngestPlugin.java @@ -103,7 +103,6 @@ public void clusterChanged(ClusterChangedEvent event) { synchronized (this) { if (eventState.version() > clusterStateVersion) { latestAliasAndIndicesLookup = eventState.metaData().getAliasAndIndexLookup(); - logger.trace("New version of metadata: {} > {}", eventState.version(), clusterStateVersion); clusterStateVersion = eventState.version(); } } @@ -118,7 +117,6 @@ Any modification of index metadata (including adding index aliases) is done on t */ if (event.localNodeMaster()) { - logger.trace("Master node is handling new metadata"); List indices = getInitialIndicesWithoutWriteAlias(latestAliasAndIndicesLookup); if (!indices.isEmpty()) { IndicesAliasesRequestBuilder iarb = client.admin().indices().prepareAliases(); @@ -133,7 +131,7 @@ Any modification of index metadata (including adding index aliases) is done on t .index(index) .alias(writeAlias) .writeIndex(true)); - logger.trace("Prepare new index alias {} request for index {}", writeAlias, index); + logger.trace("Prepared write index alias {} request for index {}", writeAlias, index); } } @@ -153,7 +151,6 @@ public void onFailure(Exception e) { logger.warn("Error occurred when adding write aliases for the following indices: {}. {}", indices, e); } }); - logger.trace("Request to create new index aliases created"); } } }