SERVER-53052 Review some recent changes introduced in setFCV command

We forgot to support the transitions from LastContinuous <-> Latest

7 files changed, 60 insertions, 58 deletions

diff --git a/jstests/multiVersion/upgrade_downgrade_sharded_cluster.js b/jstests/multiVersion/upgrade_downgrade_sharded_cluster.js
index 7d273c8a138..3aaecf06fb1 100644
--- a/jstests/multiVersion/upgrade_downgrade_sharded_cluster.js
+++ b/jstests/multiVersion/upgrade_downgrade_sharded_cluster.js
@@ -1,18 +1,24 @@
 /**
- * This test checks several upgrade/downgrade routines related to sharding, namely:
- *  - The entries in the config server's config.collections that were marked as 'dropped: true' are
- *    deleted. (SERVER-52630: Remove once 5.0 becomes the LastLTS)
- *  - The entries in the config server's config.collections contain a 'distributionMode' field.
- *    (SERVER-51900: Remove once 5.0 becomes the LastLTS)
+ * The goal of this test is to verify that some metadata is properly updated when
+ *upgrading/downgrading a sharded cluster. More specifically:
+ *
+ *	1. We create a sharded cluster running and old binary version (lastLTSFCV or lastContinuousFCV)
+ *	2. We run some operations that involve the creation of some metadata
+ *	3. We upgrade the binaries of the sharded cluster to the latest version + set FCV to latestFCV
+ *	4. We verify that the metadata has been properly upgraded
+ *	5. We set FCV to old bin version + downgrade the binaries of the sharded cluster to that version
+ *	6. We verify that the metadata has been properly downgraded
  */
 (function() {
 "use strict";
 
 load('./jstests/multiVersion/libs/multi_cluster.js');  // for upgradeCluster()
 
-// Test 1: it checks two things after upgrading from 4.4:
+// Test 1: it checks two things after upgrading from versions prior 4.9:
 // - dropped collections are not present in csrs config.collections
 // - Entries on config.collections doesn't have the 'distributionMode' and the 'dropped' fields
+//
+// This test must be removed once 5.0 is defined as the lastLTS (SERVER-52630)
 function test1Setup() {
     let csrs_config_db = st.configRS.getPrimary().getDB('config');
     // Setup sharded collections
@@ -48,7 +54,9 @@ function test1ChecksAfterUpgrade() {
 }
 
 // Test 2: it checks that the 'allowMigrations' field in the
-// config.cache.collections entries is removed when downgrading to 4.4
+// config.cache.collections entries is removed when downgrading to prior 4.9
+//
+// This test must be removed once 5.0 is defined as the lastLTS (SERVER-52632)
 function test2Setup() {
     assert.commandWorked(st.s.getDB("sharded").getCollection("test2").insert({_id: 0}));
     assert.commandWorked(st.s.getDB("sharded").getCollection("test2").insert({_id: 1}));
@@ -97,40 +105,41 @@ function runChecksAfterDowngrade() {
     test2ChecksAfterDowngrade();
 }
 
-// Start a sharded cluster on version 4.4
-let old_version = '4.4';
-var st = new ShardingTest({
-    shards: 2,
-    mongos: 1,
-    other: {
-        mongosOptions: {binVersion: old_version},
-        configOptions: {binVersion: old_version},
-        shardOptions: {binVersion: old_version},
-
-        rsOptions: {binVersion: old_version},
-        rs: true,
-    }
-});
-st.configRS.awaitReplication();
+for (let oldVersion of [lastLTSFCV, lastContinuousFCV]) {
+    var st = new ShardingTest({
+        shards: 2,
+        mongos: 1,
+        other: {
+            mongosOptions: {binVersion: oldVersion},
+            configOptions: {binVersion: oldVersion},
+            shardOptions: {binVersion: oldVersion},
+
+            rsOptions: {binVersion: oldVersion},
+            rs: true,
+        }
+    });
 
-// Setup initial conditions on 4.4
-setupInitialStateOnOldVersion();
+    st.configRS.awaitReplication();
 
-// Upgrade the entire cluster to the latest version.
-jsTest.log('upgrading cluster');
-st.upgradeCluster('latest');
-assert.commandWorked(st.s.adminCommand({setFeatureCompatibilityVersion: latestFCV}));
+    // Setup initial conditions
+    setupInitialStateOnOldVersion();
 
-// Tests after upgrade
-runChecksAfterUpgrade();
+    // Upgrade the entire cluster to the latest version.
+    jsTest.log('upgrading cluster');
+    st.upgradeCluster(latestFCV);
+    assert.commandWorked(st.s.adminCommand({setFeatureCompatibilityVersion: latestFCV}));
 
-// Downgrade back to 4.4
-jsTest.log('downgrading cluster');
-assert.commandWorked(st.s.adminCommand({setFeatureCompatibilityVersion: old_version}));
-st.upgradeCluster(old_version);
+    // Tests after upgrade
+    runChecksAfterUpgrade();
 
-// Tests after downgrade to 4.4
-runChecksAfterDowngrade();
+    // Downgrade back to oldVersion
+    jsTest.log('downgrading cluster');
+    assert.commandWorked(st.s.adminCommand({setFeatureCompatibilityVersion: oldVersion}));
+    st.upgradeCluster(oldVersion);
 
-st.stop();
+    // Tests after downgrade to oldVersion
+    runChecksAfterDowngrade();
+
+    st.stop();
+}
 })();
diff --git a/src/mongo/db/commands/set_feature_compatibility_version_command.cpp b/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
index b89f52ff278..bca6a26e20b 100644
--- a/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
+++ b/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
@@ -243,16 +243,9 @@ public:
             }
 
             if (serverGlobalParams.clusterRole == ClusterRole::ConfigServer) {
-                if (actualVersion ==
-                        ServerGlobalParams::FeatureCompatibility::Version::kFullyDowngradedTo44 ||
-                    actualVersion ==
-                        ServerGlobalParams::FeatureCompatibility::Version::kUpgradingFrom44To47 ||
-                    actualVersion ==
-                        ServerGlobalParams::FeatureCompatibility::Version::kUpgradingFrom44To48 ||
-                    actualVersion ==
-                        ServerGlobalParams::FeatureCompatibility::Version::kUpgradingFrom44To49) {
+                if (requestedVersion >= FeatureCompatibilityParams::Version::kVersion49) {
                     // SERVER-52630: Remove once 5.0 becomes the LastLTS
-                    ShardingCatalogManager::get(opCtx)->removePre44LegacyMetadata(opCtx);
+                    ShardingCatalogManager::get(opCtx)->removePre49LegacyMetadata(opCtx);
                 }
 
                 // Upgrade shards before config finishes its upgrade.
@@ -334,9 +327,9 @@ public:
                 LOGV2(20502, "Downgrade: dropping config.rangeDeletions collection");
                 migrationutil::dropRangeDeletionsCollection(opCtx);
 
-                if (requestedVersion == FeatureCompatibilityParams::Version::kFullyDowngradedTo44) {
+                if (requestedVersion < FeatureCompatibilityParams::Version::kVersion49) {
                     // SERVER-52632: Remove once 5.0 becomes the LastLTS
-                    shardmetadatautil::downgradeShardConfigCollectionEntriesTo44(opCtx);
+                    shardmetadatautil::downgradeShardConfigCollectionEntriesToPre49(opCtx);
                 }
 
 
diff --git a/src/mongo/db/s/config/sharding_catalog_manager.cpp b/src/mongo/db/s/config/sharding_catalog_manager.cpp
index 0475c5f5d2b..4e9a619db01 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager.cpp
+++ b/src/mongo/db/s/config/sharding_catalog_manager.cpp
@@ -438,7 +438,7 @@ Status ShardingCatalogManager::setFeatureCompatibilityVersionOnShards(OperationC
     return Status::OK();
 }
 
-void ShardingCatalogManager::removePre44LegacyMetadata(OperationContext* opCtx) {
+void ShardingCatalogManager::removePre49LegacyMetadata(OperationContext* opCtx) {
     const auto catalogClient = Grid::get(opCtx)->catalogClient();
     DBDirectClient client(opCtx);
 
diff --git a/src/mongo/db/s/config/sharding_catalog_manager.h b/src/mongo/db/s/config/sharding_catalog_manager.h
index 2f07bd0376c..5b9030f3d84 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager.h
+++ b/src/mongo/db/s/config/sharding_catalog_manager.h
@@ -451,9 +451,9 @@ public:
      * as 'dropped: true'. As those are no longer needed, this method cleans up the leftover
      * metadata.
      *
-     * It shall be called on FCV 4.4 to 5.0 upgrade.
+     * It shall be called when upgrading to 4.9 or newer versions.
      */
-    void removePre44LegacyMetadata(OperationContext* opCtx);
+    void removePre49LegacyMetadata(OperationContext* opCtx);
 
     //
     // For Diagnostics
diff --git a/src/mongo/db/s/shard_metadata_util.cpp b/src/mongo/db/s/shard_metadata_util.cpp
index 21703a67340..dd2a0357293 100644
--- a/src/mongo/db/s/shard_metadata_util.cpp
+++ b/src/mongo/db/s/shard_metadata_util.cpp
@@ -484,7 +484,7 @@ Status deleteDatabasesEntry(OperationContext* opCtx, StringData dbName) {
     }
 }
 
-void downgradeShardConfigCollectionEntriesTo44(OperationContext* opCtx) {
+void downgradeShardConfigCollectionEntriesToPre49(OperationContext* opCtx) {
     // Clear the 'allowMigrations' field from config.cache.collections
     LOGV2(5189100, "Starting downgrade of config.cache.collections");
     write_ops::Update clearAllowMigrations(NamespaceString::kShardConfigCollectionsNamespace, [] {
diff --git a/src/mongo/db/s/shard_metadata_util.h b/src/mongo/db/s/shard_metadata_util.h
index 159c2bdd46b..76775b95b7e 100644
--- a/src/mongo/db/s/shard_metadata_util.h
+++ b/src/mongo/db/s/shard_metadata_util.h
@@ -223,10 +223,10 @@ Status deleteDatabasesEntry(OperationContext* opCtx, StringData dbName);
 
 
 /**
- * Downgrades the config.cache.collections entries to the 4.4 version. More specifically, it removes
- * the allowMigrations field from all the documents of config.cache.collections
+ * Downgrades the config.cache.collections entries to prior 4.9 version. More specifically, it
+ * removes the allowMigrations field from all the documents of config.cache.collections
  */
-void downgradeShardConfigCollectionEntriesTo44(OperationContext* opCtx);
+void downgradeShardConfigCollectionEntriesToPre49(OperationContext* opCtx);
 
 }  // namespace shardmetadatautil
 }  // namespace mongo
diff --git a/src/mongo/db/s/shard_metadata_util_test.cpp b/src/mongo/db/s/shard_metadata_util_test.cpp
index 561b7e556a0..f3512d077bf 100644
--- a/src/mongo/db/s/shard_metadata_util_test.cpp
+++ b/src/mongo/db/s/shard_metadata_util_test.cpp
@@ -346,10 +346,10 @@ TEST_F(ShardMetadataUtilTest, DowngradeShardConfigCollectionEntriesTo44) {
     // allowMigrations was explicitly defined to false by the previous statement
     checkShardCollectionAllowMigrationsFlag(/* expectedValue */ false);
 
-    downgradeShardConfigCollectionEntriesTo44(operationContext());
+    downgradeShardConfigCollectionEntriesToPre49(operationContext());
 
-    // The downgrade process to 4.4 removes the allowMigration flag from all collections. Thus, this
-    // flag will not be present which implies that its value is true
+    // The downgrade process to prior 4.9 removes the allowMigration flag from all collections.
+    // Thus, this flag will not be present which implies that its value is true
     checkShardCollectionAllowMigrationsFlag(/* expectedValue */ true);
 }