diff options
author | Huayu Ouyang <huayu.ouyang@mongodb.com> | 2021-05-13 19:00:01 +0000 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2021-05-13 19:52:18 +0000 |
commit | e009f6a207358b8f6b4f1fa97fc1f52e63f71b40 (patch) | |
tree | a858a6366cf439725ba4e2455c4452a3023e4752 /src/mongo/db/repl | |
parent | 541015db8073ba776b122223ffd7f402247ad37b (diff) | |
download | mongo-e009f6a207358b8f6b4f1fa97fc1f52e63f71b40.tar.gz |
Revert "SERVER-56241 Don't allow setting getLastErrorDefaults on startup/reconfig"
This reverts commit f6b96a603dfdc1b763886db2a12ebfd226f5fc8e.
Diffstat (limited to 'src/mongo/db/repl')
-rw-r--r-- | src/mongo/db/repl/repl_set_config.cpp | 19 | ||||
-rw-r--r-- | src/mongo/db/repl/repl_set_config.h | 5 | ||||
-rw-r--r-- | src/mongo/db/repl/repl_set_config_checks.cpp | 39 | ||||
-rw-r--r-- | src/mongo/db/repl/repl_set_config_checks_test.cpp | 41 | ||||
-rw-r--r-- | src/mongo/db/repl/repl_set_config_test.cpp | 73 | ||||
-rw-r--r-- | src/mongo/db/repl/replication_coordinator_impl.cpp | 10 |
6 files changed, 109 insertions, 78 deletions
diff --git a/src/mongo/db/repl/repl_set_config.cpp b/src/mongo/db/repl/repl_set_config.cpp index c4703327e90..0c797138984 100644 --- a/src/mongo/db/repl/repl_set_config.cpp +++ b/src/mongo/db/repl/repl_set_config.cpp @@ -382,6 +382,17 @@ Status ReplSetConfig::_validate(bool allowSplitHorizonIP) const { "one non-arbiter member with priority > 0"); } + // This validation must be done outside the IDL because we need to parse the settings object + // completely to get the custom write modes. + const auto& defaultWriteConcern = getSettings()->getDefaultWriteConcern(); + if (!defaultWriteConcern.wMode.empty() && + WriteConcernOptions::kMajority != defaultWriteConcern.wMode && + !findCustomWriteMode(defaultWriteConcern.wMode).isOK()) { + return Status(ErrorCodes::BadValue, + str::stream() << "Default write concern requires undefined write mode " + << defaultWriteConcern.wMode); + } + if (getConfigServer()) { if (arbiterCount > 0) { return Status(ErrorCodes::BadValue, @@ -717,14 +728,6 @@ bool ReplSetConfig::isImplicitDefaultWriteConcernMajority() const { return arbiters == 0 || _writableVotingMembersCount > _majorityVoteCount; } -bool ReplSetConfig::containsCustomizedGetLastErrorDefaults() const { - // Since the ReplSetConfig always has a WriteConcernOptions, the only way to know if it has been - // customized through getLastErrorDefaults is if it's different from { w: 1, wtimeout: 0 }. - const auto& getLastErrorDefaults = getDefaultWriteConcern(); - return !(getLastErrorDefaults.wNumNodes == 1 && getLastErrorDefaults.wTimeout == 0 && - getLastErrorDefaults.syncMode == WriteConcernOptions::SyncMode::UNSET); -} - MemberConfig* MutableReplSetConfig::_findMemberByID(MemberId id) { for (auto it = getMembers().begin(); it != getMembers().end(); ++it) { if (it->getId() == id) { diff --git a/src/mongo/db/repl/repl_set_config.h b/src/mongo/db/repl/repl_set_config.h index 57aebd97956..db5608d0b6c 100644 --- a/src/mongo/db/repl/repl_set_config.h +++ b/src/mongo/db/repl/repl_set_config.h @@ -536,11 +536,6 @@ public: return getNumMembers() == 3 && getNumDataBearingMembers() == 2; } - /** - * Returns true if the getLastErrorDefaults has been customized. - */ - bool containsCustomizedGetLastErrorDefaults() const; - private: /** * Sets replica set ID to 'defaultReplicaSetId' if 'cfg' does not contain an ID. diff --git a/src/mongo/db/repl/repl_set_config_checks.cpp b/src/mongo/db/repl/repl_set_config_checks.cpp index d8cef779e13..4cc7c86c499 100644 --- a/src/mongo/db/repl/repl_set_config_checks.cpp +++ b/src/mongo/db/repl/repl_set_config_checks.cpp @@ -431,15 +431,6 @@ StatusWith<int> validateConfigForStartUp(ReplicationCoordinatorExternalState* ex if (!status.isOK()) { return StatusWith<int>(status); } - if (newConfig.containsCustomizedGetLastErrorDefaults()) { - fassertFailedWithStatusNoTrace( - 5624100, - {ErrorCodes::IllegalOperation, - str::stream() << "Failed to start up: Replica set config contains customized " - "getLastErrorDefaults, which " - "has been deprecated and is now ignored. Use setDefaultRWConcern " - "instead to set a cluster-wide default writeConcern."}); - } return findSelfInConfig(externalState, newConfig, ctx); } @@ -456,14 +447,10 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e return StatusWith<int>(status); } - if (newConfig.containsCustomizedGetLastErrorDefaults()) { - fassertFailedWithStatusNoTrace( - 5624101, - {ErrorCodes::IllegalOperation, - str::stream() << "Failed to initiate: Replica set config contains customized " - "getLastErrorDefaults, which " - "has been deprecated and is now ignored. Use setDefaultRWConcern " - "instead to set a cluster-wide default writeConcern."}); + status = newConfig.checkIfWriteConcernCanBeSatisfied(newConfig.getDefaultWriteConcern()); + if (!status.isOK()) { + return status.withContext( + "Found invalid default write concern in 'getLastErrorDefaults' field"); } status = validateArbiterPriorities(newConfig); @@ -509,12 +496,11 @@ Status validateConfigForReconfig(const ReplSetConfig& oldConfig, } } - uassert(5624102, - "Failed to reconfig: Replica set config contains customized " - "getLastErrorDefaults, which has " - "been deprecated and is now ignored. Use setDefaultRWConcern instead to " - "set a cluster-wide default writeConcern.", - !newConfig.containsCustomizedGetLastErrorDefaults()); + status = newConfig.checkIfWriteConcernCanBeSatisfied(newConfig.getDefaultWriteConcern()); + if (!status.isOK()) { + return status.withContext( + "Found invalid default write concern in 'getLastErrorDefaults' field"); + } status = validateOldAndNewConfigsCompatible(oldConfig, newConfig); if (!status.isOK()) { @@ -548,13 +534,6 @@ StatusWith<int> validateConfigForHeartbeatReconfig( return StatusWith<int>(status); } - tassert(5624103, - "Replica set config during heartbeat reconfig contains " - "customized getLastErrorDefaults, which has " - "been deprecated and is now ignored. Use setDefaultRWConcern instead to " - "set a cluster-wide default writeConcern.", - !newConfig.containsCustomizedGetLastErrorDefaults()); - return findSelfInConfig(externalState, newConfig, ctx); } 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..e65b5079c83 100644 --- a/src/mongo/db/repl/repl_set_config_checks_test.cpp +++ b/src/mongo/db/repl/repl_set_config_checks_test.cpp @@ -40,7 +40,6 @@ #include "mongo/db/server_options.h" #include "mongo/db/service_context.h" #include "mongo/db/service_context_test_fixture.h" -#include "mongo/unittest/death_test.h" #include "mongo/unittest/ensure_fcv.h" #include "mongo/unittest/unittest.h" @@ -156,9 +155,7 @@ TEST_F(ServiceContextTest, ValidateConfigForInitiate_SelfMustBeElectable) { .getStatus()); } -DEATH_TEST_REGEX_F(ServiceContextTest, - ValidateConfigForInitiate_NonDefaultGetLastErrorDefaults, - "Fatal assertion.*5624101") { +TEST_F(ServiceContextTest, ValidateConfigForInitiate_WriteConcernMustBeSatisfiable) { ReplSetConfig config; OID newReplSetId = OID::gen(); config = @@ -173,7 +170,10 @@ DEATH_TEST_REGEX_F(ServiceContextTest, newReplSetId); ReplicationCoordinatorExternalStateMock presentOnceExternalState; presentOnceExternalState.addSelf(HostAndPort("h2")); - validateConfigForInitiate(&presentOnceExternalState, config, getServiceContext()).getStatus(); + + ASSERT_EQUALS(ErrorCodes::UnsatisfiableWriteConcern, + validateConfigForInitiate(&presentOnceExternalState, config, getServiceContext()) + .getStatus()); } TEST_F(ServiceContextTest, ValidateConfigForInitiate_ArbiterPriorityMustBeZeroOrOne) { @@ -771,7 +771,7 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigInvalid) { validateConfigForReconfig(oldConfig, newConfig, true, false, false)); } -TEST_F(ServiceContextTest, ValidateConfigForReconfig_NonDefaultGetLastErrorDefaults) { +TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigWriteConcernNotSatisifiable) { // The new config is not valid due to an unsatisfiable write concern. This tests that if the // new config is invalid, validateConfigForReconfig will return a status indicating what is // wrong with the new config. @@ -793,13 +793,11 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NonDefaultGetLastErrorDefau ReplicationCoordinatorExternalStateMock presentOnceExternalState; presentOnceExternalState.addSelf(HostAndPort("h2")); - ASSERT_THROWS_CODE(validateConfigForReconfig(oldConfig, newConfig, false, false, false), - AssertionException, - 5624102); + ASSERT_EQUALS(ErrorCodes::UnsatisfiableWriteConcern, + validateConfigForReconfig(oldConfig, newConfig, false, false, false)); // Forced reconfigs also do not allow this. - ASSERT_THROWS_CODE(validateConfigForReconfig(oldConfig, newConfig, true, false, false), - AssertionException, - 5624102); + ASSERT_EQUALS(ErrorCodes::UnsatisfiableWriteConcern, + validateConfigForReconfig(oldConfig, newConfig, true, false, false)); } TEST_F(ServiceContextTest, ValidateConfigForStartUp_NewConfigInvalid) { @@ -842,9 +840,7 @@ TEST_F(ServiceContextTest, ValidateConfigForStartUp_NewConfigValid) { .getStatus()); } -DEATH_TEST_REGEX_F(ServiceContextTest, - ValidateConfigForStartUp_NewConfigNonDefaultGetLastErrorDefaults, - "Fatal assertion.*5624100") { +TEST_F(ServiceContextTest, ValidateConfigForStartUp_NewConfigWriteConcernNotSatisfiable) { // The new config contains an unsatisfiable write concern. We don't allow these configs to be // created anymore, but we allow any which exist to pass and the database to start up to // maintain backwards compatibility. @@ -859,7 +855,8 @@ DEATH_TEST_REGEX_F(ServiceContextTest, ReplicationCoordinatorExternalStateMock presentOnceExternalState; presentOnceExternalState.addSelf(HostAndPort("h2")); - validateConfigForStartUp(&presentOnceExternalState, newConfig, getServiceContext()).getStatus(); + ASSERT_OK(validateConfigForStartUp(&presentOnceExternalState, newConfig, getServiceContext()) + .getStatus()); } TEST_F(ServiceContextTest, ValidateConfigForHeartbeatReconfig_NewConfigInvalid) { @@ -902,9 +899,7 @@ TEST_F(ServiceContextTest, ValidateConfigForHeartbeatReconfig_NewConfigValid) { .getStatus()); } -DEATH_TEST_REGEX_F(ServiceContextTest, - ValidateConfigForHeartbeatReconfig_NonDefaultGetLastErrorDefaults, - "Tripwire assertion.*5624103") { +TEST_F(ServiceContextTest, ValidateConfigForHeartbeatReconfig_NewConfigWriteConcernNotSatisfiable) { // The new config contains an unsatisfiable write concern. We don't allow these configs to be // created anymore, but we allow any which exist to be received in a heartbeat. ReplSetConfig newConfig; @@ -920,11 +915,9 @@ DEATH_TEST_REGEX_F(ServiceContextTest, ReplicationCoordinatorExternalStateMock presentOnceExternalState; presentOnceExternalState.addSelf(HostAndPort("h2")); - ASSERT_THROWS_CODE(validateConfigForHeartbeatReconfig( - &presentOnceExternalState, newConfig, getServiceContext()) - .getStatus(), - AssertionException, - 5624103); + ASSERT_OK(validateConfigForHeartbeatReconfig( + &presentOnceExternalState, newConfig, getServiceContext()) + .getStatus()); } TEST_F(ServiceContextTest, ValidateForReconfig_ForceStillNeedsValidConfig) { diff --git a/src/mongo/db/repl/repl_set_config_test.cpp b/src/mongo/db/repl/repl_set_config_test.cpp index 2e1e133d713..c845ca27752 100644 --- a/src/mongo/db/repl/repl_set_config_test.cpp +++ b/src/mongo/db/repl/repl_set_config_test.cpp @@ -118,7 +118,10 @@ TEST(ReplSetConfig, ParseLargeConfigAndCheckAccessors) { << BSON("NYC" << "NY"))) << "protocolVersion" << 1 << "settings" - << BSON("getLastErrorModes" + << BSON("getLastErrorDefaults" + << BSON("w" + << "majority") + << "getLastErrorModes" << BSON("eastCoast" << BSON("NYC" << 1)) << "chainingAllowed" << false << "heartbeatIntervalMillis" << 5000 << "heartbeatTimeoutSecs" << 120 @@ -129,6 +132,8 @@ TEST(ReplSetConfig, ParseLargeConfigAndCheckAccessors) { ASSERT_EQUALS(1, config.getConfigTerm()); ASSERT_EQUALS(1, config.getNumMembers()); ASSERT_EQUALS(MemberId(234), config.membersBegin()->getId()); + ASSERT_EQUALS(0, config.getDefaultWriteConcern().wNumNodes); + ASSERT_EQUALS("majority", config.getDefaultWriteConcern().wMode); ASSERT_FALSE(config.isChainingAllowed()); ASSERT_TRUE(config.getWriteConcernMajorityShouldJournal()); ASSERT_FALSE(config.getConfigServer()); @@ -1225,6 +1230,59 @@ TEST(ReplSetConfig, HeartbeatTimeoutField) { DBException); } +TEST(ReplSetConfig, GleDefaultField) { + ReplSetConfig config( + ReplSetConfig::parse(BSON("_id" + << "rs0" + << "version" << 1 << "protocolVersion" << 1 << "members" + << BSON_ARRAY(BSON("_id" << 0 << "host" + << "localhost:12345")) + << "settings" + << BSON("getLastErrorDefaults" << BSON("w" + << "majority"))))); + ASSERT_OK(config.validate()); + ASSERT_EQUALS("majority", config.getDefaultWriteConcern().wMode); + + config = ReplSetConfig::parse(BSON("_id" + << "rs0" + << "version" << 1 << "protocolVersion" << 1 << "members" + << BSON_ARRAY(BSON("_id" << 0 << "host" + << "localhost:12345")) + << "settings" + << BSON("getLastErrorDefaults" << BSON("w" + << "frim")))); + ASSERT_EQUALS(ErrorCodes::BadValue, config.validate()); + + // Test that default write concern must have at least one member. + ASSERT_THROWS( + ReplSetConfig::parse(BSON("_id" + << "rs0" + << "version" << 1 << "protocolVersion" << 1 << "members" + << BSON_ARRAY(BSON("_id" << 0 << "host" + << "localhost:12345")) + << "settings" << BSON("getLastErrorDefaults" << BSON("w" << 0)))), + DBException); + + + config = ReplSetConfig::parse(BSON("_id" + << "rs0" + << "version" << 1 << "protocolVersion" << 1 << "members" + << BSON_ARRAY(BSON("_id" << 0 << "host" + << "localhost:12345" + << "tags" + << BSON("a" + << "v"))) + << "settings" + << BSON("getLastErrorDefaults" + << BSON("w" + << "frim") + << "getLastErrorModes" + << BSON("frim" << BSON("a" << 1))))); + ASSERT_OK(config.validate()); + ASSERT_EQUALS("frim", config.getDefaultWriteConcern().wMode); + ASSERT_OK(config.findCustomWriteMode("frim").getStatus()); +} + bool operator==(const MemberConfig& a, const MemberConfig& b) { // do tag comparisons for (MemberConfig::TagIterator itrA = a.tagsBegin(); itrA != a.tagsEnd(); ++itrA) { @@ -1358,11 +1416,14 @@ TEST(ReplSetConfig, toBSONRoundTripAbilityLarge) { << "true"))) << "protocolVersion" << 1 << "settings" - << BSON("heartbeatIntervalMillis" - << 5000 << "heartbeatTimeoutSecs" << 20 << "electionTimeoutMillis" << 4 - << "chainingAllowed" << true << "getLastErrorModes" - << BSON("disks" << BSON("ssd" << 1 << "hdd" << 1) << "coasts" - << BSON("coast" << 2))))); + << BSON("heartbeatIntervalMillis" << 5000 << "heartbeatTimeoutSecs" << 20 + << "electionTimeoutMillis" << 4 << "chainingAllowed" + << true << "getLastErrorDefaults" + << BSON("w" + << "majority") + << "getLastErrorModes" + << BSON("disks" << BSON("ssd" << 1 << "hdd" << 1) + << "coasts" << BSON("coast" << 2))))); BSONObj configObjA = configA.toBSON(); configB = ReplSetConfig::parse(configObjA); ASSERT_TRUE(configA == configB); diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp index adbcaec5a27..b5c99c43d82 100644 --- a/src/mongo/db/repl/replication_coordinator_impl.cpp +++ b/src/mongo/db/repl/replication_coordinator_impl.cpp @@ -4563,17 +4563,17 @@ ReplicationCoordinatorImpl::_setCurrentRSConfig(WithLock lk, } } - // Check that getLastErrorDefaults has not been changed from the default settings of - // { w: 1, wtimeout: 0 }. - if (newConfig.containsCustomizedGetLastErrorDefaults()) { + // Since the ReplSetConfig always has a WriteConcernOptions, the only way to know if it has been + // customized is if it's different to the implicit defaults of { w: 1, wtimeout: 0 }. + if (const auto& wc = newConfig.getDefaultWriteConcern(); + !(wc.wNumNodes == 1 && wc.wTimeout == 0)) { LOGV2_OPTIONS(21387, {logv2::LogTag::kStartupWarnings}, ""); LOGV2_OPTIONS(21388, {logv2::LogTag::kStartupWarnings}, "** WARNING: Replica set config contains customized getLastErrorDefaults,"); LOGV2_OPTIONS(21389, {logv2::LogTag::kStartupWarnings}, - "** which have been deprecated and are now ignored. Use " - "setDefaultRWConcern instead to set a"); + "** which are deprecated. Use setDefaultRWConcern instead to set a"); LOGV2_OPTIONS(21390, {logv2::LogTag::kStartupWarnings}, "** cluster-wide default writeConcern."); |