From df618480de2e9d01cc4a15ddf5e9a557b2309107 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 16:39:27 -0400 Subject: [PATCH 01/27] Implement few operator handlers Implement operator handlers for cluster state and ILM --- server/src/main/java/module-info.java | 2 + .../TransportClusterUpdateSettingsAction.java | 18 +++ .../master/TransportMasterNodeAction.java | 30 +++++ .../OperatorClusterUpdateSettingsAction.java | 90 ++++++++++++++ ...ratorClusterUpdateSettingsActionTests.java | 100 ++++++++++++++++ .../ilm/action/DeleteLifecycleAction.java | 7 ++ .../plugin/ilm/src/main/java/module-info.java | 2 + .../TransportDeleteLifecycleAction.java | 18 +++ .../action/TransportPutLifecycleAction.java | 48 +++++++- .../operator/ILMOperatorHandlerProvider.java | 31 +++++ .../action/OperatorLifecycleAction.java | 113 ++++++++++++++++++ .../TransportDeleteLifecycleActionTests.java | 36 ++++++ .../TransportPutLifecycleActionTests.java | 49 ++++++++ 13 files changed, 538 insertions(+), 6 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsAction.java create mode 100644 server/src/test/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsActionTests.java create mode 100644 x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java create mode 100644 x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java create mode 100644 x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java diff --git a/server/src/main/java/module-info.java b/server/src/main/java/module-info.java index a7fb33167dc23..19e1cd3780ddb 100644 --- a/server/src/main/java/module-info.java +++ b/server/src/main/java/module-info.java @@ -359,4 +359,6 @@ org.elasticsearch.cluster.coordination.NodeToolCliProvider, org.elasticsearch.index.shard.ShardToolCliProvider; provides org.apache.logging.log4j.util.PropertySource with org.elasticsearch.common.logging.ESSystemPropertiesPropertySource; + + uses org.elasticsearch.operator.OperatorHandlerProvider; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java index 4f19aad41bc5e..c182862edfd3c 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java @@ -29,11 +29,13 @@ import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.SuppressForbidden; +import org.elasticsearch.operator.action.OperatorClusterUpdateSettingsAction; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import java.util.HashSet; +import java.util.Optional; import java.util.Set; import static org.elasticsearch.common.settings.AbstractScopedSettings.ARCHIVED_SETTINGS_PREFIX; @@ -128,6 +130,17 @@ private static boolean checkClearedBlockAndArchivedSettings( return true; } + @Override + protected Optional operatorHandlerName() { + return Optional.of(OperatorClusterUpdateSettingsAction.NAME); + } + + @Override + protected Set modifiedKeys(ClusterUpdateSettingsRequest request) { + Settings allSettings = Settings.builder().put(request.persistentSettings()).put(request.transientSettings()).build(); + return allSettings.keySet(); + } + private static final String UPDATE_TASK_SOURCE = "cluster_update_settings"; private static final String REROUTE_TASK_SOURCE = "reroute_after_cluster_update_settings"; @@ -243,6 +256,11 @@ public ClusterUpdateSettingsTask( this.request = request; } + // Used by the operator handler + public ClusterUpdateSettingsTask(final ClusterSettings clusterSettings, ClusterUpdateSettingsRequest request) { + this(clusterSettings, Priority.IMMEDIATE, request, null); + } + @Override public ClusterState execute(final ClusterState currentState) { final ClusterState clusterState = updater.updateSettings( diff --git a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index d92fb8f2956f9..a13a8d19601dc 100644 --- a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -42,6 +42,9 @@ import org.elasticsearch.transport.TransportException; import org.elasticsearch.transport.TransportService; +import java.util.Collections; +import java.util.Optional; +import java.util.Set; import java.util.function.Predicate; import static org.elasticsearch.core.Strings.format; @@ -142,6 +145,33 @@ private ClusterBlockException checkBlockIfStateRecovered(Request request, Cluste } } + /** + * Override this method if the master node action also has an {@link org.elasticsearch.operator.OperatorHandler} + * interaction. + *

+ * We need to check if certain settings or entities are allowed to be modified by the master node + * action, depending on if they are set already in operator mode. + * + * @return an Optional of the operator handler name + */ + protected Optional operatorHandlerName() { + return Optional.empty(); + } + + /** + * Override this method to return the keys of the cluster state or cluster entities that are modified by + * the Request object. + *

+ * This method is used by the operator handler logic (see {@link org.elasticsearch.operator.OperatorHandler}) + * to verify if the keys don't conflict with an existing key set in operator mode. + * + * @param request the TransportMasterNode request + * @return set of String keys intended to be modified/set/deleted by this request + */ + protected Set modifiedKeys(Request request) { + return Collections.emptySet(); + } + @Override protected void doExecute(Task task, final Request request, ActionListener listener) { ClusterState state = clusterService.state(); diff --git a/server/src/main/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsAction.java b/server/src/main/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsAction.java new file mode 100644 index 0000000000000..81e210081b751 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsAction.java @@ -0,0 +1,90 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator.action; + +import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; +import org.elasticsearch.action.admin.cluster.settings.TransportClusterUpdateSettingsAction; +import org.elasticsearch.client.internal.Requests; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.operator.OperatorHandler; +import org.elasticsearch.operator.TransformState; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.elasticsearch.common.util.Maps.asMap; + +/** + * This Action is the Operator version of RestClusterUpdateSettingsAction + *

+ * It is used by the OperatorClusterStateController to update the persistent cluster settings. + * Since transient cluster settings are deprecated, this action doesn't support updating cluster settings. + */ +public class OperatorClusterUpdateSettingsAction implements OperatorHandler { + + public static final String NAME = "cluster_settings"; + + private final ClusterSettings clusterSettings; + + public OperatorClusterUpdateSettingsAction(ClusterSettings clusterSettings) { + this.clusterSettings = clusterSettings; + } + + @Override + public String name() { + return NAME; + } + + @SuppressWarnings("unchecked") + private ClusterUpdateSettingsRequest prepare(Object input, Set previouslySet) { + final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = Requests.clusterUpdateSettingsRequest(); + + Map source = asMap(input); + Map persistentSettings = new HashMap<>(); + Set toDelete = new HashSet<>(previouslySet); + + source.forEach((k, v) -> { + persistentSettings.put(k, v); + toDelete.remove(k); + }); + + toDelete.forEach(k -> persistentSettings.put(k, null)); + + clusterUpdateSettingsRequest.persistentSettings(persistentSettings); + return clusterUpdateSettingsRequest; + } + + @Override + public TransformState transform(Object input, TransformState prevState) { + ClusterUpdateSettingsRequest request = prepare(input, prevState.keys()); + + // allow empty requests, this is how we clean up settings + if (request.persistentSettings().isEmpty() == false) { + validate(request); + } + + ClusterState state = prevState.state(); + + TransportClusterUpdateSettingsAction.ClusterUpdateSettingsTask updateSettingsTask = + new TransportClusterUpdateSettingsAction.ClusterUpdateSettingsTask(clusterSettings, request); + + state = updateSettingsTask.execute(state); + Set currentKeys = request.persistentSettings() + .keySet() + .stream() + .filter(k -> request.persistentSettings().hasValue(k)) + .collect(Collectors.toSet()); + + return new TransformState(state, currentKeys); + } +} diff --git a/server/src/test/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsActionTests.java b/server/src/test/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsActionTests.java new file mode 100644 index 0000000000000..c2e29a10117fa --- /dev/null +++ b/server/src/test/java/org/elasticsearch/operator/action/OperatorClusterUpdateSettingsActionTests.java @@ -0,0 +1,100 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator.action; + +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.operator.TransformState; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; + +import java.util.Collections; + +import static org.hamcrest.Matchers.containsInAnyOrder; + +public class OperatorClusterUpdateSettingsActionTests extends ESTestCase { + + private TransformState processJSON(OperatorClusterUpdateSettingsAction action, TransformState prevState, String json) throws Exception { + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { + return action.transform(parser.map(), prevState); + } + } + + public void testValidation() throws Exception { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + + ClusterState state = ClusterState.builder(new ClusterName("elasticsearch")).build(); + TransformState prevState = new TransformState(state, Collections.emptySet()); + OperatorClusterUpdateSettingsAction action = new OperatorClusterUpdateSettingsAction(clusterSettings); + + String badPolicyJSON = """ + { + "indices.recovery.min_bytes_per_sec": "50mb" + }"""; + + assertEquals( + "persistent setting [indices.recovery.min_bytes_per_sec], not recognized", + expectThrows(IllegalArgumentException.class, () -> processJSON(action, prevState, badPolicyJSON)).getMessage() + ); + } + + public void testSetUnsetSettings() throws Exception { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + + ClusterState state = ClusterState.builder(new ClusterName("elasticsearch")).build(); + TransformState prevState = new TransformState(state, Collections.emptySet()); + OperatorClusterUpdateSettingsAction action = new OperatorClusterUpdateSettingsAction(clusterSettings); + + String emptyJSON = ""; + + TransformState updatedState = processJSON(action, prevState, emptyJSON); + assertEquals(0, updatedState.keys().size()); + assertEquals(prevState.state(), updatedState.state()); + + String settingsJSON = """ + { + "indices.recovery.max_bytes_per_sec": "50mb", + "cluster": { + "remote": { + "cluster_one": { + "seeds": [ + "127.0.0.1:9300" + ] + } + } + } + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, settingsJSON); + assertThat(updatedState.keys(), containsInAnyOrder("indices.recovery.max_bytes_per_sec", "cluster.remote.cluster_one.seeds")); + assertEquals("50mb", updatedState.state().metadata().persistentSettings().get("indices.recovery.max_bytes_per_sec")); + assertEquals("[127.0.0.1:9300]", updatedState.state().metadata().persistentSettings().get("cluster.remote.cluster_one.seeds")); + + String oneSettingJSON = """ + { + "indices.recovery.max_bytes_per_sec": "25mb" + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, oneSettingJSON); + assertThat(updatedState.keys(), containsInAnyOrder("indices.recovery.max_bytes_per_sec")); + assertEquals("25mb", updatedState.state().metadata().persistentSettings().get("indices.recovery.max_bytes_per_sec")); + assertNull(updatedState.state().metadata().persistentSettings().get("cluster.remote.cluster_one.seeds")); + + prevState = updatedState; + updatedState = processJSON(action, prevState, emptyJSON); + assertEquals(0, updatedState.keys().size()); + assertNull(updatedState.state().metadata().persistentSettings().get("indices.recovery.max_bytes_per_sec")); + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/DeleteLifecycleAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/DeleteLifecycleAction.java index 623c9797ffde1..2d59d07a55d0d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/DeleteLifecycleAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/action/DeleteLifecycleAction.java @@ -18,6 +18,8 @@ import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.core.Strings.format; + public class DeleteLifecycleAction extends ActionType { public static final DeleteLifecycleAction INSTANCE = new DeleteLifecycleAction(); public static final String NAME = "cluster:admin/ilm/delete"; @@ -75,6 +77,11 @@ public boolean equals(Object obj) { return Objects.equals(policyName, other.policyName); } + @Override + public String toString() { + return format("delete lifecycle policy [%s]", policyName); + } + } } diff --git a/x-pack/plugin/ilm/src/main/java/module-info.java b/x-pack/plugin/ilm/src/main/java/module-info.java index dfcb404fb4e56..69348eedcfc60 100644 --- a/x-pack/plugin/ilm/src/main/java/module-info.java +++ b/x-pack/plugin/ilm/src/main/java/module-info.java @@ -16,4 +16,6 @@ exports org.elasticsearch.xpack.ilm to org.elasticsearch.server; exports org.elasticsearch.xpack.slm.action to org.elasticsearch.server; exports org.elasticsearch.xpack.slm to org.elasticsearch.server; + + provides org.elasticsearch.operator.OperatorHandlerProvider with org.elasticsearch.xpack.ilm.operator.ILMOperatorHandlerProvider; } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java index 1387093782e48..5fc29ed66f5fc 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java @@ -29,8 +29,11 @@ import org.elasticsearch.xpack.core.ilm.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction.Request; +import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; import java.util.List; +import java.util.Optional; +import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; @@ -70,6 +73,11 @@ public DeleteLifecyclePolicyTask(Request request, ActionListener operatorHandlerName() { + return Optional.of(OperatorLifecycleAction.NAME); + } + + @Override + protected Set modifiedKeys(Request request) { + return Set.of(request.getPolicyName()); + } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java index 7baecc4754672..d1d75d7333d85 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java @@ -41,10 +41,14 @@ import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction.Request; import org.elasticsearch.xpack.core.slm.SnapshotLifecycleMetadata; +import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; import java.time.Instant; +import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; @@ -111,7 +115,7 @@ protected void masterOperation(Task task, Request request, ClusterState state, A submitUnbatchedTask( "put-lifecycle-" + request.getPolicy().getName(), - new UpdateLifecyclePolicyTask(request, listener, licenseState, filteredHeaders, xContentRegistry, client) + new UpdateLifecyclePolicyTask(request, listener, licenseState, filteredHeaders, xContentRegistry, client, true) ); } @@ -121,6 +125,7 @@ public static class UpdateLifecyclePolicyTask extends AckedClusterStateUpdateTas private final Map filteredHeaders; private final NamedXContentRegistry xContentRegistry; private final Client client; + private final boolean verboseLogging; public UpdateLifecyclePolicyTask( Request request, @@ -128,7 +133,8 @@ public UpdateLifecyclePolicyTask( XPackLicenseState licenseState, Map filteredHeaders, NamedXContentRegistry xContentRegistry, - Client client + Client client, + boolean verboseLogging ) { super(request, listener); this.request = request; @@ -136,6 +142,24 @@ public UpdateLifecyclePolicyTask( this.filteredHeaders = filteredHeaders; this.xContentRegistry = xContentRegistry; this.client = client; + this.verboseLogging = verboseLogging; + } + + /** + * Constructor used in operator mode. It disables verbose logging and has no filtered headers. + * + * @param request + * @param licenseState + * @param xContentRegistry + * @param client + */ + public UpdateLifecyclePolicyTask( + Request request, + XPackLicenseState licenseState, + NamedXContentRegistry xContentRegistry, + Client client + ) { + this(request, null, licenseState, new HashMap<>(), xContentRegistry, client, false); } @Override @@ -161,10 +185,12 @@ public ClusterState execute(ClusterState currentState) throws Exception { Instant.now().toEpochMilli() ); LifecyclePolicyMetadata oldPolicy = newPolicies.put(lifecyclePolicyMetadata.getName(), lifecyclePolicyMetadata); - if (oldPolicy == null) { - logger.info("adding index lifecycle policy [{}]", request.getPolicy().getName()); - } else { - logger.info("updating index lifecycle policy [{}]", request.getPolicy().getName()); + if (verboseLogging) { + if (oldPolicy == null) { + logger.info("adding index lifecycle policy [{}]", request.getPolicy().getName()); + } else { + logger.info("updating index lifecycle policy [{}]", request.getPolicy().getName()); + } } IndexLifecycleMetadata newMetadata = new IndexLifecycleMetadata(newPolicies, currentMetadata.getOperationMode()); stateBuilder.metadata(Metadata.builder(currentState.getMetadata()).putCustom(IndexLifecycleMetadata.TYPE, newMetadata).build()); @@ -285,4 +311,14 @@ private static void validatePrerequisites(LifecyclePolicy policy, ClusterState s protected ClusterBlockException checkBlock(Request request, ClusterState state) { return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE); } + + @Override + protected Optional operatorHandlerName() { + return Optional.of(OperatorLifecycleAction.NAME); + } + + @Override + protected Set modifiedKeys(Request request) { + return Set.of(request.getPolicy().getName()); + } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java new file mode 100644 index 0000000000000..5577401719cfd --- /dev/null +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java @@ -0,0 +1,31 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.ilm.operator; + +import org.elasticsearch.operator.OperatorHandler; +import org.elasticsearch.operator.OperatorHandlerProvider; + +import java.util.Collection; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + +/** + * ILM Provider implementation for the OperatorHandlerProvider service interface + */ +public class ILMOperatorHandlerProvider implements OperatorHandlerProvider { + private static final Set> handlers = ConcurrentHashMap.newKeySet(); + + @Override + public Collection> handlers() { + return handlers; + } + + public static void handler(OperatorHandler handler) { + handlers.add(handler); + } +} diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java new file mode 100644 index 0000000000000..0092db643568c --- /dev/null +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java @@ -0,0 +1,113 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.ilm.operator.action; + +import org.elasticsearch.client.internal.Client; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.license.XPackLicenseState; +import org.elasticsearch.operator.OperatorHandler; +import org.elasticsearch.operator.TransformState; +import org.elasticsearch.xcontent.NamedXContentRegistry; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xpack.core.ilm.LifecyclePolicy; +import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; +import org.elasticsearch.xpack.core.template.LifecyclePolicyConfig; +import org.elasticsearch.xpack.ilm.action.TransportDeleteLifecycleAction; +import org.elasticsearch.xpack.ilm.action.TransportPutLifecycleAction; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.elasticsearch.common.util.Maps.asMap; +import static org.elasticsearch.common.xcontent.XContentHelper.mapToXContentParser; + +/** + * This {@link OperatorHandler} is responsible for CRUD operations on ILM policies in + * operator mode, e.g. file based settings. + *

+ * Internally it uses {@link TransportPutLifecycleAction} and + * {@link TransportDeleteLifecycleAction} to add, update and delete ILM policies. + */ +public class OperatorLifecycleAction implements OperatorHandler { + + private final NamedXContentRegistry xContentRegistry; + private final Client client; + private final XPackLicenseState licenseState; + + public static final String NAME = "ilm"; + + public OperatorLifecycleAction(NamedXContentRegistry xContentRegistry, Client client, XPackLicenseState licenseState) { + this.xContentRegistry = xContentRegistry; + this.client = client; + this.licenseState = licenseState; + } + + @Override + public String name() { + return NAME; + } + + @SuppressWarnings("unchecked") + public Collection prepare(Object input) throws IOException { + List result = new ArrayList<>(); + + Map source = asMap(input); + + for (String name : source.keySet()) { + Map content = (Map) source.get(name); + var config = XContentParserConfiguration.EMPTY.withRegistry(LifecyclePolicyConfig.DEFAULT_X_CONTENT_REGISTRY); + try (XContentParser parser = mapToXContentParser(config, content)) { + LifecyclePolicy policy = LifecyclePolicy.parse(parser, name); + PutLifecycleAction.Request request = new PutLifecycleAction.Request(policy); + validate(request); + result.add(request); + } + } + + return result; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + var requests = prepare(source); + + ClusterState state = prevState.state(); + + for (var request : requests) { + TransportPutLifecycleAction.UpdateLifecyclePolicyTask task = new TransportPutLifecycleAction.UpdateLifecyclePolicyTask( + request, + licenseState, + xContentRegistry, + client + ); + + state = task.execute(state); + } + + Set entities = requests.stream().map(r -> r.getPolicy().getName()).collect(Collectors.toSet()); + + Set toDelete = new HashSet<>(prevState.keys()); + toDelete.removeAll(entities); + + for (var policyToDelete : toDelete) { + TransportDeleteLifecycleAction.DeleteLifecyclePolicyTask task = new TransportDeleteLifecycleAction.DeleteLifecyclePolicyTask( + policyToDelete + ); + state = task.execute(state); + } + + return new TransformState(state, entities); + } +} diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java new file mode 100644 index 0000000000000..51df10fb7a190 --- /dev/null +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java @@ -0,0 +1,36 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.ilm.action; + +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction; +import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; + +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.mockito.Mockito.mock; + +public class TransportDeleteLifecycleActionTests extends ESTestCase { + public void testOperatorHandler() { + TransportDeleteLifecycleAction putAction = new TransportDeleteLifecycleAction( + mock(TransportService.class), + mock(ClusterService.class), + mock(ThreadPool.class), + mock(ActionFilters.class), + mock(IndexNameExpressionResolver.class) + ); + assertEquals(OperatorLifecycleAction.NAME, putAction.operatorHandlerName().get()); + + DeleteLifecycleAction.Request request = new DeleteLifecycleAction.Request("my_timeseries_lifecycle2"); + assertThat(putAction.modifiedKeys(request), containsInAnyOrder("my_timeseries_lifecycle2")); + } +} diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java index b2202f0e5126f..6731957cb43ab 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java @@ -7,13 +7,29 @@ package org.elasticsearch.xpack.ilm.action; +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.client.internal.Client; +import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xcontent.NamedXContentRegistry; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.core.ilm.LifecyclePolicy; import org.elasticsearch.xpack.core.ilm.LifecyclePolicyMetadata; +import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; import org.elasticsearch.xpack.ilm.LifecyclePolicyTestsUtils; +import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; import java.util.Map; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.mockito.Mockito.mock; + public class TransportPutLifecycleActionTests extends ESTestCase { public void testIsNoop() { LifecyclePolicy policy1 = LifecyclePolicyTestsUtils.randomTimeseriesLifecyclePolicy("policy"); @@ -29,4 +45,37 @@ public void testIsNoop() { assertFalse(TransportPutLifecycleAction.isNoopUpdate(existing, policy1, headers2)); assertFalse(TransportPutLifecycleAction.isNoopUpdate(null, policy1, headers1)); } + + public void testOperatorHandler() throws Exception { + TransportPutLifecycleAction putAction = new TransportPutLifecycleAction( + mock(TransportService.class), + mock(ClusterService.class), + mock(ThreadPool.class), + mock(ActionFilters.class), + mock(IndexNameExpressionResolver.class), + mock(NamedXContentRegistry.class), + mock(XPackLicenseState.class), + mock(Client.class) + ); + assertEquals(OperatorLifecycleAction.NAME, putAction.operatorHandlerName().get()); + + String json = """ + { + "policy": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + } + } + } + } + }"""; + + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { + PutLifecycleAction.Request request = PutLifecycleAction.Request.parseRequest("my_timeseries_lifecycle2", parser); + + assertThat(putAction.modifiedKeys(request), containsInAnyOrder("my_timeseries_lifecycle2")); + } + } } From 407fd72a89137145acc474fbbfd8b4521dc6a9ff Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 27 Jun 2022 16:59:26 -0400 Subject: [PATCH 02/27] Add few ILM operator handler tests --- .../operator/OperatorILMControllerTests.java | 213 ++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java new file mode 100644 index 0000000000000..ed279c213693d --- /dev/null +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java @@ -0,0 +1,213 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.ilm.action.operator; + +import org.elasticsearch.client.internal.Client; +import org.elasticsearch.cluster.ClusterModule; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.license.XPackLicenseState; +import org.elasticsearch.operator.TransformState; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xcontent.NamedXContentRegistry; +import org.elasticsearch.xcontent.ParseField; +import org.elasticsearch.xcontent.XContentParseException; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; +import org.elasticsearch.xpack.core.ilm.AllocateAction; +import org.elasticsearch.xpack.core.ilm.DeleteAction; +import org.elasticsearch.xpack.core.ilm.ForceMergeAction; +import org.elasticsearch.xpack.core.ilm.FreezeAction; +import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata; +import org.elasticsearch.xpack.core.ilm.LifecycleAction; +import org.elasticsearch.xpack.core.ilm.LifecycleType; +import org.elasticsearch.xpack.core.ilm.MigrateAction; +import org.elasticsearch.xpack.core.ilm.ReadOnlyAction; +import org.elasticsearch.xpack.core.ilm.RolloverAction; +import org.elasticsearch.xpack.core.ilm.RollupILMAction; +import org.elasticsearch.xpack.core.ilm.SearchableSnapshotAction; +import org.elasticsearch.xpack.core.ilm.SetPriorityAction; +import org.elasticsearch.xpack.core.ilm.ShrinkAction; +import org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType; +import org.elasticsearch.xpack.core.ilm.UnfollowAction; +import org.elasticsearch.xpack.core.ilm.WaitForSnapshotAction; +import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class OperatorILMControllerTests extends ESTestCase { + + protected NamedXContentRegistry xContentRegistry() { + List entries = new ArrayList<>(ClusterModule.getNamedXWriteables()); + entries.addAll( + Arrays.asList( + new NamedXContentRegistry.Entry( + LifecycleType.class, + new ParseField(TimeseriesLifecycleType.TYPE), + (p) -> TimeseriesLifecycleType.INSTANCE + ), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(AllocateAction.NAME), AllocateAction::parse), + new NamedXContentRegistry.Entry( + LifecycleAction.class, + new ParseField(WaitForSnapshotAction.NAME), + WaitForSnapshotAction::parse + ), + new NamedXContentRegistry.Entry( + LifecycleAction.class, + new ParseField(SearchableSnapshotAction.NAME), + SearchableSnapshotAction::parse + ), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(DeleteAction.NAME), DeleteAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ForceMergeAction.NAME), ForceMergeAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ReadOnlyAction.NAME), ReadOnlyAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(RolloverAction.NAME), RolloverAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(ShrinkAction.NAME), ShrinkAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(FreezeAction.NAME), FreezeAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(SetPriorityAction.NAME), SetPriorityAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(MigrateAction.NAME), MigrateAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(UnfollowAction.NAME), UnfollowAction::parse), + new NamedXContentRegistry.Entry(LifecycleAction.class, new ParseField(RollupILMAction.NAME), RollupILMAction::parse) + ) + ); + return new NamedXContentRegistry(entries); + } + + private TransformState processJSON(OperatorLifecycleAction action, TransformState prevState, String json) throws Exception { + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { + return action.transform(parser.map(), prevState); + } + } + + public void testValidationFails() { + Client client = mock(Client.class); + when(client.settings()).thenReturn(Settings.EMPTY); + final ClusterName clusterName = new ClusterName("elasticsearch"); + + ClusterState state = ClusterState.builder(clusterName).build(); + OperatorLifecycleAction action = new OperatorLifecycleAction(xContentRegistry(), client, mock(XPackLicenseState.class)); + TransformState prevState = new TransformState(state, Collections.emptySet()); + + String badPolicyJSON = """ + { + "my_timeseries_lifecycle": { + "phase": { + "warm": { + "min_age": "10s", + "actions": { + } + } + } + } + }"""; + + assertEquals( + "[1:2] [lifecycle_policy] unknown field [phase] did you mean [phases]?", + expectThrows(XContentParseException.class, () -> processJSON(action, prevState, badPolicyJSON)).getMessage() + ); + } + + public void testActionAddRemove() throws Exception { + Client client = mock(Client.class); + when(client.settings()).thenReturn(Settings.EMPTY); + final ClusterName clusterName = new ClusterName("elasticsearch"); + + ClusterState state = ClusterState.builder(clusterName).build(); + + OperatorLifecycleAction action = new OperatorLifecycleAction(xContentRegistry(), client, mock(XPackLicenseState.class)); + + String emptyJSON = ""; + + TransformState prevState = new TransformState(state, Collections.emptySet()); + + TransformState updatedState = processJSON(action, prevState, emptyJSON); + assertEquals(0, updatedState.keys().size()); + assertEquals(prevState.state(), updatedState.state()); + + String twoPoliciesJSON = """ + { + "my_timeseries_lifecycle": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + } + } + } + }, + "my_timeseries_lifecycle1": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + } + }, + "delete": { + "min_age": "30s", + "actions": { + } + } + } + } + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, twoPoliciesJSON); + assertThat(updatedState.keys(), containsInAnyOrder("my_timeseries_lifecycle", "my_timeseries_lifecycle1")); + IndexLifecycleMetadata ilmMetadata = updatedState.state() + .metadata() + .custom(IndexLifecycleMetadata.TYPE, IndexLifecycleMetadata.EMPTY); + assertThat(ilmMetadata.getPolicyMetadatas().keySet(), containsInAnyOrder("my_timeseries_lifecycle", "my_timeseries_lifecycle1")); + + String onePolicyRemovedJSON = """ + { + "my_timeseries_lifecycle": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + } + } + } + } + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, onePolicyRemovedJSON); + assertThat(updatedState.keys(), containsInAnyOrder("my_timeseries_lifecycle")); + ilmMetadata = updatedState.state().metadata().custom(IndexLifecycleMetadata.TYPE, IndexLifecycleMetadata.EMPTY); + assertThat(ilmMetadata.getPolicyMetadatas().keySet(), containsInAnyOrder("my_timeseries_lifecycle")); + + String onePolicyRenamedJSON = """ + { + "my_timeseries_lifecycle2": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + } + } + } + } + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, onePolicyRenamedJSON); + assertThat(updatedState.keys(), containsInAnyOrder("my_timeseries_lifecycle2")); + ilmMetadata = updatedState.state().metadata().custom(IndexLifecycleMetadata.TYPE, IndexLifecycleMetadata.EMPTY); + assertThat(ilmMetadata.getPolicyMetadatas().keySet(), containsInAnyOrder("my_timeseries_lifecycle2")); + } +} From 2836ff77f412983cf2b380b30925bcb519f30a5a Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 29 Jun 2022 19:12:50 -0400 Subject: [PATCH 03/27] Rename operator to immutablestate --- server/src/main/java/module-info.java | 2 +- .../TransportClusterUpdateSettingsAction.java | 10 +- .../master/TransportMasterNodeAction.java | 12 +-- .../ImmutableClusterSettingsAction.java | 90 ++++++++++++++++ .../OperatorClusterUpdateSettingsAction.java | 0 .../ImmutableClusterSettingsActionTests.java | 100 ++++++++++++++++++ ...ratorClusterUpdateSettingsActionTests.java | 0 .../plugin/ilm/src/main/java/module-info.java | 4 +- .../TransportDeleteLifecycleAction.java | 12 ++- .../action/TransportPutLifecycleAction.java | 11 +- .../ILMImmutableStateHandlerProvider.java | 31 ++++++ .../action/ImmutableLifecycleAction.java} | 14 +-- .../operator/ILMOperatorHandlerProvider.java | 31 ------ .../TransportDeleteLifecycleActionTests.java | 6 +- .../TransportPutLifecycleActionTests.java | 6 +- .../ImmutableILMStateControllerTests.java} | 14 +-- 16 files changed, 272 insertions(+), 71 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java delete mode 100644 server/src/main/java/org/elasticsearch/immutablestate/action/OperatorClusterUpdateSettingsAction.java create mode 100644 server/src/test/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsActionTests.java delete mode 100644 server/src/test/java/org/elasticsearch/immutablestate/action/OperatorClusterUpdateSettingsActionTests.java create mode 100644 x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java rename x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/{operator/action/OperatorLifecycleAction.java => immutablestate/action/ImmutableLifecycleAction.java} (86%) delete mode 100644 x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java rename x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/{operator/OperatorILMControllerTests.java => immutablestate/ImmutableILMStateControllerTests.java} (93%) diff --git a/server/src/main/java/module-info.java b/server/src/main/java/module-info.java index 8cc384df8b5b7..c6dcf3306729c 100644 --- a/server/src/main/java/module-info.java +++ b/server/src/main/java/module-info.java @@ -360,5 +360,5 @@ org.elasticsearch.index.shard.ShardToolCliProvider; provides org.apache.logging.log4j.util.PropertySource with org.elasticsearch.common.logging.ESSystemPropertiesPropertySource; - uses org.elasticsearch.operator.OperatorHandlerProvider; + uses org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java index c182862edfd3c..90b70368bbb7d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java @@ -29,7 +29,7 @@ import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.SuppressForbidden; -import org.elasticsearch.operator.action.OperatorClusterUpdateSettingsAction; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; @@ -131,8 +131,8 @@ private static boolean checkClearedBlockAndArchivedSettings( } @Override - protected Optional operatorHandlerName() { - return Optional.of(OperatorClusterUpdateSettingsAction.NAME); + protected Optional immutableStateHandlerName() { + return Optional.of(ImmutableClusterSettingsAction.NAME); } @Override @@ -256,7 +256,9 @@ public ClusterUpdateSettingsTask( this.request = request; } - // Used by the operator handler + /** + * Used by the immutable state handler {@link ImmutableClusterSettingsAction} + */ public ClusterUpdateSettingsTask(final ClusterSettings clusterSettings, ClusterUpdateSettingsRequest request) { this(clusterSettings, Priority.IMMEDIATE, request, null); } diff --git a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index a13a8d19601dc..777b0c2356a72 100644 --- a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -146,15 +146,15 @@ private ClusterBlockException checkBlockIfStateRecovered(Request request, Cluste } /** - * Override this method if the master node action also has an {@link org.elasticsearch.operator.OperatorHandler} + * Override this method if the master node action also has an {@link org.elasticsearch.immutablestate.ImmutableClusterStateHandler} * interaction. *

* We need to check if certain settings or entities are allowed to be modified by the master node - * action, depending on if they are set already in operator mode. + * action, depending on if they are set as immutable in 'operator' mode (file based settings, modules, plugins). * - * @return an Optional of the operator handler name + * @return an Optional of the {@link org.elasticsearch.immutablestate.ImmutableClusterStateHandler} name */ - protected Optional operatorHandlerName() { + protected Optional immutableStateHandlerName() { return Optional.empty(); } @@ -162,8 +162,8 @@ protected Optional operatorHandlerName() { * Override this method to return the keys of the cluster state or cluster entities that are modified by * the Request object. *

- * This method is used by the operator handler logic (see {@link org.elasticsearch.operator.OperatorHandler}) - * to verify if the keys don't conflict with an existing key set in operator mode. + * This method is used by the immutable state handler logic (see {@link org.elasticsearch.immutablestate.ImmutableClusterStateHandler}) + * to verify if the keys don't conflict with an existing key set as immutable. * * @param request the TransportMasterNode request * @return set of String keys intended to be modified/set/deleted by this request diff --git a/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java b/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java new file mode 100644 index 0000000000000..cd82f17ecfa8f --- /dev/null +++ b/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java @@ -0,0 +1,90 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.action; + +import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; +import org.elasticsearch.action.admin.cluster.settings.TransportClusterUpdateSettingsAction; +import org.elasticsearch.client.internal.Requests; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.TransformState; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.elasticsearch.common.util.Maps.asMap; + +/** + * This Action is the immutable state save version of RestClusterUpdateSettingsAction + *

+ * It is used by the ImmutableClusterStateController to update the persistent cluster settings. + * Since transient cluster settings are deprecated, this action doesn't support updating transient cluster settings. + */ +public class ImmutableClusterSettingsAction implements ImmutableClusterStateHandler { + + public static final String NAME = "cluster_settings"; + + private final ClusterSettings clusterSettings; + + public ImmutableClusterSettingsAction(ClusterSettings clusterSettings) { + this.clusterSettings = clusterSettings; + } + + @Override + public String name() { + return NAME; + } + + @SuppressWarnings("unchecked") + private ClusterUpdateSettingsRequest prepare(Object input, Set previouslySet) { + final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = Requests.clusterUpdateSettingsRequest(); + + Map source = asMap(input); + Map persistentSettings = new HashMap<>(); + Set toDelete = new HashSet<>(previouslySet); + + source.forEach((k, v) -> { + persistentSettings.put(k, v); + toDelete.remove(k); + }); + + toDelete.forEach(k -> persistentSettings.put(k, null)); + + clusterUpdateSettingsRequest.persistentSettings(persistentSettings); + return clusterUpdateSettingsRequest; + } + + @Override + public TransformState transform(Object input, TransformState prevState) { + ClusterUpdateSettingsRequest request = prepare(input, prevState.keys()); + + // allow empty requests, this is how we clean up settings + if (request.persistentSettings().isEmpty() == false) { + validate(request); + } + + ClusterState state = prevState.state(); + + TransportClusterUpdateSettingsAction.ClusterUpdateSettingsTask updateSettingsTask = + new TransportClusterUpdateSettingsAction.ClusterUpdateSettingsTask(clusterSettings, request); + + state = updateSettingsTask.execute(state); + Set currentKeys = request.persistentSettings() + .keySet() + .stream() + .filter(k -> request.persistentSettings().hasValue(k)) + .collect(Collectors.toSet()); + + return new TransformState(state, currentKeys); + } +} diff --git a/server/src/main/java/org/elasticsearch/immutablestate/action/OperatorClusterUpdateSettingsAction.java b/server/src/main/java/org/elasticsearch/immutablestate/action/OperatorClusterUpdateSettingsAction.java deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/server/src/test/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsActionTests.java b/server/src/test/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsActionTests.java new file mode 100644 index 0000000000000..3ff01b2627021 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsActionTests.java @@ -0,0 +1,100 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.action; + +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.immutablestate.TransformState; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; + +import java.util.Collections; + +import static org.hamcrest.Matchers.containsInAnyOrder; + +public class ImmutableClusterSettingsActionTests extends ESTestCase { + + private TransformState processJSON(ImmutableClusterSettingsAction action, TransformState prevState, String json) throws Exception { + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { + return action.transform(parser.map(), prevState); + } + } + + public void testValidation() throws Exception { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + + ClusterState state = ClusterState.builder(new ClusterName("elasticsearch")).build(); + TransformState prevState = new TransformState(state, Collections.emptySet()); + ImmutableClusterSettingsAction action = new ImmutableClusterSettingsAction(clusterSettings); + + String badPolicyJSON = """ + { + "indices.recovery.min_bytes_per_sec": "50mb" + }"""; + + assertEquals( + "persistent setting [indices.recovery.min_bytes_per_sec], not recognized", + expectThrows(IllegalArgumentException.class, () -> processJSON(action, prevState, badPolicyJSON)).getMessage() + ); + } + + public void testSetUnsetSettings() throws Exception { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + + ClusterState state = ClusterState.builder(new ClusterName("elasticsearch")).build(); + TransformState prevState = new TransformState(state, Collections.emptySet()); + ImmutableClusterSettingsAction action = new ImmutableClusterSettingsAction(clusterSettings); + + String emptyJSON = ""; + + TransformState updatedState = processJSON(action, prevState, emptyJSON); + assertEquals(0, updatedState.keys().size()); + assertEquals(prevState.state(), updatedState.state()); + + String settingsJSON = """ + { + "indices.recovery.max_bytes_per_sec": "50mb", + "cluster": { + "remote": { + "cluster_one": { + "seeds": [ + "127.0.0.1:9300" + ] + } + } + } + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, settingsJSON); + assertThat(updatedState.keys(), containsInAnyOrder("indices.recovery.max_bytes_per_sec", "cluster.remote.cluster_one.seeds")); + assertEquals("50mb", updatedState.state().metadata().persistentSettings().get("indices.recovery.max_bytes_per_sec")); + assertEquals("[127.0.0.1:9300]", updatedState.state().metadata().persistentSettings().get("cluster.remote.cluster_one.seeds")); + + String oneSettingJSON = """ + { + "indices.recovery.max_bytes_per_sec": "25mb" + }"""; + + prevState = updatedState; + updatedState = processJSON(action, prevState, oneSettingJSON); + assertThat(updatedState.keys(), containsInAnyOrder("indices.recovery.max_bytes_per_sec")); + assertEquals("25mb", updatedState.state().metadata().persistentSettings().get("indices.recovery.max_bytes_per_sec")); + assertNull(updatedState.state().metadata().persistentSettings().get("cluster.remote.cluster_one.seeds")); + + prevState = updatedState; + updatedState = processJSON(action, prevState, emptyJSON); + assertEquals(0, updatedState.keys().size()); + assertNull(updatedState.state().metadata().persistentSettings().get("indices.recovery.max_bytes_per_sec")); + } +} diff --git a/server/src/test/java/org/elasticsearch/immutablestate/action/OperatorClusterUpdateSettingsActionTests.java b/server/src/test/java/org/elasticsearch/immutablestate/action/OperatorClusterUpdateSettingsActionTests.java deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/x-pack/plugin/ilm/src/main/java/module-info.java b/x-pack/plugin/ilm/src/main/java/module-info.java index 69348eedcfc60..10e68f0a13ff2 100644 --- a/x-pack/plugin/ilm/src/main/java/module-info.java +++ b/x-pack/plugin/ilm/src/main/java/module-info.java @@ -1,3 +1,5 @@ +import org.elasticsearch.xpack.ilm.immutablestate.ILMImmutableStateHandlerProvider; + /* * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one * or more contributor license agreements. Licensed under the Elastic License @@ -17,5 +19,5 @@ exports org.elasticsearch.xpack.slm.action to org.elasticsearch.server; exports org.elasticsearch.xpack.slm to org.elasticsearch.server; - provides org.elasticsearch.operator.OperatorHandlerProvider with org.elasticsearch.xpack.ilm.operator.ILMOperatorHandlerProvider; + provides org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider with ILMImmutableStateHandlerProvider; } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java index 5fc29ed66f5fc..4d3cf3d613a11 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java @@ -29,7 +29,7 @@ import org.elasticsearch.xpack.core.ilm.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction.Request; -import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; +import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.util.List; import java.util.Optional; @@ -73,7 +73,11 @@ public DeleteLifecyclePolicyTask(Request request, ActionListener operatorHandlerName() { - return Optional.of(OperatorLifecycleAction.NAME); + protected Optional immutableStateHandlerName() { + return Optional.of(ImmutableLifecycleAction.NAME); } @Override diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java index d1d75d7333d85..08b9147f55376 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleAction.java @@ -41,7 +41,7 @@ import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction.Request; import org.elasticsearch.xpack.core.slm.SnapshotLifecycleMetadata; -import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; +import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.time.Instant; import java.util.HashMap; @@ -146,7 +146,10 @@ public UpdateLifecyclePolicyTask( } /** - * Constructor used in operator mode. It disables verbose logging and has no filtered headers. + * Used by the {@link org.elasticsearch.immutablestate.ImmutableClusterStateHandler} for ILM + * {@link ImmutableLifecycleAction} + *

+ * It disables verbose logging and has no filtered headers. * * @param request * @param licenseState @@ -313,8 +316,8 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) } @Override - protected Optional operatorHandlerName() { - return Optional.of(OperatorLifecycleAction.NAME); + protected Optional immutableStateHandlerName() { + return Optional.of(ImmutableLifecycleAction.NAME); } @Override diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java new file mode 100644 index 0000000000000..764a087dd9264 --- /dev/null +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java @@ -0,0 +1,31 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.ilm.immutablestate; + +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; + +import java.util.Collection; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + +/** + * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface + */ +public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider { + private static final Set> handlers = ConcurrentHashMap.newKeySet(); + + @Override + public Collection> handlers() { + return handlers; + } + + public static void handler(ImmutableClusterStateHandler handler) { + handlers.add(handler); + } +} diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/action/ImmutableLifecycleAction.java similarity index 86% rename from x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java rename to x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/action/ImmutableLifecycleAction.java index 0092db643568c..9efaa3434915b 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/action/OperatorLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/action/ImmutableLifecycleAction.java @@ -5,13 +5,13 @@ * 2.0. */ -package org.elasticsearch.xpack.ilm.operator.action; +package org.elasticsearch.xpack.ilm.immutablestate.action; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.TransformState; import org.elasticsearch.license.XPackLicenseState; -import org.elasticsearch.operator.OperatorHandler; -import org.elasticsearch.operator.TransformState; import org.elasticsearch.xcontent.NamedXContentRegistry; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; @@ -34,13 +34,13 @@ import static org.elasticsearch.common.xcontent.XContentHelper.mapToXContentParser; /** - * This {@link OperatorHandler} is responsible for CRUD operations on ILM policies in - * operator mode, e.g. file based settings. + * This {@link org.elasticsearch.immutablestate.ImmutableClusterStateHandler} is responsible for immutable state + * CRUD operations on ILM policies in, e.g. file based settings. *

* Internally it uses {@link TransportPutLifecycleAction} and * {@link TransportDeleteLifecycleAction} to add, update and delete ILM policies. */ -public class OperatorLifecycleAction implements OperatorHandler { +public class ImmutableLifecycleAction implements ImmutableClusterStateHandler { private final NamedXContentRegistry xContentRegistry; private final Client client; @@ -48,7 +48,7 @@ public class OperatorLifecycleAction implements OperatorHandler public static final String NAME = "ilm"; - public OperatorLifecycleAction(NamedXContentRegistry xContentRegistry, Client client, XPackLicenseState licenseState) { + public ImmutableLifecycleAction(NamedXContentRegistry xContentRegistry, Client client, XPackLicenseState licenseState) { this.xContentRegistry = xContentRegistry; this.client = client; this.licenseState = licenseState; diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java deleted file mode 100644 index 5577401719cfd..0000000000000 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/operator/ILMOperatorHandlerProvider.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.ilm.operator; - -import org.elasticsearch.operator.OperatorHandler; -import org.elasticsearch.operator.OperatorHandlerProvider; - -import java.util.Collection; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; - -/** - * ILM Provider implementation for the OperatorHandlerProvider service interface - */ -public class ILMOperatorHandlerProvider implements OperatorHandlerProvider { - private static final Set> handlers = ConcurrentHashMap.newKeySet(); - - @Override - public Collection> handlers() { - return handlers; - } - - public static void handler(OperatorHandler handler) { - handlers.add(handler); - } -} diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java index 51df10fb7a190..db1edda71b39f 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java @@ -14,13 +14,13 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction; -import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; +import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.mockito.Mockito.mock; public class TransportDeleteLifecycleActionTests extends ESTestCase { - public void testOperatorHandler() { + public void testImmutableHandler() { TransportDeleteLifecycleAction putAction = new TransportDeleteLifecycleAction( mock(TransportService.class), mock(ClusterService.class), @@ -28,7 +28,7 @@ public void testOperatorHandler() { mock(ActionFilters.class), mock(IndexNameExpressionResolver.class) ); - assertEquals(OperatorLifecycleAction.NAME, putAction.operatorHandlerName().get()); + assertEquals(ImmutableLifecycleAction.NAME, putAction.immutableStateHandlerName().get()); DeleteLifecycleAction.Request request = new DeleteLifecycleAction.Request("my_timeseries_lifecycle2"); assertThat(putAction.modifiedKeys(request), containsInAnyOrder("my_timeseries_lifecycle2")); diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java index 6731957cb43ab..030bd55703e94 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java @@ -23,7 +23,7 @@ import org.elasticsearch.xpack.core.ilm.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; import org.elasticsearch.xpack.ilm.LifecyclePolicyTestsUtils; -import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; +import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.util.Map; @@ -46,7 +46,7 @@ public void testIsNoop() { assertFalse(TransportPutLifecycleAction.isNoopUpdate(null, policy1, headers1)); } - public void testOperatorHandler() throws Exception { + public void testImmutableStateHandler() throws Exception { TransportPutLifecycleAction putAction = new TransportPutLifecycleAction( mock(TransportService.class), mock(ClusterService.class), @@ -57,7 +57,7 @@ public void testOperatorHandler() throws Exception { mock(XPackLicenseState.class), mock(Client.class) ); - assertEquals(OperatorLifecycleAction.NAME, putAction.operatorHandlerName().get()); + assertEquals(ImmutableLifecycleAction.NAME, putAction.immutableStateHandlerName().get()); String json = """ { diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/immutablestate/ImmutableILMStateControllerTests.java similarity index 93% rename from x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java rename to x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/immutablestate/ImmutableILMStateControllerTests.java index ed279c213693d..4f8c672a24ab5 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/operator/OperatorILMControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/immutablestate/ImmutableILMStateControllerTests.java @@ -5,15 +5,15 @@ * 2.0. */ -package org.elasticsearch.xpack.ilm.action.operator; +package org.elasticsearch.xpack.ilm.action.immutablestate; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.immutablestate.TransformState; import org.elasticsearch.license.XPackLicenseState; -import org.elasticsearch.operator.TransformState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.NamedXContentRegistry; import org.elasticsearch.xcontent.ParseField; @@ -38,7 +38,7 @@ import org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType; import org.elasticsearch.xpack.core.ilm.UnfollowAction; import org.elasticsearch.xpack.core.ilm.WaitForSnapshotAction; -import org.elasticsearch.xpack.ilm.operator.action.OperatorLifecycleAction; +import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.util.ArrayList; import java.util.Arrays; @@ -49,7 +49,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -public class OperatorILMControllerTests extends ESTestCase { +public class ImmutableILMStateControllerTests extends ESTestCase { protected NamedXContentRegistry xContentRegistry() { List entries = new ArrayList<>(ClusterModule.getNamedXWriteables()); @@ -86,7 +86,7 @@ protected NamedXContentRegistry xContentRegistry() { return new NamedXContentRegistry(entries); } - private TransformState processJSON(OperatorLifecycleAction action, TransformState prevState, String json) throws Exception { + private TransformState processJSON(ImmutableLifecycleAction action, TransformState prevState, String json) throws Exception { try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { return action.transform(parser.map(), prevState); } @@ -98,7 +98,7 @@ public void testValidationFails() { final ClusterName clusterName = new ClusterName("elasticsearch"); ClusterState state = ClusterState.builder(clusterName).build(); - OperatorLifecycleAction action = new OperatorLifecycleAction(xContentRegistry(), client, mock(XPackLicenseState.class)); + ImmutableLifecycleAction action = new ImmutableLifecycleAction(xContentRegistry(), client, mock(XPackLicenseState.class)); TransformState prevState = new TransformState(state, Collections.emptySet()); String badPolicyJSON = """ @@ -127,7 +127,7 @@ public void testActionAddRemove() throws Exception { ClusterState state = ClusterState.builder(clusterName).build(); - OperatorLifecycleAction action = new OperatorLifecycleAction(xContentRegistry(), client, mock(XPackLicenseState.class)); + ImmutableLifecycleAction action = new ImmutableLifecycleAction(xContentRegistry(), client, mock(XPackLicenseState.class)); String emptyJSON = ""; From 2c11e746ac4faecb3f775db07aa48b791aa8dc26 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 30 Jun 2022 15:49:02 -0400 Subject: [PATCH 04/27] Address PR review feedback --- .../plugin/ilm/src/main/java/module-info.java | 2 +- .../ILMImmutableStateHandlerProvider.java | 7 +++-- .../xpack/ilm/IndexLifecycle.java | 5 ++++ .../action/ImmutableLifecycleAction.java | 4 +-- .../TransportDeleteLifecycleAction.java | 4 +-- .../action/TransportPutLifecycleAction.java | 30 ++++++++----------- .../ImmutableILMStateControllerTests.java | 3 +- .../TransportDeleteLifecycleActionTests.java | 1 - .../TransportPutLifecycleActionTests.java | 1 - 9 files changed, 25 insertions(+), 32 deletions(-) rename x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/{immutablestate => }/ILMImmutableStateHandlerProvider.java (81%) rename x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/{immutablestate => }/action/ImmutableLifecycleAction.java (95%) rename x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/{immutablestate => }/ImmutableILMStateControllerTests.java (98%) diff --git a/x-pack/plugin/ilm/src/main/java/module-info.java b/x-pack/plugin/ilm/src/main/java/module-info.java index 10e68f0a13ff2..b15ff4baecb30 100644 --- a/x-pack/plugin/ilm/src/main/java/module-info.java +++ b/x-pack/plugin/ilm/src/main/java/module-info.java @@ -1,4 +1,4 @@ -import org.elasticsearch.xpack.ilm.immutablestate.ILMImmutableStateHandlerProvider; +import org.elasticsearch.xpack.ilm.ILMImmutableStateHandlerProvider; /* * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java similarity index 81% rename from x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java rename to x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java index 764a087dd9264..afd2397b8fb6f 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/ILMImmutableStateHandlerProvider.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java @@ -5,11 +5,12 @@ * 2.0. */ -package org.elasticsearch.xpack.ilm.immutablestate; +package org.elasticsearch.xpack.ilm; import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; +import java.util.Arrays; import java.util.Collection; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -25,7 +26,7 @@ public Collection> handlers() { return handlers; } - public static void handler(ImmutableClusterStateHandler handler) { - handlers.add(handler); + public static void registerHandlers(ImmutableClusterStateHandler... stateHandlers) { + handlers.addAll(Arrays.asList(stateHandlers)); } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java index c18b0d632a8bc..b7f824b4c2845 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycle.java @@ -83,6 +83,7 @@ import org.elasticsearch.xpack.core.slm.action.PutSnapshotLifecycleAction; import org.elasticsearch.xpack.core.slm.action.StartSLMAction; import org.elasticsearch.xpack.core.slm.action.StopSLMAction; +import org.elasticsearch.xpack.ilm.action.ImmutableLifecycleAction; import org.elasticsearch.xpack.ilm.action.RestDeleteLifecycleAction; import org.elasticsearch.xpack.ilm.action.RestExplainLifecycleAction; import org.elasticsearch.xpack.ilm.action.RestGetLifecycleAction; @@ -267,6 +268,10 @@ public Collection createComponents( components.addAll(Arrays.asList(snapshotLifecycleService.get(), snapshotHistoryStore.get(), snapshotRetentionService.get())); ilmHealthIndicatorService.set(new IlmHealthIndicatorService(clusterService)); slmHealthIndicatorService.set(new SlmHealthIndicatorService(clusterService)); + + ILMImmutableStateHandlerProvider.registerHandlers( + new ImmutableLifecycleAction(xContentRegistry, client, XPackPlugin.getSharedLicenseState()) + ); return components; } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/action/ImmutableLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java similarity index 95% rename from x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/action/ImmutableLifecycleAction.java rename to x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java index 9efaa3434915b..68b6d994ae63f 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/immutablestate/action/ImmutableLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java @@ -5,7 +5,7 @@ * 2.0. */ -package org.elasticsearch.xpack.ilm.immutablestate.action; +package org.elasticsearch.xpack.ilm.action; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterState; @@ -18,8 +18,6 @@ import org.elasticsearch.xpack.core.ilm.LifecyclePolicy; import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; import org.elasticsearch.xpack.core.template.LifecyclePolicyConfig; -import org.elasticsearch.xpack.ilm.action.TransportDeleteLifecycleAction; -import org.elasticsearch.xpack.ilm.action.TransportPutLifecycleAction; import java.io.IOException; import java.util.ArrayList; diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java index 4d3cf3d613a11..29282740072f4 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleAction.java @@ -29,7 +29,6 @@ import org.elasticsearch.xpack.core.ilm.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction.Request; -import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.util.List; import java.util.Optional; @@ -76,9 +75,8 @@ public DeleteLifecyclePolicyTask(Request request, ActionListener filteredHeaders, NamedXContentRegistry xContentRegistry, - Client client, - boolean verboseLogging + Client client ) { super(request, listener); this.request = request; @@ -142,7 +140,7 @@ public UpdateLifecyclePolicyTask( this.filteredHeaders = filteredHeaders; this.xContentRegistry = xContentRegistry; this.client = client; - this.verboseLogging = verboseLogging; + this.verboseLogging = true; } /** @@ -150,19 +148,15 @@ public UpdateLifecyclePolicyTask( * {@link ImmutableLifecycleAction} *

* It disables verbose logging and has no filtered headers. - * - * @param request - * @param licenseState - * @param xContentRegistry - * @param client */ - public UpdateLifecyclePolicyTask( - Request request, - XPackLicenseState licenseState, - NamedXContentRegistry xContentRegistry, - Client client - ) { - this(request, null, licenseState, new HashMap<>(), xContentRegistry, client, false); + UpdateLifecyclePolicyTask(Request request, XPackLicenseState licenseState, NamedXContentRegistry xContentRegistry, Client client) { + super(request, null); + this.request = request; + this.licenseState = licenseState; + this.filteredHeaders = Collections.emptyMap(); + this.xContentRegistry = xContentRegistry; + this.client = client; + this.verboseLogging = false; } @Override diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/immutablestate/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java similarity index 98% rename from x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/immutablestate/ImmutableILMStateControllerTests.java rename to x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index 4f8c672a24ab5..a9c776f100623 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/immutablestate/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -5,7 +5,7 @@ * 2.0. */ -package org.elasticsearch.xpack.ilm.action.immutablestate; +package org.elasticsearch.xpack.ilm.action; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterModule; @@ -38,7 +38,6 @@ import org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType; import org.elasticsearch.xpack.core.ilm.UnfollowAction; import org.elasticsearch.xpack.core.ilm.WaitForSnapshotAction; -import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.util.ArrayList; import java.util.Arrays; diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java index db1edda71b39f..588eee06777d8 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportDeleteLifecycleActionTests.java @@ -14,7 +14,6 @@ import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.ilm.action.DeleteLifecycleAction; -import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.mockito.Mockito.mock; diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java index 030bd55703e94..4a54b4cf696a7 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/TransportPutLifecycleActionTests.java @@ -23,7 +23,6 @@ import org.elasticsearch.xpack.core.ilm.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.ilm.action.PutLifecycleAction; import org.elasticsearch.xpack.ilm.LifecyclePolicyTestsUtils; -import org.elasticsearch.xpack.ilm.immutablestate.action.ImmutableLifecycleAction; import java.util.Map; From c636026713a50c6be237803b8706c53ebb236cd7 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 30 Jun 2022 17:54:26 -0400 Subject: [PATCH 05/27] Add ImmutableClusterStateController --- .../elasticsearch/action/ActionModule.java | 33 +- .../master/TransportMasterNodeAction.java | 33 ++ .../ImmutableClusterStateController.java | 306 +++++++++++ .../ImmutableStateUpdateErrorTask.java | 90 ++++ .../ImmutableStateUpdateStateTask.java | 174 +++++++ .../service/StateVersionMetadata.java | 57 +++ .../java/org/elasticsearch/node/Node.java | 5 +- .../action/ActionModuleTests.java | 4 + .../ClusterUpdateSettingsRequestTests.java | 49 ++ .../TransportMasterNodeActionTests.java | 112 +++- .../ImmutableClusterStateControllerTests.java | 483 ++++++++++++++++++ .../ImmutableILMStateControllerTests.java | 76 +++ .../xpack/security/SecurityTests.java | 1 + 13 files changed, 1420 insertions(+), 3 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java create mode 100644 server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateErrorTask.java create mode 100644 server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java create mode 100644 server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java create mode 100644 server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 1858d8ab28784..b24f8c60b979c 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -251,6 +251,7 @@ import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.NamedRegistry; import org.elasticsearch.common.inject.AbstractModule; import org.elasticsearch.common.inject.TypeLiteral; @@ -262,6 +263,10 @@ import org.elasticsearch.gateway.TransportNodesListGatewayStartedShards; import org.elasticsearch.health.GetHealthAction; import org.elasticsearch.health.RestGetHealthAction; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; +import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; import org.elasticsearch.index.seqno.GlobalCheckpointSyncAction; import org.elasticsearch.index.seqno.RetentionLeaseActions; import org.elasticsearch.indices.SystemIndices; @@ -273,6 +278,7 @@ import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; +import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.plugins.interceptor.RestInterceptorActionPlugin; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; @@ -448,6 +454,7 @@ public class ActionModule extends AbstractModule { private final RequestValidators mappingRequestValidators; private final RequestValidators indicesAliasesRequestRequestValidators; private final ThreadPool threadPool; + private final ImmutableClusterStateController immutableStateController; public ActionModule( Settings settings, @@ -460,7 +467,8 @@ public ActionModule( NodeClient nodeClient, CircuitBreakerService circuitBreakerService, UsageService usageService, - SystemIndices systemIndices + SystemIndices systemIndices, + ClusterService clusterService ) { this.settings = settings; this.indexNameExpressionResolver = indexNameExpressionResolver; @@ -511,6 +519,7 @@ public ActionModule( ); restController = new RestController(headers, restInterceptor, nodeClient, circuitBreakerService, usageService); + immutableStateController = new ImmutableClusterStateController(clusterService); } public Map> getActions() { @@ -888,6 +897,24 @@ public void initRestHandlers(Supplier nodesInCluster) { registerHandler.accept(new RestCatAction(catActions)); } + /** + * Initializes the immutable cluster state handlers for Elasticsearch and it's modules/plugins + * + * @param pluginsService needed to load all modules/plugins immutable state handlers through SPI + */ + public void initImmutableClusterStateHandlers(PluginsService pluginsService) { + List> handlers = new ArrayList<>(); + + List pluginHandlers = pluginsService.loadServiceProviders( + ImmutableClusterStateHandlerProvider.class + ); + + handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); + pluginHandlers.forEach(h -> handlers.addAll(h.handlers())); + + immutableStateController.initHandlers(handlers); + } + @Override protected void configure() { bind(ActionFilters.class).toInstance(actionFilters); @@ -920,4 +947,8 @@ public ActionFilters getActionFilters() { public RestController getRestController() { return restController; } + + public ImmutableClusterStateController getImmutableClusterStateController() { + return immutableStateController; + } } diff --git a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index 777b0c2356a72..26f9de8dd380b 100644 --- a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -22,6 +22,7 @@ import org.elasticsearch.cluster.block.ClusterBlock; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; @@ -42,7 +43,9 @@ import org.elasticsearch.transport.TransportException; import org.elasticsearch.transport.TransportService; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.function.Predicate; @@ -172,9 +175,39 @@ protected Set modifiedKeys(Request request) { return Collections.emptySet(); } + // package private for testing + void validateForImmutableState(Request request, ClusterState state) { + Optional handlerName = immutableStateHandlerName(); + assert handlerName.isPresent(); + + Set modified = modifiedKeys(request); + List errors = new ArrayList<>(); + + for (ImmutableStateMetadata metadata : state.metadata().immutableStateMetadata().values()) { + Set conflicts = metadata.conflicts(handlerName.get(), modified); + if (conflicts.isEmpty() == false) { + errors.add(format("[%s] set as read-only by [%s]", String.join(",", conflicts), metadata.namespace())); + } + } + + if (errors.isEmpty() == false) { + throw new IllegalArgumentException( + format("Failed to process request [%s] with errors: %s", request, String.join(System.lineSeparator(), errors)) + ); + } + } + + // package private for testing + boolean supportsImmutableState() { + return immutableStateHandlerName().isPresent(); + } + @Override protected void doExecute(Task task, final Request request, ActionListener listener) { ClusterState state = clusterService.state(); + if (supportsImmutableState()) { + validateForImmutableState(request, state); + } logger.trace("starting processing request [{}] with cluster state version [{}]", request, state.version()); if (task != null) { request.setParentTask(clusterService.localNode().getId(), task.getId()); diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java new file mode 100644 index 0000000000000..f0ca10473fec2 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java @@ -0,0 +1,306 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.service; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.Version; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateTaskConfig; +import org.elasticsearch.cluster.metadata.ImmutableStateErrorMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Priority; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.xcontent.ConstructingObjectParser; +import org.elasticsearch.xcontent.ParseField; +import org.elasticsearch.xcontent.XContentParser; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Consumer; +import java.util.function.Function; +import java.util.stream.Collectors; + +import static org.elasticsearch.core.Strings.format; + +/** + * Controller class for applying file based settings to ClusterState. + * This class contains the logic about validation, ordering and applying of + * the cluster state specified in a file. + */ +public class ImmutableClusterStateController { + private static final Logger logger = LogManager.getLogger(ImmutableClusterStateController.class); + + public static final String SETTINGS = "settings"; + public static final String METADATA = "metadata"; + + Map> handlers = null; + final ClusterService clusterService; + + public ImmutableClusterStateController(ClusterService clusterService) { + this.clusterService = clusterService; + } + + /** + * Initializes the controller with the currently implemented state handlers + * + * @param handlerList the list of supported immutable cluster state handlers + */ + public void initHandlers(List> handlerList) { + handlers = handlerList.stream().collect(Collectors.toMap(ImmutableClusterStateHandler::name, Function.identity())); + } + + /** + * A package class containing the composite immutable cluster state + *

+ * Apart from the cluster state we want to store as immutable, the package requires that + * you supply the version metadata. This version metadata (see {@link StateVersionMetadata}) is checked to ensure + * that the update is safe, and it's not unnecessarily repeated. + */ + public static class Package { + public static final ParseField STATE_FIELD = new ParseField("state"); + public static final ParseField METADATA_FIELD = new ParseField("metadata"); + @SuppressWarnings("unchecked") + private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + "immutable_cluster_state", + a -> new Package((Map) a[0], (StateVersionMetadata) a[1]) + ); + static { + PARSER.declareObject(ConstructingObjectParser.constructorArg(), (p, c) -> p.map(), STATE_FIELD); + PARSER.declareObject(ConstructingObjectParser.constructorArg(), StateVersionMetadata::parse, METADATA_FIELD); + } + + Map state; + StateVersionMetadata metadata; + + /** + * A package class containing the composite immutable cluster state + * @param state a {@link Map} of immutable state handler name and data + * @param metadata a version metadata + */ + public Package(Map state, StateVersionMetadata metadata) { + this.state = state; + this.metadata = metadata; + } + } + + /** + * Saves an immutable cluster state for a given 'namespace' from {@link XContentParser} + * + * @param namespace the namespace under which we'll store the immutable keys in the cluster state metadata + * @param parser the XContentParser to process + * @param errorListener a consumer called with IllegalStateException if the content has errors and the + * cluster state cannot be correctly applied, IncompatibleVersionException if the content is stale or + * incompatible with this node {@link Version}, null if successful. + */ + public void process(String namespace, XContentParser parser, Consumer errorListener) { + Package immutableStatePackage; + + try { + immutableStatePackage = Package.PARSER.apply(parser, null); + } catch (Exception e) { + List errors = List.of(e.getMessage()); + recordErrorState(new ImmutableUpdateErrorState(namespace, -1L, errors, ImmutableStateErrorMetadata.ErrorKind.PARSING)); + logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); + + errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); + return; + } + + process(namespace, immutableStatePackage, errorListener); + } + + /** + * Saves an immutable cluster state for a given 'namespace' from {@link Package} + * + * @param namespace the namespace under which we'll store the immutable keys in the cluster state metadata + * @param immutableStateFilePackage a {@link Package} composite state object to process + * @param errorListener a consumer called with IllegalStateException if the content has errors and the + * cluster state cannot be correctly applied, IncompatibleVersionException if the content is stale or + * incompatible with this node {@link Version}, null if successful. + */ + public void process(String namespace, Package immutableStateFilePackage, Consumer errorListener) { + Map immutableState = immutableStateFilePackage.state; + StateVersionMetadata stateVersionMetadata = immutableStateFilePackage.metadata; + + LinkedHashSet orderedHandlers; + try { + orderedHandlers = orderedStateHandlers(immutableState.keySet()); + } catch (Exception e) { + List errors = List.of(e.getMessage()); + recordErrorState( + new ImmutableUpdateErrorState( + namespace, + stateVersionMetadata.version(), + errors, + ImmutableStateErrorMetadata.ErrorKind.PARSING + ) + ); + logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); + + errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); + return; + } + + ClusterState state = clusterService.state(); + ImmutableStateMetadata existingMetadata = state.metadata().immutableStateMetadata().get(namespace); + if (checkMetadataVersion(existingMetadata, stateVersionMetadata, errorListener) == false) { + return; + } + + // Do we need to retry this, or it retries automatically? + clusterService.submitStateUpdateTask( + "immutable cluster state [" + namespace + "]", + new ImmutableStateUpdateStateTask( + namespace, + immutableStateFilePackage, + handlers, + orderedHandlers, + (errorState) -> recordErrorState(errorState), + new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty empty) { + logger.info("Successfully applied new cluster state for namespace [{}]", namespace); + errorListener.accept(null); + } + + @Override + public void onFailure(Exception e) { + logger.error("Failed to apply immutable cluster state", e); + errorListener.accept(e); + } + } + ), + ClusterStateTaskConfig.build(Priority.URGENT), + new ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor(namespace, clusterService.getRerouteService()) + ); + } + + // package private for testing + static boolean checkMetadataVersion( + ImmutableStateMetadata existingMetadata, + StateVersionMetadata stateVersionMetadata, + Consumer errorListener + ) { + if (Version.CURRENT.before(stateVersionMetadata.minCompatibleVersion())) { + errorListener.accept( + new IncompatibleVersionException( + format( + "Cluster state version [%s] is not compatible with this Elasticsearch node", + stateVersionMetadata.minCompatibleVersion() + ) + ) + ); + return false; + } + + if (existingMetadata != null && existingMetadata.version() >= stateVersionMetadata.version()) { + errorListener.accept( + new IncompatibleVersionException( + format( + "Not updating cluster state because version [%s] is less or equal to the current metadata version [%s]", + stateVersionMetadata.version(), + existingMetadata.version() + ) + ) + ); + return false; + } + + return true; + } + + record ImmutableUpdateErrorState( + String namespace, + Long version, + List errors, + ImmutableStateErrorMetadata.ErrorKind errorKind + ) {} + + private void recordErrorState(ImmutableUpdateErrorState state) { + clusterService.submitStateUpdateTask( + "immutable cluster state update error for [ " + state.namespace + "]", + new ImmutableStateUpdateErrorTask(state, new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty empty) { + logger.info("Successfully applied new immutable error state for namespace [{}]", state.namespace); + } + + @Override + public void onFailure(Exception e) { + logger.error("Failed to apply immutable error cluster state", e); + } + }), + ClusterStateTaskConfig.build(Priority.URGENT), + new ImmutableStateUpdateErrorTask.ImmutableUpdateErrorTaskExecutor() + ); + } + + // package private for testing + LinkedHashSet orderedStateHandlers(Set keys) { + LinkedHashSet orderedHandlers = new LinkedHashSet<>(); + LinkedHashSet dependencyStack = new LinkedHashSet<>(); + + for (String key : keys) { + addStateHandler(key, keys, orderedHandlers, dependencyStack); + } + + return orderedHandlers; + } + + private void addStateHandler(String key, Set keys, LinkedHashSet ordered, LinkedHashSet visited) { + if (visited.contains(key)) { + StringBuilder msg = new StringBuilder("Cycle found in settings dependencies: "); + visited.forEach(s -> { + msg.append(s); + msg.append(" -> "); + }); + msg.append(key); + throw new IllegalStateException(msg.toString()); + } + + if (ordered.contains(key)) { + // already added by another dependent handler + return; + } + + visited.add(key); + ImmutableClusterStateHandler handler = handlers.get(key); + + if (handler == null) { + throw new IllegalStateException("Unknown settings definition type: " + key); + } + + for (String dependency : handler.dependencies()) { + if (keys.contains(dependency) == false) { + throw new IllegalStateException("Missing settings dependency definition: " + key + " -> " + dependency); + } + addStateHandler(dependency, keys, ordered, visited); + } + + visited.remove(key); + ordered.add(key); + } + + /** + * {@link IncompatibleVersionException} is thrown when we try to update the cluster state + * without changing the update version id, or if we try to update cluster state on + * an incompatible Elasticsearch version in mixed cluster mode. + */ + public static class IncompatibleVersionException extends RuntimeException { + public IncompatibleVersionException(String message) { + super(message); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateErrorTask.java b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateErrorTask.java new file mode 100644 index 0000000000000..dd4b336aeded1 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateErrorTask.java @@ -0,0 +1,90 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.service; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateTaskExecutor; +import org.elasticsearch.cluster.ClusterStateTaskListener; +import org.elasticsearch.cluster.metadata.ImmutableStateErrorMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; +import org.elasticsearch.cluster.metadata.Metadata; + +import java.util.List; + +/** + * Cluster state update task that sets the error state of the immutable cluster state metadata. + *

+ * This is used when an immutable cluster state update encounters error(s) while processing + * the {@link org.elasticsearch.immutablestate.service.ImmutableClusterStateController.Package}. + */ +public class ImmutableStateUpdateErrorTask implements ClusterStateTaskListener { + + private final ImmutableClusterStateController.ImmutableUpdateErrorState errorState; + private final ActionListener listener; + + public ImmutableStateUpdateErrorTask( + ImmutableClusterStateController.ImmutableUpdateErrorState errorState, + ActionListener listener + ) { + this.errorState = errorState; + this.listener = listener; + } + + private static final Logger logger = LogManager.getLogger(ImmutableStateUpdateErrorTask.class); + + @Override + public void onFailure(Exception e) { + listener.onFailure(e); + } + + ActionListener listener() { + return listener; + } + + ClusterState execute(ClusterState currentState) { + ClusterState.Builder stateBuilder = new ClusterState.Builder(currentState); + Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata()); + ImmutableStateMetadata immutableMetadata = currentState.metadata().immutableStateMetadata().get(errorState.namespace()); + ImmutableStateMetadata.Builder immMetadataBuilder = ImmutableStateMetadata.builder(errorState.namespace(), immutableMetadata); + immMetadataBuilder.errorMetadata( + new ImmutableStateErrorMetadata(errorState.version(), errorState.errorKind(), errorState.errors()) + ); + metadataBuilder.put(immMetadataBuilder.build()); + ClusterState newState = stateBuilder.metadata(metadataBuilder).build(); + + return newState; + } + + /** + * Immutable cluster error state task executor + */ + public record ImmutableUpdateErrorTaskExecutor() implements ClusterStateTaskExecutor { + + @Override + public ClusterState execute(ClusterState currentState, List> taskContexts) + throws Exception { + for (final var taskContext : taskContexts) { + currentState = taskContext.getTask().execute(currentState); + taskContext.success( + () -> taskContext.getTask().listener().delegateFailure((l, s) -> l.onResponse(ActionResponse.Empty.INSTANCE)) + ); + } + return currentState; + } + + @Override + public void clusterStatePublished(ClusterState newClusterState) { + logger.info("Wrote new error state in immutable metadata"); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java new file mode 100644 index 0000000000000..f50edd24ecafd --- /dev/null +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java @@ -0,0 +1,174 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.service; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateTaskExecutor; +import org.elasticsearch.cluster.ClusterStateTaskListener; +import org.elasticsearch.cluster.metadata.ImmutableStateErrorMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateHandlerMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; +import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.routing.RerouteService; +import org.elasticsearch.common.Priority; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.TransformState; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Consumer; + +import static org.elasticsearch.core.Strings.format; + +/** + * Generic immutable cluster state update task + */ +public class ImmutableStateUpdateStateTask implements ClusterStateTaskListener { + private static final Logger logger = LogManager.getLogger(ImmutableStateUpdateStateTask.class); + + private final String namespace; + private final ImmutableClusterStateController.Package immutableStatePackage; + private final Map> handlers; + private final Collection orderedHandlers; + private final Consumer recordErrorState; + private final ActionListener listener; + + public ImmutableStateUpdateStateTask( + String namespace, + ImmutableClusterStateController.Package immutableStatePackage, + Map> handlers, + Collection orderedHandlers, + Consumer recordErrorState, + ActionListener listener + ) { + this.namespace = namespace; + this.immutableStatePackage = immutableStatePackage; + this.handlers = handlers; + this.orderedHandlers = orderedHandlers; + this.recordErrorState = recordErrorState; + this.listener = listener; + } + + @Override + public void onFailure(Exception e) { + listener.onFailure(e); + } + + ActionListener listener() { + return listener; + } + + protected ClusterState execute(ClusterState state) { + ImmutableStateMetadata existingMetadata = state.metadata().immutableStateMetadata().get(namespace); + Map immutableState = immutableStatePackage.state; + StateVersionMetadata stateVersionMetadata = immutableStatePackage.metadata; + + var immutableMetadataBuilder = new ImmutableStateMetadata.Builder(namespace).version(stateVersionMetadata.version()); + List errors = new ArrayList<>(); + + for (var handlerName : orderedHandlers) { + ImmutableClusterStateHandler handler = handlers.get(handlerName); + try { + Set existingKeys = keysForHandler(existingMetadata, handlerName); + TransformState transformState = handler.transform(immutableState.get(handlerName), new TransformState(state, existingKeys)); + state = transformState.state(); + immutableMetadataBuilder.putHandler(new ImmutableStateHandlerMetadata(handlerName, transformState.keys())); + } catch (Exception e) { + errors.add(format("Error processing %s state change: %s", handler.name(), e.getMessage())); + } + } + + if (errors.isEmpty() == false) { + // Check if we had previous error metadata with version information, don't spam with cluster state updates, if the + // version hasn't been updated. + if (existingMetadata != null + && existingMetadata.errorMetadata() != null + && existingMetadata.errorMetadata().version() >= stateVersionMetadata.version()) { + logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); + + throw new ImmutableClusterStateController.IncompatibleVersionException( + format( + "Not updating error state because version [%s] is less or equal to the last state error version [%s]", + stateVersionMetadata.version(), + existingMetadata.errorMetadata().version() + ) + ); + } + + recordErrorState.accept( + new ImmutableClusterStateController.ImmutableUpdateErrorState( + namespace, + stateVersionMetadata.version(), + errors, + ImmutableStateErrorMetadata.ErrorKind.VALIDATION + ) + ); + logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); + + throw new IllegalStateException("Error processing state change request for " + namespace); + } + + // remove the last error if we had previously encountered any + immutableMetadataBuilder.errorMetadata(null); + + ClusterState.Builder stateBuilder = new ClusterState.Builder(state); + Metadata.Builder metadataBuilder = Metadata.builder(state.metadata()).put(immutableMetadataBuilder.build()); + + return stateBuilder.metadata(metadataBuilder).build(); + } + + private Set keysForHandler(ImmutableStateMetadata immutableStateMetadata, String handlerName) { + if (immutableStateMetadata == null || immutableStateMetadata.handlers().get(handlerName) == null) { + return Collections.emptySet(); + } + + return immutableStateMetadata.handlers().get(handlerName).keys(); + } + + /** + * Immutable cluster state update task executor + * + * @param namespace of the state we are updating + * @param rerouteService instance of {@link RerouteService}, so that we can execute reroute after cluster state is published + */ + public record ImmutableUpdateStateTaskExecutor(String namespace, RerouteService rerouteService) + implements + ClusterStateTaskExecutor { + + @Override + public ClusterState execute(ClusterState currentState, List> taskContexts) + throws Exception { + for (final var taskContext : taskContexts) { + currentState = taskContext.getTask().execute(currentState); + taskContext.success(() -> taskContext.getTask().listener().onResponse(ActionResponse.Empty.INSTANCE)); + } + return currentState; + } + + @Override + public void clusterStatePublished(ClusterState newClusterState) { + rerouteService.reroute( + "reroute after applying immutable cluster state for namespace [" + namespace + "]", + Priority.NORMAL, + ActionListener.wrap( + r -> logger.trace("reroute after applying immutable cluster state for [{}] succeeded", namespace), + e -> logger.debug("reroute after applying immutable cluster state failed", e) + ) + ); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java b/server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java new file mode 100644 index 0000000000000..807197c638d3b --- /dev/null +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java @@ -0,0 +1,57 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.service; + +import org.elasticsearch.Version; +import org.elasticsearch.xcontent.ConstructingObjectParser; +import org.elasticsearch.xcontent.ParseField; +import org.elasticsearch.xcontent.XContentParser; + +/** + * File settings metadata class that holds information about + * versioning and Elasticsearch version compatibility + */ +public class StateVersionMetadata { + public static final ParseField VERSION = new ParseField("version"); + public static final ParseField COMPATIBILITY = new ParseField("compatibility"); + + private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + "immutable_cluster_state_version_metadata", + a -> { + Long updateId = Long.parseLong((String) a[0]); + Version minCompatVersion = Version.fromString((String) a[1]); + + return new StateVersionMetadata(updateId, minCompatVersion); + } + ); + static { + PARSER.declareString(ConstructingObjectParser.constructorArg(), VERSION); + PARSER.declareString(ConstructingObjectParser.constructorArg(), COMPATIBILITY); + } + + private Long version; + private Version compatibleWith; + + public StateVersionMetadata(Long version, Version compatibleWith) { + this.version = version; + this.compatibleWith = compatibleWith; + } + + public static StateVersionMetadata parse(XContentParser parser, Void v) { + return PARSER.apply(parser, v); + } + + public Long version() { + return version; + } + + public Version minCompatibleVersion() { + return compatibleWith; + } +} diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 519ac92ceb494..ab951deb643d3 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -698,7 +698,8 @@ protected Node( client, circuitBreakerService, usageService, - systemIndices + systemIndices, + clusterService ); modules.add(actionModule); @@ -1037,6 +1038,8 @@ protected Node( logger.debug("initializing HTTP handlers ..."); actionModule.initRestHandlers(() -> clusterService.state().nodesIfRecovered()); + logger.debug("initializing operator handlers ..."); + actionModule.initImmutableClusterStateHandlers(pluginsService); logger.info("initialized"); success = true; diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index a1993bd942f91..ebc869a3aba2c 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -116,6 +116,7 @@ public void testSetupRestHandlerContainsKnownBuiltin() { null, null, usageService, + null, null ); actionModule.initRestHandlers(null); @@ -172,6 +173,7 @@ public String getName() { null, null, usageService, + null, null ); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); @@ -221,6 +223,7 @@ public List getRestHandlers( null, null, usageService, + null, null ); actionModule.initRestHandlers(null); @@ -265,6 +268,7 @@ public void test3rdPartyHandlerIsNotInstalled() { null, null, usageService, + null, null ) ); diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java index 042f7f150788a..d31c53adfae63 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java @@ -8,9 +8,17 @@ package org.elasticsearch.action.admin.cluster.settings; +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.XContentTestUtils; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.XContentParseException; import org.elasticsearch.xcontent.XContentParser; @@ -21,6 +29,8 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.mockito.Mockito.mock; public class ClusterUpdateSettingsRequestTests extends ESTestCase { @@ -71,4 +81,43 @@ private static ClusterUpdateSettingsRequest createTestItem() { request.transientSettings(ClusterUpdateSettingsResponseTests.randomClusterSettings(0, 2)); return request; } + + public void testOperatorHandler() throws IOException { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + + TransportClusterUpdateSettingsAction action = new TransportClusterUpdateSettingsAction( + mock(TransportService.class), + mock(ClusterService.class), + mock(ThreadPool.class), + mock(ActionFilters.class), + mock(IndexNameExpressionResolver.class), + clusterSettings + ); + + assertEquals(ImmutableClusterSettingsAction.NAME, action.immutableStateHandlerName().get()); + + String oneSettingJSON = """ + { + "persistent": { + "indices.recovery.max_bytes_per_sec": "25mb", + "cluster": { + "remote": { + "cluster_one": { + "seeds": [ + "127.0.0.1:9300" + ] + } + } + } + } + }"""; + + try (XContentParser parser = createParser(XContentType.JSON.xContent(), oneSettingJSON)) { + ClusterUpdateSettingsRequest parsedRequest = ClusterUpdateSettingsRequest.fromXContent(parser); + assertThat( + action.modifiedKeys(parsedRequest), + containsInAnyOrder("indices.recovery.max_bytes_per_sec", "cluster.remote.cluster_one.seeds") + ); + } + } } diff --git a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java index 9770b1c42dc0f..5bef002fc3511 100644 --- a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java @@ -14,12 +14,14 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.ActionResponse; import org.elasticsearch.action.IndicesRequest; +import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.ActionTestUtils; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.ThreadedActionListener; import org.elasticsearch.action.support.replication.ClusterStateCreationUtils; +import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.NotMasterException; import org.elasticsearch.cluster.block.ClusterBlock; @@ -27,6 +29,8 @@ import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.coordination.FailedToCommitClusterStateException; +import org.elasticsearch.cluster.metadata.ImmutableStateHandlerMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.Metadata; @@ -41,6 +45,7 @@ import org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor; import org.elasticsearch.core.TimeValue; import org.elasticsearch.discovery.MasterNotDiscoveredException; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.indices.TestIndexNameExpressionResolver; import org.elasticsearch.node.NodeClosedException; import org.elasticsearch.rest.RestStatus; @@ -65,6 +70,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; @@ -254,6 +260,63 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) } } + class ImmutableStateAction extends Action { + ImmutableStateAction(String actionName, TransportService transportService, ClusterService clusterService, ThreadPool threadPool) { + super(actionName, transportService, clusterService, threadPool, ThreadPool.Names.SAME); + } + + @Override + protected Optional immutableStateHandlerName() { + return Optional.of("test_immutable_state_action"); + } + } + + class FakeClusterStateUpdateAction extends TransportMasterNodeAction { + FakeClusterStateUpdateAction( + String actionName, + TransportService transportService, + ClusterService clusterService, + ThreadPool threadPool, + String executor + ) { + super( + actionName, + transportService, + clusterService, + threadPool, + new ActionFilters(new HashSet<>()), + ClusterUpdateSettingsRequest::new, + TestIndexNameExpressionResolver.newInstance(), + Response::new, + executor + ); + } + + @Override + protected void masterOperation( + Task task, + ClusterUpdateSettingsRequest request, + ClusterState state, + ActionListener listener + ) {} + + @Override + protected ClusterBlockException checkBlock(ClusterUpdateSettingsRequest request, ClusterState state) { + return null; + } + + @Override + protected Optional immutableStateHandlerName() { + return Optional.of(ImmutableClusterSettingsAction.NAME); + } + + @Override + protected Set modifiedKeys(ClusterUpdateSettingsRequest request) { + Settings allSettings = Settings.builder().put(request.persistentSettings()).put(request.transientSettings()).build(); + return allSettings.keySet(); + } + } + public void testLocalOperationWithoutBlocks() throws ExecutionException, InterruptedException { final boolean masterOperationFailure = randomBoolean(); @@ -686,7 +749,6 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) indexNameExpressionResolver.concreteIndexNamesWithSystemIndexAccess(state, request) ); } - }; PlainActionFuture listener = new PlainActionFuture<>(); @@ -697,6 +759,54 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) assertThat(ex.getCause().getCause(), instanceOf(ClusterBlockException.class)); } + public void testRejectImmutableConflictClusterStateUpdate() { + ImmutableStateHandlerMetadata hmOne = new ImmutableStateHandlerMetadata(ImmutableClusterSettingsAction.NAME, Set.of("a", "b")); + ImmutableStateHandlerMetadata hmThree = new ImmutableStateHandlerMetadata(ImmutableClusterSettingsAction.NAME, Set.of("e", "f")); + ImmutableStateMetadata omOne = ImmutableStateMetadata.builder("namespace_one").putHandler(hmOne).build(); + ImmutableStateMetadata omTwo = ImmutableStateMetadata.builder("namespace_two").putHandler(hmThree).build(); + + Metadata metadata = Metadata.builder().put(omOne).put(omTwo).build(); + + ClusterState clusterState = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); + + Action noHandler = new Action("internal:testAction", transportService, clusterService, threadPool, ThreadPool.Names.SAME); + + assertFalse(noHandler.supportsImmutableState()); + + noHandler = new ImmutableStateAction("internal:testOpAction", transportService, clusterService, threadPool); + + assertTrue(noHandler.supportsImmutableState()); + + // nothing should happen here, since the request doesn't touch any of the immutable state keys + noHandler.validateForImmutableState(new Request(), clusterState); + + ClusterUpdateSettingsRequest request = new ClusterUpdateSettingsRequest().persistentSettings( + Settings.builder().put("a", "a value").build() + ).transientSettings(Settings.builder().put("e", "e value").build()); + + FakeClusterStateUpdateAction action = new FakeClusterStateUpdateAction( + "internal:testClusterSettings", + transportService, + clusterService, + threadPool, + ThreadPool.Names.SAME + ); + + assertTrue(action.supportsImmutableState()); + + assertTrue( + expectThrows(IllegalArgumentException.class, () -> action.validateForImmutableState(request, clusterState)).getMessage() + .contains("with errors: [a] set as read-only by [namespace_one]\n" + "[e] set as read-only by [namespace_two]") + ); + + ClusterUpdateSettingsRequest okRequest = new ClusterUpdateSettingsRequest().persistentSettings( + Settings.builder().put("m", "m value").build() + ).transientSettings(Settings.builder().put("n", "n value").build()); + + // this should just work, no conflicts + action.validateForImmutableState(okRequest, clusterState); + } + private Runnable blockAllThreads(String executorName) throws Exception { final int numberOfThreads = threadPool.info(executorName).getMax(); final EsThreadPoolExecutor executor = (EsThreadPoolExecutor) threadPool.executor(executorName); diff --git a/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java b/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java new file mode 100644 index 0000000000000..a7a3b2eb43dca --- /dev/null +++ b/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java @@ -0,0 +1,483 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.immutablestate.service; + +import org.elasticsearch.Version; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateAckListener; +import org.elasticsearch.cluster.ClusterStateTaskExecutor; +import org.elasticsearch.cluster.metadata.ImmutableStateErrorMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateHandlerMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; +import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.routing.RerouteService; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; +import org.elasticsearch.immutablestate.TransformState; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; + +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; + +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.is; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class ImmutableClusterStateControllerTests extends ESTestCase { + + public void testOperatorController() throws IOException { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + ClusterService clusterService = mock(ClusterService.class); + final ClusterName clusterName = new ClusterName("elasticsearch"); + + ClusterState state = ClusterState.builder(clusterName).build(); + when(clusterService.state()).thenReturn(state); + + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + controller.initHandlers(List.of(new ImmutableClusterSettingsAction(clusterSettings))); + + String testJSON = """ + { + "metadata": { + "version": "1234", + "compatibility": "8.4.0" + }, + "state": { + "cluster_settings": { + "indices.recovery.max_bytes_per_sec": "50mb" + + } + } + """; + + AtomicReference x = new AtomicReference<>(); + + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { + controller.process("operator", parser, (e) -> x.set(e)); + + assertTrue(x.get() instanceof IllegalStateException); + assertEquals("Error processing state change request for operator", x.get().getMessage()); + } + + testJSON = """ + { + "metadata": { + "version": "1234", + "compatibility": "8.4.0" + }, + "state": { + "cluster_settings": { + "indices.recovery.max_bytes_per_sec": "50mb", + "cluster": { + "remote": { + "cluster_one": { + "seeds": [ + "127.0.0.1:9300" + ] + } + } + } + } + } + } + """; + + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { + controller.process("operator", parser, (e) -> { + if (e != null) { + fail("Should not fail"); + } + }); + } + } + + public void testUpdateStateTasks() throws Exception { + ClusterService clusterService = mock(ClusterService.class); + RerouteService rerouteService = mock(RerouteService.class); + + when(clusterService.getRerouteService()).thenReturn(rerouteService); + ClusterState state = ClusterState.builder(new ClusterName("test")).build(); + + ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor taskExecutor = + new ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor("test", clusterService.getRerouteService()); + + AtomicBoolean successCalled = new AtomicBoolean(false); + + ImmutableStateUpdateStateTask task = spy( + new ImmutableStateUpdateStateTask( + "test", + null, + Collections.emptyMap(), + Collections.emptySet(), + (errorState) -> {}, + new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty empty) {} + + @Override + public void onFailure(Exception e) {} + } + ) + ); + + doReturn(state).when(task).execute(any()); + + ClusterStateTaskExecutor.TaskContext taskContext = new ClusterStateTaskExecutor.TaskContext<>() { + @Override + public ImmutableStateUpdateStateTask getTask() { + return task; + } + + @Override + public void success(Runnable onPublicationSuccess) { + onPublicationSuccess.run(); + successCalled.set(true); + } + + @Override + public void success(Consumer publishedStateConsumer) {} + + @Override + public void success(Runnable onPublicationSuccess, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void success(Consumer publishedStateConsumer, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void onFailure(Exception failure) {} + }; + + ClusterState newState = taskExecutor.execute(state, List.of(taskContext)); + assertEquals(state, newState); + assertTrue(successCalled.get()); + verify(task, times(1)).execute(any()); + + taskExecutor.clusterStatePublished(state); + verify(rerouteService, times(1)).reroute(anyString(), any(), any()); + } + + public void testErrorStateTask() throws Exception { + ClusterState state = ClusterState.builder(new ClusterName("test")).build(); + + ImmutableStateUpdateErrorTask task = spy( + new ImmutableStateUpdateErrorTask( + new ImmutableClusterStateController.ImmutableUpdateErrorState( + "test", + 1L, + List.of("some parse error", "some io error"), + ImmutableStateErrorMetadata.ErrorKind.PARSING + ), + new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty empty) {} + + @Override + public void onFailure(Exception e) {} + } + ) + ); + + ImmutableStateUpdateErrorTask.ImmutableUpdateErrorTaskExecutor.TaskContext taskContext = + new ImmutableStateUpdateErrorTask.ImmutableUpdateErrorTaskExecutor.TaskContext<>() { + @Override + public ImmutableStateUpdateErrorTask getTask() { + return task; + } + + @Override + public void success(Runnable onPublicationSuccess) { + onPublicationSuccess.run(); + } + + @Override + public void success(Consumer publishedStateConsumer) {} + + @Override + public void success(Runnable onPublicationSuccess, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void success(Consumer publishedStateConsumer, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void onFailure(Exception failure) {} + }; + + ImmutableStateUpdateErrorTask.ImmutableUpdateErrorTaskExecutor executor = + new ImmutableStateUpdateErrorTask.ImmutableUpdateErrorTaskExecutor(); + + ClusterState newState = executor.execute(state, List.of(taskContext)); + + verify(task, times(1)).execute(any()); + + ImmutableStateMetadata operatorMetadata = newState.metadata().immutableStateMetadata().get("test"); + assertNotNull(operatorMetadata); + assertNotNull(operatorMetadata.errorMetadata()); + assertEquals(1L, (long) operatorMetadata.errorMetadata().version()); + assertEquals(ImmutableStateErrorMetadata.ErrorKind.PARSING, operatorMetadata.errorMetadata().errorKind()); + assertThat(operatorMetadata.errorMetadata().errors(), contains("some parse error", "some io error")); + } + + public void testUpdateTaskDuplicateError() { + ImmutableClusterStateHandler dummy = new ImmutableClusterStateHandler<>() { + @Override + public String name() { + return "one"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + throw new Exception("anything"); + } + }; + + ImmutableStateUpdateStateTask task = spy( + new ImmutableStateUpdateStateTask( + "namespace_one", + new ImmutableClusterStateController.Package(Map.of("one", "two"), new StateVersionMetadata(1L, Version.CURRENT)), + Map.of("one", dummy), + List.of(dummy.name()), + (errorState) -> {}, + new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty empty) {} + + @Override + public void onFailure(Exception e) {} + } + ) + ); + + ImmutableStateHandlerMetadata hmOne = new ImmutableStateHandlerMetadata("one", Set.of("a", "b")); + ImmutableStateErrorMetadata emOne = new ImmutableStateErrorMetadata( + 1L, + ImmutableStateErrorMetadata.ErrorKind.VALIDATION, + List.of("Test error 1", "Test error 2") + ); + + ImmutableStateMetadata operatorMetadata = ImmutableStateMetadata.builder("namespace_one") + .errorMetadata(emOne) + .version(1L) + .putHandler(hmOne) + .build(); + + Metadata metadata = Metadata.builder().put(operatorMetadata).build(); + ClusterState state = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); + + // We exit on duplicate errors before we update the cluster state error metadata + assertEquals( + "Not updating error state because version [1] is less or equal to the last state error version [1]", + expectThrows(ImmutableClusterStateController.IncompatibleVersionException.class, () -> task.execute(state)).getMessage() + ); + + emOne = new ImmutableStateErrorMetadata( + 0L, + ImmutableStateErrorMetadata.ErrorKind.VALIDATION, + List.of("Test error 1", "Test error 2") + ); + + // If we are writing with older error metadata, we should get proper IllegalStateException + operatorMetadata = ImmutableStateMetadata.builder("namespace_one").errorMetadata(emOne).version(0L).putHandler(hmOne).build(); + + metadata = Metadata.builder().put(operatorMetadata).build(); + ClusterState newState = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); + + // We exit on duplicate errors before we update the cluster state error metadata + assertEquals( + "Error processing state change request for namespace_one", + expectThrows(IllegalStateException.class, () -> task.execute(newState)).getMessage() + ); + } + + public void testCheckMetadataVersion() { + ImmutableStateMetadata operatorMetadata = ImmutableStateMetadata.builder("test").version(123L).build(); + + assertTrue( + ImmutableClusterStateController.checkMetadataVersion( + operatorMetadata, + new StateVersionMetadata(124L, Version.CURRENT), + (e) -> {} + ) + ); + + AtomicReference x = new AtomicReference<>(); + + assertFalse( + ImmutableClusterStateController.checkMetadataVersion( + operatorMetadata, + new StateVersionMetadata(123L, Version.CURRENT), + (e) -> x.set(e) + ) + ); + + assertTrue(x.get() instanceof ImmutableClusterStateController.IncompatibleVersionException); + assertTrue(x.get().getMessage().contains("is less or equal to the current metadata version")); + + assertFalse( + ImmutableClusterStateController.checkMetadataVersion( + operatorMetadata, + new StateVersionMetadata(124L, Version.fromId(Version.CURRENT.id + 1)), + (e) -> x.set(e) + ) + ); + + assertEquals(ImmutableClusterStateController.IncompatibleVersionException.class, x.get().getClass()); + assertTrue(x.get().getMessage().contains("is not compatible with this Elasticsearch node")); + } + + public void testHandlerOrdering() { + ImmutableClusterStateHandler oh1 = new ImmutableClusterStateHandler<>() { + @Override + public String name() { + return "one"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + return null; + } + + @Override + public Collection dependencies() { + return List.of("two", "three"); + } + }; + + ImmutableClusterStateHandler oh2 = new ImmutableClusterStateHandler<>() { + @Override + public String name() { + return "two"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + return null; + } + }; + + ImmutableClusterStateHandler oh3 = new ImmutableClusterStateHandler<>() { + @Override + public String name() { + return "three"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + return null; + } + + @Override + public Collection dependencies() { + return List.of("two"); + } + }; + + ClusterService clusterService = mock(ClusterService.class); + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + + controller.initHandlers(List.of(oh1, oh2, oh3)); + Collection ordered = controller.orderedStateHandlers(Set.of("one", "two", "three")); + assertThat(ordered, contains("two", "three", "one")); + + // assure that we bail on unknown handler + assertEquals( + "Unknown settings definition type: four", + expectThrows(IllegalStateException.class, () -> controller.orderedStateHandlers(Set.of("one", "two", "three", "four"))) + .getMessage() + ); + + // assure that we bail on missing dependency link + assertEquals( + "Missing settings dependency definition: one -> three", + expectThrows(IllegalStateException.class, () -> controller.orderedStateHandlers(Set.of("one", "two"))).getMessage() + ); + + // Change the second handler so that we create cycle + oh2 = new ImmutableClusterStateHandler<>() { + @Override + public String name() { + return "two"; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + return null; + } + + @Override + public Collection dependencies() { + return List.of("one"); + } + }; + + controller.initHandlers(List.of(oh1, oh2)); + assertThat( + expectThrows(IllegalStateException.class, () -> controller.orderedStateHandlers(Set.of("one", "two"))).getMessage(), + anyOf( + is("Cycle found in settings dependencies: one -> two -> one"), + is("Cycle found in settings dependencies: two -> one -> two") + ) + ); + } + + public void testDuplicateHandlerNames() { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + ClusterService clusterService = mock(ClusterService.class); + final ClusterName clusterName = new ClusterName("elasticsearch"); + + ClusterState state = ClusterState.builder(clusterName).build(); + when(clusterService.state()).thenReturn(state); + + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + + assertTrue( + expectThrows( + IllegalStateException.class, + () -> controller.initHandlers(List.of(new ImmutableClusterSettingsAction(clusterSettings), new TestHandler())) + ).getMessage().startsWith("Duplicate key cluster_settings") + ); + } + + class TestHandler implements ImmutableClusterStateHandler { + + @Override + public String name() { + return ImmutableClusterSettingsAction.NAME; + } + + @Override + public TransformState transform(Object source, TransformState prevState) throws Exception { + return prevState; + } + } +} diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index a9c776f100623..5f0a77f0f6ce2 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -11,8 +11,12 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.immutablestate.TransformState; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; +import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.NamedXContentRegistry; @@ -39,10 +43,12 @@ import org.elasticsearch.xpack.core.ilm.UnfollowAction; import org.elasticsearch.xpack.core.ilm.WaitForSnapshotAction; +import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.mockito.Mockito.mock; @@ -209,4 +215,74 @@ public void testActionAddRemove() throws Exception { ilmMetadata = updatedState.state().metadata().custom(IndexLifecycleMetadata.TYPE, IndexLifecycleMetadata.EMPTY); assertThat(ilmMetadata.getPolicyMetadatas().keySet(), containsInAnyOrder("my_timeseries_lifecycle2")); } + + public void testOperatorController() throws IOException { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + ClusterService clusterService = mock(ClusterService.class); + final ClusterName clusterName = new ClusterName("elasticsearch"); + + ClusterState state = ClusterState.builder(clusterName).build(); + when(clusterService.state()).thenReturn(state); + + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + controller.initHandlers(List.of(new ImmutableClusterSettingsAction(clusterSettings))); + + String testJSON = """ + { + "metadata": { + "version": "1234", + "compatibility": "8.4.0" + }, + "state": { + "cluster_settings": { + "indices.recovery.max_bytes_per_sec": "50mb" + }, + "ilm": { + "my_timeseries_lifecycle": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + } + }, + "delete": { + "min_age": "30s", + "actions": { + } + } + } + } + } + } + }"""; + + AtomicReference x = new AtomicReference<>(); + + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { + controller.process("operator", parser, (e) -> x.set(e)); + + assertTrue(x.get() instanceof IllegalStateException); + assertEquals("Error processing state change request for operator", x.get().getMessage()); + } + + Client client = mock(Client.class); + when(client.settings()).thenReturn(Settings.EMPTY); + + XPackLicenseState licenseState = mock(XPackLicenseState.class); + + controller.initHandlers( + List.of( + new ImmutableClusterSettingsAction(clusterSettings), + new ImmutableLifecycleAction(xContentRegistry(), client, licenseState) + ) + ); + + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { + controller.process("operator", parser, (e) -> { + if (e != null) { + fail("Should not fail"); + } + }); + } + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index 65d4267f541ec..604a521da9a8e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -778,6 +778,7 @@ public void testSecurityRestHandlerInterceptorCanBeInstalled() throws IllegalAcc null, null, usageService, + null, null ); actionModule.initRestHandlers(null); From 4f543935d95abae4bdedaf0cf652be6a35536012 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 5 Jul 2022 10:34:19 -0400 Subject: [PATCH 06/27] Fix test to run the transforms --- .../ImmutableILMStateControllerTests.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index 5f0a77f0f6ce2..8ef6a4b3d4717 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -11,12 +11,15 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateAckListener; +import org.elasticsearch.cluster.ClusterStateTaskExecutor; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.immutablestate.TransformState; import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; +import org.elasticsearch.immutablestate.service.ImmutableStateUpdateStateTask; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.NamedXContentRegistry; @@ -42,6 +45,7 @@ import org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType; import org.elasticsearch.xpack.core.ilm.UnfollowAction; import org.elasticsearch.xpack.core.ilm.WaitForSnapshotAction; +import org.mockito.stubbing.Answer; import java.io.IOException; import java.util.ArrayList; @@ -49,8 +53,12 @@ import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -277,6 +285,45 @@ public void testOperatorController() throws IOException { ) ); + doAnswer((Answer) invocation -> { + Object[] args = invocation.getArguments(); + + if ((args[3] instanceof ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor) == false) { + fail("Should have gotten a state update task to execute, instead got: " + args[3].getClass().getName()); + } + + ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor task = + (ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor) args[3]; + + ClusterStateTaskExecutor.TaskContext context = new ClusterStateTaskExecutor.TaskContext<>() { + @Override + public ImmutableStateUpdateStateTask getTask() { + return (ImmutableStateUpdateStateTask) args[1]; + } + + @Override + public void success(Runnable onPublicationSuccess) {} + + @Override + public void success(Consumer publishedStateConsumer) {} + + @Override + public void success(Runnable onPublicationSuccess, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void success(Consumer publishedStateConsumer, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void onFailure(Exception failure) { + fail("Shouldn't fail here"); + } + }; + + task.execute(state, List.of(context)); + + return null; + }).when(clusterService).submitStateUpdateTask(anyString(), any(), any(), any()); + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { controller.process("operator", parser, (e) -> { if (e != null) { From 16c3272a424c984f2cbb315a342926f0f9bff206 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 5 Jul 2022 20:54:21 -0400 Subject: [PATCH 07/27] Refactor to split out parsing in handlers --- .../org/elasticsearch/common/util/Maps.java | 14 -- .../ImmutableClusterStateHandler.java | 20 ++- .../ImmutableClusterSettingsAction.java | 16 +- .../ImmutableClusterStateController.java | 86 +++++---- .../ImmutableStateUpdateStateTask.java | 12 +- ...rsionMetadata.java => PackageVersion.java} | 21 +-- .../ImmutableClusterStateHandlerTests.java | 59 +------ .../ImmutableClusterStateControllerTests.java | 53 ++++-- .../ilm/action/ImmutableLifecycleAction.java | 37 ++-- .../ImmutableILMStateControllerTests.java | 167 ++++++++++++++---- 10 files changed, 278 insertions(+), 207 deletions(-) rename server/src/main/java/org/elasticsearch/immutablestate/service/{StateVersionMetadata.java => PackageVersion.java} (70%) diff --git a/server/src/main/java/org/elasticsearch/common/util/Maps.java b/server/src/main/java/org/elasticsearch/common/util/Maps.java index 5b02f9af64a06..f0a7f464ed8f3 100644 --- a/server/src/main/java/org/elasticsearch/common/util/Maps.java +++ b/server/src/main/java/org/elasticsearch/common/util/Maps.java @@ -284,18 +284,4 @@ static int capacity(int expectedSize) { return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0); } - /** - * Convenience method to convert the passed in input object as a map with String keys. - * - * @param input the input passed into the operator handler after parsing the content - * @return - */ - @SuppressWarnings("unchecked") - public static Map asMap(Object input) { - if (input instanceof Map source) { - return (Map) source; - } - throw new IllegalStateException("Unsupported input format"); - } - } diff --git a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandler.java b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandler.java index f8428a9423c51..5ab1023d9c0f1 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandler.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandler.java @@ -10,7 +10,9 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.master.MasterNodeRequest; +import org.elasticsearch.xcontent.XContentParser; +import java.io.IOException; import java.util.Collection; import java.util.Collections; @@ -35,7 +37,6 @@ public interface ImmutableClusterStateHandler { * cluster state update and the cluster state is typically supplied as a combined content, * unlike the REST handlers. This name must match a desired content key name in the combined * cluster state update, e.g. "ilm" or "cluster_settings" (for persistent cluster settings update). - *

* * @return a String with the handler name, e.g "ilm". */ @@ -51,7 +52,6 @@ public interface ImmutableClusterStateHandler { * state in one go. For that reason, we supply a wrapper class to the cluster state called * {@link TransformState}, which contains the current cluster state as well as any previous keys * set by this handler on prior invocation. - *

* * @param source The parsed information specific to this handler from the combined cluster state content * @param prevState The previous cluster state and keys set by this handler (if any) @@ -70,7 +70,6 @@ public interface ImmutableClusterStateHandler { * to any immutable handler to declare other immutable state handlers it depends on. Given dependencies exist, * the ImmutableClusterStateController will order those handlers such that the handlers that are dependent * on are processed first. - *

* * @return a collection of immutable state handler names */ @@ -85,7 +84,6 @@ default Collection dependencies() { * All implementations of {@link ImmutableClusterStateHandler} should call the request validate method, by calling this default * implementation. To aid in any special validation logic that may need to be implemented by the immutable cluster state handler * we provide this convenience method. - *

* * @param request the master node request that we base this immutable state handler on */ @@ -95,4 +93,18 @@ default void validate(MasterNodeRequest request) { throw new IllegalStateException("Validation error", exception); } } + + /** + * The parse content method which is called during parsing of file based content. + * + *

+ * The immutable state can be provided as XContent, which means that each handler needs + * to implement a method to convert an XContent to an object it can consume later in + * transform + * + * @param parser the XContent parser we are parsing from + * @return + * @throws IOException + */ + T fromXContent(XContentParser parser) throws IOException; } diff --git a/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java b/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java index cd82f17ecfa8f..249914276d83d 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/action/ImmutableClusterSettingsAction.java @@ -15,22 +15,22 @@ import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.immutablestate.TransformState; +import org.elasticsearch.xcontent.XContentParser; +import java.io.IOException; import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; -import static org.elasticsearch.common.util.Maps.asMap; - /** * This Action is the immutable state save version of RestClusterUpdateSettingsAction *

* It is used by the ImmutableClusterStateController to update the persistent cluster settings. * Since transient cluster settings are deprecated, this action doesn't support updating transient cluster settings. */ -public class ImmutableClusterSettingsAction implements ImmutableClusterStateHandler { +public class ImmutableClusterSettingsAction implements ImmutableClusterStateHandler> { public static final String NAME = "cluster_settings"; @@ -49,11 +49,12 @@ public String name() { private ClusterUpdateSettingsRequest prepare(Object input, Set previouslySet) { final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = Requests.clusterUpdateSettingsRequest(); - Map source = asMap(input); Map persistentSettings = new HashMap<>(); Set toDelete = new HashSet<>(previouslySet); - source.forEach((k, v) -> { + Map settings = (Map) input; + + settings.forEach((k, v) -> { persistentSettings.put(k, v); toDelete.remove(k); }); @@ -87,4 +88,9 @@ public TransformState transform(Object input, TransformState prevState) { return new TransformState(state, currentKeys); } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } } diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java index f0ca10473fec2..8edc93261597a 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java @@ -19,11 +19,13 @@ import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Priority; +import org.elasticsearch.core.Tuple; import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; import org.elasticsearch.xcontent.ConstructingObjectParser; import org.elasticsearch.xcontent.ParseField; import org.elasticsearch.xcontent.XContentParser; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -35,21 +37,45 @@ import static org.elasticsearch.core.Strings.format; /** - * Controller class for applying file based settings to ClusterState. + * Controller class for applying immutable state to ClusterState. + *

* This class contains the logic about validation, ordering and applying of - * the cluster state specified in a file. + * the cluster state specified in a file or through plugins/modules. */ public class ImmutableClusterStateController { private static final Logger logger = LogManager.getLogger(ImmutableClusterStateController.class); - public static final String SETTINGS = "settings"; - public static final String METADATA = "metadata"; + public static final ParseField STATE_FIELD = new ParseField("state"); + public static final ParseField METADATA_FIELD = new ParseField("metadata"); Map> handlers = null; final ClusterService clusterService; + @SuppressWarnings("unchecked") + private final ConstructingObjectParser packageParser = new ConstructingObjectParser<>("immutable_cluster_package", a -> { + List> tuples = (List>) a[0]; + Map stateMap = new HashMap<>(); + for (var tuple : tuples) { + stateMap.put(tuple.v1(), tuple.v2()); + } + + return new Package(stateMap, (PackageVersion) a[1]); + }); + + /** + * Controller class for saving immutable ClusterState. + * @param clusterService for fetching and saving the modified state + */ public ImmutableClusterStateController(ClusterService clusterService) { this.clusterService = clusterService; + packageParser.declareNamedObjects(ConstructingObjectParser.constructorArg(), (p, c, name) -> { + if (handlers.containsKey(name) == false) { + throw new IllegalStateException("Missing handler definition for content key [" + name + "]"); + } + p.nextToken(); + return new Tuple<>(name, handlers.get(name).fromXContent(p)); + }, STATE_FIELD); + packageParser.declareObject(ConstructingObjectParser.constructorArg(), PackageVersion::parse, METADATA_FIELD); } /** @@ -65,35 +91,10 @@ public void initHandlers(List> handlerList) { * A package class containing the composite immutable cluster state *

* Apart from the cluster state we want to store as immutable, the package requires that - * you supply the version metadata. This version metadata (see {@link StateVersionMetadata}) is checked to ensure + * you supply the version metadata. This version metadata (see {@link PackageVersion}) is checked to ensure * that the update is safe, and it's not unnecessarily repeated. */ - public static class Package { - public static final ParseField STATE_FIELD = new ParseField("state"); - public static final ParseField METADATA_FIELD = new ParseField("metadata"); - @SuppressWarnings("unchecked") - private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - "immutable_cluster_state", - a -> new Package((Map) a[0], (StateVersionMetadata) a[1]) - ); - static { - PARSER.declareObject(ConstructingObjectParser.constructorArg(), (p, c) -> p.map(), STATE_FIELD); - PARSER.declareObject(ConstructingObjectParser.constructorArg(), StateVersionMetadata::parse, METADATA_FIELD); - } - - Map state; - StateVersionMetadata metadata; - - /** - * A package class containing the composite immutable cluster state - * @param state a {@link Map} of immutable state handler name and data - * @param metadata a version metadata - */ - public Package(Map state, StateVersionMetadata metadata) { - this.state = state; - this.metadata = metadata; - } - } + public record Package(Map state, PackageVersion metadata) {} /** * Saves an immutable cluster state for a given 'namespace' from {@link XContentParser} @@ -108,7 +109,7 @@ public void process(String namespace, XContentParser parser, Consumer Package immutableStatePackage; try { - immutableStatePackage = Package.PARSER.apply(parser, null); + immutableStatePackage = packageParser.apply(parser, null); } catch (Exception e) { List errors = List.of(e.getMessage()); recordErrorState(new ImmutableUpdateErrorState(namespace, -1L, errors, ImmutableStateErrorMetadata.ErrorKind.PARSING)); @@ -132,7 +133,7 @@ public void process(String namespace, XContentParser parser, Consumer */ public void process(String namespace, Package immutableStateFilePackage, Consumer errorListener) { Map immutableState = immutableStateFilePackage.state; - StateVersionMetadata stateVersionMetadata = immutableStateFilePackage.metadata; + PackageVersion packageVersion = immutableStateFilePackage.metadata; LinkedHashSet orderedHandlers; try { @@ -140,12 +141,7 @@ public void process(String namespace, Package immutableStateFilePackage, Consume } catch (Exception e) { List errors = List.of(e.getMessage()); recordErrorState( - new ImmutableUpdateErrorState( - namespace, - stateVersionMetadata.version(), - errors, - ImmutableStateErrorMetadata.ErrorKind.PARSING - ) + new ImmutableUpdateErrorState(namespace, packageVersion.version(), errors, ImmutableStateErrorMetadata.ErrorKind.PARSING) ); logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); @@ -155,7 +151,7 @@ public void process(String namespace, Package immutableStateFilePackage, Consume ClusterState state = clusterService.state(); ImmutableStateMetadata existingMetadata = state.metadata().immutableStateMetadata().get(namespace); - if (checkMetadataVersion(existingMetadata, stateVersionMetadata, errorListener) == false) { + if (checkMetadataVersion(existingMetadata, packageVersion, errorListener) == false) { return; } @@ -190,27 +186,27 @@ public void onFailure(Exception e) { // package private for testing static boolean checkMetadataVersion( ImmutableStateMetadata existingMetadata, - StateVersionMetadata stateVersionMetadata, + PackageVersion packageVersion, Consumer errorListener ) { - if (Version.CURRENT.before(stateVersionMetadata.minCompatibleVersion())) { + if (Version.CURRENT.before(packageVersion.minCompatibleVersion())) { errorListener.accept( new IncompatibleVersionException( format( "Cluster state version [%s] is not compatible with this Elasticsearch node", - stateVersionMetadata.minCompatibleVersion() + packageVersion.minCompatibleVersion() ) ) ); return false; } - if (existingMetadata != null && existingMetadata.version() >= stateVersionMetadata.version()) { + if (existingMetadata != null && existingMetadata.version() >= packageVersion.version()) { errorListener.accept( new IncompatibleVersionException( format( "Not updating cluster state because version [%s] is less or equal to the current metadata version [%s]", - stateVersionMetadata.version(), + packageVersion.version(), existingMetadata.version() ) ) diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java index f50edd24ecafd..11e3e1362476d 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableStateUpdateStateTask.java @@ -74,10 +74,10 @@ ActionListener listener() { protected ClusterState execute(ClusterState state) { ImmutableStateMetadata existingMetadata = state.metadata().immutableStateMetadata().get(namespace); - Map immutableState = immutableStatePackage.state; - StateVersionMetadata stateVersionMetadata = immutableStatePackage.metadata; + Map immutableState = immutableStatePackage.state(); + PackageVersion packageVersion = immutableStatePackage.metadata(); - var immutableMetadataBuilder = new ImmutableStateMetadata.Builder(namespace).version(stateVersionMetadata.version()); + var immutableMetadataBuilder = new ImmutableStateMetadata.Builder(namespace).version(packageVersion.version()); List errors = new ArrayList<>(); for (var handlerName : orderedHandlers) { @@ -97,13 +97,13 @@ protected ClusterState execute(ClusterState state) { // version hasn't been updated. if (existingMetadata != null && existingMetadata.errorMetadata() != null - && existingMetadata.errorMetadata().version() >= stateVersionMetadata.version()) { + && existingMetadata.errorMetadata().version() >= packageVersion.version()) { logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); throw new ImmutableClusterStateController.IncompatibleVersionException( format( "Not updating error state because version [%s] is less or equal to the last state error version [%s]", - stateVersionMetadata.version(), + packageVersion.version(), existingMetadata.errorMetadata().version() ) ); @@ -112,7 +112,7 @@ protected ClusterState execute(ClusterState state) { recordErrorState.accept( new ImmutableClusterStateController.ImmutableUpdateErrorState( namespace, - stateVersionMetadata.version(), + packageVersion.version(), errors, ImmutableStateErrorMetadata.ErrorKind.VALIDATION ) diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java b/server/src/main/java/org/elasticsearch/immutablestate/service/PackageVersion.java similarity index 70% rename from server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java rename to server/src/main/java/org/elasticsearch/immutablestate/service/PackageVersion.java index 807197c638d3b..95a48112515da 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/service/StateVersionMetadata.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/PackageVersion.java @@ -17,40 +17,29 @@ * File settings metadata class that holds information about * versioning and Elasticsearch version compatibility */ -public class StateVersionMetadata { +public record PackageVersion(Long version, Version compatibleWith) { public static final ParseField VERSION = new ParseField("version"); public static final ParseField COMPATIBILITY = new ParseField("compatibility"); - private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( + private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( "immutable_cluster_state_version_metadata", a -> { Long updateId = Long.parseLong((String) a[0]); Version minCompatVersion = Version.fromString((String) a[1]); - return new StateVersionMetadata(updateId, minCompatVersion); + return new PackageVersion(updateId, minCompatVersion); } ); + static { PARSER.declareString(ConstructingObjectParser.constructorArg(), VERSION); PARSER.declareString(ConstructingObjectParser.constructorArg(), COMPATIBILITY); } - private Long version; - private Version compatibleWith; - - public StateVersionMetadata(Long version, Version compatibleWith) { - this.version = version; - this.compatibleWith = compatibleWith; - } - - public static StateVersionMetadata parse(XContentParser parser, Void v) { + public static PackageVersion parse(XContentParser parser, Void v) { return PARSER.apply(parser, v); } - public Long version() { - return version; - } - public Version minCompatibleVersion() { return compatibleWith; } diff --git a/server/src/test/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerTests.java b/server/src/test/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerTests.java index 86eddbaadad17..c46ac036eb823 100644 --- a/server/src/test/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerTests.java +++ b/server/src/test/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerTests.java @@ -10,18 +10,11 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.master.MasterNodeRequest; -import org.elasticsearch.common.util.Maps; -import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.indices.settings.InternalOrPrivateSettingsPlugin; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.XContentParser; -import org.elasticsearch.xcontent.XContentParserConfiguration; -import org.elasticsearch.xcontent.XContentType; import java.io.IOException; -import java.util.Map; - -import static org.hamcrest.Matchers.containsInAnyOrder; public class ImmutableClusterStateHandlerTests extends ESTestCase { public void testValidation() { @@ -35,6 +28,11 @@ public String name() { public TransformState transform(Object source, TransformState prevState) throws Exception { return prevState; } + + @Override + public ValidRequest fromXContent(XContentParser parser) throws IOException { + return new ValidRequest(); + } }; handler.validate(new ValidRequest()); @@ -44,53 +42,6 @@ public TransformState transform(Object source, TransformState prevState) throws ); } - public void testAsMapAndFromMap() throws IOException { - String someJSON = """ - { - "persistent": { - "indices.recovery.max_bytes_per_sec": "25mb", - "cluster": { - "remote": { - "cluster_one": { - "seeds": [ - "127.0.0.1:9300" - ] - } - } - } - } - }"""; - - ImmutableClusterStateHandler persistentHandler = new ImmutableClusterStateHandler<>() { - @Override - public String name() { - return "persistent"; - } - - @Override - public TransformState transform(Object source, TransformState prevState) throws Exception { - return prevState; - } - }; - - try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, someJSON)) { - Map originalMap = parser.map(); - - Map internalHandlerMap = Maps.asMap(originalMap.get(persistentHandler.name())); - assertThat(internalHandlerMap.keySet(), containsInAnyOrder("indices.recovery.max_bytes_per_sec", "cluster")); - assertEquals( - "Unsupported input format", - expectThrows(IllegalStateException.class, () -> Maps.asMap(Integer.valueOf(123))).getMessage() - ); - - try (XContentParser newParser = XContentHelper.mapToXContentParser(XContentParserConfiguration.EMPTY, originalMap)) { - Map newMap = newParser.map(); - - assertThat(newMap.keySet(), containsInAnyOrder("persistent")); - } - } - } - static class ValidRequest extends MasterNodeRequest { @Override public ActionRequestValidationException validate() { diff --git a/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java b/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java index a7a3b2eb43dca..60f841895b72b 100644 --- a/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java +++ b/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java @@ -11,7 +11,6 @@ import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateAckListener; @@ -249,7 +248,7 @@ public void onFailure(Exception failure) {} } public void testUpdateTaskDuplicateError() { - ImmutableClusterStateHandler dummy = new ImmutableClusterStateHandler<>() { + ImmutableClusterStateHandler> dummy = new ImmutableClusterStateHandler<>() { @Override public String name() { return "one"; @@ -259,12 +258,17 @@ public String name() { public TransformState transform(Object source, TransformState prevState) throws Exception { throw new Exception("anything"); } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } }; ImmutableStateUpdateStateTask task = spy( new ImmutableStateUpdateStateTask( "namespace_one", - new ImmutableClusterStateController.Package(Map.of("one", "two"), new StateVersionMetadata(1L, Version.CURRENT)), + new ImmutableClusterStateController.Package(Map.of("one", "two"), new PackageVersion(1L, Version.CURRENT)), Map.of("one", dummy), List.of(dummy.name()), (errorState) -> {}, @@ -323,11 +327,7 @@ public void testCheckMetadataVersion() { ImmutableStateMetadata operatorMetadata = ImmutableStateMetadata.builder("test").version(123L).build(); assertTrue( - ImmutableClusterStateController.checkMetadataVersion( - operatorMetadata, - new StateVersionMetadata(124L, Version.CURRENT), - (e) -> {} - ) + ImmutableClusterStateController.checkMetadataVersion(operatorMetadata, new PackageVersion(124L, Version.CURRENT), (e) -> {}) ); AtomicReference x = new AtomicReference<>(); @@ -335,7 +335,7 @@ public void testCheckMetadataVersion() { assertFalse( ImmutableClusterStateController.checkMetadataVersion( operatorMetadata, - new StateVersionMetadata(123L, Version.CURRENT), + new PackageVersion(123L, Version.CURRENT), (e) -> x.set(e) ) ); @@ -346,7 +346,7 @@ public void testCheckMetadataVersion() { assertFalse( ImmutableClusterStateController.checkMetadataVersion( operatorMetadata, - new StateVersionMetadata(124L, Version.fromId(Version.CURRENT.id + 1)), + new PackageVersion(124L, Version.fromId(Version.CURRENT.id + 1)), (e) -> x.set(e) ) ); @@ -356,7 +356,7 @@ public void testCheckMetadataVersion() { } public void testHandlerOrdering() { - ImmutableClusterStateHandler oh1 = new ImmutableClusterStateHandler<>() { + ImmutableClusterStateHandler> oh1 = new ImmutableClusterStateHandler<>() { @Override public String name() { return "one"; @@ -371,9 +371,14 @@ public TransformState transform(Object source, TransformState prevState) throws public Collection dependencies() { return List.of("two", "three"); } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } }; - ImmutableClusterStateHandler oh2 = new ImmutableClusterStateHandler<>() { + ImmutableClusterStateHandler> oh2 = new ImmutableClusterStateHandler<>() { @Override public String name() { return "two"; @@ -383,9 +388,14 @@ public String name() { public TransformState transform(Object source, TransformState prevState) throws Exception { return null; } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } }; - ImmutableClusterStateHandler oh3 = new ImmutableClusterStateHandler<>() { + ImmutableClusterStateHandler> oh3 = new ImmutableClusterStateHandler<>() { @Override public String name() { return "three"; @@ -400,6 +410,11 @@ public TransformState transform(Object source, TransformState prevState) throws public Collection dependencies() { return List.of("two"); } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } }; ClusterService clusterService = mock(ClusterService.class); @@ -438,6 +453,11 @@ public TransformState transform(Object source, TransformState prevState) throws public Collection dependencies() { return List.of("one"); } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } }; controller.initHandlers(List.of(oh1, oh2)); @@ -468,7 +488,7 @@ public void testDuplicateHandlerNames() { ); } - class TestHandler implements ImmutableClusterStateHandler { + class TestHandler implements ImmutableClusterStateHandler> { @Override public String name() { @@ -479,5 +499,10 @@ public String name() { public TransformState transform(Object source, TransformState prevState) throws Exception { return prevState; } + + @Override + public Map fromXContent(XContentParser parser) throws IOException { + return parser.map(); + } } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java index 68b6d994ae63f..0cb6b115d158e 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/ImmutableLifecycleAction.java @@ -28,7 +28,6 @@ import java.util.Set; import java.util.stream.Collectors; -import static org.elasticsearch.common.util.Maps.asMap; import static org.elasticsearch.common.xcontent.XContentHelper.mapToXContentParser; /** @@ -38,7 +37,7 @@ * Internally it uses {@link TransportPutLifecycleAction} and * {@link TransportDeleteLifecycleAction} to add, update and delete ILM policies. */ -public class ImmutableLifecycleAction implements ImmutableClusterStateHandler { +public class ImmutableLifecycleAction implements ImmutableClusterStateHandler> { private final NamedXContentRegistry xContentRegistry; private final Client client; @@ -60,18 +59,12 @@ public String name() { @SuppressWarnings("unchecked") public Collection prepare(Object input) throws IOException { List result = new ArrayList<>(); + List policies = (List) input; - Map source = asMap(input); - - for (String name : source.keySet()) { - Map content = (Map) source.get(name); - var config = XContentParserConfiguration.EMPTY.withRegistry(LifecyclePolicyConfig.DEFAULT_X_CONTENT_REGISTRY); - try (XContentParser parser = mapToXContentParser(config, content)) { - LifecyclePolicy policy = LifecyclePolicy.parse(parser, name); - PutLifecycleAction.Request request = new PutLifecycleAction.Request(policy); - validate(request); - result.add(request); - } + for (var policy : policies) { + PutLifecycleAction.Request request = new PutLifecycleAction.Request(policy); + validate(request); + result.add(request); } return result; @@ -108,4 +101,22 @@ public TransformState transform(Object source, TransformState prevState) throws return new TransformState(state, entities); } + + @Override + public List fromXContent(XContentParser parser) throws IOException { + List result = new ArrayList<>(); + + Map source = parser.map(); + var config = XContentParserConfiguration.EMPTY.withRegistry(LifecyclePolicyConfig.DEFAULT_X_CONTENT_REGISTRY); + + for (String name : source.keySet()) { + @SuppressWarnings("unchecked") + Map content = (Map) source.get(name); + try (XContentParser policyParser = mapToXContentParser(config, content)) { + result.add(LifecyclePolicy.parse(policyParser, name)); + } + } + + return result; + } } diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index 8ef6a4b3d4717..b3c2b843370cb 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.ilm.action; +import org.elasticsearch.Version; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.ClusterName; @@ -16,10 +17,12 @@ import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.immutablestate.TransformState; import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; import org.elasticsearch.immutablestate.service.ImmutableStateUpdateStateTask; +import org.elasticsearch.immutablestate.service.PackageVersion; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.NamedXContentRegistry; @@ -34,8 +37,10 @@ import org.elasticsearch.xpack.core.ilm.FreezeAction; import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata; import org.elasticsearch.xpack.core.ilm.LifecycleAction; +import org.elasticsearch.xpack.core.ilm.LifecyclePolicy; import org.elasticsearch.xpack.core.ilm.LifecycleType; import org.elasticsearch.xpack.core.ilm.MigrateAction; +import org.elasticsearch.xpack.core.ilm.Phase; import org.elasticsearch.xpack.core.ilm.ReadOnlyAction; import org.elasticsearch.xpack.core.ilm.RolloverAction; import org.elasticsearch.xpack.core.ilm.RollupILMAction; @@ -52,6 +57,8 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Map; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; @@ -224,7 +231,48 @@ public void testActionAddRemove() throws Exception { assertThat(ilmMetadata.getPolicyMetadatas().keySet(), containsInAnyOrder("my_timeseries_lifecycle2")); } - public void testOperatorController() throws IOException { + private void setupTaskMock(ClusterService clusterService, ClusterState state) { + doAnswer((Answer) invocation -> { + Object[] args = invocation.getArguments(); + + if ((args[3] instanceof ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor) == false) { + fail("Should have gotten a state update task to execute, instead got: " + args[3].getClass().getName()); + } + + ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor task = + (ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor) args[3]; + + ClusterStateTaskExecutor.TaskContext context = new ClusterStateTaskExecutor.TaskContext<>() { + @Override + public ImmutableStateUpdateStateTask getTask() { + return (ImmutableStateUpdateStateTask) args[1]; + } + + @Override + public void success(Runnable onPublicationSuccess) {} + + @Override + public void success(Consumer publishedStateConsumer) {} + + @Override + public void success(Runnable onPublicationSuccess, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void success(Consumer publishedStateConsumer, ClusterStateAckListener clusterStateAckListener) {} + + @Override + public void onFailure(Exception failure) { + fail("Shouldn't fail here"); + } + }; + + task.execute(state, List.of(context)); + + return null; + }).when(clusterService).submitStateUpdateTask(anyString(), any(), any(), any()); + } + + public void testOperatorControllerFromJSONContent() throws IOException { ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); ClusterService clusterService = mock(ClusterService.class); final ClusterName clusterName = new ClusterName("elasticsearch"); @@ -247,10 +295,34 @@ public void testOperatorController() throws IOException { }, "ilm": { "my_timeseries_lifecycle": { + "phases": { + "hot": { + "min_age": "10s", + "actions": { + "rollover": { + "max_primary_shard_size": "50gb", + "max_age": "30d" + } + } + }, + "delete": { + "min_age": "30s", + "actions": { + } + } + } + }, + "my_timeseries_lifecycle1": { "phases": { "warm": { "min_age": "10s", "actions": { + "shrink": { + "number_of_shards": 1 + }, + "forcemerge": { + "max_num_segments": 1 + } } }, "delete": { @@ -285,51 +357,74 @@ public void testOperatorController() throws IOException { ) ); - doAnswer((Answer) invocation -> { - Object[] args = invocation.getArguments(); + setupTaskMock(clusterService, state); - if ((args[3] instanceof ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor) == false) { - fail("Should have gotten a state update task to execute, instead got: " + args[3].getClass().getName()); - } + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { + controller.process("operator", parser, (e) -> { + if (e != null) { + fail("Should not fail"); + } + }); + } + } - ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor task = - (ImmutableStateUpdateStateTask.ImmutableUpdateStateTaskExecutor) args[3]; + public void testOperatorControllerWithPluginPackage() { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + ClusterService clusterService = mock(ClusterService.class); + final ClusterName clusterName = new ClusterName("elasticsearch"); - ClusterStateTaskExecutor.TaskContext context = new ClusterStateTaskExecutor.TaskContext<>() { - @Override - public ImmutableStateUpdateStateTask getTask() { - return (ImmutableStateUpdateStateTask) args[1]; - } + ClusterState state = ClusterState.builder(clusterName).build(); + when(clusterService.state()).thenReturn(state); - @Override - public void success(Runnable onPublicationSuccess) {} + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + controller.initHandlers(List.of(new ImmutableClusterSettingsAction(clusterSettings))); - @Override - public void success(Consumer publishedStateConsumer) {} + AtomicReference x = new AtomicReference<>(); - @Override - public void success(Runnable onPublicationSuccess, ClusterStateAckListener clusterStateAckListener) {} + ImmutableClusterStateController.Package pack = new ImmutableClusterStateController.Package( + Map.of( + ImmutableClusterSettingsAction.NAME, + Map.of("indices.recovery.max_bytes_per_sec", "50mb"), + ImmutableLifecycleAction.NAME, + List.of( + new LifecyclePolicy( + "my_timeseries_lifecycle", + Map.of( + "warm", + new Phase("warm", new TimeValue(10, TimeUnit.SECONDS), Collections.emptyMap()), + "delete", + new Phase("delete", new TimeValue(30, TimeUnit.SECONDS), Collections.emptyMap()) + ) + ) + ) + ), + new PackageVersion(123L, Version.CURRENT) + ); - @Override - public void success(Consumer publishedStateConsumer, ClusterStateAckListener clusterStateAckListener) {} + controller.process("operator", pack, (e) -> x.set(e)); - @Override - public void onFailure(Exception failure) { - fail("Shouldn't fail here"); - } - }; + assertTrue(x.get() instanceof IllegalStateException); + assertEquals("Error processing state change request for operator", x.get().getMessage()); - task.execute(state, List.of(context)); + Client client = mock(Client.class); + when(client.settings()).thenReturn(Settings.EMPTY); - return null; - }).when(clusterService).submitStateUpdateTask(anyString(), any(), any(), any()); + XPackLicenseState licenseState = mock(XPackLicenseState.class); - try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { - controller.process("operator", parser, (e) -> { - if (e != null) { - fail("Should not fail"); - } - }); - } + controller.initHandlers( + List.of( + new ImmutableClusterSettingsAction(clusterSettings), + new ImmutableLifecycleAction(xContentRegistry(), client, licenseState) + ) + ); + + setupTaskMock(clusterService, state); + + controller.process("operator", pack, (e) -> { + if (e != null) { + fail("Should not fail"); + } + }); } + } From d9273f4f21438ef61a4550bf535ddd307722a03b Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 5 Jul 2022 21:20:59 -0400 Subject: [PATCH 08/27] Fix test JSON parsing --- .../xpack/ilm/action/ImmutableILMStateControllerTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index b3c2b843370cb..96e013ada95e0 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -108,7 +108,7 @@ protected NamedXContentRegistry xContentRegistry() { private TransformState processJSON(ImmutableLifecycleAction action, TransformState prevState, String json) throws Exception { try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { - return action.transform(parser.map(), prevState); + return action.transform(action.fromXContent(parser), prevState); } } From 81703a75f3901e3fa554937198217334fb38f8a1 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 6 Jul 2022 17:05:32 -0400 Subject: [PATCH 09/27] Add file settings service --- .../threadpool/SimpleThreadPoolIT.java | 4 +- .../org/elasticsearch/ExceptionsHelper.java | 15 + .../common/settings/ClusterSettings.java | 4 +- .../ImmutableClusterStateController.java | 12 +- .../java/org/elasticsearch/node/Node.java | 9 + .../operator/service/FileSettingsService.java | 314 ++++++++++++++++++ .../service/FileSettingsServiceTests.java | 193 +++++++++++ .../ImmutableILMStateControllerTests.java | 91 +++++ 8 files changed, 638 insertions(+), 4 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java create mode 100644 server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java diff --git a/server/src/internalClusterTest/java/org/elasticsearch/threadpool/SimpleThreadPoolIT.java b/server/src/internalClusterTest/java/org/elasticsearch/threadpool/SimpleThreadPoolIT.java index 95e1d23fc208e..51d4fc3437c9d 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/threadpool/SimpleThreadPoolIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/threadpool/SimpleThreadPoolIT.java @@ -81,8 +81,10 @@ public void testThreadNames() throws Exception { // or the ones that are occasionally come up from ESSingleNodeTestCase if (threadName.contains("[node_s_0]") // TODO: this can't possibly be right! single node and integ test are unrelated! || threadName.contains("Keep-Alive-Timer") + || threadName.contains("readiness-service") || threadName.contains("JVMCI-native") // GraalVM Compiler Thread - || threadName.contains("readiness-service")) { + || threadName.contains("file-settings-watcher") + || threadName.contains("FileSystemWatchService")) { continue; } String nodePrefix = "(" diff --git a/server/src/main/java/org/elasticsearch/ExceptionsHelper.java b/server/src/main/java/org/elasticsearch/ExceptionsHelper.java index 7e22e1797b527..1b7381c93fafb 100644 --- a/server/src/main/java/org/elasticsearch/ExceptionsHelper.java +++ b/server/src/main/java/org/elasticsearch/ExceptionsHelper.java @@ -34,6 +34,7 @@ import java.util.Optional; import java.util.Queue; import java.util.Set; +import java.util.function.Consumer; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -262,6 +263,20 @@ public static void maybeDieOnAnotherThread(final Throwable throwable) { }); } + public static void unwrap(Throwable t, Consumer errorListener, int limit) { + int counter = 0; + Throwable cause; + Throwable prev = t; + errorListener.accept(prev); + while ((cause = prev.getCause()) != null && (prev != cause)) { + prev = cause; + errorListener.accept(prev); + if (counter++ > limit) { + return; + } + } + } + /** * Deduplicate the failures by exception message and index. */ diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 89d16c01df42e..16ead0bc34046 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -91,6 +91,7 @@ import org.elasticsearch.monitor.process.ProcessService; import org.elasticsearch.node.Node; import org.elasticsearch.node.NodeRoleSettings; +import org.elasticsearch.operator.service.FileSettingsService; import org.elasticsearch.persistent.PersistentTasksClusterService; import org.elasticsearch.persistent.decider.EnableAssignmentDecider; import org.elasticsearch.plugins.PluginsService; @@ -520,7 +521,8 @@ public void apply(Settings value, Settings current, Settings previous) { CoordinationDiagnosticsService.NODE_HAS_MASTER_LOOKUP_TIMEFRAME_SETTING, MasterHistory.MAX_HISTORY_AGE_SETTING, ReadinessService.PORT, - HealthNodeTaskExecutor.ENABLED_SETTING + HealthNodeTaskExecutor.ENABLED_SETTING, + FileSettingsService.OPERATOR_DIRECTORY ); static List> BUILT_IN_SETTING_UPGRADERS = Collections.emptyList(); diff --git a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java index 8edc93261597a..82752ab1e7e2a 100644 --- a/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java +++ b/server/src/main/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateController.java @@ -10,6 +10,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionResponse; @@ -111,11 +112,18 @@ public void process(String namespace, XContentParser parser, Consumer try { immutableStatePackage = packageParser.apply(parser, null); } catch (Exception e) { - List errors = List.of(e.getMessage()); + StringBuilder stringBuilder = new StringBuilder(); + ExceptionsHelper.unwrap(e, (t) -> stringBuilder.append(t.getMessage()).append(", "), 20); + if (stringBuilder.length() > 2) { + stringBuilder.setLength(stringBuilder.length() - 2); + } + List errors = List.of(stringBuilder.toString()); recordErrorState(new ImmutableUpdateErrorState(namespace, -1L, errors, ImmutableStateErrorMetadata.ErrorKind.PARSING)); logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); - errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); + errorListener.accept( + new IllegalStateException("Error processing state change request for " + namespace + ", with errors: " + stringBuilder, e) + ); return; } diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 440d3f0f2adb7..c99998a3c4c76 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -134,6 +134,7 @@ import org.elasticsearch.monitor.MonitorService; import org.elasticsearch.monitor.fs.FsHealthService; import org.elasticsearch.monitor.jvm.JvmInfo; +import org.elasticsearch.operator.service.FileSettingsService; import org.elasticsearch.persistent.PersistentTasksClusterService; import org.elasticsearch.persistent.PersistentTasksExecutor; import org.elasticsearch.persistent.PersistentTasksExecutorRegistry; @@ -1010,6 +1011,11 @@ protected Node( modules.add(b -> b.bind(ReadinessService.class).toInstance(new ReadinessService(clusterService, environment))); } + modules.add( + b -> b.bind(FileSettingsService.class) + .toInstance(new FileSettingsService(clusterService, actionModule.getImmutableClusterStateController(), environment)) + ); + injector = modules.createInjector(); // We allocate copies of existing shards by looking for a viable copy of the shard in the cluster and assigning the shard there. @@ -1164,6 +1170,7 @@ public Node start() throws NodeValidationException { if (ReadinessService.enabled(environment)) { injector.getInstance(ReadinessService.class).start(); } + injector.getInstance(FileSettingsService.class).start(); injector.getInstance(MappingUpdatedAction.class).setClient(client); injector.getInstance(IndicesService.class).start(); injector.getInstance(IndicesClusterStateService.class).start(); @@ -1312,6 +1319,7 @@ private Node stop() { if (ReadinessService.enabled(environment)) { injector.getInstance(ReadinessService.class).stop(); } + injector.getInstance(FileSettingsService.class).stop(); injector.getInstance(ResourceWatcherService.class).close(); injector.getInstance(HttpServerTransport.class).stop(); @@ -1395,6 +1403,7 @@ public synchronized void close() throws IOException { if (ReadinessService.enabled(environment)) { toClose.add(injector.getInstance(ReadinessService.class)); } + toClose.add(injector.getInstance(FileSettingsService.class)); for (LifecycleComponent plugin : pluginLifecycleComponents) { toClose.add(() -> stopWatch.stop().start("plugin(" + plugin.getClass().getName() + ")")); diff --git a/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java new file mode 100644 index 0000000000000..d9003753f8abf --- /dev/null +++ b/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java @@ -0,0 +1,314 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator.service; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.cluster.ClusterChangedEvent; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateListener; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.component.AbstractLifecycleComponent; +import org.elasticsearch.common.settings.Setting; +import org.elasticsearch.core.PathUtils; +import org.elasticsearch.env.Environment; +import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentType; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardWatchEventKinds; +import java.nio.file.WatchKey; +import java.nio.file.WatchService; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.concurrent.CountDownLatch; + +/** + * File based settings applier service which watches an 'operator` directory inside the config directory. + *

+ * The service expects that the operator directory will contain a single JSON file with all the settings that + * need to be applied to the cluster state. The name of the file is fixed to be settings.json. The operator + * directory name can be configured by setting the 'path.config.operator_directory' in the node properties. + *

+ * The {@link FileSettingsService} is active always, but enabled only on the current master node. We register + * the service as a listener to cluster state changes, so that we can enable the file watcher thread when this + * node becomes a master node. + */ +public class FileSettingsService extends AbstractLifecycleComponent implements ClusterStateListener { + private static final Logger logger = LogManager.getLogger(FileSettingsService.class); + + private static final String SETTINGS_FILE_NAME = "settings.json"; + private static final String NAMESPACE = "file_settings"; + + private final ImmutableClusterStateController controller; + private final Environment environment; + + private WatchService watchService; // null; + private CountDownLatch watcherThreadLatch; + + private volatile FileUpdateState fileUpdateState = null; + + private volatile boolean active = false; + private volatile boolean initialState = true; + + public static final Setting OPERATOR_DIRECTORY = Setting.simpleString( + "path.config.operator_directory", + "operator", + Setting.Property.NodeScope + ); + + /** + * Constructs the {@link FileSettingsService} + * + * @param clusterService so we can register ourselves as a cluster state change listener + * @param controller an instance of the immutable cluster state controller, so we can perform the cluster state changes + * @param environment we need the environment to pull the location of the config and operator directories + */ + public FileSettingsService(ClusterService clusterService, ImmutableClusterStateController controller, Environment environment) { + this.controller = controller; + this.environment = environment; + clusterService.addListener(this); + } + + // package private for testing + Path operatorSettingsDir() { + String dirPath = OPERATOR_DIRECTORY.get(environment.settings()); + return environment.configFile().toAbsolutePath().resolve(dirPath); + } + + // package private for testing + Path operatorSettingsFile() { + return operatorSettingsDir().resolve(SETTINGS_FILE_NAME); + } + + // platform independent way to tell if a file changed + // we compare the file modified timestamp, the absolute path (symlinks), and file id on the system + boolean watchedFileChanged(Path path) throws IOException { + if (Files.exists(path) == false) { + return false; + } + + FileUpdateState previousUpdateState = fileUpdateState; + + BasicFileAttributes attr = Files.readAttributes(path, BasicFileAttributes.class); + fileUpdateState = new FileUpdateState(attr.lastModifiedTime().toMillis(), path.toRealPath().toString(), attr.fileKey()); + + return (previousUpdateState == null || previousUpdateState.equals(fileUpdateState) == false); + } + + @Override + protected void doStart() { + // We start the file watcher when we know we are master from a cluster state change notification. + // We need this additional flag, since cluster state can change after we've shutdown the service + // causing the watcher to start again. + this.active = true; + } + + @Override + protected void doStop() { + this.active = false; + logger.debug("Stopping file settings service"); + stopWatcher(); + } + + @Override + protected void doClose() {} + + private boolean currentNodeMaster(ClusterState clusterState) { + return clusterState.nodes().getMasterNodeId().equals(clusterState.nodes().getLocalNodeId()); + } + + @Override + public void clusterChanged(ClusterChangedEvent event) { + ClusterState clusterState = event.state(); + setWatching(currentNodeMaster(clusterState), initialState); + initialState = false; + } + + private void setWatching(boolean watching, boolean initialState) { + if (watching) { + startWatcher(initialState); + } else { + stopWatcher(); + } + } + + // package private for testing + boolean watching() { + return this.watchService != null; + } + + synchronized void startWatcher(boolean onStartup) { + if (watching() || active == false) { + // already watching or inactive, nothing to do + return; + } + + logger.info("starting file settings watcher ..."); + + Path settingsDir = operatorSettingsDir(); + + /** + * We essentially watch for two things: + * - the creation of the operator directory (if it doesn't exist), symlink changes to the operator directory + * - any changes to files inside the operator directory if it exists, filtering for settings.json + */ + try { + this.watchService = PathUtils.getDefaultFileSystem().newWatchService(); + if (Files.exists(settingsDir)) { + Path settingsFilePath = operatorSettingsFile(); + if (Files.exists(settingsFilePath)) { + logger.info("found initial operator settings file [{}], applying...", settingsFilePath); + // we make a distinction here for startup, so that if we had operator settings before the node started + // we would fail startup. + processFileSettings(settingsFilePath, onStartup); + } + enableSettingsWatcher(settingsDir); + } else { + logger.info("operator settings directory [{}] not found, will watch for its creation...", settingsDir); + enableSettingsWatcher(environment.configFile()); + } + } catch (Exception e) { + if (watchService != null) { + try { + this.watchService.close(); + } catch (Exception ignore) {} finally { + this.watchService = null; + } + } + + throw new IllegalStateException("unable to launch a new watch service", e); + } + + this.watcherThreadLatch = new CountDownLatch(1); + + new Thread(() -> { + try { + logger.info("file settings service up and running [tid={}]", Thread.currentThread().getId()); + + WatchKey key; + while ((key = watchService.take()) != null) { + /** + * Reading and interpreting watch service events can vary from platform to platform. E.g: + * MacOS symlink delete and set (rm -rf operator && ln -s /file_settings/ operator): + * ENTRY_MODIFY:operator + * ENTRY_CREATE:settings.json + * ENTRY_MODIFY:settings.json + * Linux in Docker symlink delete and set (rm -rf operator && ln -s /file_settings/ operator): + * ENTRY_CREATE:operator + * After we get an indication that something has changed, we check the timestamp, file id, + * real path of our desired file. + */ + if (Files.exists(settingsDir)) { + try { + Path path = operatorSettingsFile(); + + if (logger.isDebugEnabled()) { + key.pollEvents().stream().forEach(e -> logger.debug("{}:{}", e.kind().toString(), e.context().toString())); + } + + key.pollEvents(); + key.reset(); + + enableSettingsWatcher(settingsDir); + + if (watchedFileChanged(path)) { + processFileSettings(path, false); + } + } catch (Exception e) { + logger.warn("unable to watch or read operator settings file", e); + } + } else { + key.pollEvents(); + key.reset(); + } + } + } catch (Exception e) { + if (logger.isDebugEnabled()) { + logger.debug("encountered exception watching", e); + } + logger.info("shutting down watcher thread"); + } finally { + watcherThreadLatch.countDown(); + } + }, "elasticsearch[file-settings-watcher]").start(); + } + + synchronized void stopWatcher() { + logger.debug("stopping watcher ..."); + if (watching()) { + try { + watchService.close(); + if (watcherThreadLatch != null) { + watcherThreadLatch.await(); + } + } catch (IOException | InterruptedException e) { + logger.info("encountered exception while closing watch service", e); + } finally { + watchService = null; + logger.info("watcher service stopped"); + } + } else { + logger.debug("file settings service already stopped"); + } + } + + private void enableSettingsWatcher(Path settingsDir) throws IOException { + settingsDir.register( + watchService, + StandardWatchEventKinds.ENTRY_MODIFY, + StandardWatchEventKinds.ENTRY_CREATE, + StandardWatchEventKinds.ENTRY_DELETE + ); + } + + void processFileSettings(Path path, boolean onStartup) { + logger.info("processing path [{}] for [{}]", path, NAMESPACE); + try ( + XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, Files.newInputStream(path)) + ) { + controller.process(NAMESPACE, parser, (e) -> { + if (e != null) { + if (e instanceof ImmutableClusterStateController.IncompatibleVersionException) { + logger.info(e.getMessage()); + } else { + // If we encountered an exception trying to apply the operator state at + // startup time, we throw an error to force Elasticsearch to exit. + if (onStartup) { + throw new OperatorConfigurationError("Error applying operator settings", e); + } else { + logger.error("Error processing operator settings json file", e); + } + } + } + }); + } catch (Exception e) { + logger.error("Error processing operator settings json file", e); + } + } + + /** + * Holds information about the last known state of the file we watched. We use this + * class to determine if a file has been changed. + */ + record FileUpdateState(long timestamp, String path, Object fileKey) {} + + /** + * Error subclass that is thrown when we encounter a fatal error while applying + * the operator cluster state at Elasticsearch boot time. + */ + public static class OperatorConfigurationError extends Error { + public OperatorConfigurationError(String message, Throwable t) { + super(message, t); + } + } +} diff --git a/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java new file mode 100644 index 0000000000000..6cb9ca5f25efe --- /dev/null +++ b/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java @@ -0,0 +1,193 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.operator.service; + +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.env.Environment; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; +import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.TestThreadPool; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.xcontent.XContentParser; +import org.junit.After; +import org.junit.Before; +import org.mockito.stubbing.Answer; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.time.ZoneOffset; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +public class FileSettingsServiceTests extends ESTestCase { + private Environment env; + private ClusterService clusterService; + private FileSettingsService fileSettingsService; + private ThreadPool threadpool; + + @Before + public void setUp() throws Exception { + super.setUp(); + + threadpool = new TestThreadPool("file_settings_service_tests"); + + clusterService = new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + threadpool, + null + ); + env = newEnvironment(Settings.EMPTY); + + Files.createDirectories(env.configFile()); + + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + controller.initHandlers(List.of(new ImmutableClusterSettingsAction(clusterSettings))); + + fileSettingsService = new FileSettingsService(clusterService, controller, env); + } + + @After + public void tearDown() throws Exception { + super.tearDown(); + threadpool.shutdownNow(); + } + + public void testOperatorDirName() { + Path operatorPath = fileSettingsService.operatorSettingsDir(); + assertTrue(operatorPath.startsWith(env.configFile())); + assertTrue(operatorPath.endsWith("operator")); + + Path operatorSettingsFile = fileSettingsService.operatorSettingsFile(); + assertTrue(operatorSettingsFile.startsWith(operatorPath)); + assertTrue(operatorSettingsFile.endsWith("settings.json")); + } + + public void testWatchedFile() throws Exception { + Path tmpFile = createTempFile(); + Path tmpFile1 = createTempFile(); + Path otherFile = tmpFile.getParent().resolve("other.json"); + // we return false on non-existent paths, we don't remember state + assertFalse(fileSettingsService.watchedFileChanged(otherFile)); + + // we remember the previous state + assertTrue(fileSettingsService.watchedFileChanged(tmpFile)); + assertFalse(fileSettingsService.watchedFileChanged(tmpFile)); + + // we modify the timestamp of the file, it should trigger a change + Instant now = LocalDateTime.now(ZoneId.systemDefault()).toInstant(ZoneOffset.ofHours(0)); + Files.setLastModifiedTime(tmpFile, FileTime.from(now)); + + assertTrue(fileSettingsService.watchedFileChanged(tmpFile)); + assertFalse(fileSettingsService.watchedFileChanged(tmpFile)); + + // we change to another real file, it should be changed + assertTrue(fileSettingsService.watchedFileChanged(tmpFile1)); + assertFalse(fileSettingsService.watchedFileChanged(tmpFile1)); + } + + public void testStartStop() { + fileSettingsService.start(); + fileSettingsService.startWatcher(false); + assertTrue(fileSettingsService.watching()); + fileSettingsService.stop(); + assertFalse(fileSettingsService.watching()); + fileSettingsService.close(); + } + + public void testCallsProcessing() throws Exception { + FileSettingsService service = spy(fileSettingsService); + CountDownLatch processFileLatch = new CountDownLatch(1); + + doAnswer((Answer) invocation -> { + processFileLatch.countDown(); + return null; + }).when(service).processFileSettings(any(), anyBoolean()); + + service.start(); + service.startWatcher(false); + assertTrue(service.watching()); + + Files.createDirectories(service.operatorSettingsDir()); + + Files.write(service.operatorSettingsFile(), "{}".getBytes(StandardCharsets.UTF_8)); + + // we need to wait a bit, on MacOS it may take up to 10 seconds for the Java watcher service to notice the file, + // on Linux is instantaneous. Windows??? + processFileLatch.await(30, TimeUnit.SECONDS); + + verify(service, times(1)).watchedFileChanged(any()); + + service.stop(); + assertFalse(service.watching()); + service.close(); + } + + @SuppressWarnings("unchecked") + public void testInitialFile() throws Exception { + ImmutableClusterStateController controller = mock(ImmutableClusterStateController.class); + + doAnswer((Answer) invocation -> { + ((Consumer) invocation.getArgument(2)).accept(new IllegalStateException("Some exception")); + return null; + }).when(controller).process(any(), (XContentParser) any(), any()); + + FileSettingsService service = spy(new FileSettingsService(clusterService, controller, env)); + + Files.createDirectories(service.operatorSettingsDir()); + + // contents of the JSON don't matter, we just need a file to exist + Files.write(service.operatorSettingsFile(), "{}".getBytes(StandardCharsets.UTF_8)); + + service.start(); + assertEquals( + "Error applying operator settings", + expectThrows(FileSettingsService.OperatorConfigurationError.class, () -> service.startWatcher(true)).getMessage() + ); + + verify(service, times(1)).processFileSettings(any(), eq(true)); + + service.stop(); + + clearInvocations(service); + + // Let's check that if we didn't throw an error that everything works + doAnswer((Answer) invocation -> null).when(controller).process(any(), (XContentParser) any(), any()); + + service.start(); + service.startWatcher(true); + + verify(service, times(1)).processFileSettings(any(), eq(true)); + + service.stop(); + service.close(); + } +} diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index 96e013ada95e0..c7fd91147b198 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -427,4 +427,95 @@ public void testOperatorControllerWithPluginPackage() { }); } + public void testOperatorControllerWithBadJSONContent() throws IOException { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + ClusterService clusterService = mock(ClusterService.class); + final ClusterName clusterName = new ClusterName("elasticsearch"); + + ClusterState state = ClusterState.builder(clusterName).build(); + when(clusterService.state()).thenReturn(state); + + ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); + Client client = mock(Client.class); + when(client.settings()).thenReturn(Settings.EMPTY); + + XPackLicenseState licenseState = mock(XPackLicenseState.class); + + controller.initHandlers( + List.of( + new ImmutableClusterSettingsAction(clusterSettings), + new ImmutableLifecycleAction(xContentRegistry(), client, licenseState) + ) + ); + + String testJSON = """ + { + "metadata": { + "version": "1234", + "compatibility": "8.4.0" + }, + "state": { + "cluster_settings": { + "indices.recovery.max_bytes_per_sec": "50mb" + }, + "ilm": { + "my_timeseries_lifecycle": { + "policy": { + "phases": { + "hot": { + "min_age": "10s", + "actions": { + "rollover": { + "max_primary_shard_size": "50gb", + "max_age": "30d" + } + } + }, + "delete": { + "min_age": "30s", + "actions": { + } + } + } + } + }, + "my_timeseries_lifecycle1": { + "phases": { + "warm": { + "min_age": "10s", + "actions": { + "shrink": { + "number_of_shards": 1 + }, + "forcemerge": { + "max_num_segments": 1 + } + } + }, + "delete": { + "min_age": "30s", + "actions": { + } + } + } + } + } + } + }"""; + + AtomicReference x = new AtomicReference<>(); + + try (XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, testJSON)) { + controller.process("operator", parser, (e) -> x.set(e)); + + assertTrue(x.get() instanceof IllegalStateException); + assertEquals( + "Error processing state change request for operator, with errors: " + + "[51:10] [immutable_cluster_package] failed to parse field [state], [51:10] [state] " + + "failed to parse field [ilm], [1:2] [lifecycle_policy] unknown field [policy]", + x.get().getMessage() + ); + } + } + } From dac7a6a2157542b2a91fba76df88e2c791bd8cdc Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Wed, 6 Jul 2022 17:09:22 -0400 Subject: [PATCH 10/27] Update docs/changelog/88329.yaml --- docs/changelog/88329.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/88329.yaml diff --git a/docs/changelog/88329.yaml b/docs/changelog/88329.yaml new file mode 100644 index 0000000000000..b105f533fc6ba --- /dev/null +++ b/docs/changelog/88329.yaml @@ -0,0 +1,5 @@ +pr: 88329 +summary: File Settings Service +area: Infra/Core +type: feature +issues: [] From f58b7a8598c97e54dbae7a4ee6331bfdfc8f691d Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 6 Jul 2022 18:29:11 -0400 Subject: [PATCH 11/27] Fix tests to reflect more debugging info --- .../service/ImmutableClusterStateControllerTests.java | 9 ++++++++- .../ilm/action/ImmutableILMStateControllerTests.java | 6 +++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java b/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java index 60f841895b72b..6909c8b64a71e 100644 --- a/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java +++ b/server/src/test/java/org/elasticsearch/immutablestate/service/ImmutableClusterStateControllerTests.java @@ -43,6 +43,7 @@ import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -86,7 +87,13 @@ public void testOperatorController() throws IOException { controller.process("operator", parser, (e) -> x.set(e)); assertTrue(x.get() instanceof IllegalStateException); - assertEquals("Error processing state change request for operator", x.get().getMessage()); + assertThat( + x.get().getMessage(), + containsString( + "Error processing state change request for operator, with errors: [12:1] " + + "Unexpected end-of-input: expected close marker for Object (start marker at [Source: (String)" + ) + ); } testJSON = """ diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java index c7fd91147b198..b0eeeb7d189b5 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ImmutableILMStateControllerTests.java @@ -63,6 +63,7 @@ import java.util.function.Consumer; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doAnswer; @@ -342,7 +343,10 @@ public void testOperatorControllerFromJSONContent() throws IOException { controller.process("operator", parser, (e) -> x.set(e)); assertTrue(x.get() instanceof IllegalStateException); - assertEquals("Error processing state change request for operator", x.get().getMessage()); + assertThat( + x.get().getMessage(), + containsString("[10:10] [state] failed to parse field [ilm], Missing handler definition for content key [ilm]") + ); } Client client = mock(Client.class); From f09a65858a99a16377889fbbce680c506bada66c Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 20 Jul 2022 12:16:54 -0400 Subject: [PATCH 12/27] Update to latest version --- .../elasticsearch/action/ActionModule.java | 36 +++++-------------- .../java/org/elasticsearch/node/Node.java | 21 ++++++++--- .../operator/service/FileSettingsService.java | 26 ++++++-------- .../action/ActionModuleTests.java | 13 ++++--- .../service/FileSettingsServiceTests.java | 20 ++++++----- .../xpack/security/SecurityTests.java | 3 +- 6 files changed, 59 insertions(+), 60 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index b24f8c60b979c..af69dfd7e2306 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -263,10 +263,6 @@ import org.elasticsearch.gateway.TransportNodesListGatewayStartedShards; import org.elasticsearch.health.GetHealthAction; import org.elasticsearch.health.RestGetHealthAction; -import org.elasticsearch.immutablestate.ImmutableClusterStateHandler; -import org.elasticsearch.immutablestate.ImmutableClusterStateHandlerProvider; -import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; -import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; import org.elasticsearch.index.seqno.GlobalCheckpointSyncAction; import org.elasticsearch.index.seqno.RetentionLeaseActions; import org.elasticsearch.indices.SystemIndices; @@ -278,8 +274,9 @@ import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.ActionPlugin.ActionHandler; -import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.plugins.interceptor.RestInterceptorActionPlugin; +import org.elasticsearch.reservedstate.ReservedClusterStateHandler; +import org.elasticsearch.reservedstate.service.ReservedClusterStateService; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; import org.elasticsearch.rest.RestHeaderDefinition; @@ -454,7 +451,7 @@ public class ActionModule extends AbstractModule { private final RequestValidators mappingRequestValidators; private final RequestValidators indicesAliasesRequestRequestValidators; private final ThreadPool threadPool; - private final ImmutableClusterStateController immutableStateController; + private final ReservedClusterStateService reservedClusterStateService; public ActionModule( Settings settings, @@ -468,7 +465,8 @@ public ActionModule( CircuitBreakerService circuitBreakerService, UsageService usageService, SystemIndices systemIndices, - ClusterService clusterService + ClusterService clusterService, + List> reservedStateHandlers ) { this.settings = settings; this.indexNameExpressionResolver = indexNameExpressionResolver; @@ -519,7 +517,7 @@ public ActionModule( ); restController = new RestController(headers, restInterceptor, nodeClient, circuitBreakerService, usageService); - immutableStateController = new ImmutableClusterStateController(clusterService); + reservedClusterStateService = new ReservedClusterStateService(clusterService, reservedStateHandlers); } public Map> getActions() { @@ -897,24 +895,6 @@ public void initRestHandlers(Supplier nodesInCluster) { registerHandler.accept(new RestCatAction(catActions)); } - /** - * Initializes the immutable cluster state handlers for Elasticsearch and it's modules/plugins - * - * @param pluginsService needed to load all modules/plugins immutable state handlers through SPI - */ - public void initImmutableClusterStateHandlers(PluginsService pluginsService) { - List> handlers = new ArrayList<>(); - - List pluginHandlers = pluginsService.loadServiceProviders( - ImmutableClusterStateHandlerProvider.class - ); - - handlers.add(new ImmutableClusterSettingsAction(clusterSettings)); - pluginHandlers.forEach(h -> handlers.addAll(h.handlers())); - - immutableStateController.initHandlers(handlers); - } - @Override protected void configure() { bind(ActionFilters.class).toInstance(actionFilters); @@ -948,7 +928,7 @@ public RestController getRestController() { return restController; } - public ImmutableClusterStateController getImmutableClusterStateController() { - return immutableStateController; + public ReservedClusterStateService getReservedClusterStateService() { + return reservedClusterStateService; } } diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 7ea22f3782e90..2ce448f2f81de 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -164,6 +164,9 @@ import org.elasticsearch.readiness.ReadinessService; import org.elasticsearch.repositories.RepositoriesModule; import org.elasticsearch.repositories.RepositoriesService; +import org.elasticsearch.reservedstate.ReservedClusterStateHandler; +import org.elasticsearch.reservedstate.ReservedClusterStateHandlerProvider; +import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; import org.elasticsearch.rest.RestController; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptEngine; @@ -706,6 +709,17 @@ protected Node( ) ).toList(); + List> reservedStateHandlers = new ArrayList<>(); + + // add all reserved state handlers from server + reservedStateHandlers.add(new ReservedClusterSettingsAction(settingsModule.getClusterSettings())); + + // add all reserved state handlers from plugins + List pluginHandlers = pluginsService.loadServiceProviders( + ReservedClusterStateHandlerProvider.class + ); + pluginHandlers.forEach(h -> reservedStateHandlers.addAll(h.handlers())); + ActionModule actionModule = new ActionModule( settings, clusterModule.getIndexNameExpressionResolver(), @@ -718,7 +732,8 @@ protected Node( circuitBreakerService, usageService, systemIndices, - clusterService + clusterService, + reservedStateHandlers ); modules.add(actionModule); @@ -1023,7 +1038,7 @@ protected Node( modules.add( b -> b.bind(FileSettingsService.class) - .toInstance(new FileSettingsService(clusterService, actionModule.getImmutableClusterStateController(), environment)) + .toInstance(new FileSettingsService(clusterService, actionModule.getReservedClusterStateService(), environment)) ); injector = modules.createInjector(); @@ -1063,8 +1078,6 @@ protected Node( logger.debug("initializing HTTP handlers ..."); actionModule.initRestHandlers(() -> clusterService.state().nodesIfRecovered()); - logger.debug("initializing operator handlers ..."); - actionModule.initImmutableClusterStateHandlers(pluginsService); logger.info("initialized"); success = true; diff --git a/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java index d9003753f8abf..2d327ef495476 100644 --- a/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java @@ -18,7 +18,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.core.PathUtils; import org.elasticsearch.env.Environment; -import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; +import org.elasticsearch.reservedstate.service.ReservedClusterStateService; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; import org.elasticsearch.xcontent.XContentType; @@ -49,7 +49,7 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C private static final String SETTINGS_FILE_NAME = "settings.json"; private static final String NAMESPACE = "file_settings"; - private final ImmutableClusterStateController controller; + private final ReservedClusterStateService stateService; private final Environment environment; private WatchService watchService; // null; @@ -70,11 +70,11 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C * Constructs the {@link FileSettingsService} * * @param clusterService so we can register ourselves as a cluster state change listener - * @param controller an instance of the immutable cluster state controller, so we can perform the cluster state changes + * @param stateService an instance of the immutable cluster state controller, so we can perform the cluster state changes * @param environment we need the environment to pull the location of the config and operator directories */ - public FileSettingsService(ClusterService clusterService, ImmutableClusterStateController controller, Environment environment) { - this.controller = controller; + public FileSettingsService(ClusterService clusterService, ReservedClusterStateService stateService, Environment environment) { + this.stateService = stateService; this.environment = environment; clusterService.addListener(this); } @@ -276,18 +276,14 @@ void processFileSettings(Path path, boolean onStartup) { try ( XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, Files.newInputStream(path)) ) { - controller.process(NAMESPACE, parser, (e) -> { + stateService.process(NAMESPACE, parser, (e) -> { if (e != null) { - if (e instanceof ImmutableClusterStateController.IncompatibleVersionException) { - logger.info(e.getMessage()); + // If we encountered an exception trying to apply the operator state at + // startup time, we throw an error to force Elasticsearch to exit. + if (onStartup) { + throw new OperatorConfigurationError("Error applying operator settings", e); } else { - // If we encountered an exception trying to apply the operator state at - // startup time, we throw an error to force Elasticsearch to exit. - if (onStartup) { - throw new OperatorConfigurationError("Error applying operator settings", e); - } else { - logger.error("Error processing operator settings json file", e); - } + logger.error("Error processing operator settings json file", e); } } }); diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index ebc869a3aba2c..da11d1295ec82 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -40,6 +40,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.function.Supplier; import java.util.function.UnaryOperator; @@ -117,7 +118,8 @@ public void testSetupRestHandlerContainsKnownBuiltin() { null, usageService, null, - null + null, + Collections.emptyList() ); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail @@ -174,7 +176,8 @@ public String getName() { null, usageService, null, - null + null, + Collections.emptyList() ); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); @@ -224,7 +227,8 @@ public List getRestHandlers( null, usageService, null, - null + null, + Collections.emptyList() ); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail @@ -269,7 +273,8 @@ public void test3rdPartyHandlerIsNotInstalled() { null, usageService, null, - null + null, + Collections.emptyList() ) ); assertThat( diff --git a/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java index 6cb9ca5f25efe..d66ab2fb71c68 100644 --- a/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java @@ -8,12 +8,13 @@ package org.elasticsearch.operator.service; +import org.elasticsearch.cluster.routing.RerouteService; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; -import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; -import org.elasticsearch.immutablestate.service.ImmutableClusterStateController; +import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; +import org.elasticsearch.reservedstate.service.ReservedClusterStateService; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; @@ -63,14 +64,17 @@ public void setUp() throws Exception { threadpool, null ); + clusterService.setRerouteService(mock(RerouteService.class)); env = newEnvironment(Settings.EMPTY); Files.createDirectories(env.configFile()); ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); - ImmutableClusterStateController controller = new ImmutableClusterStateController(clusterService); - controller.initHandlers(List.of(new ImmutableClusterSettingsAction(clusterSettings))); + ReservedClusterStateService controller = new ReservedClusterStateService( + clusterService, + List.of(new ReservedClusterSettingsAction(clusterSettings)) + ); fileSettingsService = new FileSettingsService(clusterService, controller, env); } @@ -153,14 +157,14 @@ public void testCallsProcessing() throws Exception { @SuppressWarnings("unchecked") public void testInitialFile() throws Exception { - ImmutableClusterStateController controller = mock(ImmutableClusterStateController.class); + ReservedClusterStateService stateService = mock(ReservedClusterStateService.class); doAnswer((Answer) invocation -> { ((Consumer) invocation.getArgument(2)).accept(new IllegalStateException("Some exception")); return null; - }).when(controller).process(any(), (XContentParser) any(), any()); + }).when(stateService).process(any(), (XContentParser) any(), any()); - FileSettingsService service = spy(new FileSettingsService(clusterService, controller, env)); + FileSettingsService service = spy(new FileSettingsService(clusterService, stateService, env)); Files.createDirectories(service.operatorSettingsDir()); @@ -180,7 +184,7 @@ public void testInitialFile() throws Exception { clearInvocations(service); // Let's check that if we didn't throw an error that everything works - doAnswer((Answer) invocation -> null).when(controller).process(any(), (XContentParser) any(), any()); + doAnswer((Answer) invocation -> null).when(stateService).process(any(), (XContentParser) any(), any()); service.start(); service.startWatcher(true); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index 604a521da9a8e..daae6e77bd965 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -779,7 +779,8 @@ public void testSecurityRestHandlerInterceptorCanBeInstalled() throws IllegalAcc null, usageService, null, - null + null, + Collections.emptyList() ); actionModule.initRestHandlers(null); From 9357c289a8fd96a969d215af34b7f52f84cb2b0d Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 20 Jul 2022 12:30:36 -0400 Subject: [PATCH 13/27] Remove unused helper --- .../java/org/elasticsearch/ExceptionsHelper.java | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ExceptionsHelper.java b/server/src/main/java/org/elasticsearch/ExceptionsHelper.java index 1b7381c93fafb..7e22e1797b527 100644 --- a/server/src/main/java/org/elasticsearch/ExceptionsHelper.java +++ b/server/src/main/java/org/elasticsearch/ExceptionsHelper.java @@ -34,7 +34,6 @@ import java.util.Optional; import java.util.Queue; import java.util.Set; -import java.util.function.Consumer; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -263,20 +262,6 @@ public static void maybeDieOnAnotherThread(final Throwable throwable) { }); } - public static void unwrap(Throwable t, Consumer errorListener, int limit) { - int counter = 0; - Throwable cause; - Throwable prev = t; - errorListener.accept(prev); - while ((cause = prev.getCause()) != null && (prev != cause)) { - prev = cause; - errorListener.accept(prev); - if (counter++ > limit) { - return; - } - } - } - /** * Deduplicate the failures by exception message and index. */ From 089fe78ae6816b0ad4a54404a8e45cfd55366851 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 20 Jul 2022 12:33:20 -0400 Subject: [PATCH 14/27] Change package --- .../org/elasticsearch/common/settings/ClusterSettings.java | 2 +- server/src/main/java/org/elasticsearch/node/Node.java | 2 +- .../service/FileSettingsService.java | 3 +-- .../service/FileSettingsServiceTests.java | 3 ++- 4 files changed, 5 insertions(+), 5 deletions(-) rename server/src/main/java/org/elasticsearch/{operator => reservedstate}/service/FileSettingsService.java (99%) rename server/src/test/java/org/elasticsearch/{operator => reservedstate}/service/FileSettingsServiceTests.java (98%) diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 4a440558becfb..679e3964f0231 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -92,7 +92,7 @@ import org.elasticsearch.monitor.process.ProcessService; import org.elasticsearch.node.Node; import org.elasticsearch.node.NodeRoleSettings; -import org.elasticsearch.operator.service.FileSettingsService; +import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.persistent.PersistentTasksClusterService; import org.elasticsearch.persistent.decider.EnableAssignmentDecider; import org.elasticsearch.plugins.PluginsService; diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 2ce448f2f81de..2d52181a9adcb 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -135,7 +135,7 @@ import org.elasticsearch.monitor.MonitorService; import org.elasticsearch.monitor.fs.FsHealthService; import org.elasticsearch.monitor.jvm.JvmInfo; -import org.elasticsearch.operator.service.FileSettingsService; +import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.persistent.PersistentTasksClusterService; import org.elasticsearch.persistent.PersistentTasksExecutor; import org.elasticsearch.persistent.PersistentTasksExecutorRegistry; diff --git a/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java similarity index 99% rename from server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java rename to server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 2d327ef495476..7382018457211 100644 --- a/server/src/main/java/org/elasticsearch/operator/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -package org.elasticsearch.operator.service; +package org.elasticsearch.reservedstate.service; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -18,7 +18,6 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.core.PathUtils; import org.elasticsearch.env.Environment; -import org.elasticsearch.reservedstate.service.ReservedClusterStateService; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; import org.elasticsearch.xcontent.XContentType; diff --git a/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java similarity index 98% rename from server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java rename to server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java index d66ab2fb71c68..2b22570946d24 100644 --- a/server/src/test/java/org/elasticsearch/operator/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -package org.elasticsearch.operator.service; +package org.elasticsearch.reservedstate.service; import org.elasticsearch.cluster.routing.RerouteService; import org.elasticsearch.cluster.service.ClusterService; @@ -14,6 +14,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; +import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.reservedstate.service.ReservedClusterStateService; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; From ecf883437bb74758d10ccfd6f9f663bae37465a9 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 20 Jul 2022 13:04:50 -0400 Subject: [PATCH 15/27] Fix MockNode --- .../common/settings/ClusterSettings.java | 2 +- .../java/org/elasticsearch/node/Node.java | 2 +- .../service/FileSettingsServiceTests.java | 2 - .../plugins/MockPluginsService.java | 43 +++++++++++++++++++ 4 files changed, 45 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 679e3964f0231..1792b21a17217 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -92,12 +92,12 @@ import org.elasticsearch.monitor.process.ProcessService; import org.elasticsearch.node.Node; import org.elasticsearch.node.NodeRoleSettings; -import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.persistent.PersistentTasksClusterService; import org.elasticsearch.persistent.decider.EnableAssignmentDecider; import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.readiness.ReadinessService; import org.elasticsearch.repositories.fs.FsRepository; +import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.SearchModule; diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 2d52181a9adcb..46af3b43c650e 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -135,7 +135,6 @@ import org.elasticsearch.monitor.MonitorService; import org.elasticsearch.monitor.fs.FsHealthService; import org.elasticsearch.monitor.jvm.JvmInfo; -import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.persistent.PersistentTasksClusterService; import org.elasticsearch.persistent.PersistentTasksExecutor; import org.elasticsearch.persistent.PersistentTasksExecutorRegistry; @@ -167,6 +166,7 @@ import org.elasticsearch.reservedstate.ReservedClusterStateHandler; import org.elasticsearch.reservedstate.ReservedClusterStateHandlerProvider; import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; +import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.rest.RestController; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptEngine; diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java index 2b22570946d24..71aec4ebe318c 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java @@ -14,8 +14,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; -import org.elasticsearch.reservedstate.service.FileSettingsService; -import org.elasticsearch.reservedstate.service.ReservedClusterStateService; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; diff --git a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java index 60c10f430c911..47abb96b3963b 100644 --- a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java +++ b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java @@ -14,12 +14,16 @@ import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; +import org.elasticsearch.plugins.spi.SPIClassIterator; +import java.lang.reflect.Constructor; import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; public class MockPluginsService extends PluginsService { @@ -76,4 +80,43 @@ public PluginsAndModules info() { List.of() ); } + + @Override + public List loadServiceProviders(Class service) { + // We use a set here to avoid duplicates because SPIClassIterator will match + // all plugins in MockNode, because all plugins are loaded by the same class loader. + Set result = new HashSet<>(); + + for (LoadedPlugin pluginTuple : plugins()) { + result.addAll(createExtensions(service, pluginTuple.instance())); + } + + return result.stream().toList(); + } + + /** + * When we load tests with MockNode, all plugins are loaded with the same class loader, + * which breaks loading service providers with our SPIClassIterator. Since all plugins are + * loaded in the same class loader, we find all plugins for any class found by the SPIClassIterator + * causing us to pass wrong plugin type to createExtension. This modified createExtensions, checks for + * the type and returns an empty list if the plugin class type is incompatible. + */ + private static List createExtensions(Class extensionPointType, Plugin plugin) { + SPIClassIterator classIterator = SPIClassIterator.get(extensionPointType, plugin.getClass().getClassLoader()); + List extensions = new ArrayList<>(); + while (classIterator.hasNext()) { + Class extensionClass = classIterator.next(); + + @SuppressWarnings("unchecked") + Constructor[] constructors = (Constructor[]) extensionClass.getConstructors(); + + for (var constructor : constructors) { + if (constructor.getParameterCount() == 1 && constructor.getParameterTypes()[0] != plugin.getClass()) { + return Collections.emptyList(); + } + } + extensions.add(createExtension(extensionClass, extensionPointType, plugin)); + } + return extensions; + } } From 7de8408b705351e28d1f7aeecd6980114a38a4c7 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 20 Jul 2022 15:51:23 -0400 Subject: [PATCH 16/27] Fix tests --- .../org/elasticsearch/action/ActionModuleTests.java | 10 ++++++---- .../elasticsearch/xpack/security/SecurityTests.java | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index da11d1295ec82..bdac66ba98acf 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Settings; @@ -50,6 +51,7 @@ import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.startsWith; +import static org.mockito.Mockito.mock; public class ActionModuleTests extends ESTestCase { public void testSetupActionsContainsKnownBuiltin() { @@ -118,7 +120,7 @@ public void testSetupRestHandlerContainsKnownBuiltin() { null, usageService, null, - null, + mock(ClusterService.class), Collections.emptyList() ); actionModule.initRestHandlers(null); @@ -176,7 +178,7 @@ public String getName() { null, usageService, null, - null, + mock(ClusterService.class), Collections.emptyList() ); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); @@ -227,7 +229,7 @@ public List getRestHandlers( null, usageService, null, - null, + mock(ClusterService.class), Collections.emptyList() ); actionModule.initRestHandlers(null); @@ -273,7 +275,7 @@ public void test3rdPartyHandlerIsNotInstalled() { null, usageService, null, - null, + mock(ClusterService.class), Collections.emptyList() ) ); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index daae6e77bd965..4e739c1111042 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -779,7 +779,7 @@ public void testSecurityRestHandlerInterceptorCanBeInstalled() throws IllegalAcc null, usageService, null, - null, + mock(ClusterService.class), Collections.emptyList() ); actionModule.initRestHandlers(null); From 31e0df15971dd537f358bcd6ceab13b63d13f4b0 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 20 Jul 2022 20:58:58 -0400 Subject: [PATCH 17/27] Clean-up duplicate error reporting Also, make sure we check error state version on all error paths, before we update the error state in the cluster state. --- .../service/ReservedClusterStateService.java | 43 +++++++++++---- .../service/ReservedStateUpdateTask.java | 16 +----- .../ReservedClusterStateServiceTests.java | 53 +++++++++++-------- 3 files changed, 67 insertions(+), 45 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java index 1c9bffc269de7..db71380376b2c 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java @@ -104,7 +104,7 @@ public void process(String namespace, XContentParser parser, Consumer } catch (Exception e) { ErrorState errorState = new ErrorState(namespace, -1L, e, ReservedStateErrorMetadata.ErrorKind.PARSING); saveErrorState(errorState); - logger.error("error processing state change request for [{}] with the following errors [{}]", namespace, errorState); + logger.debug("error processing state change request for [{}] with the following errors [{}]", namespace, errorState); errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); return; @@ -123,7 +123,7 @@ public void process(String namespace, XContentParser parser, Consumer */ public void process(String namespace, ReservedStateChunk reservedStateChunk, Consumer errorListener) { Map reservedState = reservedStateChunk.state(); - ReservedStateVersion reservedStateVersion = reservedStateChunk.metadata(); + final ReservedStateVersion reservedStateVersion = reservedStateChunk.metadata(); LinkedHashSet orderedHandlers; try { @@ -137,7 +137,7 @@ public void process(String namespace, ReservedStateChunk reservedStateChunk, Con ); saveErrorState(errorState); - logger.error("error processing state change request for [{}] with the following errors [{}]", namespace, errorState); + logger.debug("error processing state change request for [{}] with the following errors [{}]", namespace, errorState); errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); return; @@ -166,8 +166,11 @@ public void onResponse(ActionResponse.Empty empty) { @Override public void onFailure(Exception e) { - logger.error("Failed to apply reserved cluster state", e); - errorListener.accept(e); + // Don't spam the logs on repeated errors + if (isNewError(existingMetadata, reservedStateVersion.version())) { + logger.debug("Failed to apply reserved cluster state", e); + errorListener.accept(e); + } } } ), @@ -209,13 +212,35 @@ static boolean checkMetadataVersion( return true; } - private void saveErrorState(ErrorState state) { + // package private for testing + static boolean isNewError(ReservedStateMetadata existingMetadata, Long newStateVersion) { + return (existingMetadata == null + || existingMetadata.errorMetadata() == null + || existingMetadata.errorMetadata().version() < newStateVersion); + } + + private void saveErrorState(ErrorState errorState) { + ClusterState clusterState = clusterService.state(); + ReservedStateMetadata existingMetadata = clusterState.metadata().reservedStateMetadata().get(errorState.namespace()); + + if (isNewError(existingMetadata, errorState.version()) == false) { + logger.info( + () -> format( + "Not updating error state because version [%s] is less or equal to the last state error version [%s]", + errorState.version(), + existingMetadata.errorMetadata().version() + ) + ); + + return; + } + clusterService.submitStateUpdateTask( - "reserved cluster state update error for [ " + state.namespace() + "]", - new ReservedStateErrorTask(state, new ActionListener<>() { + "reserved cluster state update error for [ " + errorState.namespace() + "]", + new ReservedStateErrorTask(errorState, new ActionListener<>() { @Override public void onResponse(ActionResponse.Empty empty) { - logger.info("Successfully applied new reserved error state for namespace [{}]", state.namespace()); + logger.info("Successfully applied new reserved error state for namespace [{}]", errorState.namespace()); } @Override diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java index 421378e8ec60a..a13c7956f2901 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java @@ -98,21 +98,7 @@ protected ClusterState execute(final ClusterState currentState) { if (errors.isEmpty() == false) { // Check if we had previous error metadata with version information, don't spam with cluster state updates, if the // version hasn't been updated. - logger.error("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); - if (existingMetadata != null - && existingMetadata.errorMetadata() != null - && existingMetadata.errorMetadata().version() >= reservedStateVersion.version()) { - - logger.info( - () -> format( - "Not updating error state because version [%s] is less or equal to the last state error version [%s]", - reservedStateVersion.version(), - existingMetadata.errorMetadata().version() - ) - ); - - return currentState; - } + logger.debug("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); errorReporter.accept( new ErrorState(namespace, reservedStateVersion.version(), errors, ReservedStateErrorMetadata.ErrorKind.VALIDATION) diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java index eaf55cdb7c8c3..17de42d746365 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java @@ -278,6 +278,27 @@ public Map fromXContent(XContentParser parser) throws IOExceptio } }; + ReservedStateHandlerMetadata hmOne = new ReservedStateHandlerMetadata("one", Set.of("a", "b")); + ReservedStateErrorMetadata emOne = new ReservedStateErrorMetadata( + 1L, + ReservedStateErrorMetadata.ErrorKind.VALIDATION, + List.of("Test error 1", "Test error 2") + ); + + final ReservedStateMetadata operatorMetadata = ReservedStateMetadata.builder("namespace_one") + .errorMetadata(emOne) + .version(1L) + .putHandler(hmOne) + .build(); + + Metadata metadata = Metadata.builder().put(operatorMetadata).build(); + ClusterState state = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); + + assertFalse(ReservedClusterStateService.isNewError(operatorMetadata, 1L)); + assertFalse(ReservedClusterStateService.isNewError(operatorMetadata, 0L)); + assertTrue(ReservedClusterStateService.isNewError(operatorMetadata, 2L)); + assertTrue(ReservedClusterStateService.isNewError(null, 0L)); + // We submit a task with two handler, one will cause an exception, the other will create a new state. // When we fail to update the metadata because of version, we ensure that the returned state is equal to the // original state by pointer reference to avoid cluster state update task to run. @@ -286,7 +307,7 @@ public Map fromXContent(XContentParser parser) throws IOExceptio new ReservedStateChunk(Map.of("one", "two", "maker", "three"), new ReservedStateVersion(1L, Version.CURRENT)), Map.of(exceptionThrower.name(), exceptionThrower, newStateMaker.name(), newStateMaker), List.of(exceptionThrower.name(), newStateMaker.name()), - (errorState) -> {}, + (errorState) -> { assertFalse(ReservedClusterStateService.isNewError(operatorMetadata, errorState.version())); }, new ActionListener<>() { @Override public void onResponse(ActionResponse.Empty empty) {} @@ -296,25 +317,11 @@ public void onFailure(Exception e) {} } ); - ReservedStateHandlerMetadata hmOne = new ReservedStateHandlerMetadata("one", Set.of("a", "b")); - ReservedStateErrorMetadata emOne = new ReservedStateErrorMetadata( - 1L, - ReservedStateErrorMetadata.ErrorKind.VALIDATION, - List.of("Test error 1", "Test error 2") - ); - - ReservedStateMetadata operatorMetadata = ReservedStateMetadata.builder("namespace_one") - .errorMetadata(emOne) - .version(1L) - .putHandler(hmOne) - .build(); - - Metadata metadata = Metadata.builder().put(operatorMetadata).build(); - ClusterState state = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); - // We exit on duplicate errors before we update the cluster state error metadata - // The reference == ensures we return the same object as the current state to avoid publishing no-op state update - assertTrue(state == task.execute(state)); + assertEquals( + "Error processing state change request for namespace_one", + expectThrows(IllegalStateException.class, () -> task.execute(state)).getMessage() + ); emOne = new ReservedStateErrorMetadata( 0L, @@ -323,9 +330,13 @@ public void onFailure(Exception e) {} ); // If we are writing with older error metadata, we should get proper IllegalStateException - operatorMetadata = ReservedStateMetadata.builder("namespace_one").errorMetadata(emOne).version(0L).putHandler(hmOne).build(); + ReservedStateMetadata opMetadata = ReservedStateMetadata.builder("namespace_one") + .errorMetadata(emOne) + .version(0L) + .putHandler(hmOne) + .build(); - metadata = Metadata.builder().put(operatorMetadata).build(); + metadata = Metadata.builder().put(opMetadata).build(); ClusterState newState = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); // We exit on duplicate errors before we update the cluster state error metadata From 7877236cda65b953f623cdea521ab9e4f43a04f1 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 21 Jul 2022 12:10:38 -0400 Subject: [PATCH 18/27] Apply some PR review comments --- .../java/org/elasticsearch/node/Node.java | 12 ++-- .../service/FileSettingsService.java | 63 ++++++++++--------- .../action/ActionModuleTests.java | 9 ++- .../xpack/security/SecurityTests.java | 2 +- 4 files changed, 44 insertions(+), 42 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 46af3b43c650e..f567600051929 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -943,6 +943,12 @@ protected Node( ? new HealthMetadataService(clusterService, settings) : null; + FileSettingsService fileSettingsService = new FileSettingsService( + clusterService, + actionModule.getReservedClusterStateService(), + environment + ); + modules.add(b -> { b.bind(Node.class).toInstance(this); b.bind(NodeService.class).toInstance(nodeService); @@ -1030,17 +1036,13 @@ protected Node( b.bind(HealthNodeTaskExecutor.class).toInstance(healthNodeTaskExecutor); b.bind(HealthMetadataService.class).toInstance(healthMetadataService); } + b.bind(FileSettingsService.class).toInstance(fileSettingsService); }); if (ReadinessService.enabled(environment)) { modules.add(b -> b.bind(ReadinessService.class).toInstance(new ReadinessService(clusterService, environment))); } - modules.add( - b -> b.bind(FileSettingsService.class) - .toInstance(new FileSettingsService(clusterService, actionModule.getReservedClusterStateService(), environment)) - ); - injector = modules.createInjector(); // We allocate copies of existing shards by looking for a viable copy of the shard in the cluster and assigning the shard there. diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 7382018457211..44a95345e4586 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -20,9 +20,11 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; -import org.elasticsearch.xcontent.XContentType; +import static org.elasticsearch.xcontent.XContentType.JSON; +import java.io.BufferedInputStream; import java.io.IOException; +import java.nio.file.ClosedWatchServiceException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardWatchEventKinds; @@ -49,7 +51,7 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C private static final String NAMESPACE = "file_settings"; private final ReservedClusterStateService stateService; - private final Environment environment; + private final Path operatorSettingsDir; private WatchService watchService; // null; private CountDownLatch watcherThreadLatch; @@ -74,14 +76,16 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C */ public FileSettingsService(ClusterService clusterService, ReservedClusterStateService stateService, Environment environment) { this.stateService = stateService; - this.environment = environment; + + String dirPath = OPERATOR_DIRECTORY.get(environment.settings()); + this.operatorSettingsDir = environment.configFile().toAbsolutePath().resolve(dirPath); + clusterService.addListener(this); } // package private for testing Path operatorSettingsDir() { - String dirPath = OPERATOR_DIRECTORY.get(environment.settings()); - return environment.configFile().toAbsolutePath().resolve(dirPath); + return operatorSettingsDir; } // package private for testing @@ -156,7 +160,7 @@ synchronized void startWatcher(boolean onStartup) { Path settingsDir = operatorSettingsDir(); - /** + /* * We essentially watch for two things: * - the creation of the operator directory (if it doesn't exist), symlink changes to the operator directory * - any changes to files inside the operator directory if it exists, filtering for settings.json @@ -166,15 +170,15 @@ synchronized void startWatcher(boolean onStartup) { if (Files.exists(settingsDir)) { Path settingsFilePath = operatorSettingsFile(); if (Files.exists(settingsFilePath)) { - logger.info("found initial operator settings file [{}], applying...", settingsFilePath); + logger.debug("found initial operator settings file [{}], applying...", settingsFilePath); // we make a distinction here for startup, so that if we had operator settings before the node started // we would fail startup. processFileSettings(settingsFilePath, onStartup); } enableSettingsWatcher(settingsDir); } else { - logger.info("operator settings directory [{}] not found, will watch for its creation...", settingsDir); - enableSettingsWatcher(environment.configFile()); + logger.debug("operator settings directory [{}] not found, will watch for its creation...", settingsDir); + enableSettingsWatcher(operatorSettingsDir().getParent()); } } catch (Exception e) { if (watchService != null) { @@ -224,18 +228,17 @@ synchronized void startWatcher(boolean onStartup) { processFileSettings(path, false); } } catch (Exception e) { - logger.warn("unable to watch or read operator settings file", e); + logger.warn("error processing operator settings file", e); } } else { key.pollEvents(); key.reset(); } } - } catch (Exception e) { - if (logger.isDebugEnabled()) { - logger.debug("encountered exception watching", e); - } + } catch (ClosedWatchServiceException closedWatchServiceException) { logger.info("shutting down watcher thread"); + } catch (Exception e) { + logger.error("shutting down watcher thread with exception", e); } finally { watcherThreadLatch.countDown(); } @@ -251,7 +254,7 @@ synchronized void stopWatcher() { watcherThreadLatch.await(); } } catch (IOException | InterruptedException e) { - logger.info("encountered exception while closing watch service", e); + logger.warn("encountered exception while closing watch service", e); } finally { watchService = null; logger.info("watcher service stopped"); @@ -270,24 +273,22 @@ private void enableSettingsWatcher(Path settingsDir) throws IOException { ); } - void processFileSettings(Path path, boolean onStartup) { + void processFileSettings(Path path, boolean onStartup) throws IOException { logger.info("processing path [{}] for [{}]", path, NAMESPACE); - try ( - XContentParser parser = XContentType.JSON.xContent().createParser(XContentParserConfiguration.EMPTY, Files.newInputStream(path)) - ) { - stateService.process(NAMESPACE, parser, (e) -> { - if (e != null) { - // If we encountered an exception trying to apply the operator state at - // startup time, we throw an error to force Elasticsearch to exit. - if (onStartup) { - throw new OperatorConfigurationError("Error applying operator settings", e); - } else { - logger.error("Error processing operator settings json file", e); + try (var json = new BufferedInputStream(Files.newInputStream(path))) { + try (XContentParser parser = JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { + stateService.process(NAMESPACE, parser, (e) -> { + if (e != null) { + // If we encountered an exception trying to apply the operator state at + // startup time, we throw an error to force Elasticsearch to exit. + if (onStartup) { + throw new OperatorConfigurationError("Error applying operator settings", e); + } else { + logger.error("Error processing operator settings json file", e); + } } - } - }); - } catch (Exception e) { - logger.error("Error processing operator settings json file", e); + }); + } } } diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index bdac66ba98acf..9ca9b647becd6 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -41,7 +41,6 @@ import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.function.Supplier; import java.util.function.UnaryOperator; @@ -121,7 +120,7 @@ public void testSetupRestHandlerContainsKnownBuiltin() { usageService, null, mock(ClusterService.class), - Collections.emptyList() + List.of() ); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail @@ -179,7 +178,7 @@ public String getName() { usageService, null, mock(ClusterService.class), - Collections.emptyList() + List.of() ); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); @@ -230,7 +229,7 @@ public List getRestHandlers( usageService, null, mock(ClusterService.class), - Collections.emptyList() + List.of() ); actionModule.initRestHandlers(null); // At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail @@ -276,7 +275,7 @@ public void test3rdPartyHandlerIsNotInstalled() { usageService, null, mock(ClusterService.class), - Collections.emptyList() + List.of() ) ); assertThat( diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index 4e739c1111042..67ca91ab72b4a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -780,7 +780,7 @@ public void testSecurityRestHandlerInterceptorCanBeInstalled() throws IllegalAcc usageService, null, mock(ClusterService.class), - Collections.emptyList() + List.of() ); actionModule.initRestHandlers(null); From a702f9a0287a446c7cbacdc9491bd786ece14031 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 21 Jul 2022 21:01:22 -0400 Subject: [PATCH 19/27] Address feedback --- .../service/FileSettingsService.java | 31 ++++++++++++++----- .../service/ReservedClusterStateService.java | 8 +++-- .../service/ReservedStateUpdateTask.java | 11 +++++-- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 44a95345e4586..763b5a200d696 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -20,7 +20,6 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; -import static org.elasticsearch.xcontent.XContentType.JSON; import java.io.BufferedInputStream; import java.io.IOException; @@ -33,6 +32,8 @@ import java.nio.file.attribute.BasicFileAttributes; import java.util.concurrent.CountDownLatch; +import static org.elasticsearch.xcontent.XContentType.JSON; + /** * File based settings applier service which watches an 'operator` directory inside the config directory. *

@@ -57,6 +58,7 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C private CountDownLatch watcherThreadLatch; private volatile FileUpdateState fileUpdateState = null; + private volatile WatchKey settingsDirWatchKey = null; private volatile boolean active = false; private volatile boolean initialState = true; @@ -175,11 +177,14 @@ synchronized void startWatcher(boolean onStartup) { // we would fail startup. processFileSettings(settingsFilePath, onStartup); } - enableSettingsWatcher(settingsDir); + settingsDirWatchKey = enableSettingsWatcher(settingsDirWatchKey, settingsDir); } else { logger.debug("operator settings directory [{}] not found, will watch for its creation...", settingsDir); - enableSettingsWatcher(operatorSettingsDir().getParent()); } + // We watch the config directory always, even if initially we had an operator directory + // it can be deleted and created later. The config directory never goes away, we only + // register it once for watching. + enableSettingsWatcher(null, operatorSettingsDir().getParent()); } catch (Exception e) { if (watchService != null) { try { @@ -222,7 +227,11 @@ synchronized void startWatcher(boolean onStartup) { key.pollEvents(); key.reset(); - enableSettingsWatcher(settingsDir); + // We re-register the settings directory watch key, because we don't know + // if the file name maps to the same native file system file id. Symlinks + // are one potential cause of inconsistency here, since their handling by + // the WatchService is platform dependent. + settingsDirWatchKey = enableSettingsWatcher(settingsDirWatchKey, settingsDir); if (watchedFileChanged(path)) { processFileSettings(path, false); @@ -235,7 +244,7 @@ synchronized void startWatcher(boolean onStartup) { key.reset(); } } - } catch (ClosedWatchServiceException closedWatchServiceException) { + } catch (ClosedWatchServiceException | InterruptedException expected) { logger.info("shutting down watcher thread"); } catch (Exception e) { logger.error("shutting down watcher thread with exception", e); @@ -249,6 +258,11 @@ synchronized void stopWatcher() { logger.debug("stopping watcher ..."); if (watching()) { try { + if (settingsDirWatchKey != null) { + settingsDirWatchKey.cancel(); + settingsDirWatchKey = null; + } + fileUpdateState = null; watchService.close(); if (watcherThreadLatch != null) { watcherThreadLatch.await(); @@ -264,8 +278,11 @@ synchronized void stopWatcher() { } } - private void enableSettingsWatcher(Path settingsDir) throws IOException { - settingsDir.register( + private WatchKey enableSettingsWatcher(WatchKey previousKey, Path settingsDir) throws IOException { + if (previousKey != null) { + previousKey.cancel(); + } + return settingsDir.register( watchService, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.ENTRY_CREATE, diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java index db71380376b2c..96b5d5597f4cd 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java @@ -106,7 +106,9 @@ public void process(String namespace, XContentParser parser, Consumer saveErrorState(errorState); logger.debug("error processing state change request for [{}] with the following errors [{}]", namespace, errorState); - errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); + errorListener.accept( + new IllegalStateException("Error processing state change request for " + namespace + ", errors: " + errorState, e) + ); return; } @@ -139,7 +141,9 @@ public void process(String namespace, ReservedStateChunk reservedStateChunk, Con saveErrorState(errorState); logger.debug("error processing state change request for [{}] with the following errors [{}]", namespace, errorState); - errorListener.accept(new IllegalStateException("Error processing state change request for " + namespace, e)); + errorListener.accept( + new IllegalStateException("Error processing state change request for " + namespace + ", errors: " + errorState, e) + ); return; } diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java index a13c7956f2901..0631aee59cf6e 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateUpdateTask.java @@ -100,11 +100,16 @@ protected ClusterState execute(final ClusterState currentState) { // version hasn't been updated. logger.debug("Error processing state change request for [{}] with the following errors [{}]", namespace, errors); - errorReporter.accept( - new ErrorState(namespace, reservedStateVersion.version(), errors, ReservedStateErrorMetadata.ErrorKind.VALIDATION) + var errorState = new ErrorState( + namespace, + reservedStateVersion.version(), + errors, + ReservedStateErrorMetadata.ErrorKind.VALIDATION ); - throw new IllegalStateException("Error processing state change request for " + namespace); + errorReporter.accept(errorState); + + throw new IllegalStateException("Error processing state change request for " + namespace + ", errors: " + errorState); } // remove the last error if we had previously encountered any From 837919ad407d0011a632f6628613b49ca56205a0 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 21 Jul 2022 22:16:00 -0400 Subject: [PATCH 20/27] Update tests to handle more debug info --- .../service/ReservedClusterStateServiceTests.java | 15 ++++++++------- .../ReservedLifecycleStateServiceTests.java | 5 +++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java index 17de42d746365..478ca01f2de96 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java @@ -43,6 +43,7 @@ import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -88,7 +89,7 @@ public void testOperatorController() throws IOException { controller.process("operator", parser, (e) -> x.set(e)); assertTrue(x.get() instanceof IllegalStateException); - assertEquals("Error processing state change request for operator", x.get().getMessage()); + assertThat(x.get().getMessage(), containsString("Error processing state change request for operator")); } testJSON = """ @@ -318,9 +319,9 @@ public void onFailure(Exception e) {} ); // We exit on duplicate errors before we update the cluster state error metadata - assertEquals( - "Error processing state change request for namespace_one", - expectThrows(IllegalStateException.class, () -> task.execute(state)).getMessage() + assertThat( + expectThrows(IllegalStateException.class, () -> task.execute(state)).getMessage(), + containsString("Error processing state change request for namespace_one") ); emOne = new ReservedStateErrorMetadata( @@ -340,9 +341,9 @@ public void onFailure(Exception e) {} ClusterState newState = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); // We exit on duplicate errors before we update the cluster state error metadata - assertEquals( - "Error processing state change request for namespace_one", - expectThrows(IllegalStateException.class, () -> task.execute(newState)).getMessage() + assertThat( + expectThrows(IllegalStateException.class, () -> task.execute(newState)).getMessage(), + containsString("Error processing state change request for namespace_one") ); } diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ReservedLifecycleStateServiceTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ReservedLifecycleStateServiceTests.java index 8fa8abcdd2aa7..b9e25163c8cb8 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ReservedLifecycleStateServiceTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/action/ReservedLifecycleStateServiceTests.java @@ -65,6 +65,7 @@ import java.util.function.Consumer; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doAnswer; @@ -345,7 +346,7 @@ public void testOperatorControllerFromJSONContent() throws IOException { controller.process("operator", parser, (e) -> x.set(e)); assertTrue(x.get() instanceof IllegalStateException); - assertEquals("Error processing state change request for operator", x.get().getMessage()); + assertThat(x.get().getMessage(), containsString("Error processing state change request for operator")); } Client client = mock(Client.class); @@ -410,7 +411,7 @@ public void testOperatorControllerWithPluginPackage() { controller.process("operator", pack, (e) -> x.set(e)); assertTrue(x.get() instanceof IllegalStateException); - assertEquals("Error processing state change request for operator", x.get().getMessage()); + assertThat(x.get().getMessage(), containsString("Error processing state change request for operator")); Client client = mock(Client.class); when(client.settings()).thenReturn(Settings.EMPTY); From bdee9c9bc150f6caf8bdd79b34fab3ac3b17eda3 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 22 Jul 2022 18:50:23 -0400 Subject: [PATCH 21/27] Add integration test --- .../service/FileSettingsServiceIT.java | 240 ++++++++++++++++++ .../service/FileSettingsService.java | 2 +- .../elasticsearch/test/ESIntegTestCase.java | 4 + 3 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java diff --git a/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java new file mode 100644 index 0000000000000..f4a9d2993d188 --- /dev/null +++ b/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java @@ -0,0 +1,240 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.reservedstate.service; + +import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; +import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; +import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse; +import org.elasticsearch.client.internal.Client; +import org.elasticsearch.cluster.ClusterChangedEvent; +import org.elasticsearch.cluster.ClusterStateListener; +import org.elasticsearch.cluster.metadata.ReservedStateErrorMetadata; +import org.elasticsearch.cluster.metadata.ReservedStateHandlerMetadata; +import org.elasticsearch.cluster.metadata.ReservedStateMetadata; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.Strings; +import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; +import org.elasticsearch.test.ESIntegTestCase; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING; +import static org.elasticsearch.test.NodeRoles.dataOnlyNode; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + +@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, autoManageMasterNodes = false) +public class FileSettingsServiceIT extends ESIntegTestCase { + + private AtomicLong versionCounter = new AtomicLong(1); + + private static String testJSON = """ + { + "metadata": { + "version": "%s", + "compatibility": "8.4.0" + }, + "state": { + "cluster_settings": { + "indices.recovery.max_bytes_per_sec": "50mb" + } + } + }"""; + + private static String testErrorJSON = """ + { + "metadata": { + "version": "%s", + "compatibility": "8.4.0" + }, + "state": { + "not_cluster_settings": { + "search.allow_expensive_queries": "false" + } + } + }"""; + + private void assertMasterNode(Client client, String node) { + assertThat( + client.admin().cluster().prepareState().execute().actionGet().getState().nodes().getMasterNode().getName(), + equalTo(node) + ); + } + + private void writeJSONFile(String node, String json) throws Exception { + long version = versionCounter.incrementAndGet(); + + FileSettingsService fileSettingsService = internalCluster().getInstance(FileSettingsService.class, node); + + Files.createDirectories(fileSettingsService.operatorSettingsDir()); + Files.write(fileSettingsService.operatorSettingsFile(), Strings.format(json, version).getBytes(StandardCharsets.UTF_8)); + } + + private CountDownLatch setupClusterStateListener(String node) { + ClusterService clusterService = internalCluster().clusterService(node); + CountDownLatch savedClusterState = new CountDownLatch(1); + clusterService.addListener(new ClusterStateListener() { + @Override + public void clusterChanged(ClusterChangedEvent event) { + ReservedStateMetadata reservedState = event.state().metadata().reservedStateMetadata().get(FileSettingsService.NAMESPACE); + if (reservedState != null) { + ReservedStateHandlerMetadata handlerMetadata = reservedState.handlers().get(ReservedClusterSettingsAction.NAME); + if (handlerMetadata == null) { + fail("Should've found cluster settings in this metadata"); + } + assertThat(handlerMetadata.keys(), contains("indices.recovery.max_bytes_per_sec")); + clusterService.removeListener(this); + savedClusterState.countDown(); + } + } + }); + + return savedClusterState; + } + + private void assertClusterStateSaveOK(CountDownLatch savedClusterState) throws Exception { + boolean awaitSuccessful = savedClusterState.await(20, TimeUnit.SECONDS); + assertTrue(awaitSuccessful); + + final ClusterStateResponse clusterStateResponse = client().admin().cluster().state(new ClusterStateRequest()).actionGet(); + + assertThat( + clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey()), + equalTo("50mb") + ); + + ClusterUpdateSettingsRequest req = new ClusterUpdateSettingsRequest().persistentSettings( + Settings.builder().put(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey(), "1234kb") + ); + assertEquals( + "java.lang.IllegalArgumentException: Failed to process request " + + "[org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest/unset] " + + "with errors: [[indices.recovery.max_bytes_per_sec] set as read-only by [file_settings]]", + expectThrows(ExecutionException.class, () -> client().admin().cluster().updateSettings(req).get()).getMessage() + ); + } + + public void testSettingsApplied() throws Exception { + internalCluster().setBootstrapMasterNodeIndex(0); + logger.info("--> start data node / non master node"); + String dataNode = internalCluster().startNode(Settings.builder().put(dataOnlyNode()).put("discovery.initial_state_timeout", "1s")); + FileSettingsService dataFileSettingsService = internalCluster().getInstance(FileSettingsService.class, dataNode); + + assertFalse(dataFileSettingsService.watching()); + + logger.info("--> start master node"); + final String masterNode = internalCluster().startMasterOnlyNode(); + assertMasterNode(internalCluster().nonMasterClient(), masterNode); + var savedClusterState = setupClusterStateListener(masterNode); + + FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode); + + assertTrue(masterFileSettingsService.watching()); + assertFalse(dataFileSettingsService.watching()); + + writeJSONFile(masterNode, testJSON); + assertClusterStateSaveOK(savedClusterState); + } + + public void testSettingsAppliedOnStart() throws Exception { + internalCluster().setBootstrapMasterNodeIndex(0); + logger.info("--> start data node / non master node"); + String dataNode = internalCluster().startNode(Settings.builder().put(dataOnlyNode()).put("discovery.initial_state_timeout", "1s")); + FileSettingsService dataFileSettingsService = internalCluster().getInstance(FileSettingsService.class, dataNode); + + assertFalse(dataFileSettingsService.watching()); + var savedClusterState = setupClusterStateListener(dataNode); + + // In internal cluster tests, the nodes share the config directory, so when we write with the data node path + // the master will pick it up on start + writeJSONFile(dataNode, testJSON); + + logger.info("--> start master node"); + final String masterNode = internalCluster().startMasterOnlyNode(); + assertMasterNode(internalCluster().nonMasterClient(), masterNode); + + FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode); + + assertTrue(masterFileSettingsService.watching()); + assertFalse(dataFileSettingsService.watching()); + + assertClusterStateSaveOK(savedClusterState); + } + + private CountDownLatch setupClusterStateListenerForError(String node) { + ClusterService clusterService = internalCluster().clusterService(node); + CountDownLatch savedClusterState = new CountDownLatch(1); + clusterService.addListener(new ClusterStateListener() { + @Override + public void clusterChanged(ClusterChangedEvent event) { + ReservedStateMetadata reservedState = event.state().metadata().reservedStateMetadata().get(FileSettingsService.NAMESPACE); + if (reservedState != null) { + assertEquals(ReservedStateErrorMetadata.ErrorKind.PARSING, reservedState.errorMetadata().errorKind()); + assertThat(reservedState.errorMetadata().errors(), allOf(notNullValue(), hasSize(1))); + assertThat( + reservedState.errorMetadata().errors().get(0), + containsString("Missing handler definition for content key [not_cluster_settings]") + ); + clusterService.removeListener(this); + savedClusterState.countDown(); + } + } + }); + + return savedClusterState; + } + + private void assertClusterStateNotSaved(CountDownLatch savedClusterState) throws Exception { + boolean awaitSuccessful = savedClusterState.await(20, TimeUnit.SECONDS); + assertTrue(awaitSuccessful); + + final ClusterStateResponse clusterStateResponse = client().admin().cluster().state(new ClusterStateRequest()).actionGet(); + + assertThat(clusterStateResponse.getState().metadata().persistentSettings().get("search.allow_expensive_queries"), nullValue()); + + ClusterUpdateSettingsRequest req = new ClusterUpdateSettingsRequest().persistentSettings( + Settings.builder().put("search.allow_expensive_queries", "false") + ); + // This should succeed, nothing was reserved + client().admin().cluster().updateSettings(req).get(); + } + + public void testErrorSaved() throws Exception { + internalCluster().setBootstrapMasterNodeIndex(0); + logger.info("--> start data node / non master node"); + String dataNode = internalCluster().startNode(Settings.builder().put(dataOnlyNode()).put("discovery.initial_state_timeout", "1s")); + FileSettingsService dataFileSettingsService = internalCluster().getInstance(FileSettingsService.class, dataNode); + + assertFalse(dataFileSettingsService.watching()); + + logger.info("--> start master node"); + final String masterNode = internalCluster().startMasterOnlyNode(); + assertMasterNode(internalCluster().nonMasterClient(), masterNode); + var savedClusterState = setupClusterStateListenerForError(masterNode); + + FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode); + + assertTrue(masterFileSettingsService.watching()); + assertFalse(dataFileSettingsService.watching()); + + writeJSONFile(masterNode, testErrorJSON); + assertClusterStateNotSaved(savedClusterState); + } +} diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 763b5a200d696..44089758487e7 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -49,7 +49,7 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C private static final Logger logger = LogManager.getLogger(FileSettingsService.class); private static final String SETTINGS_FILE_NAME = "settings.json"; - private static final String NAMESPACE = "file_settings"; + static final String NAMESPACE = "file_settings"; private final ReservedClusterStateService stateService; private final Path operatorSettingsDir; diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java index 7d52f28d32d6c..72ba141bded21 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java @@ -910,6 +910,10 @@ public ClusterHealthStatus ensureYellow(String... indices) { return ensureColor(ClusterHealthStatus.YELLOW, TimeValue.timeValueSeconds(30), false, indices); } + public ClusterHealthStatus ensureRed(String... indices) { + return ensureColor(ClusterHealthStatus.RED, TimeValue.timeValueSeconds(30), false, indices); + } + /** * Ensures the cluster has a yellow state via the cluster health API and ensures the that cluster has no initializing shards * for the given indices From 2fe7fe5c1f87a36a3cee9433ba21e4fac614eedc Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 22 Jul 2022 22:05:44 -0400 Subject: [PATCH 22/27] Remove unused method --- .../main/java/org/elasticsearch/test/ESIntegTestCase.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java index 72ba141bded21..c2e9329c29c11 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java @@ -909,11 +909,7 @@ public ClusterHealthStatus ensureGreen(TimeValue timeout, String... indices) { public ClusterHealthStatus ensureYellow(String... indices) { return ensureColor(ClusterHealthStatus.YELLOW, TimeValue.timeValueSeconds(30), false, indices); } - - public ClusterHealthStatus ensureRed(String... indices) { - return ensureColor(ClusterHealthStatus.RED, TimeValue.timeValueSeconds(30), false, indices); - } - + /** * Ensures the cluster has a yellow state via the cluster health API and ensures the that cluster has no initializing shards * for the given indices From 8e6f84ee19a2c44a13d03fb1f2b9002039c40801 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Fri, 22 Jul 2022 22:12:24 -0400 Subject: [PATCH 23/27] Spotless --- .../src/main/java/org/elasticsearch/test/ESIntegTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java index c2e9329c29c11..7d52f28d32d6c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java @@ -909,7 +909,7 @@ public ClusterHealthStatus ensureGreen(TimeValue timeout, String... indices) { public ClusterHealthStatus ensureYellow(String... indices) { return ensureColor(ClusterHealthStatus.YELLOW, TimeValue.timeValueSeconds(30), false, indices); } - + /** * Ensures the cluster has a yellow state via the cluster health API and ensures the that cluster has no initializing shards * for the given indices From dce6e177a7043d326668ba4b02988c223fb31eae Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 25 Jul 2022 16:35:07 -0400 Subject: [PATCH 24/27] Fix tests for Windows. --- .../reservedstate/service/FileSettingsService.java | 3 +++ .../reservedstate/service/FileSettingsServiceTests.java | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 44089758487e7..1bd70b324c339 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -213,6 +213,9 @@ synchronized void startWatcher(boolean onStartup) { * ENTRY_MODIFY:settings.json * Linux in Docker symlink delete and set (rm -rf operator && ln -s /file_settings/ operator): * ENTRY_CREATE:operator + * Windows + * ENTRY_CREATE:operator + * ENTRY_MODIFY:operator * After we get an indication that something has changed, we check the timestamp, file id, * real path of our desired file. */ diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java index 71aec4ebe318c..91685fcbcdaff 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java @@ -20,6 +20,7 @@ import org.elasticsearch.xcontent.XContentParser; import org.junit.After; import org.junit.Before; +import org.mockito.Mockito; import org.mockito.stubbing.Answer; import java.nio.charset.StandardCharsets; @@ -144,10 +145,11 @@ public void testCallsProcessing() throws Exception { Files.write(service.operatorSettingsFile(), "{}".getBytes(StandardCharsets.UTF_8)); // we need to wait a bit, on MacOS it may take up to 10 seconds for the Java watcher service to notice the file, - // on Linux is instantaneous. Windows??? + // on Linux is instantaneous. Windows is instantaneous too. processFileLatch.await(30, TimeUnit.SECONDS); - verify(service, times(1)).watchedFileChanged(any()); + verify(service, Mockito.atLeast(1)).watchedFileChanged(any()); + verify(service, times(1)).processFileSettings(any(), eq(false)); service.stop(); assertFalse(service.watching()); From 610b63be12d4f6906ce77e6c6c0bf0c9a194f9ba Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 25 Jul 2022 18:12:36 -0400 Subject: [PATCH 25/27] Address further feedback --- .../common/settings/ClusterSettings.java | 2 - .../java/org/elasticsearch/node/Node.java | 2 +- .../service/FileSettingsService.java | 65 ++++++++++--------- .../service/FileSettingsServiceTests.java | 58 ++++++++++++----- 4 files changed, 76 insertions(+), 51 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 1792b21a17217..c4024e0ba543d 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -97,7 +97,6 @@ import org.elasticsearch.plugins.PluginsService; import org.elasticsearch.readiness.ReadinessService; import org.elasticsearch.repositories.fs.FsRepository; -import org.elasticsearch.reservedstate.service.FileSettingsService; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.SearchModule; @@ -525,7 +524,6 @@ public void apply(Settings value, Settings current, Settings previous) { CoordinationDiagnosticsService.NODE_HAS_MASTER_LOOKUP_TIMEFRAME_SETTING, MasterHistory.MAX_HISTORY_AGE_SETTING, ReadinessService.PORT, - FileSettingsService.OPERATOR_DIRECTORY, HealthNode.isEnabled() ? HealthNodeTaskExecutor.ENABLED_SETTING : null ).filter(Objects::nonNull).collect(Collectors.toSet()); diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 4fd56cf60eda2..4a53bbd10e31c 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -1194,7 +1194,6 @@ public Node start() throws NodeValidationException { if (ReadinessService.enabled(environment)) { injector.getInstance(ReadinessService.class).start(); } - injector.getInstance(FileSettingsService.class).start(); injector.getInstance(MappingUpdatedAction.class).setClient(client); injector.getInstance(IndicesService.class).start(); injector.getInstance(IndicesClusterStateService.class).start(); @@ -1306,6 +1305,7 @@ public void onTimeout(TimeValue timeout) { } } + injector.getInstance(FileSettingsService.class).start(); injector.getInstance(HttpServerTransport.class).start(); if (WRITE_PORTS_FILE_SETTING.get(settings())) { diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 1bd70b324c339..7518e62a2af29 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -15,8 +15,6 @@ import org.elasticsearch.cluster.ClusterStateListener; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.component.AbstractLifecycleComponent; -import org.elasticsearch.common.settings.Setting; -import org.elasticsearch.core.PathUtils; import org.elasticsearch.env.Environment; import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.XContentParserConfiguration; @@ -31,6 +29,7 @@ import java.nio.file.WatchService; import java.nio.file.attribute.BasicFileAttributes; import java.util.concurrent.CountDownLatch; +import java.util.function.Consumer; import static org.elasticsearch.xcontent.XContentType.JSON; @@ -51,6 +50,7 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C private static final String SETTINGS_FILE_NAME = "settings.json"; static final String NAMESPACE = "file_settings"; + private final ClusterService clusterService; private final ReservedClusterStateService stateService; private final Path operatorSettingsDir; @@ -63,11 +63,7 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C private volatile boolean active = false; private volatile boolean initialState = true; - public static final Setting OPERATOR_DIRECTORY = Setting.simpleString( - "path.config.operator_directory", - "operator", - Setting.Property.NodeScope - ); + public static final String OPERATOR_DIRECTORY = "operator"; /** * Constructs the {@link FileSettingsService} @@ -77,12 +73,9 @@ public class FileSettingsService extends AbstractLifecycleComponent implements C * @param environment we need the environment to pull the location of the config and operator directories */ public FileSettingsService(ClusterService clusterService, ReservedClusterStateService stateService, Environment environment) { + this.clusterService = clusterService; this.stateService = stateService; - - String dirPath = OPERATOR_DIRECTORY.get(environment.settings()); - this.operatorSettingsDir = environment.configFile().toAbsolutePath().resolve(dirPath); - - clusterService.addListener(this); + this.operatorSettingsDir = environment.configFile().toAbsolutePath().resolve(OPERATOR_DIRECTORY); } // package private for testing @@ -113,9 +106,11 @@ boolean watchedFileChanged(Path path) throws IOException { @Override protected void doStart() { // We start the file watcher when we know we are master from a cluster state change notification. - // We need this additional flag, since cluster state can change after we've shutdown the service + // We need the additional active flag, since cluster state can change after we've shutdown the service // causing the watcher to start again. this.active = true; + startIfMaster(clusterService.state()); + clusterService.addListener(this); } @Override @@ -129,12 +124,16 @@ protected void doStop() { protected void doClose() {} private boolean currentNodeMaster(ClusterState clusterState) { - return clusterState.nodes().getMasterNodeId().equals(clusterState.nodes().getLocalNodeId()); + return clusterState.nodes().getLocalNodeId().equals(clusterState.nodes().getMasterNodeId()); } @Override public void clusterChanged(ClusterChangedEvent event) { ClusterState clusterState = event.state(); + startIfMaster(clusterState); + } + + private void startIfMaster(ClusterState clusterState) { setWatching(currentNodeMaster(clusterState), initialState); initialState = false; } @@ -168,14 +167,20 @@ synchronized void startWatcher(boolean onStartup) { * - any changes to files inside the operator directory if it exists, filtering for settings.json */ try { - this.watchService = PathUtils.getDefaultFileSystem().newWatchService(); + this.watchService = operatorSettingsDir().getParent().getFileSystem().newWatchService(); if (Files.exists(settingsDir)) { Path settingsFilePath = operatorSettingsFile(); if (Files.exists(settingsFilePath)) { logger.debug("found initial operator settings file [{}], applying...", settingsFilePath); // we make a distinction here for startup, so that if we had operator settings before the node started // we would fail startup. - processFileSettings(settingsFilePath, onStartup); + processFileSettings(settingsFilePath, (e) -> { + if (onStartup) { + throw new FileSettingsStartupException("Error applying operator settings", e); + } else { + logger.error("Error processing operator settings json file", e); + } + }).await(); } settingsDirWatchKey = enableSettingsWatcher(settingsDirWatchKey, settingsDir); } else { @@ -237,10 +242,10 @@ synchronized void startWatcher(boolean onStartup) { settingsDirWatchKey = enableSettingsWatcher(settingsDirWatchKey, settingsDir); if (watchedFileChanged(path)) { - processFileSettings(path, false); + processFileSettings(path, (e) -> logger.error("Error processing operator settings json file", e)).await(); } - } catch (Exception e) { - logger.warn("error processing operator settings file", e); + } catch (IOException e) { + logger.warn("encountered I/O error while watching file settings", e); } } else { key.pollEvents(); @@ -270,8 +275,10 @@ synchronized void stopWatcher() { if (watcherThreadLatch != null) { watcherThreadLatch.await(); } - } catch (IOException | InterruptedException e) { + } catch (IOException e) { logger.warn("encountered exception while closing watch service", e); + } catch (InterruptedException interruptedException) { + logger.info("interrupted while closing the watch service", interruptedException); } finally { watchService = null; logger.info("watcher service stopped"); @@ -293,23 +300,21 @@ private WatchKey enableSettingsWatcher(WatchKey previousKey, Path settingsDir) t ); } - void processFileSettings(Path path, boolean onStartup) throws IOException { + CountDownLatch processFileSettings(Path path, Consumer errorHandler) throws IOException { + CountDownLatch waitForCompletion = new CountDownLatch(1); logger.info("processing path [{}] for [{}]", path, NAMESPACE); try (var json = new BufferedInputStream(Files.newInputStream(path))) { try (XContentParser parser = JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { stateService.process(NAMESPACE, parser, (e) -> { if (e != null) { - // If we encountered an exception trying to apply the operator state at - // startup time, we throw an error to force Elasticsearch to exit. - if (onStartup) { - throw new OperatorConfigurationError("Error applying operator settings", e); - } else { - logger.error("Error processing operator settings json file", e); - } + errorHandler.accept(e); } + waitForCompletion.countDown(); }); } } + + return waitForCompletion; } /** @@ -322,8 +327,8 @@ record FileUpdateState(long timestamp, String path, Object fileKey) {} * Error subclass that is thrown when we encounter a fatal error while applying * the operator cluster state at Elasticsearch boot time. */ - public static class OperatorConfigurationError extends Error { - public OperatorConfigurationError(String message, Throwable t) { + public static class FileSettingsStartupException extends RuntimeException { + public FileSettingsStartupException(String message, Throwable t) { super(message, t); } } diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java index 91685fcbcdaff..9db5bba768a3b 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java @@ -8,6 +8,11 @@ package org.elasticsearch.reservedstate.service; +import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.routing.RerouteService; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.settings.ClusterSettings; @@ -36,9 +41,10 @@ import java.util.concurrent.TimeUnit; import java.util.function.Consumer; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.hasToString; +import static org.hamcrest.Matchers.instanceOf; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -58,12 +64,21 @@ public void setUp() throws Exception { threadpool = new TestThreadPool("file_settings_service_tests"); - clusterService = new ClusterService( - Settings.EMPTY, - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - threadpool, - null + clusterService = spy( + new ClusterService( + Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), + threadpool, + null + ) ); + + final DiscoveryNode localNode = new DiscoveryNode("node", buildNewFakeTransportAddress(), Version.CURRENT); + final ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT) + .nodes(DiscoveryNodes.builder().add(localNode).localNodeId(localNode.getId()).masterNodeId(localNode.getId())) + .build(); + doAnswer((Answer) invocation -> clusterState).when(clusterService).state(); + clusterService.setRerouteService(mock(RerouteService.class)); env = newEnvironment(Settings.EMPTY); @@ -120,7 +135,6 @@ public void testWatchedFile() throws Exception { public void testStartStop() { fileSettingsService.start(); - fileSettingsService.startWatcher(false); assertTrue(fileSettingsService.watching()); fileSettingsService.stop(); assertFalse(fileSettingsService.watching()); @@ -134,10 +148,9 @@ public void testCallsProcessing() throws Exception { doAnswer((Answer) invocation -> { processFileLatch.countDown(); return null; - }).when(service).processFileSettings(any(), anyBoolean()); + }).when(service).processFileSettings(any(), any()); service.start(); - service.startWatcher(false); assertTrue(service.watching()); Files.createDirectories(service.operatorSettingsDir()); @@ -149,7 +162,7 @@ public void testCallsProcessing() throws Exception { processFileLatch.await(30, TimeUnit.SECONDS); verify(service, Mockito.atLeast(1)).watchedFileChanged(any()); - verify(service, times(1)).processFileSettings(any(), eq(false)); + verify(service, times(1)).processFileSettings(any(), any()); service.stop(); assertFalse(service.watching()); @@ -172,25 +185,34 @@ public void testInitialFile() throws Exception { // contents of the JSON don't matter, we just need a file to exist Files.write(service.operatorSettingsFile(), "{}".getBytes(StandardCharsets.UTF_8)); - service.start(); - assertEquals( - "Error applying operator settings", - expectThrows(FileSettingsService.OperatorConfigurationError.class, () -> service.startWatcher(true)).getMessage() + Exception startupException = expectThrows(IllegalStateException.class, () -> service.start()); + assertThat( + startupException.getCause(), + allOf( + instanceOf(FileSettingsService.FileSettingsStartupException.class), + hasToString( + "org.elasticsearch.reservedstate.service.FileSettingsService$FileSettingsStartupException: " + + "Error applying operator settings" + ) + ) ); - verify(service, times(1)).processFileSettings(any(), eq(true)); + verify(service, times(1)).processFileSettings(any(), any()); service.stop(); clearInvocations(service); // Let's check that if we didn't throw an error that everything works - doAnswer((Answer) invocation -> null).when(stateService).process(any(), (XContentParser) any(), any()); + doAnswer((Answer) invocation -> { + ((Consumer) invocation.getArgument(2)).accept(null); + return null; + }).when(stateService).process(any(), (XContentParser) any(), any()); service.start(); service.startWatcher(true); - verify(service, times(1)).processFileSettings(any(), eq(true)); + verify(service, times(1)).processFileSettings(any(), any()); service.stop(); service.close(); From 6552723e3f51d220c33222eaaeea6a22ce4ac658 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 25 Jul 2022 19:08:53 -0400 Subject: [PATCH 26/27] Fix issue with missing config dir in tests --- .../reservedstate/service/FileSettingsService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java index 7518e62a2af29..a8141e8f711fa 100644 --- a/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java +++ b/server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java @@ -108,7 +108,7 @@ protected void doStart() { // We start the file watcher when we know we are master from a cluster state change notification. // We need the additional active flag, since cluster state can change after we've shutdown the service // causing the watcher to start again. - this.active = true; + this.active = Files.exists(operatorSettingsDir().getParent()); startIfMaster(clusterService.state()); clusterService.addListener(this); } From 50e2fe16693ac171f1a57df65456d401e3510ba6 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 25 Jul 2022 20:13:42 -0400 Subject: [PATCH 27/27] Fix merge issue --- .../java/org/elasticsearch/xpack/security/SecurityTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index f4765b52ef23f..8f5e156a1cfd9 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -780,7 +780,7 @@ public void testSecurityRestHandlerInterceptorCanBeInstalled() throws IllegalAcc null, usageService, null, - Tracer.NOOP + Tracer.NOOP, mock(ClusterService.class), List.of() );