From 5638444e3db3e884336beac916aed65fc7b929a2 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 5 Dec 2022 16:03:33 -0500 Subject: [PATCH 1/3] Check reserved state in Metadata.isGlobalStateEquals This fixes a bug where we didn't properly check for the reserved state in global state changes. The bug manifests itself as transient reserved state between node restarts. --- .../cluster/metadata/Metadata.java | 3 + .../cluster/metadata/MetadataTests.java | 56 +++++++++++++++++++ 2 files changed, 59 insertions(+) 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 21dd59e791993..aabee85ab13b5 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java @@ -1297,6 +1297,9 @@ public static boolean isGlobalStateEquals(Metadata metadata1, Metadata metadata2 if (customCount1 != customCount2) { return false; } + if (Objects.equals(metadata1.reservedStateMetadata, metadata2.reservedStateMetadata) == false) { + return false; + } return true; } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java index 92b228b17a8f7..6502b088fc212 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.SuppressForbidden; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.IndexSettings; @@ -42,6 +43,7 @@ import org.elasticsearch.xcontent.json.JsonXContent; import java.io.IOException; +import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; @@ -2269,6 +2271,60 @@ public void testEmptyDiffReturnsSameInstance() throws IOException { assertSame(instance, deserializedDiff.apply(instance)); } + /** + * With this test we ensure that we consider whether a new field added to Metadata should be checked + * in Metadata.isGlobalStateEquals. We force the instance fields to be either in the checked list + * or in the excluded list. + *

+ * This prevents from accidentally forgetting that a new field should be checked in isGlobalStateEquals. + */ + @SuppressForbidden(reason = "need access to all fields, they are mostly private") + public void testEnsureMetadataFieldCheckedForGlobalStateChanges() { + Set checkedForGlobalStateChanges = Set.of( + "coordinationMetadata", + "persistentSettings", + "hashesOfConsistentSettings", + "templates", + "clusterUUID", + "clusterUUIDCommitted", + "customs", + "reservedStateMetadata" + ); + Set excludedFromGlobalStateCheck = Set.of( + "version", + "transientSettings", + "settings", + "indices", + "aliasedIndices", + "totalNumberOfShards", + "totalOpenIndexShards", + "allIndices", + "visibleIndices", + "allOpenIndices", + "visibleOpenIndices", + "allClosedIndices", + "visibleClosedIndices", + "indicesLookup", + "mappingsByHash", + "oldestIndexVersion" + ); + + var diff = new HashSet<>(checkedForGlobalStateChanges); + diff.removeAll(excludedFromGlobalStateCheck); + + // sanity check that the two field sets are mutually exclusive + assertEquals(checkedForGlobalStateChanges, diff); + + // any declared non-static field in metadata must be either in the list of fields + // we check for global state changes, or in the fields excluded from the global state check. + var unclassifiedFields = Arrays.stream(Metadata.class.getDeclaredFields()) + .filter(f -> Modifier.isStatic(f.getModifiers()) == false) + .map(f -> f.getName()) + .filter(n -> (checkedForGlobalStateChanges.contains(n) || excludedFromGlobalStateCheck.contains(n)) == false) + .collect(Collectors.toSet()); + assertThat(unclassifiedFields, empty()); + } + public static Metadata randomMetadata() { return randomMetadata(1); } From 4fb8e3404a4543c561acebcb8c50ca2bac8aba5d Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Mon, 5 Dec 2022 16:07:14 -0500 Subject: [PATCH 2/3] Update docs/changelog/92124.yaml --- docs/changelog/92124.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/92124.yaml diff --git a/docs/changelog/92124.yaml b/docs/changelog/92124.yaml new file mode 100644 index 0000000000000..ea4e7088ee1ae --- /dev/null +++ b/docs/changelog/92124.yaml @@ -0,0 +1,5 @@ +pr: 92124 +summary: Check reserved state in Metadata.isGlobalStateEquals +area: Infra/Core +type: bug +issues: [] From c7976997135dee87d5103aeb54566d587e2c3406 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 5 Dec 2022 16:29:25 -0500 Subject: [PATCH 3/3] Add integration test --- .../service/FileSettingsServiceIT.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java index cfde48d088db9..60c04da7ad070 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java @@ -186,6 +186,38 @@ public void testSettingsAppliedOnStart() throws Exception { assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2()); } + public void testReservedStatePersistsOnRestart() throws Exception { + internalCluster().setBootstrapMasterNodeIndex(0); + logger.info("--> start master node"); + final String masterNode = internalCluster().startMasterOnlyNode(); + assertMasterNode(internalCluster().masterClient(), masterNode); + var savedClusterState = setupClusterStateListener(masterNode); + + FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode); + + assertTrue(masterFileSettingsService.watching()); + + logger.info("--> write some settings"); + writeJSONFile(masterNode, testJSON); + assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2()); + + logger.info("--> restart master"); + internalCluster().restartNode(masterNode); + + final ClusterStateResponse clusterStateResponse = client().admin().cluster().state(new ClusterStateRequest()).actionGet(); + assertEquals( + 1, + clusterStateResponse.getState() + .metadata() + .reservedStateMetadata() + .get(FileSettingsService.NAMESPACE) + .handlers() + .get(ReservedClusterSettingsAction.NAME) + .keys() + .size() + ); + } + private Tuple setupClusterStateListenerForError(String node) { ClusterService clusterService = internalCluster().clusterService(node); CountDownLatch savedClusterState = new CountDownLatch(1);