Skip to content

Add state query param to Get snapshots API #123618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add some simple smoke tests to rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/snapshot.get/10_basic.yml just to check that we are parsing the parameter ok. No need for anything exhaustive, just check that e.g. SUCCESS and SUCCESS,PARTIAL both return the expected snapshots.

"type": "list",
"description": "Filter snapshots by a comma-separated list of states. Valid state values are 'SUCCESS', 'IN_PROGRESS', 'FAILED', 'PARTIAL', or 'INCOMPATIBLE'."
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -633,6 +635,55 @@ public void testRetrievingSnapshotsWhenRepositoryIsMissing() throws Exception {
expectThrows(RepositoryMissingException.class, multiRepoFuture::actionGet);
}

public void testFilterByState() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add this feature to testAllFeatures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added some checks there as well. Let me know if I need to add any other tests.

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
Expand Down Expand Up @@ -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<SnapshotState> state = EnumSet.of(randomFrom(SnapshotState.values()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we test a random nonempty subset rather than just a singleton?

// 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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should fold the state filter into snapshotInfoPredicate, since we're really implementing it alongside the SLM policy check etc.

.sorted(sortKey.getSnapshotInfoComparator(order))
.toList();

Expand All @@ -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
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ static TransportVersion def(int id) {
public static final TransportVersion STORED_SCRIPT_CONTENT_LENGTH = def(9_019_0_00);
public static final TransportVersion JINA_AI_EMBEDDING_TYPE_SUPPORT_ADDED = def(9_020_0_00);
public static final TransportVersion RE_REMOVE_MIN_COMPATIBLE_SHARD_NODE = def(9_021_0_00);
public static final TransportVersion STATE_PARAM_GET_SNAPSHOT = def(9_022_0_00);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,6 +41,7 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>
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;

Expand Down Expand Up @@ -77,6 +80,8 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>

private boolean includeIndexNames = true;

private EnumSet<SnapshotState> state = EnumSet.noneOf(SnapshotState.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private EnumSet<SnapshotState> state = EnumSet.noneOf(SnapshotState.class);
private EnumSet<SnapshotState> state = EnumSet.allOf(SnapshotState.class);


public GetSnapshotsRequest(TimeValue masterNodeTimeout) {
super(masterNodeTimeout);
}
Expand Down Expand Up @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid state being null here, by setting it to EnumSet.allOf(SnapshotState.class) on the else branch

}

@Override
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a mixed-version cluster where not all nodes understand this feature we shouldn't permit users to use it rather than just silently ignoring it as done here. IOW we need an else branch here that checks state == EnumSet.allOf() and if it isn't then we should assert false and then throw an IllegalStateException. We also need to adjust the REST handler to look at the node versions in the cluster and reject the parameter if the cluster has any nodes that don't understand it.

Specifically, we need to introduce a NodeFeature for this feature and then update ActionModule#initRestHandlers to pass in the features predicate registerHandler.accept(new RestGetSnapshotsAction(clusterSupportsFeature));

}
}

@Override
Expand Down Expand Up @@ -342,6 +353,15 @@ public boolean verbose() {
return verbose;
}

public EnumSet<SnapshotState> state() {
return state;
}

public GetSnapshotsRequest state(EnumSet<SnapshotState> state) {
this.state = state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validation nit

Suggested change
this.state = state;
this.state = Objects.requireNonNull(state);

return this;
}

@Override
public Task createTask(long id, String type, String action, TaskId parentTaskId, Map<String, String> headers) {
return new CancellableTask(id, type, action, getDescription(), parentTaskId, headers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -150,4 +153,8 @@ public GetSnapshotsRequestBuilder setIncludeIndexNames(boolean indices) {

}

public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming nit

Suggested change
public GetSnapshotsRequestBuilder setState(EnumSet<SnapshotState> state) {
public GetSnapshotsRequestBuilder setStates(EnumSet<SnapshotState> state) {

request.state(state);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -160,7 +162,8 @@ protected void masterOperation(
request.size(),
SnapshotsInProgress.get(state),
request.verbose(),
request.includeIndexNames()
request.includeIndexNames(),
request.state()
).runOperation(listener);
}

Expand All @@ -181,6 +184,7 @@ private class GetSnapshotsOperation {
private final SnapshotNamePredicate snapshotNamePredicate;
private final SnapshotPredicates fromSortValuePredicates;
private final Predicate<String> slmPolicyPredicate;
private final EnumSet<SnapshotState> state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming nit

Suggested change
private final EnumSet<SnapshotState> state;
private final EnumSet<SnapshotState> states;


// snapshot ordering/pagination
private final SnapshotSortKey sortBy;
Expand Down Expand Up @@ -224,7 +228,8 @@ private class GetSnapshotsOperation {
int size,
SnapshotsInProgress snapshotsInProgress,
boolean verbose,
boolean indices
boolean indices,
EnumSet<SnapshotState> state
) {
this.cancellableTask = cancellableTask;
this.repositories = repositories;
Expand All @@ -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);
Expand Down Expand Up @@ -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())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style check will not like the unary ! operators here, you need to say state.contains(details.getSnapshotState()) == false instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh also we need to handle the cases where details or details.getSnapshotState() are null. In that case, we cannot do the pre-flight check, we will need to retrieve the SnapshotInfo and then check that in the other matchesPredicates() method

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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: suggest using org.elasticsearch.common.Strings#hasLength(java.lang.String) here

getSnapshotsRequest.state(EnumSet.noneOf(SnapshotState.class));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest defaulting to EnumSet.allOf rather than having a special case for the empty set lower down. We could reasonably reject the empty set in org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest#validate I suppose since otherwise that'll just filter out everything.

} 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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming nit: maybe fromString rather than of? As written here it makes sense if you call the method directly but it's a little awkward to read .map(SnapshotState::of), I'd prefer .map(SnapshotState::fromString).

Also could we not use the built-in valueOf()?

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 + "]");
};
}
}