From 23d02cc64d9d25ae761ae8f300b8096e03eb1106 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sat, 23 Oct 2021 23:22:39 +0200 Subject: [PATCH 01/12] Cache for getStep --- .../xpack/ilm/IndexLifecycleService.java | 3 +++ .../xpack/ilm/PolicyStepsRegistry.java | 16 +++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java index 813003afb3b5b..1f767a1e64964 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java @@ -322,6 +322,9 @@ public void applyClusterState(ClusterChangedEvent event) { policyRegistry.update(ilmMetadata); } } + for (Index index : event.indicesDeleted()) { + policyRegistry.delete(index); + } } private void cancelJob() { diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 7d7020d566f66..5ba5cbc3cdecf 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -18,6 +18,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; +import org.elasticsearch.core.Tuple; import org.elasticsearch.index.Index; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.xcontent.DeprecationHandler; @@ -49,6 +50,7 @@ import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; +import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; public class PolicyStepsRegistry { @@ -64,6 +66,8 @@ public class PolicyStepsRegistry { private final Map> stepMap; private final NamedXContentRegistry xContentRegistry; + private final Map> cachedSteps = new ConcurrentHashMap<>(); + public PolicyStepsRegistry(NamedXContentRegistry xContentRegistry, Client client, XPackLicenseState licenseState) { this(new TreeMap<>(), new HashMap<>(), new HashMap<>(), xContentRegistry, client, licenseState); } @@ -155,6 +159,10 @@ public LifecyclePolicyMetadata read(StreamInput in, String key) { } } + public void delete(Index deleted) { + cachedSteps.remove(deleted); + } + /** * Return all ordered steps for the current policy for the index. Does not * resolve steps using the phase caching, but only for the currently existing policy. @@ -267,6 +275,10 @@ private List parseStepsFromPhase(String policy, String currentPhase, Strin @Nullable public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKey) { + final Tuple cachedStep = cachedSteps.get(indexMetadata.getIndex()); + if (cachedStep != null && cachedStep.v1() == indexMetadata && cachedStep.v2().getKey().equals(stepKey)) { + return cachedStep.v2(); + } if (ErrorStep.NAME.equals(stepKey.getName())) { return new ErrorStep(new Step.StepKey(stepKey.getPhase(), stepKey.getAction(), ErrorStep.NAME)); } @@ -305,7 +317,9 @@ public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKe + phaseSteps; // Return the step that matches the given stepKey or else null if we couldn't find it - return phaseSteps.stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null); + final Step s = phaseSteps.stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null); + cachedSteps.put(indexMetadata.getIndex(), Tuple.tuple(indexMetadata, s)); + return s; } /** From e6e61189d6f18cad0d50707b0fe6085b8b29efda Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 6 Jan 2022 09:44:02 -0500 Subject: [PATCH 02/12] Add some comments --- .../java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java index 1f767a1e64964..df3841825acff 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java @@ -298,8 +298,10 @@ public void clusterChanged(ClusterChangedEvent event) { if (prevIsMaster != event.localNodeMaster()) { this.isMaster = event.localNodeMaster(); if (this.isMaster) { + // we weren't the master, and now we are onMaster(event.state()); } else { + // we were the master, and now we aren't cancelJob(); } } From 92346ef9862f908d6ff369e606cad62a341c2427 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 6 Jan 2022 09:44:27 -0500 Subject: [PATCH 03/12] Move the delete logic from listener to applier --- .../org/elasticsearch/xpack/ilm/IndexLifecycleService.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java index df3841825acff..ca51bf0993d3f 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java @@ -306,6 +306,10 @@ public void clusterChanged(ClusterChangedEvent event) { } } + for (Index index : event.indicesDeleted()) { + policyRegistry.delete(index); + } + final IndexLifecycleMetadata lifecycleMetadata = event.state().metadata().custom(IndexLifecycleMetadata.TYPE); if (this.isMaster && lifecycleMetadata != null) { triggerPolicies(event.state(), true); @@ -324,9 +328,6 @@ public void applyClusterState(ClusterChangedEvent event) { policyRegistry.update(ilmMetadata); } } - for (Index index : event.indicesDeleted()) { - policyRegistry.delete(index); - } } private void cancelJob() { From 58ac8a08f3dae2103544838f74eae789dbd828ae Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 6 Jan 2022 10:01:38 -0500 Subject: [PATCH 04/12] Clear out the cache entirely on master->non-master --- .../xpack/ilm/IndexLifecycleService.java | 1 + .../elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java index ca51bf0993d3f..b777fcd988999 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java @@ -303,6 +303,7 @@ public void clusterChanged(ClusterChangedEvent event) { } else { // we were the master, and now we aren't cancelJob(); + policyRegistry.clear(); } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 5ba5cbc3cdecf..1918cd8192f58 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -163,6 +163,18 @@ public void delete(Index deleted) { cachedSteps.remove(deleted); } + /** + * Clear internal maps that were populated by update (and others). + */ + public void clear() { + // this is potentially large, so it's important to clear it + cachedSteps.clear(); + // these are relatively small, but there's no harm in clearing them + lifecyclePolicyMap.clear(); + firstStepMap.clear(); + stepMap.clear(); + } + /** * Return all ordered steps for the current policy for the index. Does not * resolve steps using the phase caching, but only for the currently existing policy. From ed38398a41ca812edaa20c6c274bfb8248530891 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 6 Jan 2022 14:53:12 -0500 Subject: [PATCH 05/12] Clear out the cache any time the policies change --- .../java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 1918cd8192f58..6b52aa370d8f3 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -103,6 +103,9 @@ Map> getStepMap() { public void update(IndexLifecycleMetadata meta) { assert meta != null : "IndexLifecycleMetadata cannot be null when updating the policy steps registry"; + // since the policies (may have) changed, the whole steps cache needs to be thrown out + cachedSteps.clear(); + DiffableUtils.MapDiff> mapDiff = DiffableUtils.diff( lifecyclePolicyMap, meta.getPolicyMetadatas(), From 38a1711685780fff1d8adfa89da91042b7950295 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 12 Jan 2022 10:28:55 -0500 Subject: [PATCH 06/12] Whitespace --- .../java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 6b52aa370d8f3..74bbd20a1de5a 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -58,6 +58,7 @@ public class PolicyStepsRegistry { private final Client client; private final XPackLicenseState licenseState; + // keeps track of existing policies in the cluster state private final SortedMap lifecyclePolicyMap; // keeps track of what the first step in a policy is, the key is policy name @@ -294,6 +295,7 @@ public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKe if (cachedStep != null && cachedStep.v1() == indexMetadata && cachedStep.v2().getKey().equals(stepKey)) { return cachedStep.v2(); } + if (ErrorStep.NAME.equals(stepKey.getName())) { return new ErrorStep(new Step.StepKey(stepKey.getPhase(), stepKey.getAction(), ErrorStep.NAME)); } From de5271615b1eaa09c52dcd1153743af5d37d2d3b Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 12 Jan 2022 10:31:53 -0500 Subject: [PATCH 07/12] Put these first fields in public constructor order --- .../java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 74bbd20a1de5a..976ee86f0f4e1 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -56,6 +56,7 @@ public class PolicyStepsRegistry { private static final Logger logger = LogManager.getLogger(PolicyStepsRegistry.class); + private final NamedXContentRegistry xContentRegistry; private final Client client; private final XPackLicenseState licenseState; @@ -65,7 +66,6 @@ public class PolicyStepsRegistry { private final Map firstStepMap; // keeps track of a mapping from policy/step-name to respective Step, the key is policy name private final Map> stepMap; - private final NamedXContentRegistry xContentRegistry; private final Map> cachedSteps = new ConcurrentHashMap<>(); From 5f48e35b29ff8d9945bc5c14735409e0b84c549e Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 12 Jan 2022 10:32:14 -0500 Subject: [PATCH 08/12] Add a comment that explains this cache --- .../java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 976ee86f0f4e1..a5b7f6dc94766 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -67,6 +67,9 @@ public class PolicyStepsRegistry { // keeps track of a mapping from policy/step-name to respective Step, the key is policy name private final Map> stepMap; + // tracks an index->step cache, where the indexmetadata is also tracked for cache invalidation/eviction purposes. + // for a given index, the step can be cached as long as the indexmetadata (and the policy!) hasn't changed. since + // policies change infrequently, the entire cache is cleared on policy change. private final Map> cachedSteps = new ConcurrentHashMap<>(); public PolicyStepsRegistry(NamedXContentRegistry xContentRegistry, Client client, XPackLicenseState licenseState) { From 25605ebe14538f99abc63a213d3531550bc51a75 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 27 Jan 2022 10:29:27 -0500 Subject: [PATCH 09/12] Update docs/changelog/82316.yaml --- docs/changelog/82316.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/82316.yaml diff --git a/docs/changelog/82316.yaml b/docs/changelog/82316.yaml new file mode 100644 index 0000000000000..66dec7e1f46eb --- /dev/null +++ b/docs/changelog/82316.yaml @@ -0,0 +1,5 @@ +pr: 82316 +summary: Add an index->step cache to the `PolicyStepsRegistry` +area: ILM+SLM +type: enhancement +issues: [] From 68161e8a3b8168c5d9a64a55fe7bbbd30f0c8afe Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 27 Jan 2022 13:13:00 -0500 Subject: [PATCH 10/12] Add some explanatory comments --- .../elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 99e132617f53d..762e8e0689ebf 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -165,6 +165,14 @@ public LifecyclePolicyMetadata read(StreamInput in, String key) { } } + /** + * Remove the entry for an index from the index->step cache. + * + * We clear the map entirely when the master of the cluster changes, and when any + * policy changes, but in a long-lived cluster that doesn't happen to experience + * either of those events (and where indices are removed regularly) we still want + * the cache to trim deleted indices. + */ public void delete(Index deleted) { cachedSteps.remove(deleted); } @@ -294,6 +302,9 @@ private List parseStepsFromPhase(String policy, String currentPhase, Strin @Nullable public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKey) { final Tuple cachedStep = cachedSteps.get(indexMetadata.getIndex()); + // n.b. we're using instance equality here for the IndexMetadata rather than object equality because it's fast, + // this means that we're erring on the side of cache misses (if the IndexMetadata changed in any way, it'll be + // a new instance, so we'll miss-and-repopulate the cache for the index in question) if (cachedStep != null && cachedStep.v1() == indexMetadata && cachedStep.v2().getKey().equals(stepKey)) { return cachedStep.v2(); } From 4954725add7628418152ab31ce684983266cfcf1 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 27 Jan 2022 13:13:22 -0500 Subject: [PATCH 11/12] Surround this logic in an isMaster check Promoting the isMaster check around triggerPolicies up to include both these blocks. --- .../xpack/ilm/IndexLifecycleService.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java index 4704ebedb2baf..b10c097d79e31 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleService.java @@ -310,13 +310,16 @@ public void clusterChanged(ClusterChangedEvent event) { } } - for (Index index : event.indicesDeleted()) { - policyRegistry.delete(index); - } + // if we're the master, then process deleted indices and trigger policies + if (this.isMaster) { + for (Index index : event.indicesDeleted()) { + policyRegistry.delete(index); + } - final IndexLifecycleMetadata lifecycleMetadata = event.state().metadata().custom(IndexLifecycleMetadata.TYPE); - if (this.isMaster && lifecycleMetadata != null) { - triggerPolicies(event.state(), true); + final IndexLifecycleMetadata lifecycleMetadata = event.state().metadata().custom(IndexLifecycleMetadata.TYPE); + if (lifecycleMetadata != null) { + triggerPolicies(event.state(), true); + } } } From 19ecdd3f6860ffeb4fa097096f9f065ad4c3fcc5 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 27 Jan 2022 13:21:39 -0500 Subject: [PATCH 12/12] Add a note about a race condition --- .../org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java index 762e8e0689ebf..33ad84b517189 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistry.java @@ -172,6 +172,12 @@ public LifecyclePolicyMetadata read(StreamInput in, String key) { * policy changes, but in a long-lived cluster that doesn't happen to experience * either of those events (and where indices are removed regularly) we still want * the cache to trim deleted indices. + * + * n.b. even with this, there's still a pretty small chance that a given index + * could leak, if we're right in the middle of populating the cache for that + * index (in getStep) when we process the delete here, then we'll end up with an + * entry that doesn't get deleted until the master changes or a policy changes + * -- it's harmless enough */ public void delete(Index deleted) { cachedSteps.remove(deleted);