Fix SearchableSnapshotIndexMetadataUpgrader (#72004)

The upgrader that applies the shard limit group had a flaw in that it might not upgrade indices that were mounted during rolling upgrades. Closes #71973
elastic · Apr 27, 2021 · d39f797 · d39f797
1 parent fa92fc4
commit d39f797
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 21 deletions.
diff --git a/...sticsearch/xpack/searchablesnapshots/upgrade/SearchableSnapshotIndexMetadataUpgrader.java b/...sticsearch/xpack/searchablesnapshots/upgrade/SearchableSnapshotIndexMetadataUpgrader.java
@@ -92,7 +92,7 @@ public void onFailure(String source, Exception e) {
 
     static boolean needsUpgrade(ClusterState state) {
         return StreamSupport.stream(state.metadata().spliterator(), false)
-            .filter(imd -> imd.getCreationVersion().onOrAfter(Version.V_7_12_0) && imd.getCreationVersion().before(Version.V_7_13_0))
+            .filter(imd -> imd.getCreationVersion().onOrAfter(Version.V_7_12_0) && imd.getCreationVersion().before(Version.V_8_0_0))
             .map(IndexMetadata::getSettings)
             .filter(SearchableSnapshotsConstants::isPartialSearchableSnapshotIndex)
             .anyMatch(SearchableSnapshotIndexMetadataUpgrader::notFrozenShardLimitGroup);
@@ -104,7 +104,7 @@ static ClusterState upgradeIndices(ClusterState currentState) {
         }
         Metadata.Builder builder = Metadata.builder(currentState.metadata());
         StreamSupport.stream(currentState.metadata().spliterator(), false)
-            .filter(imd -> imd.getCreationVersion().onOrAfter(Version.V_7_12_0) && imd.getCreationVersion().before(Version.V_7_13_0))
+            .filter(imd -> imd.getCreationVersion().onOrAfter(Version.V_7_12_0) && imd.getCreationVersion().before(Version.V_8_0_0))
             .filter(
                 imd -> SearchableSnapshotsConstants.isPartialSearchableSnapshotIndex(imd.getSettings())
                     && notFrozenShardLimitGroup(imd.getSettings())

diff --git a/...earch/xpack/searchablesnapshots/upgrade/SearchableSnapshotIndexMetadataUpgraderTests.java b/...earch/xpack/searchablesnapshots/upgrade/SearchableSnapshotIndexMetadataUpgraderTests.java
@@ -30,23 +30,38 @@
 public class SearchableSnapshotIndexMetadataUpgraderTests extends ESTestCase {
 
     public void testNoUpgradeNeeded() {
-        Metadata.Builder metadataBuilder = randomMetadata(normal(), full(), partial_7_13plus(), shardLimitGroupFrozen(partial_7_12()));
+        Metadata.Builder metadataBuilder = randomMetadata(
+            normal(),
+            full(),
+            partial_8plusNoShardLimit(),
+            shardLimitGroupFrozen(partial_7_13plus()),
+            shardLimitGroupFrozen(partialNeedsUpgrade())
+        );
         assertThat(needsUpgrade(metadataBuilder), is(false));
     }
 
     public void testNeedsUpgrade() {
-        Metadata.Builder metadataBuilder = addIndex(
-            partial_7_12(),
-            randomMetadata(normal(), full(), partial_7_13plus(), partial_7_12(), shardLimitGroupFrozen(partial_7_12()))
+        assertThat(
+            needsUpgrade(
+                addIndex(
+                    partialNeedsUpgrade(),
+                    randomMetadata(
+                        normal(),
+                        full(),
+                        partial_7_13plus(),
+                        partialNeedsUpgrade(),
+                        shardLimitGroupFrozen(partialNeedsUpgrade())
+                    )
+                )
+            ),
+            is(true)
         );
-        assertThat(needsUpgrade(metadataBuilder), is(true));
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/71973")
     public void testUpgradeIndices() {
         Metadata.Builder metadataBuilder = addIndex(
-            partial_7_12(),
-            randomMetadata(normal(), full(), partial_7_13plus(), partial_7_12(), shardLimitGroupFrozen(partial_7_12()))
+            partialNeedsUpgrade(),
+            randomMetadata(normal(), full(), partial_7_13plus(), partialNeedsUpgrade(), shardLimitGroupFrozen(partialNeedsUpgrade()))
         );
 
         ClusterState originalState = clusterState(metadataBuilder);
@@ -75,10 +90,18 @@ public void testUpgradeIndices() {
                 return true;
             }
         }));
+
+        assertThat(SearchableSnapshotIndexMetadataUpgrader.needsUpgrade(upgradedState), is(false));
     }
 
     public void testNoopUpgrade() {
-        Metadata.Builder metadataBuilder = randomMetadata(normal(), full(), partial_7_13plus(), shardLimitGroupFrozen(partial_7_12()));
+        Metadata.Builder metadataBuilder = randomMetadata(
+            normal(),
+            full(),
+            partial_7_13plus(),
+            shardLimitGroupFrozen(partialNeedsUpgrade()),
+            partial_8plusNoShardLimit()
+        );
         ClusterState originalState = clusterState(metadataBuilder);
         ClusterState upgradedState = SearchableSnapshotIndexMetadataUpgrader.upgradeIndices(originalState);
         assertThat(upgradedState, sameInstance(originalState));
@@ -88,20 +111,31 @@ private Settings normal() {
         return settings(VersionUtils.randomVersion(random())).build();
     }
 
-    private Settings partial_7_12() {
-        return searchableSnapshotSettings(VersionUtils.randomVersionBetween(random(), Version.V_7_12_0, Version.V_7_12_1), true);
+    /**
+     * Simulate an index mounted with no shard limit group. Notice that due to not applying the group during rolling upgrades, we can see
+     * other than 7.12 versions here, but not 8.0 (since a rolling upgrade to 8.0 requires an upgrade to 7.latest first).
+     */
+    private Settings partialNeedsUpgrade() {
+        return searchableSnapshotSettings(
+            VersionUtils.randomVersionBetween(random(), Version.V_7_12_0, VersionUtils.getPreviousVersion(Version.V_8_0_0)),
+            true
+        );
     }
 
+    /**
+     * Simulate a 7.13plus mounted index with shard limit.
+     */
     private Settings partial_7_13plus() {
-        Settings settings = searchableSnapshotSettings(
-            VersionUtils.randomVersionBetween(random(), Version.V_7_13_0, Version.CURRENT),
-            true
+        return shardLimitGroupFrozen(
+            searchableSnapshotSettings(VersionUtils.randomVersionBetween(random(), Version.V_7_13_0, Version.CURRENT), true)
         );
-        if (randomBoolean()) {
-            return shardLimitGroupFrozen(settings);
-        } else {
-            return settings;
-        }
+    }
+
+    /**
+     * This is an illegal state, but we simulate it to capture that we do the version check
+     */
+    private Settings partial_8plusNoShardLimit() {
+        return searchableSnapshotSettings(VersionUtils.randomVersionBetween(random(), Version.V_8_0_0, Version.CURRENT), true);
     }
 
     private Settings full() {