mirrors/mongodb
mirrors/mongodb
0
0
Fork 0
mirror of https://github.com/mongodb/mongo.git synced 2024-12-01 09:32:32 +01:00
Code Issues Releases Activity

SERVER-54829 consolidate validateConfigForReconfig paths into a single one

Browse Source
This commit is contained in:
Pavi Vetriselvan 2021-04-01 12:54:53 -04:00 committed by Evergreen Agent
parent 490d13e5e3
commit f3a6c48b49
4 changed files with 47 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/mongo/db/repl/repl_set_config_checks.cpp
View File

@ -426,10 +426,10 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e
return findSelfInConfigIfElectable(externalState, newConfig, ctx);
}
Status _validateConfigForReconfig(const ReplSetConfig& oldConfig,
const ReplSetConfig& newConfig,
bool force,
bool allowSplitHorizonIP) {
Status validateConfigForReconfig(const ReplSetConfig& oldConfig,
const ReplSetConfig& newConfig,
bool force,
bool allowSplitHorizonIP) {
Status status =
allowSplitHorizonIP ? newConfig.validateAllowingSplitHorizonIP() : newConfig.validate();
if (!status.isOK()) {
@ -470,18 +470,6 @@ Status _validateConfigForReconfig(const ReplSetConfig& oldConfig,
return Status::OK();
}
Status validateConfigForReconfig(const ReplSetConfig& oldConfig,
const ReplSetConfig& newConfig,
bool force) {
return _validateConfigForReconfig(oldConfig, newConfig, force, false);
}
Status validateConfigForOplogReconfig(const ReplSetConfig& oldConfig,
const ReplSetConfig& newConfig) {
return _validateConfigForReconfig(oldConfig, newConfig, true, true);
}
StatusWith<int> validateConfigForHeartbeatReconfig(
ReplicationCoordinatorExternalState* externalState,
const ReplSetConfig& newConfig,

16
src/mongo/db/repl/repl_set_config_checks.h
View File

@ -93,21 +93,15 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e
*
* If "force" is set to true, then the single node change requirement is not checked.
*
* If "allowSplitHorizonIP" is set to true, skips checking whether an IP address exists in
* split horizon configuration.
*
* Returns an indicative error on validation failure.
*/
Status validateConfigForReconfig(const ReplSetConfig& oldConfig,
const ReplSetConfig& newConfig,
bool force);
/**
* Validates that "newConfig" is a legal successor configuration to "oldConfig" that can be
* initiated by the current node (identified via "externalState"). Ignores an error condition
* when an IP address exists in split horizon configuration
*
* Returns an indicative error on validation failure.
*/
Status validateConfigForOplogReconfig(const ReplSetConfig& oldConfig,
const ReplSetConfig& newConfig);
bool force,
bool allowSplitHorizonIP);
/**
* Validates that "newConfig" is an acceptable configuration when received in a heartbeat

63
src/mongo/db/repl/repl_set_config_checks_test.cpp
View File

@ -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));
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));
validateConfigForReconfig(oldConfig, oldConfig, false, false));
// Cannot reconfig from new to old (versions must increase).
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(newConfig, oldConfig, 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));
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));
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));
validateConfigForReconfig(oldConfig, newConfig, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(newConfig, oldConfig, true));
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);
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));
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));
ASSERT_OK(validateConfigForReconfig(oldConfig, oldConfigRefresh, false, false));
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(oldConfig, newConfig, false));
validateConfigForReconfig(oldConfig, newConfig, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(oldConfig, newConfig, true));
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));
ASSERT_OK(validateConfigForReconfig(oldConfig, oldConfigRefresh, false, false));
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(oldConfig, newConfig, false));
validateConfigForReconfig(oldConfig, newConfig, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(oldConfig, newConfig, true));
validateConfigForReconfig(oldConfig, newConfig, true, false));
}
TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricted) {
@ -607,7 +607,8 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricte
validateConfigForReconfig(oldConfig,
legalNewConfigWithNewHostAndId,
// Use 'force' since we're adding and removing a node atomically.
true));
true,
false));
//
// Here, the new config is invalid because we've reused host name "h2" with
@ -625,10 +626,10 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricte
<< "h3"))));
ASSERT_OK(illegalNewConfigReusingHost.validate());
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, false));
validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::NewReplicaSetConfigurationIncompatible,
validateConfigForReconfig(oldConfig, illegalNewConfigReusingHost, true));
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.
@ -644,7 +645,7 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_HostAndIdRemappingRestricte
<< BSON("_id" << 3 << "host"
<< "h3"))));
ASSERT_OK(illegalNewConfigReusingId.validate());
ASSERT_OK(validateConfigForReconfig(oldConfig, illegalNewConfigReusingId, false));
ASSERT_OK(validateConfigForReconfig(oldConfig, illegalNewConfigReusingId, false, false));
}
TEST_F(ServiceContextTest, ValidateConfigForReconfig_ArbiterPriorityValueMustBeZeroOrOne) {
@ -705,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));
ASSERT_OK(validateConfigForReconfig(oldConfig, oneConfig, false));
ASSERT_OK(validateConfigForReconfig(oldConfig, zeroConfig, false, false));
ASSERT_OK(validateConfigForReconfig(oldConfig, oneConfig, false, false));
ASSERT_EQUALS(ErrorCodes::InvalidReplicaSetConfig,
validateConfigForReconfig(oldConfig, twoConfig, false));
validateConfigForReconfig(oldConfig, twoConfig, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::InvalidReplicaSetConfig,
validateConfigForReconfig(oldConfig, twoConfig, true));
validateConfigForReconfig(oldConfig, twoConfig, true, false));
}
TEST_F(ServiceContextTest, ValidateConfigForInitiate_NewConfigInvalid) {
@ -760,9 +761,11 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigInvalid) {
ReplicationCoordinatorExternalStateMock presentOnceExternalState;
presentOnceExternalState.addSelf(HostAndPort("h2"));
ASSERT_EQUALS(ErrorCodes::BadValue, validateConfigForReconfig(oldConfig, newConfig, false));
ASSERT_EQUALS(ErrorCodes::BadValue,
validateConfigForReconfig(oldConfig, newConfig, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::BadValue, validateConfigForReconfig(oldConfig, newConfig, true));
ASSERT_EQUALS(ErrorCodes::BadValue,
validateConfigForReconfig(oldConfig, newConfig, true, false));
}
TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigWriteConcernNotSatisifiable) {
@ -788,10 +791,10 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigWriteConcernNotSat
ReplicationCoordinatorExternalStateMock presentOnceExternalState;
presentOnceExternalState.addSelf(HostAndPort("h2"));
ASSERT_EQUALS(ErrorCodes::UnsatisfiableWriteConcern,
validateConfigForReconfig(oldConfig, newConfig, false));
validateConfigForReconfig(oldConfig, newConfig, false, false));
// Forced reconfigs also do not allow this.
ASSERT_EQUALS(ErrorCodes::UnsatisfiableWriteConcern,
validateConfigForReconfig(oldConfig, newConfig, true));
validateConfigForReconfig(oldConfig, newConfig, true, false));
}
TEST_F(ServiceContextTest, ValidateConfigForStartUp_NewConfigInvalid) {
@ -938,7 +941,8 @@ TEST_F(ServiceContextTest, ValidateForReconfig_ForceStillNeedsValidConfig) {
ReplicationCoordinatorExternalStateMock presentOnceExternalState;
presentOnceExternalState.addSelf(HostAndPort("h2"));
ASSERT_EQUALS(ErrorCodes::BadValue, validateConfigForReconfig(oldConfig, newConfig, true));
ASSERT_EQUALS(ErrorCodes::BadValue,
validateConfigForReconfig(oldConfig, newConfig, true, false));
}
//
@ -996,7 +1000,8 @@ Status validateMemberReconfig(BSONArray oldMembers, BSONArray newMembers, BSONOb
// Do reconfig.
const bool force = false;
return validateConfigForReconfig(oldConfig, newConfig, force);
const bool allowSplitHorizonIP = false;
return validateConfigForReconfig(oldConfig, newConfig, force, allowSplitHorizonIP);
}
TEST_F(ServiceContextTest, ValidateForReconfig_SingleNodeAdditionAllowed) {

8
src/mongo/db/repl/replication_coordinator_impl.cpp
View File

@ -3447,10 +3447,10 @@ Status ReplicationCoordinatorImpl::_doReplSetReconfig(OperationContext* opCtx,
BSONObj newConfigObj = newConfig.toBSON();
audit::logReplSetReconfig(opCtx->getClient(), &oldConfigObj, &newConfigObj);
bool isManualReconfig = opCtx->getClient()->hasRemote();
Status validateStatus = isManualReconfig
? validateConfigForReconfig(oldConfig, newConfig, force)
: validateConfigForOplogReconfig(oldConfig, newConfig);
// User reconfigs should not allow IP addresses in split horizons.
bool allowSplitHorizonIP = !opCtx->getClient()->hasRemote();
Status validateStatus =
validateConfigForReconfig(oldConfig, newConfig, force, allowSplitHorizonIP);
if (!validateStatus.isOK()) {
LOGV2_ERROR(21420,
"replSetReconfig got {error} while validating {newConfig}",