From b5b877ea224eb2d0c05958d0e88dc5fd51b541c7 Mon Sep 17 00:00:00 2001 From: Elena Stoeva Date: Thu, 27 Feb 2025 14:39:41 +0000 Subject: [PATCH 01/11] Add state query param to Get snapshots API --- .../rest-api-spec/api/snapshot.get.json | 4 ++ .../snapshots/GetSnapshotsIT.java | 63 ++++++++++++++++++- .../org/elasticsearch/TransportVersions.java | 2 +- .../snapshots/get/GetSnapshotsRequest.java | 20 ++++++ .../get/GetSnapshotsRequestBuilder.java | 7 +++ .../get/TransportGetSnapshotsAction.java | 17 ++++- .../admin/cluster/RestGetSnapshotsAction.java | 11 ++++ .../snapshots/SnapshotState.java | 17 +++++ 8 files changed, 135 insertions(+), 6 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/snapshot.get.json b/rest-api-spec/src/main/resources/rest-api-spec/api/snapshot.get.json index 23f5f737995d0..f40042c1b0dac 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/api/snapshot.get.json +++ b/rest-api-spec/src/main/resources/rest-api-spec/api/snapshot.get.json @@ -85,6 +85,10 @@ "verbose":{ "type":"boolean", "description":"Whether to show verbose snapshot info or only show the basic info found in the repository index blob" + }, + "state": { + "type": "list", + "description": "Filter snapshots by a comma-separated list of states. Valid state values are 'SUCCESS', 'IN_PROGRESS', 'FAILED', 'PARTIAL', or 'INCOMPATIBLE'." } } } diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java index c0e63520fee9e..fcc9c055450ba 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java @@ -55,6 +55,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -69,6 +70,7 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.is; +import static org.hamcrest.core.StringContains.containsString; public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { @@ -633,6 +635,55 @@ public void testRetrievingSnapshotsWhenRepositoryIsMissing() throws Exception { expectThrows(RepositoryMissingException.class, multiRepoFuture::actionGet); } + public void testFilterByState() throws Exception { + final String repoName = "test-repo"; + final Path repoPath = randomRepoPath(); + createRepository(repoName, "mock", repoPath); + + // Create a successful snapshot + String successSnapshot = "snapshot-success"; + createFullSnapshot(repoName, successSnapshot); + + // Fetch snapshots with state=SUCCESS + GetSnapshotsResponse responseSuccess = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) + .setState(EnumSet.of(SnapshotState.SUCCESS)) + .get(); + assertThat(responseSuccess.getSnapshots(), hasSize(1)); + assertThat(responseSuccess.getSnapshots().get(0).state(), is(SnapshotState.SUCCESS)); + + // Create a snapshot in progress + String inProgressSnapshot = "snapshot-in-progress"; + blockAllDataNodes(repoName); + startFullSnapshot(repoName, inProgressSnapshot); + awaitNumberOfSnapshotsInProgress(1); + + // Fetch snapshots with state=IN_PROGRESS + GetSnapshotsResponse responseInProgress = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) + .setState(EnumSet.of(SnapshotState.IN_PROGRESS)) + .get(); + assertThat(responseInProgress.getSnapshots(), hasSize(1)); + assertThat(responseInProgress.getSnapshots().get(0).state(), is(SnapshotState.IN_PROGRESS)); + + // Fetch snapshots with multiple states (SUCCESS, IN_PROGRESS) + GetSnapshotsResponse responseMultipleStates = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) + .setState(EnumSet.of(SnapshotState.SUCCESS, SnapshotState.IN_PROGRESS)) + .get(); + assertThat(responseMultipleStates.getSnapshots(), hasSize(2)); + assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.SUCCESS)); + assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.IN_PROGRESS)); + + // Fetch all snapshots (without state) + GetSnapshotsResponse responseAll = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get(); + assertThat(responseAll.getSnapshots(), hasSize(2)); + + // Fetch snapshots with an invalid state + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setState(EnumSet.of(SnapshotState.of("FOO"))).get() + ); + assertThat(e.getMessage(), containsString("Unknown state name [FOO]")); + } + // Create a snapshot that is guaranteed to have a unique start time and duration for tests around ordering by either. // Don't use this with more than 3 snapshots on platforms with low-resolution clocks as the durations could always collide there // causing an infinite loop @@ -912,9 +963,15 @@ public void testAllFeatures() { // INDICES and by SHARDS. The actual sorting behaviour for these cases is tested elsewhere, here we're just checking that sorting // interacts correctly with the other parameters to the API. + final EnumSet state = EnumSet.of(randomFrom(SnapshotState.values())); + // Note: The selected state may not match any existing snapshots. + // The actual filtering behaviour for such cases is tested in the dedicated test. + // Here we're just checking that state interacts correctly with the other parameters to the API. + // compute the ordered sequence of snapshots which match the repository/snapshot name filters and SLM policy filter final var selectedSnapshots = snapshotInfos.stream() .filter(snapshotInfoPredicate) + .filter(s -> state.contains(s.state())) .sorted(sortKey.getSnapshotInfoComparator(order)) .toList(); @@ -923,7 +980,8 @@ public void testAllFeatures() { ) // apply sorting params .sort(sortKey) - .order(order); + .order(order) + .state(state); // sometimes use ?from_sort_value to skip some items; note that snapshots skipped in this way are subtracted from // GetSnapshotsResponse.totalCount whereas snapshots skipped by ?after and ?offset are not @@ -1010,7 +1068,8 @@ public void testAllFeatures() { .sort(sortKey) .order(order) .size(nextSize) - .after(SnapshotSortKey.decodeAfterQueryParam(nextRequestAfter)); + .after(SnapshotSortKey.decodeAfterQueryParam(nextRequestAfter)) + .state(state); final GetSnapshotsResponse nextResponse = safeAwait(l -> client().execute(TransportGetSnapshotsAction.TYPE, nextRequest, l)); assertEquals( diff --git a/server/src/main/java/org/elasticsearch/TransportVersions.java b/server/src/main/java/org/elasticsearch/TransportVersions.java index 3aa8bf1dfdc4e..5701871a5e0c6 100644 --- a/server/src/main/java/org/elasticsearch/TransportVersions.java +++ b/server/src/main/java/org/elasticsearch/TransportVersions.java @@ -269,7 +269,7 @@ static TransportVersion def(int id) { public static final TransportVersion SETTINGS_IN_DATA_STREAMS_DRY_RUN = def(9_081_0_00); public static final TransportVersion ML_INFERENCE_SAGEMAKER_CHAT_COMPLETION = def(9_082_0_00); public static final TransportVersion ML_INFERENCE_VERTEXAI_CHATCOMPLETION_ADDED = def(9_083_0_00); - + public static final TransportVersion STATE_PARAM_GET_SNAPSHOT = def(9_084_0_00); /* * STOP! READ THIS FIRST! No, really, * ____ _____ ___ ____ _ ____ _____ _ ____ _____ _ _ ___ ____ _____ ___ ____ ____ _____ _ diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java index ac3da1db420a5..78c46b39ea6b1 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java @@ -19,12 +19,14 @@ import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.search.sort.SortOrder; +import org.elasticsearch.snapshots.SnapshotState; import org.elasticsearch.tasks.CancellableTask; import org.elasticsearch.tasks.Task; import org.elasticsearch.tasks.TaskId; import java.io.IOException; import java.util.Arrays; +import java.util.EnumSet; import java.util.Map; import static org.elasticsearch.action.ValidateActions.addValidationError; @@ -39,6 +41,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest public static final boolean DEFAULT_VERBOSE_MODE = true; private static final TransportVersion INDICES_FLAG_VERSION = TransportVersions.V_8_3_0; + private static final TransportVersion STATE_FLAG_VERSION = TransportVersions.STATE_PARAM_GET_SNAPSHOT; public static final int NO_LIMIT = -1; @@ -77,6 +80,8 @@ public class GetSnapshotsRequest extends MasterNodeRequest private boolean includeIndexNames = true; + private EnumSet state = EnumSet.noneOf(SnapshotState.class); + public GetSnapshotsRequest(TimeValue masterNodeTimeout) { super(masterNodeTimeout); } @@ -118,6 +123,9 @@ public GetSnapshotsRequest(StreamInput in) throws IOException { if (in.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) { includeIndexNames = in.readBoolean(); } + if (in.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) { + state = in.readEnumSet(SnapshotState.class); + } } @Override @@ -137,6 +145,9 @@ public void writeTo(StreamOutput out) throws IOException { if (out.getTransportVersion().onOrAfter(INDICES_FLAG_VERSION)) { out.writeBoolean(includeIndexNames); } + if (out.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) { + out.writeEnumSet(state); + } } @Override @@ -342,6 +353,15 @@ public boolean verbose() { return verbose; } + public EnumSet state() { + return state; + } + + public GetSnapshotsRequest state(EnumSet state) { + this.state = state; + return this; + } + @Override public Task createTask(long id, String type, String action, TaskId parentTaskId, Map headers) { return new CancellableTask(id, type, action, getDescription(), parentTaskId, headers); diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java index 4ace7536a86e1..24dc15f5788cc 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java @@ -15,6 +15,9 @@ import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.search.sort.SortOrder; +import org.elasticsearch.snapshots.SnapshotState; + +import java.util.EnumSet; /** * Get snapshots request builder @@ -150,4 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) { } + public GetSnapshotsRequestBuilder setState(EnumSet state) { + request.state(state); + return this; + } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java index 3e0411930191a..b87e484cde9f5 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java @@ -45,6 +45,7 @@ import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.snapshots.SnapshotInfo; import org.elasticsearch.snapshots.SnapshotMissingException; +import org.elasticsearch.snapshots.SnapshotState; import org.elasticsearch.snapshots.SnapshotsService; import org.elasticsearch.tasks.CancellableTask; import org.elasticsearch.tasks.Task; @@ -54,6 +55,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -160,7 +162,8 @@ protected void masterOperation( request.size(), SnapshotsInProgress.get(state), request.verbose(), - request.includeIndexNames() + request.includeIndexNames(), + request.state() ).runOperation(listener); } @@ -181,6 +184,7 @@ private class GetSnapshotsOperation { private final SnapshotNamePredicate snapshotNamePredicate; private final SnapshotPredicates fromSortValuePredicates; private final Predicate slmPolicyPredicate; + private final EnumSet state; // snapshot ordering/pagination private final SnapshotSortKey sortBy; @@ -224,7 +228,8 @@ private class GetSnapshotsOperation { int size, SnapshotsInProgress snapshotsInProgress, boolean verbose, - boolean indices + boolean indices, + EnumSet state ) { this.cancellableTask = cancellableTask; this.repositories = repositories; @@ -237,6 +242,7 @@ private class GetSnapshotsOperation { this.snapshotsInProgress = snapshotsInProgress; this.verbose = verbose; this.indices = indices; + this.state = state; this.snapshotNamePredicate = SnapshotNamePredicate.forSnapshots(ignoreUnavailable, snapshots); this.fromSortValuePredicates = SnapshotPredicates.forFromSortValue(fromSortValue, sortBy, order); @@ -558,11 +564,16 @@ private boolean matchesPredicates(SnapshotId snapshotId, RepositoryData reposito return false; } + final var details = repositoryData.getSnapshotDetails(snapshotId); + + if (!state.isEmpty() && !state.contains(details.getSnapshotState())) { + return false; + } + if (slmPolicyPredicate == SlmPolicyPredicate.MATCH_ALL_POLICIES) { return true; } - final var details = repositoryData.getSnapshotDetails(snapshotId); return details == null || details.getSlmPolicy() == null || slmPolicyPredicate.test(details.getSlmPolicy()); } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index b3a6430822020..2589d28f34078 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -20,8 +20,11 @@ import org.elasticsearch.rest.action.RestCancellableNodeClient; import org.elasticsearch.rest.action.RestRefCountedChunkedToXContentListener; import org.elasticsearch.search.sort.SortOrder; +import org.elasticsearch.snapshots.SnapshotState; import java.io.IOException; +import java.util.Arrays; +import java.util.EnumSet; import java.util.List; import java.util.Set; @@ -82,6 +85,14 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC final SortOrder order = SortOrder.fromString(request.param("order", getSnapshotsRequest.order().toString())); getSnapshotsRequest.order(order); getSnapshotsRequest.includeIndexNames(request.paramAsBoolean(INDEX_NAMES_XCONTENT_PARAM, getSnapshotsRequest.includeIndexNames())); + + final String stateString = request.param("state"); + if (stateString == null || stateString.isEmpty()) { + getSnapshotsRequest.state(EnumSet.noneOf(SnapshotState.class)); + } else { + getSnapshotsRequest.state(EnumSet.copyOf(Arrays.stream(stateString.split(",")).map(SnapshotState::of).toList())); + } + return channel -> new RestCancellableNodeClient(client, request.getHttpChannel()).admin() .cluster() .getSnapshots(getSnapshotsRequest, new RestRefCountedChunkedToXContentListener<>(channel)); diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java index dcf90327e1098..df593d18f453b 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java @@ -89,4 +89,21 @@ public static SnapshotState fromValue(byte value) { default -> throw new IllegalArgumentException("No snapshot state for value [" + value + "]"); }; } + + /** + * Generate snapshot state from a string (case-insensitive) + * + * @param name the state name + * @return state + */ + public static SnapshotState of(String name) { + return switch (name.toUpperCase()) { + case "IN_PROGRESS" -> IN_PROGRESS; + case "SUCCESS" -> SUCCESS; + case "FAILED" -> FAILED; + case "PARTIAL" -> PARTIAL; + case "INCOMPATIBLE" -> INCOMPATIBLE; + default -> throw new IllegalArgumentException("Unknown state name [" + name + "]"); + }; + } } From 442b5a678b83e2a398fa831ae208fdeea180c9f7 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 29 May 2025 14:43:28 -0400 Subject: [PATCH 02/11] Address code review comments from PR 123618 --- .../test/snapshot.get/10_basic.yml | 61 ++++++++++++++++ .../snapshots/GetSnapshotsIT.java | 71 +++++++++++-------- server/src/main/java/module-info.java | 1 + .../elasticsearch/action/ActionModule.java | 2 +- .../snapshots/get/GetSnapshotsRequest.java | 21 ++++-- .../get/GetSnapshotsRequestBuilder.java | 4 +- .../get/TransportGetSnapshotsAction.java | 17 +++-- .../admin/cluster/GetSnapshotsFeatures.java | 24 +++++++ .../admin/cluster/RestGetSnapshotsAction.java | 16 +++-- .../snapshots/SnapshotState.java | 17 ----- ...lasticsearch.features.FeatureSpecification | 1 + 11 files changed, 168 insertions(+), 67 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml index ca79a4419b813..079e52564a2f2 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml @@ -303,3 +303,64 @@ setup: snapshot.delete: repository: test_repo_get_1 snapshot: test_snapshot_no_repo_name + +--- +"Get snapshot using state parameter": + + - do: + indices.create: + index: test_index + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + + - do: + snapshot.create: + repository: test_repo_get_1 + snapshot: test_snapshot_with_state_param + wait_for_completion: true + + - do: + snapshot.get: + repository: test_repo_get_1 + snapshot: test_snapshot_with_state_param + state: SUCCESS + + - is_true: snapshots + - match: { snapshots.0.snapshot: test_snapshot_with_state_param } + - match: { snapshots.0.state: SUCCESS } + + - do: + snapshot.get: + repository: test_repo_get_1 + snapshot: test_snapshot_with_state_param + state: SUCCESS,PARTIAL + + - is_true: snapshots + - match: { snapshots.0.snapshot: test_snapshot_with_state_param } + - match: { snapshots.0.state: SUCCESS } + + - do: + snapshot.get: + repository: test_repo_get_1 + snapshot: test_snapshot_with_state_param + state: FAILED + + - is_true: snapshots + - length: { snapshots: 0 } + + - do: + catch: bad_request + snapshot.get: + repository: test_repo_get_1 + snapshot: test_snapshot_with_state_param + state: FOO + + - match: { error.type: "illegal_argument_exception" } + - match: { error.reason: "No enum constant org.elasticsearch.snapshots.SnapshotState.FOO" } + + - do: + snapshot.delete: + repository: test_repo_get_1 + snapshot: test_snapshot_with_state_param diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java index fcc9c055450ba..0b77ff97b9e92 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java @@ -53,6 +53,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; @@ -61,6 +62,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -70,7 +72,6 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.is; -import static org.hamcrest.core.StringContains.containsString; public class GetSnapshotsIT extends AbstractSnapshotIntegTestCase { @@ -641,47 +642,55 @@ public void testFilterByState() throws Exception { createRepository(repoName, "mock", repoPath); // Create a successful snapshot - String successSnapshot = "snapshot-success"; - createFullSnapshot(repoName, successSnapshot); + createFullSnapshot(repoName, "snapshot-success"); + + final Function, List> getSnapshotsForStates = (states) -> { + return clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setStates(states).get().getSnapshots(); + }; // Fetch snapshots with state=SUCCESS - GetSnapshotsResponse responseSuccess = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) - .setState(EnumSet.of(SnapshotState.SUCCESS)) - .get(); - assertThat(responseSuccess.getSnapshots(), hasSize(1)); - assertThat(responseSuccess.getSnapshots().get(0).state(), is(SnapshotState.SUCCESS)); + var snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.SUCCESS)); + assertThat(snapshots, hasSize(1)); + assertThat(snapshots.getFirst().state(), is(SnapshotState.SUCCESS)); // Create a snapshot in progress - String inProgressSnapshot = "snapshot-in-progress"; blockAllDataNodes(repoName); - startFullSnapshot(repoName, inProgressSnapshot); + startFullSnapshot(repoName, "snapshot-in-progress"); awaitNumberOfSnapshotsInProgress(1); // Fetch snapshots with state=IN_PROGRESS - GetSnapshotsResponse responseInProgress = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) - .setState(EnumSet.of(SnapshotState.IN_PROGRESS)) - .get(); - assertThat(responseInProgress.getSnapshots(), hasSize(1)); - assertThat(responseInProgress.getSnapshots().get(0).state(), is(SnapshotState.IN_PROGRESS)); + snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.IN_PROGRESS)); + assertThat(snapshots, hasSize(1)); + assertThat(snapshots.getFirst().state(), is(SnapshotState.IN_PROGRESS)); // Fetch snapshots with multiple states (SUCCESS, IN_PROGRESS) - GetSnapshotsResponse responseMultipleStates = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName) - .setState(EnumSet.of(SnapshotState.SUCCESS, SnapshotState.IN_PROGRESS)) - .get(); - assertThat(responseMultipleStates.getSnapshots(), hasSize(2)); - assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.SUCCESS)); - assertTrue(responseMultipleStates.getSnapshots().stream().map(SnapshotInfo::state).toList().contains(SnapshotState.IN_PROGRESS)); + snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.SUCCESS, SnapshotState.IN_PROGRESS)); + assertThat(snapshots, hasSize(2)); + var states = snapshots.stream().map(SnapshotInfo::state).collect(Collectors.toSet()); + assertTrue(states.contains(SnapshotState.SUCCESS)); + assertTrue(states.contains(SnapshotState.IN_PROGRESS)); // Fetch all snapshots (without state) - GetSnapshotsResponse responseAll = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get(); - assertThat(responseAll.getSnapshots(), hasSize(2)); + snapshots = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get().getSnapshots(); + assertThat(snapshots, hasSize(2)); // Fetch snapshots with an invalid state IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).setState(EnumSet.of(SnapshotState.of("FOO"))).get() + () -> getSnapshotsForStates.apply(EnumSet.of(SnapshotState.valueOf("FOO"))) ); - assertThat(e.getMessage(), containsString("Unknown state name [FOO]")); + assertThat(e.getMessage(), is("No enum constant org.elasticsearch.snapshots.SnapshotState.FOO")); + + // Allow the IN_PROGRESS snapshot to finish, then verify GET using SUCCESS has results and IN_PROGRESS does not. + unblockAllDataNodes(repoName); + awaitNumberOfSnapshotsInProgress(0); + snapshots = clusterAdmin().prepareGetSnapshots(TEST_REQUEST_TIMEOUT, repoName).get().getSnapshots(); + assertThat(snapshots, hasSize(2)); + states = snapshots.stream().map(SnapshotInfo::state).collect(Collectors.toSet()); + assertThat(states, hasSize(1)); + assertTrue(states.contains(SnapshotState.SUCCESS)); + snapshots = getSnapshotsForStates.apply(EnumSet.of(SnapshotState.IN_PROGRESS)); + assertThat(snapshots, hasSize(0)); } // Create a snapshot that is guaranteed to have a unique start time and duration for tests around ordering by either. @@ -963,15 +972,15 @@ public void testAllFeatures() { // INDICES and by SHARDS. The actual sorting behaviour for these cases is tested elsewhere, here we're just checking that sorting // interacts correctly with the other parameters to the API. - final EnumSet state = EnumSet.of(randomFrom(SnapshotState.values())); - // Note: The selected state may not match any existing snapshots. + final EnumSet states = EnumSet.copyOf(randomNonEmptySubsetOf(Arrays.asList(SnapshotState.values()))); + // Note: The selected state(s) may not match any existing snapshots. // The actual filtering behaviour for such cases is tested in the dedicated test. - // Here we're just checking that state interacts correctly with the other parameters to the API. + // Here we're just checking that states interacts correctly with the other parameters to the API. + snapshotInfoPredicate = snapshotInfoPredicate.and(si -> states.contains(si.state())); // compute the ordered sequence of snapshots which match the repository/snapshot name filters and SLM policy filter final var selectedSnapshots = snapshotInfos.stream() .filter(snapshotInfoPredicate) - .filter(s -> state.contains(s.state())) .sorted(sortKey.getSnapshotInfoComparator(order)) .toList(); @@ -981,7 +990,7 @@ public void testAllFeatures() { // apply sorting params .sort(sortKey) .order(order) - .state(state); + .states(states); // sometimes use ?from_sort_value to skip some items; note that snapshots skipped in this way are subtracted from // GetSnapshotsResponse.totalCount whereas snapshots skipped by ?after and ?offset are not @@ -1069,7 +1078,7 @@ public void testAllFeatures() { .order(order) .size(nextSize) .after(SnapshotSortKey.decodeAfterQueryParam(nextRequestAfter)) - .state(state); + .states(states); final GetSnapshotsResponse nextResponse = safeAwait(l -> client().execute(TransportGetSnapshotsAction.TYPE, nextRequest, l)); assertEquals( diff --git a/server/src/main/java/module-info.java b/server/src/main/java/module-info.java index 8da4f403c29bd..f6d596f6f2fd6 100644 --- a/server/src/main/java/module-info.java +++ b/server/src/main/java/module-info.java @@ -424,6 +424,7 @@ org.elasticsearch.action.bulk.BulkFeatures, org.elasticsearch.features.InfrastructureFeatures, org.elasticsearch.rest.action.admin.cluster.ClusterRerouteFeatures, + org.elasticsearch.rest.action.admin.cluster.GetSnapshotsFeatures, org.elasticsearch.index.mapper.MapperFeatures, org.elasticsearch.index.IndexFeatures, org.elasticsearch.search.SearchFeatures, diff --git a/server/src/main/java/org/elasticsearch/action/ActionModule.java b/server/src/main/java/org/elasticsearch/action/ActionModule.java index 2f9f4340bfa70..6dfe61e581083 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/server/src/main/java/org/elasticsearch/action/ActionModule.java @@ -865,7 +865,7 @@ public void initRestHandlers(Supplier nodesInCluster, Predicate< registerHandler.accept(new RestDeleteRepositoryAction()); registerHandler.accept(new RestVerifyRepositoryAction()); registerHandler.accept(new RestCleanupRepositoryAction()); - registerHandler.accept(new RestGetSnapshotsAction()); + registerHandler.accept(new RestGetSnapshotsAction(clusterSupportsFeature)); registerHandler.accept(new RestCreateSnapshotAction()); registerHandler.accept(new RestCloneSnapshotAction()); registerHandler.accept(new RestRestoreSnapshotAction()); diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java index 78c46b39ea6b1..442108fbc6d09 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java @@ -28,6 +28,7 @@ import java.util.Arrays; import java.util.EnumSet; import java.util.Map; +import java.util.Objects; import static org.elasticsearch.action.ValidateActions.addValidationError; @@ -80,7 +81,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest private boolean includeIndexNames = true; - private EnumSet state = EnumSet.noneOf(SnapshotState.class); + private EnumSet states = EnumSet.allOf(SnapshotState.class); public GetSnapshotsRequest(TimeValue masterNodeTimeout) { super(masterNodeTimeout); @@ -124,7 +125,9 @@ public GetSnapshotsRequest(StreamInput in) throws IOException { includeIndexNames = in.readBoolean(); } if (in.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) { - state = in.readEnumSet(SnapshotState.class); + states = in.readEnumSet(SnapshotState.class); + } else { + states = EnumSet.allOf(SnapshotState.class); } } @@ -146,7 +149,11 @@ public void writeTo(StreamOutput out) throws IOException { out.writeBoolean(includeIndexNames); } if (out.getTransportVersion().onOrAfter(STATE_FLAG_VERSION)) { - out.writeEnumSet(state); + out.writeEnumSet(states); + } else if (states.equals(EnumSet.allOf(SnapshotState.class)) == false) { + final var errorString = "GetSnapshotsRequest [states] field is not supported on all nodes in the cluster"; + assert false : errorString; + throw new IllegalStateException(errorString); } } @@ -353,12 +360,12 @@ public boolean verbose() { return verbose; } - public EnumSet state() { - return state; + public EnumSet states() { + return states; } - public GetSnapshotsRequest state(EnumSet state) { - this.state = state; + public GetSnapshotsRequest states(EnumSet states) { + this.states = Objects.requireNonNull(states); return this; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java index 24dc15f5788cc..176523a4725f0 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java @@ -153,8 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) { } - public GetSnapshotsRequestBuilder setState(EnumSet state) { - request.state(state); + public GetSnapshotsRequestBuilder setStates(EnumSet states) { + request.states(states); return this; } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java index b87e484cde9f5..f8c59701d8d30 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java @@ -163,7 +163,7 @@ protected void masterOperation( SnapshotsInProgress.get(state), request.verbose(), request.includeIndexNames(), - request.state() + request.states() ).runOperation(listener); } @@ -184,7 +184,7 @@ private class GetSnapshotsOperation { private final SnapshotNamePredicate snapshotNamePredicate; private final SnapshotPredicates fromSortValuePredicates; private final Predicate slmPolicyPredicate; - private final EnumSet state; + private final EnumSet states; // snapshot ordering/pagination private final SnapshotSortKey sortBy; @@ -229,7 +229,7 @@ private class GetSnapshotsOperation { SnapshotsInProgress snapshotsInProgress, boolean verbose, boolean indices, - EnumSet state + EnumSet states ) { this.cancellableTask = cancellableTask; this.repositories = repositories; @@ -242,7 +242,7 @@ private class GetSnapshotsOperation { this.snapshotsInProgress = snapshotsInProgress; this.verbose = verbose; this.indices = indices; - this.state = state; + this.states = states; this.snapshotNamePredicate = SnapshotNamePredicate.forSnapshots(ignoreUnavailable, snapshots); this.fromSortValuePredicates = SnapshotPredicates.forFromSortValue(fromSortValue, sortBy, order); @@ -566,7 +566,10 @@ private boolean matchesPredicates(SnapshotId snapshotId, RepositoryData reposito final var details = repositoryData.getSnapshotDetails(snapshotId); - if (!state.isEmpty() && !state.contains(details.getSnapshotState())) { + if (states.isEmpty() == false + && details != null + && details.getSnapshotState() != null + && states.contains(details.getSnapshotState()) == false) { return false; } @@ -582,6 +585,10 @@ private boolean matchesPredicates(SnapshotInfo snapshotInfo) { return false; } + if (states.isEmpty() == false && snapshotInfo.state() != null && states.contains(snapshotInfo.state()) == false) { + return false; + } + if (slmPolicyPredicate == SlmPolicyPredicate.MATCH_ALL_POLICIES) { return true; } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java new file mode 100644 index 0000000000000..f66aa25e0b994 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java @@ -0,0 +1,24 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +package org.elasticsearch.rest.action.admin.cluster; + +import org.elasticsearch.features.FeatureSpecification; +import org.elasticsearch.features.NodeFeature; + +import java.util.Set; + +public class GetSnapshotsFeatures implements FeatureSpecification { + public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("get.snapshots.state_parameter"); + + @Override + public Set getFeatures() { + return Set.of(GET_SNAPSHOTS_STATE_PARAMETER); + } +} diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index 2589d28f34078..1c74ac2dcf787 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -13,6 +13,7 @@ import org.elasticsearch.action.admin.cluster.snapshots.get.SnapshotSortKey; import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.common.Strings; +import org.elasticsearch.features.NodeFeature; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.Scope; @@ -27,6 +28,7 @@ import java.util.EnumSet; import java.util.List; import java.util.Set; +import java.util.function.Predicate; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout; @@ -40,7 +42,11 @@ @ServerlessScope(Scope.INTERNAL) public class RestGetSnapshotsAction extends BaseRestHandler { - public RestGetSnapshotsAction() {} + private final Predicate clusterSupportsFeature; + + public RestGetSnapshotsAction(Predicate clusterSupportsFeature) { + this.clusterSupportsFeature = clusterSupportsFeature; + } @Override public List routes() { @@ -87,10 +93,12 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC getSnapshotsRequest.includeIndexNames(request.paramAsBoolean(INDEX_NAMES_XCONTENT_PARAM, getSnapshotsRequest.includeIndexNames())); final String stateString = request.param("state"); - if (stateString == null || stateString.isEmpty()) { - getSnapshotsRequest.state(EnumSet.noneOf(SnapshotState.class)); + if (Strings.hasLength(stateString) == false) { + getSnapshotsRequest.states(EnumSet.allOf(SnapshotState.class)); + } else if (clusterSupportsFeature.test(GetSnapshotsFeatures.GET_SNAPSHOTS_STATE_PARAMETER)) { + getSnapshotsRequest.states(EnumSet.copyOf(Arrays.stream(stateString.split(",")).map(SnapshotState::valueOf).toList())); } else { - getSnapshotsRequest.state(EnumSet.copyOf(Arrays.stream(stateString.split(",")).map(SnapshotState::of).toList())); + throw new IllegalStateException("[state] parameter is not supported on all nodes in the cluster"); } return channel -> new RestCancellableNodeClient(client, request.getHttpChannel()).admin() diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java index df593d18f453b..dcf90327e1098 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotState.java @@ -89,21 +89,4 @@ public static SnapshotState fromValue(byte value) { default -> throw new IllegalArgumentException("No snapshot state for value [" + value + "]"); }; } - - /** - * Generate snapshot state from a string (case-insensitive) - * - * @param name the state name - * @return state - */ - public static SnapshotState of(String name) { - return switch (name.toUpperCase()) { - case "IN_PROGRESS" -> IN_PROGRESS; - case "SUCCESS" -> SUCCESS; - case "FAILED" -> FAILED; - case "PARTIAL" -> PARTIAL; - case "INCOMPATIBLE" -> INCOMPATIBLE; - default -> throw new IllegalArgumentException("Unknown state name [" + name + "]"); - }; - } } diff --git a/server/src/main/resources/META-INF/services/org.elasticsearch.features.FeatureSpecification b/server/src/main/resources/META-INF/services/org.elasticsearch.features.FeatureSpecification index 82c971f9144d8..14749330f09da 100644 --- a/server/src/main/resources/META-INF/services/org.elasticsearch.features.FeatureSpecification +++ b/server/src/main/resources/META-INF/services/org.elasticsearch.features.FeatureSpecification @@ -10,6 +10,7 @@ org.elasticsearch.action.bulk.BulkFeatures org.elasticsearch.features.InfrastructureFeatures org.elasticsearch.rest.action.admin.cluster.ClusterRerouteFeatures +org.elasticsearch.rest.action.admin.cluster.GetSnapshotsFeatures org.elasticsearch.index.IndexFeatures org.elasticsearch.index.mapper.MapperFeatures org.elasticsearch.search.SearchFeatures From 04fcc5523c1adb18e52be46d8f5c21c347567820 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 29 May 2025 15:02:46 -0400 Subject: [PATCH 03/11] Update docs/changelog/128635.yaml --- docs/changelog/128635.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/128635.yaml diff --git a/docs/changelog/128635.yaml b/docs/changelog/128635.yaml new file mode 100644 index 0000000000000..19a1dd0404ce1 --- /dev/null +++ b/docs/changelog/128635.yaml @@ -0,0 +1,6 @@ +pr: 128635 +summary: Add `state` query param to Get snapshots API +area: Snapshot/Restore +type: enhancement +issues: + - 97446 From 6a560fad83954ae0e4f7a099916bbd2278f62a01 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Fri, 30 May 2025 20:12:20 -0400 Subject: [PATCH 04/11] Use test_runner_features: capabilities in new yaml test --- .../test/snapshot.get/10_basic.yml | 7 +++ .../admin/cluster/RestGetSnapshotsAction.java | 47 ++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml index 079e52564a2f2..56c1e4534f9dc 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml @@ -306,6 +306,13 @@ setup: --- "Get snapshot using state parameter": + - requires: + test_runner_features: capabilities + capabilities: + - method: GET + path: /_snapshot/{repository}/{snapshot} + parameters: [ state ] + reason: "state parameter was introduced in 9.1" - do: indices.create: diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index 1c74ac2dcf787..bfeb83a89af21 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -13,9 +13,12 @@ import org.elasticsearch.action.admin.cluster.snapshots.get.SnapshotSortKey; import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.core.Assertions; import org.elasticsearch.features.NodeFeature; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.rest.RestUtils; import org.elasticsearch.rest.Scope; import org.elasticsearch.rest.ServerlessScope; import org.elasticsearch.rest.action.RestCancellableNodeClient; @@ -42,6 +45,30 @@ @ServerlessScope(Scope.INTERNAL) public class RestGetSnapshotsAction extends BaseRestHandler { + private static final Set SUPPORTED_RESPONSE_PARAMETERS = Set.of( + INCLUDE_REPOSITORY_XCONTENT_PARAM, + INDEX_DETAILS_XCONTENT_PARAM, + INDEX_NAMES_XCONTENT_PARAM + ); + + private static final Set SUPPORTED_QUERY_PARAMETERS = Set.of( + RestUtils.REST_MASTER_TIMEOUT_PARAM, + "after", + "from_sort_value", + "ignore_unavailable", + "offset", + "order", + "size", + "slm_policy_filter", + "sort", + "state", + "verbose" + ); + + private static final Set ALL_SUPPORTED_PARAMETERS = Set.copyOf( + Sets.union(SUPPORTED_QUERY_PARAMETERS, SUPPORTED_RESPONSE_PARAMETERS, Set.of("repository", "snapshot")) + ); + private final Predicate clusterSupportsFeature; public RestGetSnapshotsAction(Predicate clusterSupportsFeature) { @@ -60,7 +87,17 @@ public String getName() { @Override protected Set responseParams() { - return Set.of(INDEX_DETAILS_XCONTENT_PARAM, INCLUDE_REPOSITORY_XCONTENT_PARAM, INDEX_NAMES_XCONTENT_PARAM); + return SUPPORTED_RESPONSE_PARAMETERS; + } + + @Override + public Set supportedQueryParameters() { + return SUPPORTED_QUERY_PARAMETERS; + } + + @Override + public Set allSupportedParameters() { + return ALL_SUPPORTED_PARAMETERS; } @Override @@ -101,6 +138,14 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC throw new IllegalStateException("[state] parameter is not supported on all nodes in the cluster"); } + // Consume these response parameters used in SnapshotInfo now, to avoid assertion errors in BaseRestHandler for requests where they + // may not get used. + if (Assertions.ENABLED) { + for (final var responseParameter : SUPPORTED_RESPONSE_PARAMETERS) { + request.param(responseParameter); + } + } + return channel -> new RestCancellableNodeClient(client, request.getHttpChannel()).admin() .cluster() .getSnapshots(getSnapshotsRequest, new RestRefCountedChunkedToXContentListener<>(channel)); From 8bb5b22e740bdb4e7564d21148693debbe443bfc Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Mon, 2 Jun 2025 12:15:54 -0400 Subject: [PATCH 05/11] Add cluster_features clause to new yaml test to check for new NodeFeature --- .../resources/rest-api-spec/test/snapshot.get/10_basic.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml index 56c1e4534f9dc..cec24e3a7778c 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml @@ -307,6 +307,7 @@ setup: --- "Get snapshot using state parameter": - requires: + cluster_features: "get.snapshots.state_parameter" test_runner_features: capabilities capabilities: - method: GET From 64d3729a5dcd3cd0a1c8cadbdd45046746b775fa Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 12 Jun 2025 16:59:44 -0400 Subject: [PATCH 06/11] Add handling for null (missing) and present but empty state param values --- .../rest/action/admin/cluster/RestGetSnapshotsAction.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index bfeb83a89af21..7fe6339823d1a 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -130,8 +130,10 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC getSnapshotsRequest.includeIndexNames(request.paramAsBoolean(INDEX_NAMES_XCONTENT_PARAM, getSnapshotsRequest.includeIndexNames())); final String stateString = request.param("state"); - if (Strings.hasLength(stateString) == false) { + if (stateString == null) { getSnapshotsRequest.states(EnumSet.allOf(SnapshotState.class)); + } else if (Strings.hasText(stateString) == false) { + throw new IllegalArgumentException("[state] parameter must not be empty"); } else if (clusterSupportsFeature.test(GetSnapshotsFeatures.GET_SNAPSHOTS_STATE_PARAMETER)) { getSnapshotsRequest.states(EnumSet.copyOf(Arrays.stream(stateString.split(",")).map(SnapshotState::valueOf).toList())); } else { From 548fe0e54cb77d18df6bd6d33ca98f0e0aad9a91 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 12 Jun 2025 17:01:43 -0400 Subject: [PATCH 07/11] Use IllegalArgumentException when state is used in mixed cluster --- .../rest/action/admin/cluster/RestGetSnapshotsAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index 7fe6339823d1a..e48a4e74e6474 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -137,7 +137,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC } else if (clusterSupportsFeature.test(GetSnapshotsFeatures.GET_SNAPSHOTS_STATE_PARAMETER)) { getSnapshotsRequest.states(EnumSet.copyOf(Arrays.stream(stateString.split(",")).map(SnapshotState::valueOf).toList())); } else { - throw new IllegalStateException("[state] parameter is not supported on all nodes in the cluster"); + throw new IllegalArgumentException("[state] parameter is not supported on all nodes in the cluster"); } // Consume these response parameters used in SnapshotInfo now, to avoid assertion errors in BaseRestHandler for requests where they From 3ed93509d61b3037bd6bbaf16d0a0af29a506569 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 12 Jun 2025 17:12:05 -0400 Subject: [PATCH 08/11] Remove unnecessary check for states.isEmpty() in predicates, enforce states is not empty in validate() --- .../admin/cluster/snapshots/get/GetSnapshotsRequest.java | 3 +++ .../cluster/snapshots/get/TransportGetSnapshotsAction.java | 7 ++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java index 442108fbc6d09..628e48ab1ceb3 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java @@ -195,6 +195,9 @@ public ActionRequestValidationException validate() { } else if (after != null && fromSortValue != null) { validationException = addValidationError("can't use after and from_sort_value simultaneously", validationException); } + if (states.isEmpty()) { + validationException = addValidationError("states is empty", validationException); + } return validationException; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java index b6063453664aa..eab763165a74c 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java @@ -580,10 +580,7 @@ private boolean matchesPredicates(SnapshotId snapshotId, RepositoryData reposito final var details = repositoryData.getSnapshotDetails(snapshotId); - if (states.isEmpty() == false - && details != null - && details.getSnapshotState() != null - && states.contains(details.getSnapshotState()) == false) { + if (details != null && details.getSnapshotState() != null && states.contains(details.getSnapshotState()) == false) { return false; } @@ -599,7 +596,7 @@ private boolean matchesPredicates(SnapshotInfo snapshotInfo) { return false; } - if (states.isEmpty() == false && snapshotInfo.state() != null && states.contains(snapshotInfo.state()) == false) { + if (snapshotInfo.state() != null && states.contains(snapshotInfo.state()) == false) { return false; } From 627c9cc3d731ee7f3c5eaafc96902213ca6f2f78 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 12 Jun 2025 16:49:42 -0400 Subject: [PATCH 09/11] Add exclusions for unconsumed response parameters --- .../java/org/elasticsearch/rest/BaseRestHandler.java | 4 ++++ .../action/admin/cluster/RestGetSnapshotsAction.java | 9 --------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index 509086b982319..16a734b654f16 100644 --- a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java @@ -154,6 +154,10 @@ private boolean assertConsumesSupportedParams(@Nullable Set supported, R supportedAndCommon.removeAll(RestRequest.INTERNAL_MARKER_REQUEST_PARAMETERS); final var consumed = new TreeSet<>(request.consumedParams()); consumed.removeAll(RestRequest.INTERNAL_MARKER_REQUEST_PARAMETERS); + // Add exclusions for response parameters since they may not always be consumed for every request. + final var unconsumedResponseParams = new HashSet<>(responseParams(request.getRestApiVersion())); + unconsumedResponseParams.removeAll(consumed); + supportedAndCommon.removeAll(unconsumedResponseParams); assert supportedAndCommon.equals(consumed) : getName() + ": consumed params " + consumed + " while supporting " + supportedAndCommon; } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java index e48a4e74e6474..4b58fae1741c7 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetSnapshotsAction.java @@ -14,7 +14,6 @@ import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.common.Strings; import org.elasticsearch.common.util.set.Sets; -import org.elasticsearch.core.Assertions; import org.elasticsearch.features.NodeFeature; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestRequest; @@ -140,14 +139,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC throw new IllegalArgumentException("[state] parameter is not supported on all nodes in the cluster"); } - // Consume these response parameters used in SnapshotInfo now, to avoid assertion errors in BaseRestHandler for requests where they - // may not get used. - if (Assertions.ENABLED) { - for (final var responseParameter : SUPPORTED_RESPONSE_PARAMETERS) { - request.param(responseParameter); - } - } - return channel -> new RestCancellableNodeClient(client, request.getHttpChannel()).admin() .cluster() .getSnapshots(getSnapshotsRequest, new RestRefCountedChunkedToXContentListener<>(channel)); From 0656d9e11b760ad3e7b7fbb3f3f3cf6369b5686e Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Fri, 13 Jun 2025 10:58:42 -0400 Subject: [PATCH 10/11] Add response params to consumed before assertion check, adjust comment --- .../main/java/org/elasticsearch/rest/BaseRestHandler.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index 16a734b654f16..6834c01d2d0e9 100644 --- a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java @@ -154,10 +154,8 @@ private boolean assertConsumesSupportedParams(@Nullable Set supported, R supportedAndCommon.removeAll(RestRequest.INTERNAL_MARKER_REQUEST_PARAMETERS); final var consumed = new TreeSet<>(request.consumedParams()); consumed.removeAll(RestRequest.INTERNAL_MARKER_REQUEST_PARAMETERS); - // Add exclusions for response parameters since they may not always be consumed for every request. - final var unconsumedResponseParams = new HashSet<>(responseParams(request.getRestApiVersion())); - unconsumedResponseParams.removeAll(consumed); - supportedAndCommon.removeAll(unconsumedResponseParams); + // Response parameters are implicitly consumed since they are made available to response renderings. + consumed.addAll(responseParams(request.getRestApiVersion())); assert supportedAndCommon.equals(consumed) : getName() + ": consumed params " + consumed + " while supporting " + supportedAndCommon; } From f8627ba3d4e3d7309c232cb5d490d886cbd322a7 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Mon, 16 Jun 2025 12:07:38 -0400 Subject: [PATCH 11/11] Change the NodeFeature to snapshots.get.state_parameter --- .../resources/rest-api-spec/test/snapshot.get/10_basic.yml | 2 +- .../rest/action/admin/cluster/GetSnapshotsFeatures.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml index cec24e3a7778c..e37939f71b1de 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml @@ -307,7 +307,7 @@ setup: --- "Get snapshot using state parameter": - requires: - cluster_features: "get.snapshots.state_parameter" + cluster_features: "snapshots.get.state_parameter" test_runner_features: capabilities capabilities: - method: GET diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java index f66aa25e0b994..050d77b62db86 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/GetSnapshotsFeatures.java @@ -15,7 +15,7 @@ import java.util.Set; public class GetSnapshotsFeatures implements FeatureSpecification { - public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("get.snapshots.state_parameter"); + public static final NodeFeature GET_SNAPSHOTS_STATE_PARAMETER = new NodeFeature("snapshots.get.state_parameter"); @Override public Set getFeatures() {