From a3cea019b398d1b05887ea183cc630ba5fc1db15 Mon Sep 17 00:00:00 2001 From: Niels Bauman <33722607+nielsbauman@users.noreply.github.com> Date: Thu, 10 Apr 2025 16:35:32 +0200 Subject: [PATCH] Fix equality bug in `WaitForIndexColorStep` (#126605) The `indexNameSupplier` was included in the equality and is of type `BiFunction`, which doesn't implement a proper `equals` method by default - and thus neither do the lambdas. This meant that two instances of this step would only be considered equal if they were the same instance. By excluding `indexNameSupplier` from the `equals` method, we ensure the method works as intended and is able to properly tell the equality between two instances. As a side effect, we expect/hope this change will fix a number of tests that were failing because `WaitForIndexColorStep` missed the last cluster state update in the test, causing ILM to get stuck and the test to time out. Fixes #125683 Fixes #125789 Fixes #125867 Fixes #125911 Fixes #126053 Fixes #126354 (cherry picked from commit 3231eb29267487b049c358a13e01237c465d2531) # Conflicts: # muted-tests.yml # x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java --- docs/changelog/126605.yaml | 5 ++ .../xpack/core/ilm/WaitForIndexColorStep.java | 6 +-- .../actions/SearchableSnapshotActionIT.java | 48 ++++++------------- 3 files changed, 22 insertions(+), 37 deletions(-) create mode 100644 docs/changelog/126605.yaml diff --git a/docs/changelog/126605.yaml b/docs/changelog/126605.yaml new file mode 100644 index 0000000000000..44031f5d51616 --- /dev/null +++ b/docs/changelog/126605.yaml @@ -0,0 +1,5 @@ +pr: 126605 +summary: Fix equality bug in `WaitForIndexColorStep` +area: ILM+SLM +type: bug +issues: [] diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java index b4c66c2f5ac22..5ba1a27f86ddc 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForIndexColorStep.java @@ -65,7 +65,7 @@ BiFunction getIndexNameSupplier() { @Override public int hashCode() { - return Objects.hash(super.hashCode(), this.color, this.indexNameSupplier); + return Objects.hash(super.hashCode(), this.color); } @Override @@ -77,9 +77,7 @@ public boolean equals(Object obj) { return false; } WaitForIndexColorStep other = (WaitForIndexColorStep) obj; - return super.equals(obj) - && Objects.equals(this.color, other.color) - && Objects.equals(this.indexNameSupplier, other.indexNameSupplier); + return super.equals(obj) && Objects.equals(this.color, other.color); } @Override diff --git a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java index 8eae3f17ae5ef..7c9e1160bc95c 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java @@ -113,10 +113,11 @@ public void testSearchableSnapshotAction() throws Exception { } }, 30, TimeUnit.SECONDS)); - assertBusy(() -> { - triggerStateChange(); - assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); - }, 30, TimeUnit.SECONDS); + assertBusy( + () -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); }, + 30, + TimeUnit.SECONDS + ); } public void testSearchableSnapshotForceMergesIndexToOneSegment() throws Exception { @@ -173,10 +174,11 @@ public void testSearchableSnapshotForceMergesIndexToOneSegment() throws Exceptio } }, 60, TimeUnit.SECONDS)); - assertBusy(() -> { - triggerStateChange(); - assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); - }, 30, TimeUnit.SECONDS); + assertBusy( + () -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); }, + 30, + TimeUnit.SECONDS + ); } @SuppressWarnings("unchecked") @@ -314,7 +316,6 @@ public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws }, 30, TimeUnit.SECONDS)); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredIndexName); assertThat(stepKeyForIndex.phase(), is("hot")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -337,7 +338,6 @@ public void testUpdatePolicyToAddPhasesYieldsInvalidActionsToBeSkipped() throws // even though the index is now mounted as a searchable snapshot, the actions that can't operate on it should // skip and ILM should not be blocked (not should the managed index move into the ERROR step) assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredIndexName); assertThat(stepKeyForIndex.phase(), is("cold")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -392,7 +392,6 @@ public void testRestoredIndexManagedByLocalPolicySkipsIllegalActions() throws Ex }, 30, TimeUnit.SECONDS)); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName); assertThat(stepKeyForIndex.phase(), is("hot")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -483,7 +482,6 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception { }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName); assertThat(stepKeyForIndex.phase(), is("cold")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -545,7 +543,6 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName); assertThat(stepKeyForIndex.phase(), is("frozen")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -628,7 +625,6 @@ public void testResumingSearchableSnapshotFromFullToPartial() throws Exception { }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), fullMountedIndexName); assertThat(stepKeyForIndex.phase(), is("cold")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -649,7 +645,6 @@ public void testResumingSearchableSnapshotFromFullToPartial() throws Exception { }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), partiallyMountedIndexName); assertThat(stepKeyForIndex.phase(), is("frozen")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -739,7 +734,6 @@ public void testResumingSearchableSnapshotFromPartialToFull() throws Exception { }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), partialMountedIndexName); assertThat(stepKeyForIndex.phase(), is("frozen")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -760,7 +754,6 @@ public void testResumingSearchableSnapshotFromPartialToFull() throws Exception { }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), restoredPartiallyMountedIndexName); assertThat(stepKeyForIndex.phase(), is("cold")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -920,10 +913,11 @@ public void testSearchableSnapshotInvokesAsyncActionOnNewIndex() throws Exceptio } }, 30, TimeUnit.SECONDS)); - assertBusy(() -> { - triggerStateChange(); - assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); - }, 30, TimeUnit.SECONDS); + assertBusy( + () -> { assertThat(explainIndex(client(), restoredIndexName).get("step"), is(PhaseCompleteStep.NAME)); }, + 30, + TimeUnit.SECONDS + ); } public void testSearchableSnapshotTotalShardsPerNode() throws Exception { @@ -964,7 +958,6 @@ public void testSearchableSnapshotTotalShardsPerNode() throws Exception { assertTrue(indexExists(searchableSnapMountedIndexName)); }, 30, TimeUnit.SECONDS); assertBusy(() -> { - triggerStateChange(); Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName); assertThat(stepKeyForIndex.phase(), is("frozen")); assertThat(stepKeyForIndex.name(), is(PhaseCompleteStep.NAME)); @@ -1028,7 +1021,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception { // check that the index is in the expected step and has the expected step_info.message assertBusy(() -> { - triggerStateChange(); Map explainResponse = explainIndex(client(), restoredIndexName); assertThat(explainResponse.get("step"), is(WaitUntilReplicateForTimePassesStep.NAME)); @SuppressWarnings("unchecked") @@ -1066,7 +1058,6 @@ public void testSearchableSnapshotReplicateFor() throws Exception { // check that the index has progressed because enough time has passed now that the policy is different assertBusy(() -> { - triggerStateChange(); Map explainResponse = explainIndex(client(), restoredIndexName); assertThat(explainResponse.get("phase"), is("cold")); assertThat(explainResponse.get("step"), is(PhaseCompleteStep.NAME)); @@ -1080,13 +1071,4 @@ public void testSearchableSnapshotReplicateFor() throws Exception { assertThat(numberOfReplicas, is(0)); } } - - /** - * Cause a bit of cluster activity using an empty reroute call in case the `wait-for-index-colour` ILM step missed the - * notification that partial-index is now GREEN. - */ - private void triggerStateChange() throws IOException { - Request rerouteRequest = new Request("POST", "/_cluster/reroute?metric=none"); - client().performRequest(rerouteRequest); - } }