Add indexAllowlist to snapshot create command #1009

mikaylathompson · 2024-09-24T17:01:41Z

Description

Add an --index-allowlist flag to the CreateSnapshot command, which passes the list of fields to the snapshot creation api call.

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

Testing

Snapshot tests added.

Check List

New functionality includes testing
- All tests pass, including unit test, integration test and doctest
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mikayla Thompson <[email protected]>

codecov · 2024-09-24T17:02:48Z

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.46%. Comparing base (b574434) to head (d80dde0).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
...ch/migrations/bulkload/common/SnapshotCreator.java	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1009      +/-   ##
============================================
+ Coverage     80.22%   80.46%   +0.24%     
- Complexity     2721     2734      +13     
============================================
  Files           365      365              
  Lines         13595    13624      +29     
  Branches        939      942       +3     
============================================
+ Hits          10906    10962      +56     
+ Misses         2118     2084      -34     
- Partials        571      578       +7

Flag	Coverage Δ
gradle-test	`78.48% <78.57%> (+0.24%)`	⬆️
python-test	`90.11% <ø> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson · 2024-09-25T15:50:15Z

CreateSnapshot/src/test/java/org/opensearch/migrations/TestCreateSnapshot.java

+        final FullHttpRequest[] createSnapshotRequest = new FullHttpRequest[1];
+        final String[] createSnapshotRequestContent = {""};


These arrays were IntelliJ's suggestion to fix the issue that you can't use a non-final in a lambda. Open to a better suggestion if this is not the right way to resolve it.

Yeah - sorry. This is a java-ism. It makes me realize why the capture specifiers in C++ [=foo,&bar] really aren't that bad!

You can also do an AtomicReference/AtomicInteger, etc. I usually do the AtomicReference thing because it seems more clear to me. Our codebase isn't to the level of consistency where it matters, especially for test cases!

an AtomicReference/AtomicString would make more sense here

Okay, cool, thanks -- updated this to AtomicReferences

Signed-off-by: Mikayla Thompson <[email protected]>

AndreKurait · 2024-09-27T17:36:16Z

CreateSnapshot/src/main/java/org/opensearch/migrations/CreateSnapshot.java

@@ -59,6 +61,10 @@ public static class Args {
            "--s3-role-arn" }, required = false, description = "The role ARN the cluster will assume to write a snapshot to S3")
        public String s3RoleArn;

+        @Parameter(names = {
+            "--index-allowlist" }, required = false, description = "A comma separated list of indices to include in the snapshot. If not provided, all indices are included.")


Do we want to align this with document migration which excludes system indicies by default

I lean towards including them by default. I'm fine with not transferring them by default, but if a user ends up wanting to move some/all of them, it would be very annoying to discover that the snapshot didn't include them.

gregschohn · 2024-09-27T17:19:24Z

CreateSnapshot/src/main/java/org/opensearch/migrations/CreateSnapshot.java

+        @Parameter(names = {
+            "--index-allowlist" }, required = false, description = "A comma separated list of indices to include in the snapshot. If not provided, all indices are included.")


nit: It would be nice for this to be can you format these to put each annotation parameter its own line (like what's done for CaptureProxy & TrafficReplayer). Like this

@Parameter( required = false, names = {"--insecure" }, arity = 0, description = "Do not check the server's certificate") boolean allowInsecureConnections;

gregschohn · 2024-09-27T17:22:14Z

CreateSnapshot/src/test/java/org/opensearch/migrations/TestCreateSnapshot.java

+        final FullHttpRequest[] createSnapshotRequest = new FullHttpRequest[1];
+        final String[] createSnapshotRequestContent = {""};


Yeah - sorry. This is a java-ism. It makes me realize why the capture specifiers in C++ [=foo,&bar] really aren't that bad!

You can also do an AtomicReference/AtomicInteger, etc. I usually do the AtomicReference thing because it seems more clear to me. Our codebase isn't to the level of consistency where it matters, especially for test cases!

gregschohn · 2024-09-27T17:25:11Z

CreateSnapshot/src/test/java/org/opensearch/migrations/TestCreateSnapshot.java

+            assert registerRepoRequest.uri().equals("/_snapshot/migration_assistant_repo");
+            assert registerRepoRequest.method().toString().equals("PUT");
+            assert registerRepoRequestContent.equals("{\"type\":\"s3\",\"settings\":{\"bucket\":\"new-bucket\",\"region\":\"us-east-2\",\"base_path\":null}}");
+            assert createSnapshotRequest.uri().equals("/_snapshot/migration_assistant_repo/" + snapshotName);
+            assert createSnapshotRequest.method().toString().equals("PUT");
+            assert createSnapshotRequestContent.equals("{\"indices\":\"_all\",\"ignore_unavailable\":true,\"include_global_state\":true}");


these are java language assertions. If the unit tests were run w/out -ea, these wouldn't be evaluated. If one fails, we probably won't always get the same support that we get in all of our tools to give more hints about what the issues were.

Import Assertions and use Assertions.assertEquals(EXPECTED, ACTUAL, OPTIONAL_AND_RARELY_USED_MESSAGE) and the other many helpers w/in the Assertions class. Just find "Assertions." for many examples.

This is so much better!

gregschohn · 2024-09-27T17:28:57Z

CreateSnapshot/src/test/java/org/opensearch/migrations/TestCreateSnapshot.java

+import java.util.List;
+import java.util.Map;
+
+import org.apache.commons.lang3.tuple.Pair;


this is a bit risky since you're (I think) depending on a transitive dependency.
For a test case, it's reasonable to take on extra dependencies that you normally wouldn't want to take on.
That said, the lack of a Pair<> type is well-known for Java devs. Normally, we just do Map.Entry<> for the type and instantiate new AbstractMap.SimpleEntry<>(V1, V2). For test code, I'm fine w/ another dependency on this or vavr.io or whatever, it will probably eventually be refactored away to keep dependency graphs smaller.

gregschohn · 2024-09-27T17:41:19Z

CreateSnapshot/src/test/java/org/opensearch/migrations/TestCreateSnapshot.java

+
+            assert registerRepoRequest.uri().equals("/_snapshot/migration_assistant_repo");
+            assert registerRepoRequest.method().toString().equals("PUT");
+            assert registerRepoRequestContent.equals("{\"type\":\"s3\",\"settings\":{\"bucket\":\"new-bucket\",\"region\":\"us-east-2\",\"base_path\":null}}");


is base_path = null right?
You might want to normalize the json in the test and from the java class so that if one pretty-prints, you aren't in trouble with a difference.
hamcrest lets you do fancy deep mappings too - but it's another thing to learn and therefore has been a barrier to entry for me (others use it a lot)

I like the indenting job that you did. Thanks

Signed-off-by: Mikayla Thompson <[email protected]>

gregschohn · 2024-09-27T22:46:34Z

CreateSnapshot/src/test/java/org/opensearch/migrations/TestCreateSnapshot.java

+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;


nit: I hate all static imports (others disagree though).

Add indexAllowlist to snapshot create command

0e3d9ab

Signed-off-by: Mikayla Thompson <[email protected]>

Spotless fix

4eb8674

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson force-pushed the snapshot-filtering branch from 5ac0352 to 4eb8674 Compare September 24, 2024 17:18

mikaylathompson added 2 commits September 25, 2024 09:44

add tests

165ae89

Signed-off-by: Mikayla Thompson <[email protected]>

Clean up dependencies

d3e180c

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson commented Sep 25, 2024

View reviewed changes

Merge branch 'main' into snapshot-filtering

22f19ce

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson marked this pull request as ready for review September 25, 2024 16:02

mikaylathompson requested review from AndreKurait, chelma, gregschohn, lewijacn, okhasawn, peternied and sumobrian as code owners September 25, 2024 16:02

AndreKurait reviewed Sep 27, 2024

View reviewed changes

gregschohn reviewed Sep 27, 2024

View reviewed changes

Address review comments

0ab7895

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson requested a review from gregschohn September 27, 2024 20:00

Add base_path to the example input

d80dde0

Signed-off-by: Mikayla Thompson <[email protected]>

gregschohn approved these changes Sep 27, 2024

View reviewed changes

mikaylathompson merged commit 7ba296e into opensearch-project:main Sep 30, 2024
13 of 14 checks passed

mikaylathompson deleted the snapshot-filtering branch September 30, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add indexAllowlist to snapshot create command #1009

Add indexAllowlist to snapshot create command #1009

mikaylathompson commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

mikaylathompson Sep 25, 2024

gregschohn Sep 27, 2024

AndreKurait Sep 27, 2024

mikaylathompson Sep 27, 2024

AndreKurait Sep 27, 2024 •

edited

Loading

mikaylathompson Sep 27, 2024

gregschohn Sep 27, 2024

gregschohn Sep 27, 2024

gregschohn Sep 27, 2024

mikaylathompson Sep 27, 2024

gregschohn Sep 27, 2024

gregschohn Sep 27, 2024

gregschohn Sep 29, 2024

gregschohn Sep 27, 2024

		final FullHttpRequest[] createSnapshotRequest = new FullHttpRequest[1];
		final String[] createSnapshotRequestContent = {""};

		@Parameter(names = {
		"--index-allowlist" }, required = false, description = "A comma separated list of indices to include in the snapshot. If not provided, all indices are included.")

		import static org.hamcrest.CoreMatchers.containsString;
		import static org.hamcrest.MatcherAssert.assertThat;

Add indexAllowlist to snapshot create command #1009

Add indexAllowlist to snapshot create command #1009

Conversation

mikaylathompson commented Sep 24, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreKurait Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaylathompson commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

AndreKurait Sep 27, 2024 •

edited

Loading