diff options
-rw-r--r-- | src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp | 93 | ||||
-rw-r--r-- | src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp | 79 |
2 files changed, 140 insertions, 32 deletions
diff --git a/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp b/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp index 9b4e340e508..a71478246f9 100644 --- a/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp +++ b/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp @@ -1288,7 +1288,6 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) { existingShard.toBSON(), ShardingCatalogClient::kMajorityWriteConcern)); assertShardExists(existingShard); - // Adding the same connection string with a different shard name should fail. std::string differentName = "anotherShardName"; auto future1 = launchAsync([&] { @@ -1303,6 +1302,18 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) { // Ensure that the shard document was unchanged. assertShardExists(existingShard); + // Adding a different connection string with the same shard name should fail. + ConnectionString otherHostConnString2 = + assertGet(ConnectionString::parse("mySet1/host2:12345")); + auto future2 = launchAsync([&] { + ThreadClient tc(getServiceContext()); + auto opCtx = Client::getCurrent()->makeOperationContext(); + ASSERT_EQUALS(ErrorCodes::IllegalOperation, + ShardingCatalogManager::get(opCtx.get()) + ->addShard(opCtx.get(), &existingShardName, otherHostConnString2)); + }); + future2.timed_get(kLongFutureTimeout); + // Ensure that the shard document was unchanged. assertShardExists(existingShard); @@ -1367,7 +1378,7 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) { assertShardExists(existingShard); // Adding the same replica set but different host membership (but otherwise the same options) - // should succeed + // should fail. auto otherHost = connString.getServers().back(); ConnectionString otherHostConnString = assertGet(ConnectionString::parse("mySet/host2:12345")); { @@ -1381,9 +1392,9 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) { auto future7 = launchAsync([&] { ThreadClient tc(getServiceContext()); auto opCtx = Client::getCurrent()->makeOperationContext(); - auto shardName = assertGet(ShardingCatalogManager::get(opCtx.get()) - ->addShard(opCtx.get(), nullptr, otherHostConnString)); - ASSERT_EQUALS(existingShardName, shardName); + ASSERT_EQUALS(ErrorCodes::IllegalOperation, + ShardingCatalogManager::get(opCtx.get()) + ->addShard(opCtx.get(), nullptr, otherHostConnString)); }); future7.timed_get(kLongFutureTimeout); @@ -1391,5 +1402,77 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) { assertShardExists(existingShard); } +// Tests both that trying to add a shard with a different replica set as an existing shard but with +// overlapping hosts fails, and that adding a shard with the same replica set as an existing shard +// with overlapping hosts succeeds. +TEST_F(AddShardTest, AddShardWithOverlappingHosts) { + std::unique_ptr<RemoteCommandTargeterMock> replsetTargeter( + std::make_unique<RemoteCommandTargeterMock>()); + ConnectionString connString = + assertGet(ConnectionString::parse("mySet/host1:12345,host2:12345,host3:12345")); + replsetTargeter->setConnectionStringReturnValue(connString); + HostAndPort shardTarget = connString.getServers().front(); + replsetTargeter->setFindHostReturnValue(shardTarget); + targeterFactory()->addTargeterToReturn(connString, std::move(replsetTargeter)); + + std::string existingShardName = "myShard"; + ShardType existingShard; + existingShard.setName(existingShardName); + existingShard.setHost(connString.toString()); + existingShard.setState(ShardType::ShardState::kShardAware); + + // Make sure the shard already exists. + ASSERT_OK(catalogClient()->insertConfigDocument(operationContext(), + NamespaceString::kConfigsvrShardsNamespace, + existingShard.toBSON(), + ShardingCatalogClient::kMajorityWriteConcern)); + assertShardExists(existingShard); + + // Adding a shard with a different replica set name but with some common hosts should fail. + auto otherHost = connString.getServers().front(); + ConnectionString otherHostConnString = + assertGet(ConnectionString::parse("mySet1/host1:12345,host2:12345,host4:12345")); + { + // Add a targeter for the different seed string this addShard request will use. + std::unique_ptr<RemoteCommandTargeterMock> otherHostTargeter( + std::make_unique<RemoteCommandTargeterMock>()); + otherHostTargeter->setConnectionStringReturnValue(otherHostConnString); + otherHostTargeter->setFindHostReturnValue(otherHost); + targeterFactory()->addTargeterToReturn(otherHostConnString, std::move(otherHostTargeter)); + } + auto future1 = launchAsync([&] { + ThreadClient tc(getServiceContext()); + auto opCtx = Client::getCurrent()->makeOperationContext(); + ASSERT_EQUALS(ErrorCodes::IllegalOperation, + ShardingCatalogManager::get(opCtx.get()) + ->addShard(opCtx.get(), nullptr, otherHostConnString)); + }); + future1.timed_get(kLongFutureTimeout); + // Ensure that the shard document was unchanged. + assertShardExists(existingShard); + + // Adding a shard with the same replica set name and some common hosts should pass. + ConnectionString otherHostConnString1 = + assertGet(ConnectionString::parse("mySet/host1:12345,host2:12345,host4:12345")); + { + // Add a targeter for the different seed string this addShard request will use. + std::unique_ptr<RemoteCommandTargeterMock> otherHostTargeter( + std::make_unique<RemoteCommandTargeterMock>()); + otherHostTargeter->setConnectionStringReturnValue(otherHostConnString1); + otherHostTargeter->setFindHostReturnValue(otherHost); + targeterFactory()->addTargeterToReturn(otherHostConnString1, std::move(otherHostTargeter)); + } + auto future2 = launchAsync([&] { + ThreadClient tc(getServiceContext()); + auto opCtx = Client::getCurrent()->makeOperationContext(); + auto shardName = + assertGet(ShardingCatalogManager::get(opCtx.get()) + ->addShard(opCtx.get(), &existingShardName, otherHostConnString1)); + ASSERT_EQUALS(existingShardName, shardName); + }); + future2.timed_get(kLongFutureTimeout); + // Ensure that the shard document was unchanged. + assertShardExists(existingShard); +} } // namespace } // namespace mongo diff --git a/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp b/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp index 274ca03fdc5..c8cf7edde7f 100644 --- a/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp +++ b/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp @@ -262,15 +262,55 @@ StatusWith<boost::optional<ShardType>> ShardingCatalogManager::_checkIfShardExis return true; }; + // Function for determining if there is an overlap amongst the hosts of the existing shard + // and the propsed shard. + auto checkIfHostsAreEquivalent = + [&](bool checkShardEquivalency) -> StatusWith<boost::optional<ShardType>> { + for (const auto& existingHost : existingShardConnStr.getServers()) { + for (const auto& addingHost : proposedShardConnectionString.getServers()) { + if (existingHost == addingHost) { + if (checkShardEquivalency) { + // At least one of the hosts in the shard being added already exists in + // an existing shard. If the options aren't the same, then this is an + // error, but if the options match then the addShard operation should be + // immediately considered a success and terminated. + if (shardsAreEquivalent()) { + return {existingShard}; + } else { + return {ErrorCodes::IllegalOperation, + str::stream() + << "'" << addingHost.toString() << "' " + << "is already a member of the existing shard '" + << existingShard.getHost() << "' (" + << existingShard.getName() << ")."}; + } + } else { + return {existingShard}; + } + } + } + } + return {boost::none}; + }; + if (existingShardConnStr.type() == ConnectionString::ConnectionType::kReplicaSet && proposedShardConnectionString.type() == ConnectionString::ConnectionType::kReplicaSet && existingShardConnStr.getSetName() == proposedShardConnectionString.getSetName()) { // An existing shard has the same replica set name as the shard being added. - // If the options aren't the same, then this is an error, - // but if the options match then the addShard operation should be immediately - // considered a success and terminated. + // If the options aren't the same, then this is an error. + // If the shards are equivalent, there must be some overlap amongst their hosts, if not + // then addShard results in an error. if (shardsAreEquivalent()) { - return {existingShard}; + auto hostsAreEquivalent = checkIfHostsAreEquivalent(false); + if (hostsAreEquivalent.isOK() && hostsAreEquivalent.getValue() != boost::none) { + return {existingShard}; + } else { + return {ErrorCodes::IllegalOperation, + str::stream() + << "A shard named " << existingShardConnStr.getSetName() + << " containing the replica set '" + << existingShardConnStr.getSetName() << "' already exists"}; + } } else { return {ErrorCodes::IllegalOperation, str::stream() << "A shard already exists containing the replica set '" @@ -278,32 +318,17 @@ StatusWith<boost::optional<ShardType>> ShardingCatalogManager::_checkIfShardExis } } - for (const auto& existingHost : existingShardConnStr.getServers()) { - // Look if any of the hosts in the existing shard are present within the shard trying - // to be added. - for (const auto& addingHost : proposedShardConnectionString.getServers()) { - if (existingHost == addingHost) { - // At least one of the hosts in the shard being added already exists in an - // existing shard. If the options aren't the same, then this is an error, - // but if the options match then the addShard operation should be immediately - // considered a success and terminated. - if (shardsAreEquivalent()) { - return {existingShard}; - } else { - return {ErrorCodes::IllegalOperation, - str::stream() << "'" << addingHost.toString() << "' " - << "is already a member of the existing shard '" - << existingShard.getHost() << "' (" - << existingShard.getName() << ")."}; - } - } - } + // Look if any of the hosts in the existing shard are present within the shard trying + // to be added. + auto hostsAreEquivalent = checkIfHostsAreEquivalent(true); + if (!hostsAreEquivalent.isOK() || hostsAreEquivalent.getValue() != boost::none) { + return hostsAreEquivalent; } if (proposedShardName && *proposedShardName == existingShard.getName()) { - // If we get here then we're trying to add a shard with the same name as an existing - // shard, but there was no overlap in the hosts between the existing shard and the - // proposed connection string for the new shard. + // If we get here then we're trying to add a shard with the same name as an + // existing shard, but there was no overlap in the hosts between the existing + // shard and the proposed connection string for the new shard. return {ErrorCodes::IllegalOperation, str::stream() << "A shard named " << *proposedShardName << " already exists"}; } |