From 3750e4fefe36b2de91d451d70228bc130ed0cfe9 Mon Sep 17 00:00:00 2001 From: marcobizzarri-mdb Date: Wed, 6 May 2026 12:14:31 +0100 Subject: [PATCH] SERVER-119484 Add kEnableTargetFeatures and kCommitAddedFeatures no-op phases to FCV protocol (#52032) GitOrigin-RevId: acc64f79a875f00b50a6ed0ece4b12251147f278 --- .../upgrade_during_vectored_insert.js | 7 +- jstests/replsets/rollback_set_fcv.js | 4 +- .../feature_compatibility_version.cpp | 4 +- .../feature_compatibility_version_test.cpp | 75 ++++++++++++++++--- ..._feature_compatibility_version_command.cpp | 40 +++++++++- ...feature_compatibility_version_document.idl | 6 ++ 6 files changed, 118 insertions(+), 18 deletions(-) diff --git a/jstests/multiVersion/genericBinVersion/upgrade_during_vectored_insert.js b/jstests/multiVersion/genericBinVersion/upgrade_during_vectored_insert.js index 047ed53bf83..40738491bf3 100644 --- a/jstests/multiVersion/genericBinVersion/upgrade_during_vectored_insert.js +++ b/jstests/multiVersion/genericBinVersion/upgrade_during_vectored_insert.js @@ -25,10 +25,9 @@ assert.commandWorked(testDB.adminCommand({setFeatureCompatibilityVersion: lastLT // Insert a document to cause an error in batch insertion assert.commandWorked(testColl.insert([{"_id": "5"}])); -// Switch to upgrade FCV first. We have to stop this in the middle so we can get the collmods in -// _upgradeServerMetadata to finish before allowing FCV to actually change. This is a very -// unlikely race! -let upgradeFp = configureFailPoint(primary, "hangWhileUpgrading"); +// Switch to upgrade FCV first. We have to stop this in the middle so we can get all upgrade +// metadata changes to finish before allowing FCV to actually change. This is a very unlikely race! +let upgradeFp = configureFailPoint(primary, "hangBeforeFinalizingFCV"); jsTestLog("Switching to upgrade FCV"); let upgradeFCVThread = new Thread(function (host) { let conn = new Mongo(host); diff --git a/jstests/replsets/rollback_set_fcv.js b/jstests/replsets/rollback_set_fcv.js index f81fdcc00b1..e38a9656881 100644 --- a/jstests/replsets/rollback_set_fcv.js +++ b/jstests/replsets/rollback_set_fcv.js @@ -417,10 +417,10 @@ rollbackFCVFromDowngradingOrUpgrading(latestFCV, lastLTSFCV); // Tests the case where we roll back the FCV state from fully downgraded to downgrading (while in // isCleaningServerMetadata state). -rollbackFCVFromDowngradedOrUpgraded(lastLTSFCV, latestFCV, "hangBeforeTransitioningToDowngraded"); +rollbackFCVFromDowngradedOrUpgraded(lastLTSFCV, latestFCV, "hangBeforeFinalizingFCV"); // Tests the case where we roll back the FCV state from fully upgraded to upgrading. -rollbackFCVFromDowngradedOrUpgraded(latestFCV, lastLTSFCV, "hangWhileUpgrading"); +rollbackFCVFromDowngradedOrUpgraded(latestFCV, lastLTSFCV, "hangBeforeFinalizingFCV"); // Tests the case where we roll back the FCV state from upgrading to downgrading. rollbackFCV_FromUpgradingToDowngrading_or_FromDowngradingToUpgrading(latestFCV, lastLTSFCV); diff --git a/src/mongo/db/commands/feature_compatibility_version.cpp b/src/mongo/db/commands/feature_compatibility_version.cpp index ebfc2068c8f..dfb9ab687a0 100644 --- a/src/mongo/db/commands/feature_compatibility_version.cpp +++ b/src/mongo/db/commands/feature_compatibility_version.cpp @@ -283,7 +283,9 @@ public: return ResolvedFCVTransition{it->second.transitionalVersion, startPhase, - SetFCVPhaseEnum::kComplete, + gFeatureFlagSymmetricFCV.isEnabled() + ? SetFCVPhaseEnum::kCommitAddedFeatures + : SetFCVPhaseEnum::kComplete, changeTimestamp}; } diff --git a/src/mongo/db/commands/feature_compatibility_version_test.cpp b/src/mongo/db/commands/feature_compatibility_version_test.cpp index fa578869b1d..0c7039d355f 100644 --- a/src/mongo/db/commands/feature_compatibility_version_test.cpp +++ b/src/mongo/db/commands/feature_compatibility_version_test.cpp @@ -264,27 +264,78 @@ TEST_F(FeatureCompatibilityVersionTestFixture, ResolveReturnToOriginalFCVDuringC struct FCVTestParams { SetFCVPhaseEnum phase; - bool isCleaningServerMetadata; + boost::optional isCleaningServerMetadata; + bool symmetricFCVEnabled; + SetFCVPhaseEnum expectedStartPhase; + SetFCVPhaseEnum expectedEndPhase; }; class SetFeatureCompatibilityVersionParamTestFixture : public FeatureCompatibilityVersionTestFixture, public testing::WithParamInterface {}; -INSTANTIATE_TEST_SUITE_P(UpgradingFromDifferentStartingPhases, +// Without symmetric FCV, the phase stored in the FCV document is ignored and the transition always +// restarts from kStart with a freshly generated timestamp (legacy "restart from beginning" +// behavior). +INSTANTIATE_TEST_SUITE_P(UpgradingFromDifferentStartingPhasesWithoutSymmetricFCV, SetFeatureCompatibilityVersionParamTestFixture, testing::ValuesIn({ - FCVTestParams{SetFCVPhaseEnum::kStart, false}, - FCVTestParams{SetFCVPhaseEnum::kPrepare, false}, - FCVTestParams{SetFCVPhaseEnum::kComplete, true}, + FCVTestParams{SetFCVPhaseEnum::kStart, + false, + false, + SetFCVPhaseEnum::kStart, + SetFCVPhaseEnum::kComplete}, + FCVTestParams{SetFCVPhaseEnum::kPrepare, + false, + false, + SetFCVPhaseEnum::kStart, + SetFCVPhaseEnum::kComplete}, + FCVTestParams{SetFCVPhaseEnum::kComplete, + true, + false, + SetFCVPhaseEnum::kStart, + SetFCVPhaseEnum::kComplete}, + })); + +// With symmetric FCV, the phase stored in the FCV document is used as the start phase, so the +// transition resumes from where it was interrupted and reuses the existing change timestamp. +INSTANTIATE_TEST_SUITE_P(UpgradingFromDifferentStartingPhasesWithSymmetricFCV, + SetFeatureCompatibilityVersionParamTestFixture, + testing::ValuesIn({ + FCVTestParams{SetFCVPhaseEnum::kStart, + false, + true, + SetFCVPhaseEnum::kStart, + SetFCVPhaseEnum::kCommitAddedFeatures}, + FCVTestParams{SetFCVPhaseEnum::kPrepare, + false, + true, + SetFCVPhaseEnum::kPrepare, + SetFCVPhaseEnum::kCommitAddedFeatures}, + FCVTestParams{SetFCVPhaseEnum::kComplete, + true, + true, + SetFCVPhaseEnum::kComplete, + SetFCVPhaseEnum::kCommitAddedFeatures}, + FCVTestParams{SetFCVPhaseEnum::kEnableTargetFeatures, + boost::none, + true, + SetFCVPhaseEnum::kEnableTargetFeatures, + SetFCVPhaseEnum::kCommitAddedFeatures}, + FCVTestParams{SetFCVPhaseEnum::kCommitAddedFeatures, + boost::none, + true, + SetFCVPhaseEnum::kCommitAddedFeatures, + SetFCVPhaseEnum::kCommitAddedFeatures}, })); TEST_P(SetFeatureCompatibilityVersionParamTestFixture, ResolveResumeInterruptedUpgrade) { - RAIIServerParameterControllerForTest symmetricFCV{"featureFlagSymmetricFCV", true}; + const auto& params = GetParam(); + RAIIServerParameterControllerForTest symmetricFCV{"featureFlagSymmetricFCV", + params.symmetricFCVEnabled}; const Timestamp lastChangeTimestamp = VectorClockMutable::get(operationContext())->tickClusterTime(2).asTimestamp(); serverGlobalParams.clusterRole = {ClusterRole::ShardServer, ClusterRole::ConfigServer}; - const auto& params = GetParam(); doStartupFCVSequence(multiversion::GenericFCV::kLastLTS); FeatureCompatibilityVersion::updateFeatureCompatibilityVersionDocument( @@ -299,9 +350,13 @@ TEST_P(SetFeatureCompatibilityVersionParamTestFixture, ResolveResumeInterruptedU operationContext(), request, multiversion::GenericFCV::kUpgradingFromLastLTSToLatest); ASSERT_EQ(result.transitionalVersion, multiversion::GenericFCV::kUpgradingFromLastLTSToLatest); - ASSERT_EQ(result.startPhase, params.phase); - ASSERT_EQ(result.endPhase, SetFCVPhaseEnum::kComplete); - ASSERT_EQ(result.changeTimestamp, lastChangeTimestamp); + ASSERT_EQ(result.startPhase, params.expectedStartPhase); + ASSERT_EQ(result.endPhase, params.expectedEndPhase); + if (params.symmetricFCVEnabled) { + ASSERT_EQ(result.changeTimestamp, lastChangeTimestamp); + } else { + ASSERT_GT(result.changeTimestamp, lastChangeTimestamp); + } } 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 9b506b10e11..c52477a0dbe 100644 --- a/src/mongo/db/commands/set_feature_compatibility_version_command.cpp +++ b/src/mongo/db/commands/set_feature_compatibility_version_command.cpp @@ -132,6 +132,7 @@ MONGO_FAIL_POINT_DEFINE(failBeforeUpdatingFcvDoc); MONGO_FAIL_POINT_DEFINE(failTransitionDuringIsCleaningServerMetadata); MONGO_FAIL_POINT_DEFINE(hangBeforeTransitioningToDowngraded); MONGO_FAIL_POINT_DEFINE(hangTransitionBeforeIsCleaningServerMetadata); +MONGO_FAIL_POINT_DEFINE(hangBeforeFinalizingFCV); MONGO_FAIL_POINT_DEFINE(failAfterReachingTransitioningState); MONGO_FAIL_POINT_DEFINE(hangAtSetFCVStart); MONGO_FAIL_POINT_DEFINE(failAfterSendingShardsToDowngradingOrUpgrading); @@ -583,7 +584,6 @@ public: if (resolvedTransition.shouldRun(SetFCVPhaseEnum::kComplete)) { invariant(serverGlobalParams.featureCompatibility.acquireFCVSnapshot() .isUpgradingOrDowngrading()); - const bool isDowngradeTransition = requestedVersion < actualVersion; if (isDowngradeTransition || repl::feature_flags::gFeatureFlagUpgradingToDowngrading.isEnabled()) { @@ -619,7 +619,45 @@ public: } else { _runUpgrade(opCtx, request, changeTimestamp); } + } + // ---------- kEnableTargetFeatures phase ---------- + if (resolvedTransition.shouldRun(SetFCVPhaseEnum::kEnableTargetFeatures)) { + FeatureCompatibilityVersion::updateFeatureCompatibilityVersionDocument( + opCtx, + resolvedTransition.transitionalVersion, + SetFCVPhaseEnum::kEnableTargetFeatures, + changeTimestamp, + boost::none /* setIsCleaningServerMetadata */); + + if (role && role->has(ClusterRole::ConfigServer)) { + _sendEnterSetFCVPhaseRequestToShard( + opCtx, request, changeTimestamp, SetFCVPhaseEnum::kEnableTargetFeatures); + } + } + + // ---------- kCommitAddedFeatures phase ---------- + if (resolvedTransition.shouldRun(SetFCVPhaseEnum::kCommitAddedFeatures)) { + FeatureCompatibilityVersion::updateFeatureCompatibilityVersionDocument( + opCtx, + resolvedTransition.transitionalVersion, + SetFCVPhaseEnum::kCommitAddedFeatures, + changeTimestamp, + boost::none /* setIsCleaningServerMetadata */); + + if (role && role->has(ClusterRole::ConfigServer)) { + _sendEnterSetFCVPhaseRequestToShard( + opCtx, request, changeTimestamp, SetFCVPhaseEnum::kCommitAddedFeatures); + } + } + + // With symmetric FCV the last protocol phase is kCommitAddedFeatures; without it the last + // meaningful phase is kComplete (kEnableTargetFeatures/kCommitAddedFeatures are no-ops). + // Either way this finalization must execute after all feature-specific work is done. + if (gFeatureFlagSymmetricFCV.isEnabled() + ? resolvedTransition.shouldRun(SetFCVPhaseEnum::kCommitAddedFeatures) + : resolvedTransition.shouldRun(SetFCVPhaseEnum::kComplete)) { + hangBeforeFinalizingFCV.pauseWhileSet(opCtx); { // Complete transition by updating the local FCV document to the fully upgraded or // downgraded requestedVersion. diff --git a/src/mongo/db/feature_compatibility_version_document.idl b/src/mongo/db/feature_compatibility_version_document.idl index d15c41ad206..1ff4e40a81a 100644 --- a/src/mongo/db/feature_compatibility_version_document.idl +++ b/src/mongo/db/feature_compatibility_version_document.idl @@ -76,6 +76,8 @@ structs: type: SetFCVPhase optional: true +# Note: the order of the phases in the enum below matters, as the code relies on the order to determine whether a phase +# should be run or not based on the current phase stored in the FCV document. enums: SetFCVPhase: description: "Enum that defines the phases of the 3-phase setFCV protocol." @@ -89,6 +91,10 @@ enums: # Tells shards to execute possibly metadata-changing upgrade/downgrade actions and # transition to the fully upgraded/downgraded FCV. kComplete: "complete" + # Tells shards to enable target-version features. + kEnableTargetFeatures: "enable_target_features" + # Tells shards to commit features added in the target version. + kCommitAddedFeatures: "commit_added_features" feature_flags: featureFlagSymmetricFCV: