Skip to content

Commit 34d3478

Browse files
andreidanandreidan
authored
ILM MountSnapshotStep: use correct logic operator (#54675)
* ILM MountSnapshotStep: use correct logic operator * Test the mount response status handling and fix client assertion * Fix a test edge case where the original index doesn't exist anymore
1 parent 4ef4204 commit 34d3478

File tree

4 files changed

+107
-24
lines changed

4 files changed

+107
-24
lines changed

‎server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public class RestoreSnapshotResponse extends ActionResponse implements ToXConten
4545
@Nullable
4646
private RestoreInfo restoreInfo;
4747

48-
RestoreSnapshotResponse(@Nullable RestoreInfo restoreInfo) {
48+
public RestoreSnapshotResponse(@Nullable RestoreInfo restoreInfo) {
4949
this.restoreInfo = restoreInfo;
5050
}
5151

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/MountSnapshotStep.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ void performDuringNoSnapshot(IndexMetadata indexMetadata, ClusterState currentCl
9292
false);
9393
getClient().execute(MountSearchableSnapshotAction.INSTANCE, mountSearchableSnapshotRequest,
9494
ActionListener.wrap(response -> {
95-
if (response.status() != RestStatus.OK || response.status() != RestStatus.ACCEPTED) {
95+
if (response.status() != RestStatus.OK && response.status() != RestStatus.ACCEPTED) {
9696
logger.debug("mount snapshot response failed to complete");
9797
throw new ElasticsearchException("mount snapshot response failed to complete, got response " + response.status());
9898
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/MountSnapshotStepTests.java

Lines changed: 92 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,18 @@
1010
import org.elasticsearch.action.ActionRequest;
1111
import org.elasticsearch.action.ActionResponse;
1212
import org.elasticsearch.action.ActionType;
13-
import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotAction;
14-
import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest;
13+
import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
1514
import org.elasticsearch.cluster.ClusterState;
1615
import org.elasticsearch.cluster.metadata.IndexMetadata;
1716
import org.elasticsearch.cluster.metadata.Metadata;
17+
import org.elasticsearch.snapshots.RestoreInfo;
1818
import org.elasticsearch.test.client.NoOpClient;
1919
import org.elasticsearch.xpack.core.ilm.Step.StepKey;
20+
import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotAction;
21+
import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest;
2022

2123
import java.util.HashMap;
24+
import java.util.List;
2225
import java.util.Map;
2326

2427
import static org.elasticsearch.xpack.core.ilm.AbstractStepMasterTimeoutTestCase.emptyClusterState;
@@ -28,6 +31,8 @@
2831

2932
public class MountSnapshotStepTests extends AbstractStepTestCase<MountSnapshotStep> {
3033

34+
private static final String RESTORED_INDEX_PREFIX = "restored-";
35+
3136
@Override
3237
public MountSnapshotStep createRandomInstance() {
3338
StepKey stepKey = randomStepKey();
@@ -127,6 +132,8 @@ public void testPerformAction() {
127132
Map<String, String> ilmCustom = new HashMap<>();
128133
String snapshotName = indexName + "-" + policyName;
129134
ilmCustom.put("snapshot_name", snapshotName);
135+
String repository = "repository";
136+
ilmCustom.put("snapshot_repository", repository);
130137

131138
IndexMetadata.Builder indexMetadataBuilder =
132139
IndexMetadata.builder(indexName).settings(settings(Version.CURRENT).put(LifecycleSettings.LIFECYCLE_NAME, policyName))
@@ -137,42 +144,109 @@ public void testPerformAction() {
137144
ClusterState clusterState =
138145
ClusterState.builder(emptyClusterState()).metadata(Metadata.builder().put(indexMetaData, true).build()).build();
139146

140-
String repository = "repository";
141-
String restoredIndexPrefix = "restored-";
142-
try (NoOpClient client = getRestoreSnapshotRequestAssertingClient(repository, snapshotName, indexName, restoredIndexPrefix)) {
143-
MountSnapshotStep step = new MountSnapshotStep(randomStepKey(), randomStepKey(), client, restoredIndexPrefix);
147+
try (NoOpClient client = getRestoreSnapshotRequestAssertingClient(repository, snapshotName, indexName, RESTORED_INDEX_PREFIX)) {
148+
MountSnapshotStep step = new MountSnapshotStep(randomStepKey(), randomStepKey(), client, RESTORED_INDEX_PREFIX);
144149
step.performAction(indexMetaData, clusterState, null, new AsyncActionStep.Listener() {
145150
@Override
146151
public void onResponse(boolean complete) {
152+
assertThat(complete, is(true));
147153
}
148154

149155
@Override
150156
public void onFailure(Exception e) {
157+
fail("expecting successful response but got: [" + e.getMessage() + "]");
151158
}
152159
});
153160
}
154161
}
155162

163+
public void testResponseStatusHandling() {
164+
String indexName = randomAlphaOfLength(10);
165+
String policyName = "test-ilm-policy";
166+
Map<String, String> ilmCustom = new HashMap<>();
167+
String snapshotName = indexName + "-" + policyName;
168+
ilmCustom.put("snapshot_name", snapshotName);
169+
String repository = "repository";
170+
ilmCustom.put("snapshot_repository", repository);
171+
172+
IndexMetadata.Builder indexMetadataBuilder =
173+
IndexMetadata.builder(indexName).settings(settings(Version.CURRENT).put(LifecycleSettings.LIFECYCLE_NAME, policyName))
174+
.putCustom(LifecycleExecutionState.ILM_CUSTOM_METADATA_KEY, ilmCustom)
175+
.numberOfShards(randomIntBetween(1, 5)).numberOfReplicas(randomIntBetween(0, 5));
176+
IndexMetadata indexMetaData = indexMetadataBuilder.build();
177+
178+
ClusterState clusterState =
179+
ClusterState.builder(emptyClusterState()).metadata(Metadata.builder().put(indexMetaData, true).build()).build();
180+
181+
{
182+
RestoreSnapshotResponse responseWithOKStatus = new RestoreSnapshotResponse(new RestoreInfo("test", List.of(), 1, 1));
183+
try (NoOpClient clientPropagatingOKResponse = getClientTriggeringResponse(responseWithOKStatus)) {
184+
MountSnapshotStep step = new MountSnapshotStep(randomStepKey(), randomStepKey(), clientPropagatingOKResponse,
185+
RESTORED_INDEX_PREFIX);
186+
step.performAction(indexMetaData, clusterState, null, new AsyncActionStep.Listener() {
187+
@Override
188+
public void onResponse(boolean complete) {
189+
assertThat(complete, is(true));
190+
}
191+
192+
@Override
193+
public void onFailure(Exception e) {
194+
fail("expecting successful response but got: [" + e.getMessage() + "]");
195+
}
196+
});
197+
}
198+
}
199+
200+
{
201+
RestoreSnapshotResponse responseWithACCEPTEDStatus = new RestoreSnapshotResponse((RestoreInfo) null);
202+
try (NoOpClient clientPropagatingACCEPTEDResponse = getClientTriggeringResponse(responseWithACCEPTEDStatus)) {
203+
MountSnapshotStep step = new MountSnapshotStep(randomStepKey(), randomStepKey(), clientPropagatingACCEPTEDResponse,
204+
RESTORED_INDEX_PREFIX);
205+
step.performAction(indexMetaData, clusterState, null, new AsyncActionStep.Listener() {
206+
@Override
207+
public void onResponse(boolean complete) {
208+
assertThat(complete, is(true));
209+
}
210+
211+
@Override
212+
public void onFailure(Exception e) {
213+
fail("expecting successful response but got: [" + e.getMessage() + "]");
214+
}
215+
});
216+
}
217+
}
218+
}
219+
220+
@SuppressWarnings("unchecked")
221+
private NoOpClient getClientTriggeringResponse(RestoreSnapshotResponse response) {
222+
return new NoOpClient(getTestName()) {
223+
@Override
224+
protected <Request extends ActionRequest, Response extends ActionResponse> void doExecute(ActionType<Response> action,
225+
Request request,
226+
ActionListener<Response> listener) {
227+
listener.onResponse((Response) response);
228+
}
229+
};
230+
}
231+
232+
@SuppressWarnings("unchecked")
156233
private NoOpClient getRestoreSnapshotRequestAssertingClient(String expectedRepoName, String expectedSnapshotName, String indexName,
157234
String restoredIndexPrefix) {
158235
return new NoOpClient(getTestName()) {
159236
@Override
160237
protected <Request extends ActionRequest, Response extends ActionResponse> void doExecute(ActionType<Response> action,
161238
Request request,
162239
ActionListener<Response> listener) {
163-
assertThat(action.name(), is(RestoreSnapshotAction.NAME));
164-
assertTrue(request instanceof RestoreSnapshotRequest);
165-
RestoreSnapshotRequest restoreSnapshotRequest = (RestoreSnapshotRequest) request;
166-
assertThat(restoreSnapshotRequest.repository(), is(expectedRepoName));
167-
assertThat(restoreSnapshotRequest.snapshot(), is(expectedSnapshotName));
240+
assertThat(action.name(), is(MountSearchableSnapshotAction.NAME));
241+
assertTrue(request instanceof MountSearchableSnapshotRequest);
242+
MountSearchableSnapshotRequest mountSearchableSnapshotRequest = (MountSearchableSnapshotRequest) request;
243+
assertThat(mountSearchableSnapshotRequest.repositoryName(), is(expectedRepoName));
244+
assertThat(mountSearchableSnapshotRequest.snapshotName(), is(expectedSnapshotName));
168245
assertThat("another ILM step will wait for the restore to complete. the " + MountSnapshotStep.NAME + " step should not",
169-
restoreSnapshotRequest.waitForCompletion(), is(false));
170-
assertThat("another ILM step will transfer the aliases to the restored index", restoreSnapshotRequest.includeAliases(),
171-
is(false));
172-
assertThat(restoreSnapshotRequest.ignoreIndexSettings(), is(notNullValue()));
173-
assertThat(restoreSnapshotRequest.ignoreIndexSettings()[0], is(LifecycleSettings.LIFECYCLE_NAME));
174-
assertThat(restoreSnapshotRequest.renameReplacement(), is(restoredIndexPrefix + indexName));
175-
assertThat(restoreSnapshotRequest.renamePattern(), is(indexName));
246+
mountSearchableSnapshotRequest.waitForCompletion(), is(false));
247+
assertThat(mountSearchableSnapshotRequest.ignoreIndexSettings(), is(notNullValue()));
248+
assertThat(mountSearchableSnapshotRequest.ignoreIndexSettings()[0], is(LifecycleSettings.LIFECYCLE_NAME));
249+
assertThat(mountSearchableSnapshotRequest.mountedIndexName(), is(restoredIndexPrefix + indexName));
176250
}
177251
};
178252
}

‎x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1587,7 +1587,8 @@ public void testDeleteActionDeletesSearchableSnapshot() throws Exception {
15871587
Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo));
15881588
Map<String, Phase> phases = new HashMap<>();
15891589
phases.put("cold", new Phase("cold", TimeValue.ZERO, coldActions));
1590-
phases.put("delete", new Phase("delete", TimeValue.timeValueMillis(5000), singletonMap(DeleteAction.NAME, new DeleteAction(true))));
1590+
phases.put("delete", new Phase("delete", TimeValue.timeValueMillis(10000), singletonMap(DeleteAction.NAME,
1591+
new DeleteAction(true))));
15911592
LifecyclePolicy lifecyclePolicy = new LifecyclePolicy(policy, phases);
15921593
// PUT policy
15931594
XContentBuilder builder = jsonBuilder();
@@ -1606,16 +1607,20 @@ public void testDeleteActionDeletesSearchableSnapshot() throws Exception {
16061607
randomBoolean());
16071608

16081609
String[] snapshotName = new String[1];
1610+
String restoredIndexName = SearchableSnapshotAction.RESTORED_INDEX_PREFIX + this.index;
16091611
assertTrue(waitUntil(() -> {
16101612
try {
16111613
Map<String, Object> explainIndex = explainIndex(index);
1614+
if(explainIndex == null) {
1615+
// in case we missed the original index and it was deleted
1616+
explainIndex = explainIndex(restoredIndexName);
1617+
}
16121618
snapshotName[0] = (String) explainIndex.get("snapshot_name");
16131619
return snapshotName[0] != null;
16141620
} catch (IOException e) {
16151621
return false;
16161622
}
16171623
}, 30, TimeUnit.SECONDS));
1618-
String restoredIndexName = SearchableSnapshotAction.RESTORED_INDEX_PREFIX + this.index;
16191624
assertBusy(() -> assertFalse(indexExists(restoredIndexName)));
16201625

16211626
assertTrue("the snapshot we generate in the cold phase should be deleted by the delete phase", waitUntil(() -> {
@@ -1638,7 +1643,7 @@ public void testDeleteActionDoesntDeleteSearchableSnapshot() throws Exception {
16381643
Map.of(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo));
16391644
Map<String, Phase> phases = new HashMap<>();
16401645
phases.put("cold", new Phase("cold", TimeValue.ZERO, coldActions));
1641-
phases.put("delete", new Phase("delete", TimeValue.timeValueMillis(5000), singletonMap(DeleteAction.NAME,
1646+
phases.put("delete", new Phase("delete", TimeValue.timeValueMillis(10000), singletonMap(DeleteAction.NAME,
16421647
new DeleteAction(false))));
16431648
LifecyclePolicy lifecyclePolicy = new LifecyclePolicy(policy, phases);
16441649
// PUT policy
@@ -1658,16 +1663,20 @@ public void testDeleteActionDoesntDeleteSearchableSnapshot() throws Exception {
16581663
randomBoolean());
16591664

16601665
String[] snapshotName = new String[1];
1666+
String restoredIndexName = SearchableSnapshotAction.RESTORED_INDEX_PREFIX + this.index;
16611667
assertTrue(waitUntil(() -> {
16621668
try {
16631669
Map<String, Object> explainIndex = explainIndex(index);
1670+
if(explainIndex == null) {
1671+
// in case we missed the original index and it was deleted
1672+
explainIndex = explainIndex(restoredIndexName);
1673+
}
16641674
snapshotName[0] = (String) explainIndex.get("snapshot_name");
16651675
return snapshotName[0] != null;
16661676
} catch (IOException e) {
16671677
return false;
16681678
}
16691679
}, 30, TimeUnit.SECONDS));
1670-
String restoredIndexName = SearchableSnapshotAction.RESTORED_INDEX_PREFIX + this.index;
16711680
assertBusy(() -> assertFalse(indexExists(restoredIndexName)));
16721681

16731682
assertTrue("the snapshot we generate in the cold phase should not be deleted by the delete phase", waitUntil(() -> {

0 commit comments

Comments
 (0)