SERVER-53354 Remove generic FCV references in isVersionCompatible function in repl_set_config_checks.cpp

6 files changed, 36 insertions, 146 deletions

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 bdc58f37352..dbe749e2c3a 100644
--- a/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
+++ b/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
@@ -86,7 +86,6 @@ using FeatureCompatibility = ServerGlobalParams::FeatureCompatibility;
 
 MONGO_FAIL_POINT_DEFINE(failUpgrading);
 MONGO_FAIL_POINT_DEFINE(hangWhileUpgrading);
-MONGO_FAIL_POINT_DEFINE(hangAfterStartingFCVTransition);
 MONGO_FAIL_POINT_DEFINE(failDowngrading);
 MONGO_FAIL_POINT_DEFINE(hangWhileDowngrading);
 MONGO_FAIL_POINT_DEFINE(hangBeforeUpdatingFcvDoc);
@@ -340,8 +339,6 @@ public:
         invariant(serverGlobalParams.featureCompatibility.isUpgradingOrDowngrading());
         invariant(!request.getPhase() || request.getPhase() == SetFCVPhaseEnum::kComplete);
 
-        hangAfterStartingFCVTransition.pauseWhileSet(opCtx);
-
         if (requestedVersion > actualVersion) {
             _runUpgrade(opCtx, request, changeTimestamp);
         } else {
diff --git a/src/mongo/db/repl/member_config.h b/src/mongo/db/repl/member_config.h
index c27c35b6f6e..e68c24b6f2d 100644
--- a/src/mongo/db/repl/member_config.h
+++ b/src/mongo/db/repl/member_config.h
@@ -175,13 +175,6 @@ public:
     }
 
     /**
-     * Returns true if this member has the field 'slaveDelay'.
-     */
-    bool hasSlaveDelay() const {
-        return false;
-    }
-
-    /**
      * Returns true if this member has the field 'secondaryDelaySecs'.
      */
     bool hasSecondaryDelaySecs() const {
diff --git a/src/mongo/db/repl/repl_set_config_checks.cpp b/src/mongo/db/repl/repl_set_config_checks.cpp
index d8cef779e13..a4c2ee442d4 100644
--- a/src/mongo/db/repl/repl_set_config_checks.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks.cpp
@@ -100,68 +100,6 @@ Status ensureNoNewlyAddedMembers(const ReplSetConfig& config) {
 }
 
 /**
- * Checks that the current feature compatibility version is compatible with
- * each member config's delay field name. If the feature flag 'featureFlagUseSecondaryDelaySecs' is
- * enabled, the nodes must use the 'secondaryDelaySecs' field instead of the 'slaveDelay' field.
- */
-Status isFCVCompatible(const ReplSetConfig& config) {
-    auto version = serverGlobalParams.featureCompatibility.getVersion();
-    // New shard servers using 'latest' binaries will default to the 'lastLTS' FCV prior to being
-    // added to the cluster. If they have not yet been added to a sharded cluster via addShard,
-    // allow them to use 'secondaryDelaySecs'.
-    bool isNewShardServer = (serverGlobalParams.clusterRole == ClusterRole::ShardServer &&
-                             !ShardingState::get(getGlobalServiceContext())->enabled());
-    // TODO (SERVER-53354) If we are currently upgrading, we check if the feature flag is enabled
-    // for the target version. We use the generic FCV references here to avoid having to update the
-    // FCV constants used after each continuous release. After release, we should make sure to
-    // remove these references while removing the feature flag.
-    //
-    //(Generic FCV reference): feature flag support
-    if (version == ServerGlobalParams::FeatureCompatibility::kUpgradingFromLastLTSToLatest ||
-        version == ServerGlobalParams::FeatureCompatibility::kUpgradingFromLastContinuousToLatest) {
-        version = ServerGlobalParams::FeatureCompatibility::kLatest;
-    }
-    //(Generic FCV reference): feature flag support
-    if (version ==
-        ServerGlobalParams::FeatureCompatibility::kUpgradingFromLastLTSToLastContinuous) {
-        version = ServerGlobalParams::FeatureCompatibility::kLastContinuous;
-    }
-
-    ServerGlobalParams::FeatureCompatibility targetFCV;
-    targetFCV.setVersion(version);
-    bool isEnabled = feature_flags::gUseSecondaryDelaySecs.isEnabled(targetFCV);
-    // We must check that every member config has a valid delay field name.
-    for (auto iter = config.membersBegin(); iter != config.membersEnd(); ++iter) {
-        // TODO (SERVER-53354) If the feature flag is disabled, getVersion() will throw. In this
-        // case, the version should default to kLatest. We use the generic FCV references here
-        // to avoid having to update the FCV constants used after each continuous release. After
-        // release, we should make sure to remove these references while removing the feature
-        // flag.
-        //
-        //(Generic FCV reference): feature flag support
-        if (isEnabled && iter->hasSlaveDelay()) {
-            auto featureFlagVersion = FeatureCompatibilityVersionParser::toString(
-                feature_flags::gUseSecondaryDelaySecs.getVersion());
-            return Status(ErrorCodes::BadValue,
-                          str::stream()
-                              << "Incompatible delay field name. If the node is in FCV "
-                              << featureFlagVersion << ", it must use secondaryDelaySecs.");
-        }
-        //(Generic FCV reference): feature flag support
-        if (!isEnabled && iter->hasSecondaryDelaySecs() && !isNewShardServer) {
-            auto latestVersion = FeatureCompatibilityVersionParser::toString(
-                ServerGlobalParams::FeatureCompatibility::kLatest);
-            return Status(ErrorCodes::BadValue,
-                          str::stream() << "Incompatible delay field name. In FCV versions below "
-                                        << latestVersion << ", nodes must use slaveDelay.");
-        }
-    }
-
-
-    return Status::OK();
-}
-
-/**
  * Compares two initialized and validated replica set configurations and checks to see if the
  * transition from 'oldConfig' to 'newConfig' adds or removes at most 1 voting node.
  *
@@ -451,11 +389,6 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e
         return StatusWith<int>(status);
     }
 
-    status = isFCVCompatible(newConfig);
-    if (!status.isOK()) {
-        return StatusWith<int>(status);
-    }
-
     if (newConfig.containsCustomizedGetLastErrorDefaults()) {
         fassertFailedWithStatusNoTrace(
             5624101,
@@ -494,21 +427,13 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e
 Status validateConfigForReconfig(const ReplSetConfig& oldConfig,
                                  const ReplSetConfig& newConfig,
                                  bool force,
-                                 bool allowSplitHorizonIP,
-                                 bool skipFCVCompatibilityCheck) {
+                                 bool allowSplitHorizonIP) {
     Status status =
         allowSplitHorizonIP ? newConfig.validateAllowingSplitHorizonIP() : newConfig.validate();
     if (!status.isOK()) {
         return status;
     }
 
-    if (!skipFCVCompatibilityCheck) {
-        status = isFCVCompatible(newConfig);
-        if (!status.isOK()) {
-            return status;
-        }
-    }
-
     uassert(5624102,
             "Failed to reconfig: Replica set config contains customized "
             "getLastErrorDefaults, which has "
diff --git a/src/mongo/db/repl/repl_set_config_checks.h b/src/mongo/db/repl/repl_set_config_checks.h
index 3f4ea67acc3..d4b64335e20 100644
--- a/src/mongo/db/repl/repl_set_config_checks.h
+++ b/src/mongo/db/repl/repl_set_config_checks.h
@@ -96,16 +96,12 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e
  * If "allowSplitHorizonIP" is set to true, skips checking whether an IP address exists in
  * split horizon configuration.
  *
- * If "skipFCVCompatibilityCheck" is set to true, skips checking whether the
- * 'secondaryDelaySecs' field name is compatible with the current featureCompatibilityVersion.
- *
  * Returns an indicative error on validation failure.
  */
 Status validateConfigForReconfig(const ReplSetConfig& oldConfig,
                                  const ReplSetConfig& newConfig,
                                  bool force,
-                                 bool allowSplitHorizonIP,
-                                 bool skipFCVCompatibilityCheck);
+                                 bool allowSplitHorizonIP);
 
 /**
  * Validates that "newConfig" is an acceptable configuration when
diff --git a/src/mongo/db/repl/repl_set_config_checks_test.cpp b/src/mongo/db/repl/repl_set_config_checks_test.cpp
index 1639e7d3b74..03f9965053c 100644
--- a/src/mongo/db/repl/repl_set_config_checks_test.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks_test.cpp
@@ -311,16 +311,16 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigVersionNumberMustB
     ASSERT_OK(newConfig.validate());
 
     // Can reconfig from old to new.
-    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false, false));
 
 
     // Cannot reconfig from old to old (versions must be different).
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(oldConfig, oldConfig, false, false, false));
+                  validateConfigForReconfig(oldConfig, oldConfig, false, false));
 
     // Cannot reconfig from new to old (versions must increase).
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(newConfig, oldConfig, false, false, false));
+                  validateConfigForReconfig(newConfig, oldConfig, false, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_memberId) {
@@ -345,7 +345,7 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_memberId) {
                                                                       << "h2"))));
     ASSERT_OK(oldConfig.validate());
     ASSERT_OK(newConfig.validate());
-    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false, false));
 
     // Case 2: Change the member config setting for the existing member with member id > 255.
     oldConfig = ReplSetConfig::parse(BSON("_id"
@@ -365,7 +365,7 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_memberId) {
                                                                       << "priority" << 0))));
     ASSERT_OK(oldConfig.validate());
     ASSERT_OK(newConfig.validate());
-    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false, false));
 }
 
 
@@ -400,10 +400,10 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotChangeSetNa
     ASSERT_OK(oldConfig.validate());
     ASSERT_OK(newConfig.validate());
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, false, false));
     // Forced reconfigs also do not allow this.
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(newConfig, oldConfig, true, false, false));
+                  validateConfigForReconfig(newConfig, oldConfig, true, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotChangeSetId) {
@@ -438,13 +438,13 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotChangeSetId
 
     ASSERT_OK(oldConfig.validate());
     ASSERT_OK(newConfig.validate());
-    const auto status = validateConfigForReconfig(oldConfig, newConfig, false, false, false);
+    const auto status = validateConfigForReconfig(oldConfig, newConfig, false, false);
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible, status);
     ASSERT_STRING_CONTAINS(status.reason(), "New and old configurations differ in replica set ID");
 
     // Forced reconfigs also do not allow this.
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(newConfig, oldConfig, true, false, false));
+                  validateConfigForReconfig(newConfig, oldConfig, true, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotFlipBuildIndexesFlag) {
@@ -498,13 +498,13 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotFlipBuildIn
     ASSERT_OK(oldConfig.validate());
     ASSERT_OK(newConfig.validate());
     ASSERT_OK(oldConfigRefresh.validate());
-    ASSERT_OK(validateConfigForReconfig(oldConfig, oldConfigRefresh, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, oldConfigRefresh, false, false));
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, false, false));
 
     // Forced reconfigs also do not allow this.
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(oldConfig, newConfig, true, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, true, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotFlipArbiterFlag) {
@@ -555,12 +555,12 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotFlipArbiter
     ASSERT_OK(oldConfig.validate());
     ASSERT_OK(newConfig.validate());
     ASSERT_OK(oldConfigRefresh.validate());
-    ASSERT_OK(validateConfigForReconfig(oldConfig, oldConfigRefresh, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, oldConfigRefresh, false, false));
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, false, false));
     // Forced reconfigs also do not allow this.
     ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
-                  validateConfigForReconfig(oldConfig, newConfig, true, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, true, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricted) {
@@ -608,7 +608,6 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricte
                                   legalNewConfigWithNewHostAndId,
                                   // Use 'force' since we're adding and removing a node atomically.
                                   true,
-                                  false,
                                   false));
 
     //
@@ -626,13 +625,11 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricte
                                                 << BSON("_id" << 3 << "host"
                                                               << "h3"))));
     ASSERT_OK(illegalNewConfigReusingHost.validate());
-    ASSERT_EQUALS(
-        ErrorCodes::NewReplicaSetConfigurationIncompatible,
-        validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, false, false, false));
+    ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
+                  validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, false, false));
     // Forced reconfigs also do not allow this.
-    ASSERT_EQUALS(
-        ErrorCodes::NewReplicaSetConfigurationIncompatible,
-        validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, true, false, false));
+    ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
+                  validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, true, false));
     //
     // Here, the new config is valid, because all we've changed is the name of
     // the host representing _id 2.
@@ -648,7 +645,7 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricte
                                                 << BSON("_id" << 3 << "host"
                                                               << "h3"))));
     ASSERT_OK(illegalNewConfigReusingId.validate());
-    ASSERT_OK(validateConfigForReconfig(oldConfig, illegalNewConfigReusingId, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, illegalNewConfigReusingId, false, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_ArbiterPriorityValueMustBeZeroOrOne) {
@@ -709,13 +706,13 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_ArbiterPriorityValueMustBeZ
     ASSERT_OK(zeroConfig.validate());
     ASSERT_OK(oneConfig.validate());
     ASSERT_OK(twoConfig.validate());
-    ASSERT_OK(validateConfigForReconfig(oldConfig, zeroConfig, false, false, false));
-    ASSERT_OK(validateConfigForReconfig(oldConfig, oneConfig, false, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, zeroConfig, false, false));
+    ASSERT_OK(validateConfigForReconfig(oldConfig, oneConfig, false, false));
     ASSERT_EQUALS(ErrorCodes::InvalidReplicaSetConfig,
-                  validateConfigForReconfig(oldConfig, twoConfig, false, false, false));
+                  validateConfigForReconfig(oldConfig, twoConfig, false, false));
     // Forced reconfigs also do not allow this.
     ASSERT_EQUALS(ErrorCodes::InvalidReplicaSetConfig,
-                  validateConfigForReconfig(oldConfig, twoConfig, true, false, false));
+                  validateConfigForReconfig(oldConfig, twoConfig, true, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForInitiate_NewConfigInvalid) {
@@ -765,10 +762,10 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigInvalid) {
     ReplicationCoordinatorExternalStateMock presentOnceExternalState;
     presentOnceExternalState.addSelf(HostAndPort("h2"));
     ASSERT_EQUALS(ErrorCodes::BadValue,
-                  validateConfigForReconfig(oldConfig, newConfig, false, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, false, false));
     // Forced reconfigs also do not allow this.
     ASSERT_EQUALS(ErrorCodes::BadValue,
-                  validateConfigForReconfig(oldConfig, newConfig, true, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, true, false));
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_NonDefaultGetLastErrorDefaults) {
@@ -793,13 +790,11 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NonDefaultGetLastErrorDefau
 
     ReplicationCoordinatorExternalStateMock presentOnceExternalState;
     presentOnceExternalState.addSelf(HostAndPort("h2"));
-    ASSERT_THROWS_CODE(validateConfigForReconfig(oldConfig, newConfig, false, false, false),
-                       AssertionException,
-                       5624102);
+    ASSERT_THROWS_CODE(
+        validateConfigForReconfig(oldConfig, newConfig, false, false), AssertionException, 5624102);
     // Forced reconfigs also do not allow this.
-    ASSERT_THROWS_CODE(validateConfigForReconfig(oldConfig, newConfig, true, false, false),
-                       AssertionException,
-                       5624102);
+    ASSERT_THROWS_CODE(
+        validateConfigForReconfig(oldConfig, newConfig, true, false), AssertionException, 5624102);
 }
 
 TEST_F(ServiceContextTest, ValidateConfigForStartUp_NewConfigInvalid) {
@@ -952,7 +947,7 @@ TEST_F(ServiceContextTest, ValidateForReconfig_ForceStillNeedsValidConfig) {
     ReplicationCoordinatorExternalStateMock presentOnceExternalState;
     presentOnceExternalState.addSelf(HostAndPort("h2"));
     ASSERT_EQUALS(ErrorCodes::BadValue,
-                  validateConfigForReconfig(oldConfig, newConfig, true, false, false));
+                  validateConfigForReconfig(oldConfig, newConfig, true, false));
 }
 
 //
@@ -1011,9 +1006,7 @@ Status validateMemberReconfig(BSONArray oldMembers, BSONArray newMembers, BSONOb
     // Do reconfig.
     const bool force = false;
     const bool allowSplitHorizonIP = false;
-    const bool skipFCVCompatibilityCheck = false;
-    return validateConfigForReconfig(
-        oldConfig, newConfig, force, allowSplitHorizonIP, skipFCVCompatibilityCheck);
+    return validateConfigForReconfig(oldConfig, newConfig, force, allowSplitHorizonIP);
 }
 
 TEST_F(ServiceContextTest, ValidateForReconfig_SingleNodeAdditionAllowed) {
diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp
index f6c475aa472..44b1c72a7c9 100644
--- a/src/mongo/db/repl/replication_coordinator_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl.cpp
@@ -3561,24 +3561,10 @@ Status ReplicationCoordinatorImpl::_doReplSetReconfig(OperationContext* opCtx,
     BSONObj newConfigObj = newConfig.toBSON();
     audit::logReplSetReconfig(opCtx->getClient(), &oldConfigObj, &newConfigObj);
 
-    // Stepdown can interrupt the setFeatureCompatibilityVersion command after we've transitioned
-    // to the intermediary kUpgrading/kDowngrading state but before the reconfig to change the
-    // 'secondaryDelaySecs' field name. During a subsequent stepup's automatic reconfig, the config
-    // could have an incompatible field name based on the intermediate FCV, but since we must retry
-    // setFeatureCompatibilityVersion until we complete the upgrade/downgrade procedure, the
-    // reconfig to change the delay field name to the correct value will eventually succeed.
-    // Therefore, it is safe for us to skip the FCV compatibility check during the stepup reconfig
-    // to avoid crashing.
-    //
-    // (Generic FCV reference): feature flag support
-    // TODO (SERVER-53354): Remove this check once 5.0 becomes 'lastLTS'.
-    bool skipFCVCompatibilityCheck =
-        serverGlobalParams.featureCompatibility.isUpgradingOrDowngrading() && skipSafetyChecks;
-
     bool allowSplitHorizonIP = !opCtx->getClient()->hasRemote();
 
-    Status validateStatus = validateConfigForReconfig(
-        oldConfig, newConfig, force, allowSplitHorizonIP, skipFCVCompatibilityCheck);
+    Status validateStatus =
+        validateConfigForReconfig(oldConfig, newConfig, force, allowSplitHorizonIP);
     if (!validateStatus.isOK()) {
         LOGV2_ERROR(21420,
                     "replSetReconfig got {error} while validating {newConfig}",