Automatically update replica count for az compatibility #1394

mikaylathompson · 2025-03-27T19:53:20Z

Description

This updates how the metadata migration tool sets replica counts in cases where the cluster in question (usually, but not necessarily in the AWS managed service) has enforced balancing across awareness attributes. Usually, awareness attributes means availability zones, though it could be other factors. See this page for more information on how it works in opensearch: https://opensearch.org/docs/latest/tuning-your-cluster/#advanced-step-6-configure-shard-allocation-awareness-or-forced-awareness

In this PR, we take an approach to updating replica counts where we begin with the value from the metadata command arguments. If we see an incompatible replica count error, we increment the presumed dimensionality used by the transformer (which sets the minimum replica count for each index to dimensionality - 1) and try migrating the indices again. We repeat this up to 4 times, if necessary, but exit as soon as we no longer see an incompatible replica count error.

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-2185

Testing

Check List

New functionality includes testing
Public documentation issue/PR created, if applicable.

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]>

mikaylathompson · 2025-03-28T15:31:02Z

MetadataMigration/src/main/java/org/opensearch/migrations/commands/Migrate.java

-                .exitCode(INVALID_PARAMETER_CODE)
-                .errorMessage("Invalid parameter: " + pe.getMessage())
-                .build();
+                    .exitCode(INVALID_PARAMETER_CODE)


Ugh, this is my IDE being out of sync with the usual settings. I'll go through and clean up the indents.

I had a PR once upon a time for an intellij formatter [1], there is a VS code in the project too. Maybe that could help. 🤷

Add IntelliJ formatter #846

AndreKurait · 2025-03-28T15:42:19Z

MetadataMigration/src/main/java/org/opensearch/migrations/commands/MigratorEvaluatorBase.java

+            // Check whether any indices failed with an incompatibleReplicaCount
+            boolean incompatibleReplicaCountSeen = indexResults.getIndexes().stream().anyMatch(result -> result.wasFatal() && result.getFailureType().equals(CreationResult.CreationFailureType.INCOMPATIBLE_REPLICA_COUNT_FAILURE));
+            if (incompatibleReplicaCountSeen) {
+                if (presumedClusterDimensionality >= arguments.minNumberOfReplicas + MAX_REPLICA_ADJUSTMENT_LOOPS) {


Given cluster.routing.allocation.awareness.force.zone.values can be arbritraily high, can we parse out the response and skip straight to the next-higher valid value?

Yes, we definitely could. I went with the approach that we defined in the ticket refinement, but this would also be possible.

peternied

Thanks for revisiting this, couple of thought

peternied · 2025-03-28T15:33:36Z

RFS/src/main/java/org/opensearch/migrations/bulkload/worker/IndexRunner.java

-                List<CreationResult> creationResults;
-                if (skipCreation.test(index.getName())) {
-                    log.atInfo()
+        for (SnapshotRepo.Index index : repoDataProvider.getIndicesInSnapshot(snapshotName)) {


This looks like it shifted from a stream -> for loop, is this needed?

peternied · 2025-03-28T15:39:04Z

MetadataMigration/src/main/java/org/opensearch/migrations/commands/Migrate.java

Whitespace only, lets revert?

peternied · 2025-03-28T15:45:03Z

MetadataMigration/src/main/java/org/opensearch/migrations/commands/MigratorEvaluatorBase.java

+                }
+                presumedClusterDimensionality++;
+                log.warn("Incompatible replica count seen for the cluster dimensionality. Retrying with an assumed cluster dimensionality of {}", presumedClusterDimensionality);
+                transformer = selectTransformer(clusters, presumedClusterDimensionality);


This is kind of spooky - we theoretically 'switch' which transformer is being used min-flight based on behavior. Can we replumb how this value get passed around to avoid this high level swap?

peternied · 2025-03-28T15:51:58Z

RFS/src/main/java/org/opensearch/migrations/metadata/CreationResult.java

        ALREADY_EXISTS(false, "already exists"),
        UNABLE_TO_TRANSFORM_FAILURE(true, "failed to transform to the target version"),
        TARGET_CLUSTER_FAILURE(true, "failed on target cluster"),
+        INCOMPATIBLE_REPLICA_COUNT_FAILURE(true, "failed due to incompatible replica count for awareness attribute count"),


Can we include the CLI value to add to resolve this for our users?

Revisiting after finishing the review, maybe this doesn't get sent to users?

It will only get sent to users if we can't find a working replica count after 4 increments. By the CLI value to add -- what do you mean? The right replica count to set?

The test case doesn't seem to catch this case, but also this seems highly unlikely since AWS AZ are going to be a number between 2-6.

peternied · 2025-03-28T15:53:31Z

...c/testFixtures/java/org/opensearch/migrations/bulkload/framework/SearchClusterContainer.java

        this.containerVersion = version;
    }

+    public SearchClusterContainer(final ContainerVersion version, Map<String, String> supplementaryEnvVariables) {


This is replacing the other constructor implementation causing code duplicate, lets instead have the previous version call the new constructor with an empty map.

peternied · 2025-03-28T16:01:48Z

RFS/src/main/java/org/opensearch/migrations/bulkload/common/InvalidResponse.java

+     for instance), but the type is different (either `validation_exception` or `illegal_argument_exception`). For this reason,
+     we're matching based on a regex against the message instead of also checking the type.
+    **/
+    public Optional<String> containsAwarenessAttributeException() {


Does the message need to be parsed and returned by this function, that would aid the user if we displayed it to them? If not, this function could be reduced to a return a boolean value.

I should revisit exactly what we're doing with it down the line, but the reason I kept the whole thing is because it looks like Validation Failed: 1: expected total copies needs to be a multiple of total awareness attributes [3]; and that number at the end shows the number of awareness attributes, which could be very useful for a user trying to troubleshoot/understand this behavior.

peternied · 2025-03-28T16:03:33Z

RFS/src/main/java/org/opensearch/migrations/bulkload/version_os_2_11/IndexCreator_OS_2_11.java

                result.failureType(CreationFailureType.ALREADY_EXISTS);
            }
        } catch (InvalidResponse invalidResponse) {
+            var potentialAwarenessAttributeException = invalidResponse.containsAwarenessAttributeException();


Naming is a little messy, not sure if this was caught in a bulk rename.

Suggested change

var potentialAwarenessAttributeException = invalidResponse.containsAwarenessAttributeException();

var hasInvalidReplicaCountForZoneSize = invalidResponse.containsAwarenessAttributeException();

peternied · 2025-03-28T16:06:08Z

MetadataMigration/src/test/java/org/opensearch/migrations/ReplicaCountWithAZsTest.java

+    private static Stream<Arguments> scenarios() {
+        return Stream.of(
+                Arguments.of(1),
+                Arguments.of(2),
+                Arguments.of(3)
+        );
+    }


I don't think there is a test coverage benefit to running this more than once, seems like one test that detect this scenario would be good enough?

I think the reason to do it is primarily that these match the scenarios customers may have and the behavior--at least for 1 AZ vs more-than-one is different. I'm open to removing the 2 AZ case.

1 AZ doesn't seem like an interesting case, because any number will match, no?

peternied · 2025-03-28T16:10:32Z

MetadataMigration/src/test/java/org/opensearch/migrations/ReplicaCountWithAZsTest.java

+        }
+    }
+
+    void verifyIndexesExistOnTargetCluster(List<String> indexNames) {


Doesn't look like the new INCOMPATIBLE_REPLICA_COUNT_FAILURE is encountered during this test, is that scenario possible?

Not for the number of zones + max number of loops we're testing here. I could set up a test with 5+ zones that would cause it to be emitted, but I didn't because 1/ that's going to be a very slow test, and 2/ while a possible situation for a self-managed service cluster, I don't think it's possible in AOS (and unlikely no matter what)

peternied · 2025-03-28T16:15:24Z

MetadataMigration/src/test/java/org/opensearch/migrations/ReplicaCountWithAZsTest.java

+public class ReplicaCountWithAZsTest extends BaseMigrationTest{
+
+    private static final SearchClusterContainer.ContainerVersion SOURCE_VERSION = SearchClusterContainer.ES_V7_10_2;
+    private static final SearchClusterContainer.ContainerVersion TARGET_VERSION = SearchClusterContainer.OS_LATEST;


Its strange that this is unused, maybe the zoned search cluster is a static class for only this test case

codecov · 2025-04-01T15:55:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (d6fd172) to head (8b535f2).
Report is 16 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff      @@
##   main   #1394   +/-   ##
============================
============================

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mikaylathompson added 2 commits March 27, 2025 13:49

Add test for replica count setting with azs

926658a

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

re-execute on replica count exception

bb88356

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

mikaylathompson had a problem deploying to migrations-cicd March 27, 2025 19:53 — with GitHub Actions Failure

mikaylathompson added 2 commits March 27, 2025 21:51

Improved implementation

638e447

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

spotless fixes

78b74af

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

mikaylathompson had a problem deploying to migrations-cicd March 28, 2025 03:53 — with GitHub Actions Failure

Improve test

c4640ca

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

mikaylathompson temporarily deployed to migrations-cicd March 28, 2025 04:04 — with GitHub Actions Inactive

mikaylathompson had a problem deploying to migrations-cicd March 28, 2025 04:04 — with GitHub Actions Failure

mikaylathompson marked this pull request as ready for review March 28, 2025 04:15

mikaylathompson requested review from AndreKurait, chelma, gregschohn, jugal-chauhan, lewijacn, peternied and sumobrian as code owners March 28, 2025 04:15

Fix test tag

a65a5f7

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

mikaylathompson had a problem deploying to migrations-cicd March 28, 2025 04:21 — with GitHub Actions Failure

mikaylathompson temporarily deployed to migrations-cicd March 28, 2025 04:21 — with GitHub Actions Inactive

attempt switch to isolated test to not be timed out

8b535f2

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

mikaylathompson had a problem deploying to migrations-cicd March 28, 2025 04:34 — with GitHub Actions Failure

mikaylathompson temporarily deployed to migrations-cicd March 28, 2025 04:34 — with GitHub Actions Inactive

mikaylathompson commented Mar 28, 2025

View reviewed changes

AndreKurait reviewed Mar 28, 2025

View reviewed changes

peternied reviewed Mar 28, 2025

View reviewed changes

mikaylathompson closed this Apr 1, 2025

	var potentialAwarenessAttributeException = invalidResponse.containsAwarenessAttributeException();
	var hasInvalidReplicaCountForZoneSize = invalidResponse.containsAwarenessAttributeException();

Automatically update replica count for az compatibility #1394

Automatically update replica count for az compatibility #1394

Uh oh!

Conversation

mikaylathompson commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternied Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

peternied 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

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!

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!

codecov bot commented Apr 1, 2025

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikaylathompson commented Mar 27, 2025 •

edited

Loading

peternied Mar 28, 2025 •

edited

Loading