Add Filtering by SLM Policy to Get Snapshots API by original-brownbear · Pull Request #77321 · elastic/elasticsearch

original-brownbear · 2021-09-06T20:02:32Z

It's in the title, add new slm_policy field as a filter to
the get snapshots API.

It's in the title, add new `slm_policy` field as a filter to the get snapshots API.

elasticmachine · 2021-09-06T20:02:35Z

Pinging @elastic/es-distributed (Team:Distributed)

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

arteam

LGTM, I've left a couple of small comments

arteam · 2021-09-07T07:11:49Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

 Numeric offset to start pagination from based on the snapshots matching this request. Using a non-zero value for this parameter is mutually
 exclusive with using the `after` parameter. Defaults to `0`.

+`slm_policy`::


Since we can have multiple polices here, does it make sense to call the parameter slm_policies or slm_policy_patterns?

I also think there is something we could do about naming here, perhaps pre of suffix it with filter likeslm_policy_filter? It would be nice to be able to clearly identify the filtering options (in case we add more of them).

Sounds good, I went with slm_policy_filter now :)

.../src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java

...in/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java

arteam · 2021-09-07T07:20:59Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

+            final String[] excludes = excludePatterns.toArray(Strings.EMPTY_ARRAY);
+            filteredSnapshotInfos = snapshotInfos.stream().filter(snapshotInfo -> {
+                final Map<String, Object> metadata = snapshotInfo.userMetadata();
+                final String policy;


I was wondering if were possible to extract getting policy from the metadata to own method in order to make policy final.

String getPolicy(Metadata metadata) { if (metadata != null) { final Object policyFound = metadata.get(SnapshotsService.POLICY_ID_METADATA_FIELD); if (policyFound instanceof String) { return (String) policyFound; } } return ""; }

Makes sense, I'll extract this logic in a follow-up then we can dry it up with SLM code in one go :)

henningandersen

I left a few comments, primarily tests and structure.

henningandersen · 2021-09-07T08:40:55Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

 Numeric offset to start pagination from based on the snapshots matching this request. Using a non-zero value for this parameter is mutually
 exclusive with using the `after` parameter. Defaults to `0`.

+`slm_policy`::


I also think there is something we could do about naming here, perhaps pre of suffix it with filter likeslm_policy_filter? It would be nice to be able to clearly identify the filtering options (in case we add more of them).

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

henningandersen · 2021-09-07T08:55:47Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

 Numeric offset to start pagination from based on the snapshots matching this request. Using a non-zero value for this parameter is mutually
 exclusive with using the `after` parameter. Defaults to `0`.

+`slm_policy`::


I think using this option is only supported in verbose mode? We should note that here and validate it.

Right, added it to the note and added a UT for the validation

henningandersen · 2021-09-07T08:57:49Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

                    GetSnapshotsRequest.NO_LIMIT,
-                    order
+                    order,
+                    slmPolicies


I think slmPolicies is not really supporetd here, since it is not verbose?

++ it's always an empty array here, resolved by splitting this method up now

henningandersen · 2021-09-07T09:51:48Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

+        final SortOrder order,
+        final String[] slmPolicies
    ) {
+        final List<SnapshotInfo> filteredSnapshotInfos;


Could we make the filtering here a separate method instead? It looks like there is at least one usage where this is not needed and it is a bit odd to include into the sort method which is also already long enough as it stands. Perhaps add a sortAndFilter that calls both too.

Sounds good, made it into 2 methods and kept the old sort method for those spots that have no filtering :)

henningandersen · 2021-09-07T09:58:25Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

+            .setPolicies("*", "-" + policyName)
+            .get()
+            .getSnapshots();
+        assertThat(snapshotsNoPolicy, is(snapshotsWithoutPolicy));


Can we add tests of:

A slm-policy that is not there.

Multiple slm-policies (where some may not exist).

An actual inclusion pattern, i.e., policyName + "*".

An actual exclusion pattern, i.e., `, "-" + policyName + ""

Could be randomly picking from some of these if that is easiest.

++ Added all 4 cases explicitly to rest and internal cluster tests :)

…y-id

original-brownbear · 2021-09-07T11:23:19Z

Thanks @henningandersen and @arteam all comments addressed now I think :)

arteam

LGTM!

henningandersen

Thanks Armin. On my second round I realized a couple more things to consider.

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

henningandersen · 2021-09-07T11:56:42Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java


+    private static List<SnapshotInfo> getAllSnapshotsForPolicies(String... policies) throws IOException {
+        final Request requestWithPolicy = new Request(HttpGet.METHOD_NAME, "/_snapshot/*/*");
+        requestWithPolicy.addParameter("slm_policy_filter", Strings.arrayToCommaDelimitedString(policies));


I wonder if we need an explicit sort here or to compare the results by converrting to sets? Since we spawn the creation of snapshots in createNSnapshots, I think the start times of two snapshots could come out of order? We also risk same timestamp but are then rescued by tiebreaking on name, which works because there are max 5 snapshots (i.e., less than 10), but it also seems a bit strange to rely on at least without a comment.

You're right, we could actually theoretically fail if everything executes in the same millisecond (... looking at some/windows systems that seems possible). Forced sort-by-name everywhere now in both tests. Thanks!

henningandersen · 2021-09-07T11:57:05Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

+    }
+
+    private static List<SnapshotInfo> getAllSnapshotsForPolicies(String... policies) {
+        return clusterAdmin().prepareGetSnapshots("*").setSnapshots("*").setPolicies(policies).get().getSnapshots();


Like for the rest test, some stable sort seems necessary.

henningandersen · 2021-09-07T12:08:00Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

+            final Map<String, Object> metadata = snapshotInfo.userMetadata();
+            final String policy;
+            if (metadata == null) {
+                policy = "";


I think we have a choice here. Since there could be no slm-policy (in contrast to repo and name), we could choose between a few options:

* also matches no policy (like here). This allows *,-a* to give all snapshots with no policy starting with a (including all those with no policy), but no easy way to find all those with no policy.

* only matches those that have a policy. This allows * to find all those with a policy and we could define -* to find all those without a policy.

So 2 seems more flexible but deviates from how we handle repo and name. But then again, we are defining this one from scratch so we have the freedom. Let me know your thoughts.

Hmm I think I like 2 better as well. I wonder though, the -* trick we'll have to document anyway. Maybe we should use a special parameter _none (like we have for the ingest pipeline name to turn off a pipeline) for this. That way we could even do queries that involve a set of policies + all snapshots without any policies (though we could do this with this pattern as well, it seem incredibly confusing to have this as the only exclude pattern that is technically a positive match pattern for null)?

++ to using _none as a special null value.

+1 to the null value pattern, too

…y-id

original-brownbear · 2021-09-07T13:35:58Z

@henningandersen explicit _none pattern and some tests for it have been added :)

henningandersen

LGTM, thanks for the extra iterations on this.

henningandersen · 2021-09-07T14:03:40Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

 */
 public class TransportGetSnapshotsAction extends TransportMasterNodeAction<GetSnapshotsRequest, GetSnapshotsResponse> {

+    public static final String NO_POLICY_PATTERN = "_none";


nit: I think this belongs on the request more than the transport action. It is part of the interface of the action.

henningandersen · 2021-09-07T14:07:23Z

.../src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java

            }
+            if (policies.length != 0) {
+                validationException = addValidationError("can't use slm policy filter with verbose=false", validationException);
+            }


Can we add a generic validation to not use -_none and perhaps even not allow _ or -_ prefixed policies?

-_none "theoretically" you could have a policy by that name I think :D

henningandersen · 2021-09-07T14:14:12Z

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

@@ -220,6 +226,19 @@ public void testFilterBySLMPolicy() throws Exception {
        );
        assertThat(getAllSnapshotsForPolicies(policyName, otherPolicyName), is(List.of(withOtherPolicy, withPolicy)));
        assertThat(getAllSnapshotsForPolicies(policyName, otherPolicyName, "no-such-policy*"), is(List.of(withOtherPolicy, withPolicy)));


I think just * case is missing?

assertThat(getAllSnapshotsForPolicies("*"), is(List.of(withOtherPolicy, withPolicy)));

henningandersen · 2021-09-07T14:15:02Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java

@@ -244,6 +249,17 @@ public void testFilterBySLMPolicy() throws Exception {
        );
        assertThat(getAllSnapshotsForPolicies(policyName, otherPolicyName), is(List.of(withOtherPolicy, withPolicy)));
        assertThat(getAllSnapshotsForPolicies(policyName, otherPolicyName, "no-such-policy*"), is(List.of(withOtherPolicy, withPolicy)));


Also add just * case here:

assertThat(getAllSnapshotsForPolicies("*"), is(List.of(withOtherPolicy, withPolicy)));

…y-id

original-brownbear · 2021-09-07T16:23:51Z

Thanks Henning & Artem!

It's in the title, add new `slm_policy_filter` param as a filter to the get snapshots API.

Add Filtering by SLM Policy to Get Snapshots API

5ce81c2

It's in the title, add new `slm_policy` field as a filter to the get snapshots API.

original-brownbear added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.16.0 labels Sep 6, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Sep 6, 2021

original-brownbear mentioned this pull request Sep 6, 2021

Improve Snapshot Repository Scalability #74350

Closed

16 tasks

original-brownbear commented Sep 6, 2021

View reviewed changes

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc Show resolved Hide resolved

arteam self-requested a review September 7, 2021 07:06

arteam reviewed Sep 7, 2021

View reviewed changes

henningandersen reviewed Sep 7, 2021

View reviewed changes

original-brownbear added 4 commits September 7, 2021 12:32

Merge remote-tracking branch 'elastic/master' into match-by-slm-polic…

5cd1e5a

…y-id

Merge remote-tracking branch 'elastic/master' into match-by-slm-polic…

406822c

…y-id

cosmetics

57be270

tests

93d488f

original-brownbear requested review from arteam and henningandersen September 7, 2021 11:22

arteam approved these changes Sep 7, 2021

View reviewed changes

henningandersen reviewed Sep 7, 2021

View reviewed changes

comments

01646f4

original-brownbear requested a review from henningandersen September 7, 2021 12:37

original-brownbear added 2 commits September 7, 2021 14:57

Merge remote-tracking branch 'elastic/master' into match-by-slm-polic…

3bf417b

…y-id

_none

d25fcc7

henningandersen approved these changes Sep 7, 2021

View reviewed changes

original-brownbear added 2 commits September 7, 2021 16:18

Merge remote-tracking branch 'elastic/master' into match-by-slm-polic…

6c70726

…y-id

move constant and add test

da2f4d7

original-brownbear merged commit 7508720 into elastic:master Sep 7, 2021

original-brownbear deleted the match-by-slm-policy-id branch September 7, 2021 16:24

original-brownbear added the backport pending label Sep 7, 2021

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

original-brownbear mentioned this pull request Oct 16, 2021

Add Filtering by SLM Policy to Get Snapshots API (#77321) #79308

Merged

original-brownbear removed the backport pending label Oct 16, 2021

original-brownbear added a commit that referenced this pull request Oct 17, 2021

Add Filtering by SLM Policy to Get Snapshots API (#77321) (#79308)

08a8365

It's in the title, add new `slm_policy_filter` param as a filter to the get snapshots API.

original-brownbear restored the match-by-slm-policy-id branch April 18, 2023 20:39

Conversation

original-brownbear commented Sep 6, 2021

Uh oh!

elasticmachine commented Sep 6, 2021

Uh oh!

Uh oh!

arteam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 7, 2021

Uh oh!

arteam left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 7, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment