From 0b1efe69174a34d30a1a77dfd890a2bb95d9e60a Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 10 Feb 2021 10:55:09 -0700 Subject: [PATCH] Disallow ILM searchable snapshot actions that use different repositories This commit adds validation when updating or creating an ILM policy that mulitple searchable snapshot actions all use the same repository. We currently do not have the infrastructure to support switching a searchable snapshot from one repository to another, so until we have that we will disallow this (edge case) when creating a policy. Relates to #68714 --- .../core/ilm/SearchableSnapshotAction.java | 4 + .../core/ilm/TimeseriesLifecycleType.java | 17 ++++ .../xpack/core/ilm/LifecyclePolicyTests.java | 3 +- .../ilm/TimeseriesLifecycleTypeTests.java | 35 +++++++ .../actions/SearchableSnapshotActionIT.java | 93 ++++++------------- 5 files changed, 88 insertions(+), 64 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SearchableSnapshotAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SearchableSnapshotAction.java index 76b56809d2d5a..cef440c877388 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SearchableSnapshotAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SearchableSnapshotAction.java @@ -117,6 +117,10 @@ public MountSearchableSnapshotRequest.Storage getStorageType() { return storageType; } + public String getSnapshotRepository() { + return snapshotRepository; + } + @Override public List toSteps(Client client, String phase, StepKey nextStepKey) { StepKey preActionBranchingKey = new StepKey(phase, NAME, CONDITIONAL_SKIP_ACTION_STEP); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java index 08712b51c46a1..62c430a4c790a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java @@ -290,6 +290,7 @@ public void validate(Collection phases) { } validateActionsFollowingSearchableSnapshot(phases); + validateAllSearchableSnapshotActionsUseSameRepository(phases); } static void validateActionsFollowingSearchableSnapshot(Collection phases) { @@ -313,6 +314,22 @@ static void validateActionsFollowingSearchableSnapshot(Collection phases) } } + static void validateAllSearchableSnapshotActionsUseSameRepository(Collection phases) { + Set allRepos = phases.stream() + .flatMap(phase -> phase.getActions().entrySet().stream()) + .filter(e -> e.getKey().equals(SearchableSnapshotAction.NAME)) + .map(Map.Entry::getValue) + .map(action -> (SearchableSnapshotAction) action) + .map(SearchableSnapshotAction::getSnapshotRepository) + .collect(Collectors.toSet()); + + if (allRepos.size() > 1) { + throw new IllegalArgumentException("policy specifies [" + SearchableSnapshotAction.NAME + + "] action multiple times with differing repositories " + allRepos + + ", the same repository must be used for all searchable snapshot actions"); + } + } + private static boolean definesAllocationRules(AllocateAction action) { return action.getRequire().isEmpty() == false || action.getInclude().isEmpty() == false || action.getExclude().isEmpty() == false; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java index af4e48762a790..5774fd4ff9439 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java @@ -30,6 +30,7 @@ import java.util.Set; import java.util.function.Function; +import static org.elasticsearch.xpack.core.ilm.SearchableSnapshotActionTests.randomStorageType; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.mockito.Mockito.mock; @@ -207,7 +208,7 @@ private static Function getNameToActionFunction() { case UnfollowAction.NAME: return new UnfollowAction(); case SearchableSnapshotAction.NAME: - return new SearchableSnapshotAction(randomAlphaOfLengthBetween(1, 10)); + return new SearchableSnapshotAction("repo", randomBoolean(), randomStorageType()); case MigrateAction.NAME: return new MigrateAction(false); case RollupILMAction.NAME: diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java index 763e389549ea4..bd9fb017d9535 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java @@ -39,6 +39,7 @@ import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.VALID_PHASES; import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.VALID_WARM_ACTIONS; import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.WARM_PHASE; +import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.validateAllSearchableSnapshotActionsUseSameRepository; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -592,6 +593,40 @@ public void testShouldMigrateDataToTiers() { } } + public void testValidatingSearchableSnapshotRepos() { + Map hotActions = new HashMap<>(); + Map coldActions = new HashMap<>(); + Map frozenActions = new HashMap<>(); + + { + hotActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null)); + coldActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null)); + frozenActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null)); + + Phase hotPhase = new Phase(HOT_PHASE, TimeValue.ZERO, hotActions); + Phase coldPhase = new Phase(HOT_PHASE, TimeValue.ZERO, coldActions); + Phase frozenPhase = new Phase(HOT_PHASE, TimeValue.ZERO, frozenActions); + + validateAllSearchableSnapshotActionsUseSameRepository(Arrays.asList(hotPhase, coldPhase, frozenPhase)); + } + + { + hotActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null)); + coldActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo2", randomBoolean(), null)); + frozenActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null)); + + Phase hotPhase = new Phase(HOT_PHASE, TimeValue.ZERO, hotActions); + Phase coldPhase = new Phase(HOT_PHASE, TimeValue.ZERO, coldActions); + Phase frozenPhase = new Phase(HOT_PHASE, TimeValue.ZERO, frozenActions); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> validateAllSearchableSnapshotActionsUseSameRepository(Arrays.asList(hotPhase, coldPhase, frozenPhase))); + assertThat(e.getMessage(), containsString("policy specifies [searchable_snapshot]" + + " action multiple times with differing repositories [repo2, repo1]," + + " the same repository must be used for all searchable snapshot actions")); + } + } + private void assertNextActionName(String phaseName, String currentAction, String expectedNextAction, String... availableActionNames) { Map availableActions = convertActionNamesToActions(availableActionNames); Phase phase = new Phase(phaseName, TimeValue.ZERO, availableActions); 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 a3b657e3cb9fb..86a00fe5c23ba 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 @@ -56,6 +56,7 @@ import static org.elasticsearch.xpack.TimeSeriesRestDriver.getStepKeyForIndex; import static org.elasticsearch.xpack.TimeSeriesRestDriver.indexDocument; import static org.elasticsearch.xpack.TimeSeriesRestDriver.rolloverMaxOneDocCondition; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; @@ -374,6 +375,7 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception { .put(LifecycleSettings.LIFECYCLE_NAME, policy) .build()); ensureGreen(index); + indexDocument(client(), index, true); final String searchableSnapMountedIndexName = (storage == MountSearchableSnapshotRequest.Storage.FULL_COPY ? SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX : SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX) + index; @@ -399,6 +401,10 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception { ((List>) ((Map) ((List) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1)); + + Request hitCount = new Request("GET", "/" + searchableSnapMountedIndexName + "/_count"); + Map count = entityAsMap(client().performRequest(hitCount)); + assertThat("expected a single document but got: " + count, (int) count.get("count"), equalTo(1)); } @SuppressWarnings("unchecked") @@ -419,7 +425,7 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception .put(LifecycleSettings.LIFECYCLE_NAME, policy) .build()); ensureGreen(index); - indexDocument(client(), index); + indexDocument(client(), index, true); final String searchableSnapMountedIndexName = SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX + SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX + index; @@ -445,6 +451,10 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception ((List>) ((Map) ((List) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1)); + + Request hitCount = new Request("GET", "/" + searchableSnapMountedIndexName + "/_count"); + Map count = entityAsMap(client().performRequest(hitCount)); + assertThat("expected a single document but got: " + count, (int) count.get("count"), equalTo(1)); } @SuppressWarnings("unchecked") @@ -465,7 +475,7 @@ public void testConvertingPartialSearchableSnapshotIntoFull() throws Exception { .put(LifecycleSettings.LIFECYCLE_NAME, policy) .build()); ensureGreen(index); - indexDocument(client(), index); + indexDocument(client(), index, true); final String searchableSnapMountedIndexName = SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX + SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX + index; @@ -491,71 +501,28 @@ public void testConvertingPartialSearchableSnapshotIntoFull() throws Exception { ((List>) ((Map) ((List) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1)); + + Request hitCount = new Request("GET", "/" + searchableSnapMountedIndexName + "/_count"); + Map count = entityAsMap(client().performRequest(hitCount)); + assertThat("expected a single document but got: " + count, (int) count.get("count"), equalTo(1)); } - @SuppressWarnings("unchecked") - @AwaitsFix(bugUrl = "functionality not yet implemented") - public void testSecondSearchableSnapshotChangesRepo() throws Exception { - String index = "myindex-" + randomAlphaOfLength(4).toLowerCase(Locale.ROOT); + public void testSecondSearchableSnapshotUsingDifferentRepoThrows() throws Exception { String secondRepo = randomAlphaOfLengthBetween(10, 20); createSnapshotRepo(client(), snapshotRepo, randomBoolean()); createSnapshotRepo(client(), secondRepo, randomBoolean()); - createPolicy(client(), policy, null, null, - new Phase("cold", TimeValue.ZERO, - singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(), - MountSearchableSnapshotRequest.Storage.FULL_COPY))), - new Phase("frozen", TimeValue.ZERO, - singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(secondRepo, randomBoolean(), - MountSearchableSnapshotRequest.Storage.SHARED_CACHE))), - null - ); - - createIndex(index, Settings.builder() - .put(LifecycleSettings.LIFECYCLE_NAME, policy) - .build()); - ensureGreen(index); - indexDocument(client(), index); - - final String searchableSnapMountedIndexName = SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX + - SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX + index; - - assertBusy(() -> { - logger.info("--> waiting for [{}] to exist...", searchableSnapMountedIndexName); - assertTrue(indexExists(searchableSnapMountedIndexName)); - }, 30, TimeUnit.SECONDS); - - assertBusy(() -> { - Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName); - assertThat(stepKeyForIndex.getPhase(), is("frozen")); - assertThat(stepKeyForIndex.getName(), is(PhaseCompleteStep.NAME)); - }, 30, TimeUnit.SECONDS); - - // Check first repo has exactly 1 snapshot - { - Request getSnaps = new Request("GET", "/_snapshot/" + snapshotRepo + "/_all"); - Response response = client().performRequest(getSnaps); - Map responseMap; - try (InputStream is = response.getEntity().getContent()) { - responseMap = XContentHelper.convertToMap(XContentType.JSON.xContent(), is, true); - } - assertThat("expected to have only one snapshot, but got: " + responseMap, - ((List>) - ((Map) - ((List) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1)); - } - - // Check second repo has exactly 1 snapshot - { - Request getSnaps = new Request("GET", "/_snapshot/" + secondRepo + "/_all"); - Response response = client().performRequest(getSnaps); - Map responseMap; - try (InputStream is = response.getEntity().getContent()) { - responseMap = XContentHelper.convertToMap(XContentType.JSON.xContent(), is, true); - } - assertThat("expected to have only one snapshot, but got: " + responseMap, - ((List>) - ((Map) - ((List) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1)); - } + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + createPolicy(client(), policy, null, null, + new Phase("cold", TimeValue.ZERO, + singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(), + MountSearchableSnapshotRequest.Storage.FULL_COPY))), + new Phase("frozen", TimeValue.ZERO, + singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(secondRepo, randomBoolean(), + MountSearchableSnapshotRequest.Storage.SHARED_CACHE))), + null + )); + + assertThat(e.getMessage(), + containsString("policy specifies [searchable_snapshot] action multiple times with differing repositories")); } }