From ff517c57c4cbb4fc638e855805ff0e376c39a865 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 28 Mar 2022 10:31:10 -0400 Subject: [PATCH] Replace failing PolicyStepsRegistry sanity check assert with a test (#85346) --- .../xpack/ilm/PolicyStepsRegistry.java | 3 - .../xpack/ilm/PolicyStepsRegistryTests.java | 74 +++++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) 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 a60868a6777eb..1f5c021755fe4 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 @@ -368,9 +368,6 @@ public Step getStep(final IndexMetadata indexMetadata, final Step.StepKey stepKe final Step s = phaseSteps.stream().filter(step -> step.getKey().equals(stepKey)).findFirst().orElse(null); if (s != null) { cachedSteps.put(indexMetadata.getIndex(), Tuple.tuple(indexMetadata, s)); - // assert that the cache works as expected -- that is, if we put something into the cache, - // we should get back the same thing if we were to invoke getStep again with the same arguments - assert s == getCachedStep(indexMetadata, stepKey) : "policy step registry cache failed sanity check"; } return s; } diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistryTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistryTests.java index 1eb5dbabf8be2..fb16414c5e752 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistryTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/PolicyStepsRegistryTests.java @@ -50,6 +50,8 @@ import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; import static org.elasticsearch.cluster.metadata.LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY; import static org.hamcrest.Matchers.containsString; @@ -465,4 +467,76 @@ public void testUpdatePolicyButNoPhaseChangeIndexStepsDontChange() throws Except assertThat(((ShrinkStep) shrinkStep).getNumberOfShards(), equalTo(2)); assertThat(((ShrinkStep) gotStep).getNumberOfShards(), equalTo(1)); } + + public void testGetStepMultithreaded() throws Exception { + Client client = mock(Client.class); + Mockito.when(client.settings()).thenReturn(Settings.EMPTY); + + LifecyclePolicy policy = LifecyclePolicyTests.randomTimeseriesLifecyclePolicyWithAllPhases("policy"); + String phaseName = randomFrom(policy.getPhases().keySet()); + Phase phase = policy.getPhases().get(phaseName); + + LifecycleExecutionState lifecycleState = LifecycleExecutionState.builder() + .setPhaseDefinition(Strings.toString(new PhaseExecutionInfo(policy.getName(), phase, 1, randomNonNegativeLong()))) + .build(); + IndexMetadata indexMetadata = IndexMetadata.builder("test") + .settings( + Settings.builder() + .put("index.number_of_shards", 1) + .put("index.number_of_replicas", 0) + .put("index.version.created", Version.CURRENT) + .put(LifecycleSettings.LIFECYCLE_NAME, "policy") + .build() + ) + .putCustom(ILM_CUSTOM_METADATA_KEY, lifecycleState.asMap()) + .build(); + + SortedMap metas = new TreeMap<>(); + metas.put("policy", new LifecyclePolicyMetadata(policy, Collections.emptyMap(), 1, randomNonNegativeLong())); + IndexLifecycleMetadata meta = new IndexLifecycleMetadata(metas, OperationMode.RUNNING); + + PolicyStepsRegistry registry = new PolicyStepsRegistry(REGISTRY, client, null); + registry.update(meta); + + // test a variety of getStep calls with random actions and steps + for (int i = 0; i < scaledRandomIntBetween(100, 1000); i++) { + LifecycleAction action = randomValueOtherThan(MigrateAction.DISABLED, () -> randomFrom(phase.getActions().values())); + Step step = randomFrom(action.toSteps(client, phaseName, MOCK_STEP_KEY, null)); + // if the step's key is different from the previous iteration of the loop, then the cache will be updated, and we'll + // get a non-cached response. if the step's key happens to be the same as the previous iteration of the loop, then + // we'll get a cached response. so this loop randomly tests both cached and non-cached responses. + Step actualStep = registry.getStep(indexMetadata, step.getKey()); + assertThat(actualStep.getKey(), equalTo(step.getKey())); + } + + final CountDownLatch latch = new CountDownLatch(1); + final AtomicBoolean done = new AtomicBoolean(false); + + // now, in another thread, update the registry repeatedly as fast as possible. + // updating the registry has the side effect of clearing the cache. + Thread t = new Thread(() -> { + latch.countDown(); // signal that we're starting + while (done.get() == false) { + registry.update(meta); + } + }); + t.start(); + + try { + latch.await(); // wait until the other thread started + + // and, while the cache is being repeatedly cleared, + // test a variety of getStep calls with random actions and steps + for (int i = 0; i < scaledRandomIntBetween(100, 1000); i++) { + LifecycleAction action = randomValueOtherThan(MigrateAction.DISABLED, () -> randomFrom(phase.getActions().values())); + Step step = randomFrom(action.toSteps(client, phaseName, MOCK_STEP_KEY, null)); + Step actualStep = registry.getStep(indexMetadata, step.getKey()); + assertThat(actualStep.getKey(), equalTo(step.getKey())); + } + } finally { + // tell the other thread we're finished and wait for it to die + done.set(true); + t.join(1000); + } + } }