SERVER-53643 Wait for FCV to be majority committed before reporting it.

5 files changed, 81 insertions, 15 deletions

diff --git a/jstests/multiVersion/genericSetFCVUsage/major_version_upgrade.js b/jstests/multiVersion/genericSetFCVUsage/major_version_upgrade.js
index 89893130172..b5edaab8eef 100644
--- a/jstests/multiVersion/genericSetFCVUsage/major_version_upgrade.js
+++ b/jstests/multiVersion/genericSetFCVUsage/major_version_upgrade.js
@@ -295,6 +295,14 @@ for (let i = 0; i < versions.length; i++) {
         assert.commandWorked(primaryAdminDB.runCommand(
             {setFeatureCompatibilityVersion: version.featureCompatibilityVersion}));
         rst.awaitReplication();
+        // Make sure we reach the new featureCompatibilityVersion in the committed snapshot on
+        // on all nodes before continuing to upgrade.
+        // checkFCV does not work for version 3.4 (and below)
+        if (version.featureCompatibilityVersion != '3.4') {
+            for (let n of rst.nodes) {
+                checkFCV(n.getDB("admin"), version.featureCompatibilityVersion);
+            }
+        }
     }
 }
 
diff --git a/jstests/multiVersion/upgrade_to_44_with_34_oplog_entries.js b/jstests/multiVersion/upgrade_to_44_with_34_oplog_entries.js
index 252edb99207..d126f88aeaf 100644
--- a/jstests/multiVersion/upgrade_to_44_with_34_oplog_entries.js
+++ b/jstests/multiVersion/upgrade_to_44_with_34_oplog_entries.js
@@ -19,21 +19,32 @@ for (let i = 0; i < 10000; i++) {
 }
 bulk.execute();
 
+function waitForFCV(rst, fcv) {
+    rst.awaitReplication();
+    for (let n of rst.nodes) {
+        checkFCV(n.getDB("admin"), fcv);
+    }
+}
+
 rst.upgradeSet({binVersion: "3.6"});
 assert.commandWorked(
     rst.getPrimary().getDB(dbName).adminCommand({setFeatureCompatibilityVersion: "3.6"}));
+waitForFCV(rst, "3.6");
 
 rst.upgradeSet({binVersion: "4.0"});
 assert.commandWorked(
     rst.getPrimary().getDB(dbName).adminCommand({setFeatureCompatibilityVersion: "4.0"}));
+waitForFCV(rst, "4.0");
 
 rst.upgradeSet({binVersion: "4.2"});
 assert.commandWorked(
     rst.getPrimary().getDB(dbName).adminCommand({setFeatureCompatibilityVersion: "4.2"}));
+waitForFCV(rst, "4.2");
 
 rst.upgradeSet({binVersion: "latest"});
 assert.commandWorked(
     rst.getPrimary().getDB(dbName).adminCommand({setFeatureCompatibilityVersion: latestFCV}));
+waitForFCV(rst, latestFCV);
 
 rst.stopSet();
-})();
\ No newline at end of file
+})();
diff --git a/jstests/replsets/rollback_set_fcv.js b/jstests/replsets/rollback_set_fcv.js
index 7a787532e7b..a00c30f3bc0 100644
--- a/jstests/replsets/rollback_set_fcv.js
+++ b/jstests/replsets/rollback_set_fcv.js
@@ -21,6 +21,12 @@ function setFCV(fcv) {
                                  ErrorCodes.InterruptedDueToReplStateChange);
 }
 
+// Using getParameter results in waiting for the current FCV to be majority committed.  In this
+// test, it never will, so we need to get the FCV directly.
+function getFCVFromDocument(conn) {
+    return conn.getDB("admin").system.version.find().readConcern("local").toArray()[0];
+}
+
 // fromFCV refers to the FCV we will test rolling back from.
 // toFCV refers to the FCV we will test rolling back to.
 function rollbackFCVFromDowngradingOrUpgrading(fromFCV, toFCV) {
@@ -40,9 +46,8 @@ function rollbackFCVFromDowngradingOrUpgrading(fromFCV, toFCV) {
     // Wait for the FCV update to be reflected on the primary. This should eventually be rolled
     // back.
     assert.soon(function() {
-        let res = assert.commandWorked(
-            primary.adminCommand({getParameter: 1, featureCompatibilityVersion: 1}));
-        return res.featureCompatibilityVersion.hasOwnProperty('targetVersion');
+        let featureCompatibilityVersion = getFCVFromDocument(primary);
+        return featureCompatibilityVersion.hasOwnProperty('targetVersion');
     }, "Failed waiting for the server to set the targetVersion: " + fromFCV);
     rollbackTest.transitionToSyncSourceOperationsBeforeRollback();
     // Secondaries should never have received the FCV update.
@@ -85,10 +90,9 @@ function rollbackFCVFromDowngradedOrUpgraded(fromFCV, toFCV, failPoint) {
     // should never make it to the secondary.
     hangBeforeUnsettingTargetVersion.off();
     assert.soon(function() {
-        let res = assert.commandWorked(
-            primary.adminCommand({getParameter: 1, featureCompatibilityVersion: 1}));
-        return !res.featureCompatibilityVersion.hasOwnProperty('targetVersion') &&
-            res.featureCompatibilityVersion.version === fromFCV;
+        let featureCompatibilityVersion = getFCVFromDocument(primary);
+        return !featureCompatibilityVersion.hasOwnProperty('targetVersion') &&
+            featureCompatibilityVersion.version === fromFCV;
     }, "Failed waiting for server to unset the targetVersion or to set the FCV to " + fromFCV);
     rollbackTest.transitionToSyncSourceOperationsBeforeRollback();
     // The secondary should never have received the update to unset the targetVersion.
@@ -125,4 +129,4 @@ rollbackFCVFromDowngradedOrUpgraded(lastStableFCV, latestFCV, "hangWhileDowngrad
 rollbackFCVFromDowngradedOrUpgraded(latestFCV, lastStableFCV, "hangWhileUpgrading");
 
 rollbackTest.stop();
-}());
\ No newline at end of file
+}());
diff --git a/src/mongo/db/commands/feature_compatibility_version.cpp b/src/mongo/db/commands/feature_compatibility_version.cpp
index 66e645f4a69..b922e6f57e3 100644
--- a/src/mongo/db/commands/feature_compatibility_version.cpp
+++ b/src/mongo/db/commands/feature_compatibility_version.cpp
@@ -63,6 +63,10 @@ namespace mongo {
 using repl::UnreplicatedWritesBlock;
 
 Lock::ResourceMutex FeatureCompatibilityVersion::fcvLock("featureCompatibilityVersionLock");
+// lastFCVUpdateTimestamp contains the latest oplog entry timestamp which updated the FCV.
+// It is reset on rollback.
+Timestamp lastFCVUpdateTimestamp;
+SimpleMutex lastFCVUpdateTimestampMutex;
 
 MONGO_FAIL_POINT_DEFINE(hangBeforeAbortingRunningTransactionsOnFCVDowngrade);
 
@@ -168,7 +172,7 @@ void FeatureCompatibilityVersion::onInsertOrUpdate(OperationContext* opCtx, cons
     }
 
     opCtx->recoveryUnit()->onCommit(
-        [opCtx, newVersion](boost::optional<Timestamp>) { _setVersion(opCtx, newVersion); });
+        [opCtx, newVersion](boost::optional<Timestamp> ts) { _setVersion(opCtx, newVersion, ts); });
 }
 
 void FeatureCompatibilityVersion::updateMinWireVersion() {
@@ -192,7 +196,17 @@ void FeatureCompatibilityVersion::updateMinWireVersion() {
 }
 
 void FeatureCompatibilityVersion::_setVersion(
-    OperationContext* opCtx, ServerGlobalParams::FeatureCompatibility::Version newVersion) {
+    OperationContext* opCtx,
+    ServerGlobalParams::FeatureCompatibility::Version newVersion,
+    boost::optional<Timestamp> commitTs) {
+    // We set the last FCV update timestamp before setting the new FCV, to make sure we never
+    // read an FCV that is not stable.  We might still read a stale one.
+    {
+        stdx::lock_guard lk(lastFCVUpdateTimestampMutex);
+        if (commitTs && *commitTs > lastFCVUpdateTimestamp) {
+            lastFCVUpdateTimestamp = *commitTs;
+        }
+    }
     serverGlobalParams.featureCompatibility.setVersion(newVersion);
     updateMinWireVersion();
 
@@ -249,7 +263,10 @@ void FeatureCompatibilityVersion::onReplicationRollback(OperationContext* opCtx)
                   "Setting featureCompatibilityVersion as part of rollback",
                   "newVersion"_attr = FeatureCompatibilityVersionParser::toString(diskFcv),
                   "oldVersion"_attr = FeatureCompatibilityVersionParser::toString(memoryFcv));
-            _setVersion(opCtx, diskFcv);
+            _setVersion(opCtx, diskFcv, boost::none);
+            // The rollback FCV is already in the stable snapshot.
+            stdx::lock_guard lk(lastFCVUpdateTimestampMutex);
+            lastFCVUpdateTimestamp = Timestamp();
         }
     }
 }
@@ -321,7 +338,7 @@ void FeatureCompatibilityVersionParameter::append(OperationContext* opCtx,
             featureCompatibilityVersionBuilder.append(
                 FeatureCompatibilityVersionParser::kVersionField,
                 FeatureCompatibilityVersionParser::kVersion44);
-            return;
+            break;
         case ServerGlobalParams::FeatureCompatibility::Version::kUpgradingTo44:
             featureCompatibilityVersionBuilder.append(
                 FeatureCompatibilityVersionParser::kVersionField,
@@ -342,11 +359,35 @@ void FeatureCompatibilityVersionParameter::append(OperationContext* opCtx,
             featureCompatibilityVersionBuilder.append(
                 FeatureCompatibilityVersionParser::kVersionField,
                 FeatureCompatibilityVersionParser::kVersion42);
-            return;
+            break;
         case ServerGlobalParams::FeatureCompatibility::Version::kUnsetDefault42Behavior:
             // getVersion() does not return this value.
             MONGO_UNREACHABLE;
     }
+    // If the FCV has been recently set to the fully upgraded FCV but is not part of the majority
+    // snapshot, then if we do a binary upgrade, we may see the old FCV at startup.
+    // It is not safe to do oplog application on the new binary at that point.  So we make sure
+    // that when we report the FCV, it is in the majority snapshot.
+    // (The same consideration applies at downgrade, where if a recently-set fully downgraded FCV
+    // is not part of the majority snapshot, the downgraded binary will see the upgrade FCV and
+    // fail.)
+    const auto replCoordinator = repl::ReplicationCoordinator::get(opCtx);
+    const bool isReplSet = replCoordinator &&
+        replCoordinator->getReplicationMode() == repl::ReplicationCoordinator::modeReplSet;
+    auto neededMajorityTimestamp = [] {
+        stdx::lock_guard lk(lastFCVUpdateTimestampMutex);
+        return lastFCVUpdateTimestamp;
+    }();
+    if (isReplSet && !neededMajorityTimestamp.isNull()) {
+        auto status = replCoordinator->awaitTimestampCommitted(opCtx, neededMajorityTimestamp);
+        // If majority reads are not supported, we will take a full snapshot on clean shutdown
+        // and the new FCV will be included, so upgrade is possible.
+        if (status.code() != ErrorCodes::CommandNotSupported)
+            uassertStatusOK(
+                status.withContext("Most recent 'featureCompatibilityVersion' was not in the "
+                                   "majority snapshot on this node"));
+    }
+    return;
 }
 
 Status FeatureCompatibilityVersionParameter::setFromString(const std::string&) {
diff --git a/src/mongo/db/commands/feature_compatibility_version.h b/src/mongo/db/commands/feature_compatibility_version.h
index 84d46172bef..04e84587e5d 100644
--- a/src/mongo/db/commands/feature_compatibility_version.h
+++ b/src/mongo/db/commands/feature_compatibility_version.h
@@ -119,9 +119,11 @@ private:
     /**
      * Set the FCV to newVersion, making sure to close any outgoing connections with incompatible
      * servers and closing open transactions if necessary. Increments the server TopologyVersion.
+     * If the commitTimestamp is set, advances the lastFCVUpdateTimestamp to it.
      */
     static void _setVersion(OperationContext* opCtx,
-                            ServerGlobalParams::FeatureCompatibility::Version newVersion);
+                            ServerGlobalParams::FeatureCompatibility::Version newVersion,
+                            boost::optional<Timestamp> commitTimestamp);
 };
 
 /**