diff options
author | Varun Ravichandran <varun.ravichandran@mongodb.com> | 2022-04-05 20:41:24 +0000 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2022-04-07 04:40:40 +0000 |
commit | df9851154bc9847dc6a736dcdb0e85c2176d0520 (patch) | |
tree | 9fa02afd6a1fedb0d43c85a29116f58a056ebfa7 | |
parent | 74076ecb661127f73990cfca898fcf9b914aabd5 (diff) | |
download | mongo-df9851154bc9847dc6a736dcdb0e85c2176d0520.tar.gz |
SERVER-65128: Improve integration testing and address bugs in setClusterParameter
13 files changed, 302 insertions, 108 deletions
diff --git a/jstests/libs/cluster_server_parameter_utils.js b/jstests/libs/cluster_server_parameter_utils.js index 8800ee45af7..76a78003176 100644 --- a/jstests/libs/cluster_server_parameter_utils.js +++ b/jstests/libs/cluster_server_parameter_utils.js @@ -15,12 +15,10 @@ const clusterParameterNames = ["testStrClusterParameter", "testIntClusterParamet const clusterParametersDefault = [ { _id: "testStrClusterParameter", - clusterParameterTime: Timestamp(0, 0), strData: "off", }, { _id: "testIntClusterParameter", - clusterParameterTime: Timestamp(0, 0), intData: 16, } ]; @@ -28,12 +26,10 @@ const clusterParametersDefault = [ const clusterParametersInsert = [ { _id: "testStrClusterParameter", - clusterParameterTime: Timestamp(10, 2), strData: "on", }, { _id: "testIntClusterParameter", - clusterParameterTime: Timestamp(10, 2), intData: 17, } ]; @@ -41,12 +37,10 @@ const clusterParametersInsert = [ const clusterParametersUpdate = [ { _id: "testStrClusterParameter", - clusterParameterTime: Timestamp(20, 4), strData: "sleep", }, { _id: "testIntClusterParameter", - clusterParameterTime: Timestamp(20, 4), intData: 18, } ]; @@ -74,27 +68,22 @@ function setupSharded(st) { }); } -// TO-DO SERVER-65128: replace this function with a call to setClusterParameter. -// Upserts config.clusterParameters document with w:majority. -function simulateSetClusterParameterReplicaSet(rst, query, update) { - const clusterParametersNS = rst.getPrimary().getDB('config').clusterParameters; - assert.commandWorked( - clusterParametersNS.update(query, update, {upsert: true, writeConcern: {w: "majority"}})); -} - -// TO-DO SERVER-65128: replace this function with a call to setClusterParameter. -// Upserts config.clusterParameters document with w:majority into configsvr and all shards. -function simulateSetClusterParameterSharded(st, query, update) { - simulateSetClusterParameterReplicaSet(st.configRS, query, update); +// Upserts config.clusterParameters document with w:majority via setClusterParameter. +function runSetClusterParameter(conn, update) { + const paramName = update._id; + let updateCopy = Object.assign({}, update); + delete updateCopy._id; + delete updateCopy.clusterParameterTime; + const setClusterParameterDoc = { + [paramName]: updateCopy, + }; - const shards = [st.rs0, st.rs1, st.rs2]; - shards.forEach(function(shard) { - simulateSetClusterParameterReplicaSet(shard, query, update); - }); + const adminDB = conn.getDB('admin'); + assert.commandWorked(adminDB.runCommand({setClusterParameter: setClusterParameterDoc})); } // Runs getClusterParameter on a specific mongod or mongos node and returns true/false depending -// on whether . +// on whether the expected values were returned. function runGetClusterParameterNode(conn, getClusterParameterArgs, expectedClusterParameters) { const adminDB = conn.getDB('admin'); const actualClusterParameters = @@ -117,10 +106,15 @@ function runGetClusterParameterNode(conn, getClusterParameterArgs, expectedClust }, {}); const sortedActualClusterParameter = Object.keys(actualClusterParameter).sort().reduce(function(sorted, key) { - sorted[key] = actualClusterParameter[key]; + if (key !== 'clusterParameterTime') { + sorted[key] = actualClusterParameter[key]; + } return sorted; }, {}); + if (bsonWoCompare(sortedExpectedClusterParameter, sortedActualClusterParameter) !== 0) { + print('expected: ' + tojson(sortedExpectedClusterParameter) + + '\nactual: ' + tojson(sortedActualClusterParameter)); return false; } } @@ -148,10 +142,13 @@ function runGetClusterParameterReplicaSet(rst, getClusterParameterArgs, expected assert((numMatches / numTotalNodes) > 0.5); } -// Runs getClusterParameter on mongos and each mongod in each shard replica set. +// Runs getClusterParameter on mongos, each mongod in each shard replica set, and each mongod in +// the config server replica set. function runGetClusterParameterSharded(st, getClusterParameterArgs, expectedClusterParameters) { runGetClusterParameterNode(st.s0, getClusterParameterArgs, expectedClusterParameters); + runGetClusterParameterReplicaSet( + st.configRS, getClusterParameterArgs, expectedClusterParameters); const shards = [st.rs0, st.rs1, st.rs2]; shards.forEach(function(shard) { runGetClusterParameterReplicaSet(shard, getClusterParameterArgs, expectedClusterParameters); @@ -159,26 +156,24 @@ function runGetClusterParameterSharded(st, getClusterParameterArgs, expectedClus } // Tests valid usages of getClusterParameter and verifies that the expected values are returned. -function testValidParameters(conn) { +function testValidClusterParameterCommands(conn) { if (conn instanceof ReplSetTest) { // Run getClusterParameter in list format and '*' and ensure it returns all default values // on all nodes in the replica set. runGetClusterParameterReplicaSet(conn, clusterParameterNames, clusterParametersDefault); runGetClusterParameterReplicaSet(conn, '*', clusterParametersDefault); - // For each parameter, simulate setClusterParameter and verify that getClusterParameter + // For each parameter, run setClusterParameter and verify that getClusterParameter // returns the updated value on all nodes in the replica set. for (let i = 0; i < clusterParameterNames.length; i++) { - query = {_id: clusterParameterNames[i]}; - simulateSetClusterParameterReplicaSet(conn, query, clusterParametersInsert[i]); + runSetClusterParameter(conn.getPrimary(), clusterParametersInsert[i]); runGetClusterParameterReplicaSet( conn, clusterParameterNames[i], [clusterParametersInsert[i]]); } // Do the above again to verify that document updates are also handled properly. for (let i = 0; i < clusterParameterNames.length; i++) { - query = {_id: clusterParameterNames[i]}; - simulateSetClusterParameterReplicaSet(conn, query, clusterParametersUpdate[i]); + runSetClusterParameter(conn.getPrimary(), clusterParametersUpdate[i]); runGetClusterParameterReplicaSet( conn, clusterParameterNames[i], [clusterParametersUpdate[i]]); } @@ -196,16 +191,14 @@ function testValidParameters(conn) { // For each parameter, simulate setClusterParameter and verify that getClusterParameter // returns the updated value on all nodes in the sharded cluster. for (let i = 0; i < clusterParameterNames.length; i++) { - query = {_id: clusterParameterNames[i]}; - simulateSetClusterParameterSharded(conn, query, clusterParametersInsert[i]); + runSetClusterParameter(conn.s0, clusterParametersInsert[i]); runGetClusterParameterSharded( conn, clusterParameterNames[i], [clusterParametersInsert[i]]); } // Do the above again to verify that document updates are also handled properly. for (let i = 0; i < clusterParameterNames.length; i++) { - query = {_id: clusterParameterNames[i]}; - simulateSetClusterParameterSharded(conn, query, clusterParametersUpdate[i]); + runSetClusterParameter(conn.s0, clusterParametersUpdate[i]); runGetClusterParameterSharded( conn, clusterParameterNames[i], [clusterParametersUpdate[i]]); } @@ -217,8 +210,8 @@ function testValidParameters(conn) { } } -// Tests that invalid uses of getClusterParameter fails -function testInvalidParametersNode(conn) { +// Tests that invalid uses of getClusterParameter fails on a given node. +function testInvalidGetClusterParameter(conn) { const adminDB = conn.getDB('admin'); // Assert that specifying a nonexistent parameter returns an error. assert.commandFailedWithCode(adminDB.runCommand({getClusterParameter: "nonexistentParam"}), @@ -230,20 +223,90 @@ function testInvalidParametersNode(conn) { ErrorCodes.NoSuchKey); } -// Tests that invalid uses of getClusterParameter fail with the appropriate errors. -function testInvalidParameters(conn) { +// Tests that invalid uses of set/getClusterParameter fail with the appropriate errors. +function testInvalidClusterParameterCommands(conn) { if (conn instanceof ReplSetTest) { - testInvalidParametersNode(conn.getPrimary()); + const adminDB = conn.getPrimary().getDB('admin'); + + // Assert that invalid uses of getClusterParameter fail on the primary. + testInvalidGetClusterParameter(conn.getPrimary()); + + // Assert that setting a nonexistent parameter on the primary returns an error. + assert.commandFailedWithCode( + adminDB.runCommand({setClusterParameter: {nonexistentParam: {intData: 5}}}), + ErrorCodes.IllegalOperation); + + // Assert that running setClusterParameter with a scalar value fails. + assert.commandFailedWithCode( + adminDB.runCommand({setClusterParameter: {testIntClusterParameter: 5}}), + ErrorCodes.IllegalOperation); + conn.getSecondaries().forEach(function(secondary) { - testInvalidParametersNode(secondary); + // Assert that setClusterParameter cannot be run on a secondary. + const secondaryAdminDB = secondary.getDB('admin'); + assert.commandFailedWithCode( + secondaryAdminDB.runCommand( + {setClusterParameter: {testIntClusterParameter: {intData: 5}}}), + ErrorCodes.NotWritablePrimary); + // Assert that invalid uses of getClusterParameter fail on secondaries. + testInvalidGetClusterParameter(secondary); }); } else { - testInvalidParametersNode(conn.s0); + const adminDB = conn.s0.getDB('admin'); + + // Assert that invalid uses of getClusterParameter fail on mongos. + testInvalidGetClusterParameter(conn.s0); + + // Assert that setting a nonexistent parameter on the mongos returns an error. + assert.commandFailedWithCode( + adminDB.runCommand({setClusterParameter: {nonexistentParam: {intData: 5}}}), + ErrorCodes.IllegalOperation); + + // Assert that running setClusterParameter with a scalar value fails. + assert.commandFailedWithCode( + adminDB.runCommand({setClusterParameter: {testIntClusterParameter: 5}}), + ErrorCodes.IllegalOperation); + const shards = [conn.rs0, conn.rs1, conn.rs2]; shards.forEach(function(shard) { + // Assert that setClusterParameter cannot be run directly on a shard primary. + const shardPrimaryAdmin = shard.getPrimary().getDB('admin'); + assert.commandFailedWithCode( + shardPrimaryAdmin.runCommand( + {setClusterParameter: {testIntClusterParameter: {intData: 5}}}), + ErrorCodes.NotImplemented); + // Assert that invalid forms of getClusterParameter fail on the shard primary. + testInvalidGetClusterParameter(shard.getPrimary()); shard.getSecondaries().forEach(function(secondary) { - testInvalidParametersNode(secondary); + // Assert that setClusterParameter cannot be run on a shard secondary. + const shardSecondaryAdmin = secondary.getDB('admin'); + assert.commandFailedWithCode( + shardSecondaryAdmin.runCommand( + {setClusterParameter: {testIntClusterParameter: {intData: 5}}}), + ErrorCodes.NotWritablePrimary); + // Assert that invalid forms of getClusterParameter fail on shard secondaries. + testInvalidGetClusterParameter(secondary); }); }); + + // Assert that setClusterParameter cannot be run directly on the configsvr primary. + const configRS = conn.configRS; + const configPrimaryAdmin = configRS.getPrimary().getDB('admin'); + assert.commandFailedWithCode( + configPrimaryAdmin.runCommand( + {setClusterParameter: {testIntClusterParameter: {intData: 5}}}), + ErrorCodes.NotImplemented); + // Assert that invalid forms of getClusterParameter fail on the configsvr primary. + testInvalidGetClusterParameter(configRS.getPrimary()); + configRS.getSecondaries().forEach(function(secondary) { + // Assert that setClusterParameter cannot be run on a configsvr secondary. + const configSecondaryAdmin = secondary.getDB('admin'); + assert.commandFailedWithCode( + configSecondaryAdmin.runCommand( + {setClusterParameter: {testIntClusterParameter: {intData: 5}}}), + ErrorCodes.NotWritablePrimary); + // Assert that invalid forms of getClusterParameter fail on configsvr secondaries. + testInvalidGetClusterParameter(secondary); + }); } } diff --git a/jstests/replsets/cluster_server_parameter_commands_replset.js b/jstests/replsets/cluster_server_parameter_commands_replset.js index 5e296108349..add7533d47c 100644 --- a/jstests/replsets/cluster_server_parameter_commands_replset.js +++ b/jstests/replsets/cluster_server_parameter_commands_replset.js @@ -1,5 +1,5 @@ /** - * Checks that getClusterParameter runs as expected on replica set nodes. + * Checks that set/getClusterParameter runs as expected on replica set nodes. * * @tags: [ * # Requires all nodes to be running the latest binary. @@ -13,27 +13,22 @@ load('jstests/libs/cluster_server_parameter_utils.js'); -// Tests that getClusterParameter works on a non-sharded replica set. -function runReplSetTest() { - const rst = new ReplSetTest({ - nodes: 3, - }); - rst.startSet(); - rst.initiate(); +// Tests that set/getClusterParameter works on a non-sharded replica set. +const rst = new ReplSetTest({ + nodes: 3, +}); +rst.startSet(); +rst.initiate(); - // Setup the necessary logging level for the test. - setupReplicaSet(rst); +// Setup the necessary logging level for the test. +setupReplicaSet(rst); - // First, ensure that nonexistent parameters and unauthorized users are rejected with the - // appropriate error codes. - testInvalidParameters(rst); +// First, ensure that incorrect usages of set/getClusterParameter fail appropriately. +testInvalidClusterParameterCommands(rst); - // Then, ensure that getClusterParameter returns the expected values for all valid invocations - // of getClusterParameter. - testValidParameters(rst); +// Then, ensure that set/getClusterParameter set and retrieve the expected values on the +// majority of the nodes in the replica set. +testValidClusterParameterCommands(rst); - rst.stopSet(); -} - -runReplSetTest(); +rst.stopSet(); })(); diff --git a/jstests/replsets/set_cluster_parameter_replset.js b/jstests/replsets/set_cluster_parameter_replset.js new file mode 100644 index 00000000000..2e3cb5440b8 --- /dev/null +++ b/jstests/replsets/set_cluster_parameter_replset.js @@ -0,0 +1,101 @@ +/** + * Checks that setClusterParameter behaves as expected during initial sync and restart. + * + * @tags: [ + * # Requires all nodes to be running the latest binary. + * requires_fcv_60, + * featureFlagClusterWideConfig, + * does_not_support_stepdowns + * ] + */ +(function() { +'use strict'; + +load('jstests/libs/cluster_server_parameter_utils.js'); + +// Checks that up-to-date cluster parameters are transferred over to newly-added replica set nodes +// as part of initial sync. +function checkClusterParameterInitialSync(rst) { + // Update some parameter values. + let newIntParameter = { + _id: "testIntClusterParameter", + intData: 5, + }; + let newStrParameter = { + _id: "testStrClusterParameter", + strData: "on", + }; + runSetClusterParameter(rst.getPrimary(), newIntParameter); + runSetClusterParameter(rst.getPrimary(), newStrParameter); + + // Check that the new values are visible on the majority of the nodes. + runGetClusterParameterReplicaSet(rst, + ["testIntClusterParameter", "testStrClusterParameter"], + [newIntParameter, newStrParameter]); + + // Add a new node to the replica set, reinitiate the set, and wait for it to become a secondary + // with all data fully replicated to it. + const newNode = rst.add({}); + rst.reInitiate(); + rst.waitForState(newNode, ReplSetTest.State.SECONDARY); + rst.awaitReplication(); + + // Check that the new node has the latest cluster parameter values. + runGetClusterParameterNode(newNode, + ["testIntClusterParameter", "testStrClusterParameter"], + [newIntParameter, newStrParameter]); + + // Check that setClusterParameter properly works with the reconfigured replica set. + newIntParameter.intData = 30; + newStrParameter.strData = "sleep"; + runSetClusterParameter(rst.getPrimary(), newIntParameter); + runSetClusterParameter(rst.getPrimary(), newStrParameter); + + // Check that the new values are visible on the majority of the nodes. + runGetClusterParameterReplicaSet(rst, + ["testIntClusterParameter", "testStrClusterParameter"], + [newIntParameter, newStrParameter]); +} + +// Checks that restarted replica sets start with the most recent majority-written cluster parameter +// values. +function checkClusterParameterRestart(rst) { + // Update some parameter values. + let newIntParameter = { + _id: "testIntClusterParameter", + intData: 8, + }; + let newStrParameter = { + _id: "testStrClusterParameter", + strData: "dormant", + }; + runSetClusterParameter(rst.getPrimary(), newIntParameter); + runSetClusterParameter(rst.getPrimary(), newStrParameter); + + // Check that the new values are visible on the majority of the nodes. + runGetClusterParameterReplicaSet(rst, + ["testIntClusterParameter", "testStrClusterParameter"], + [newIntParameter, newStrParameter]); + + // Bounce restart all of the nodes. + rst.nodeList().forEach((_, index) => { + rst.restart(index); + }); + + // Check that restarted replica set still has the most recent setClusterParameter values. + runGetClusterParameterReplicaSet(rst, + ["testIntClusterParameter", "testStrClusterParameter"], + [newIntParameter, newStrParameter]); +} + +const rst = new ReplSetTest({ + nodes: 2, +}); +rst.startSet(); +rst.initiate(); + +checkClusterParameterInitialSync(rst); +checkClusterParameterRestart(rst); + +rst.stopSet(); +})(); diff --git a/jstests/sharding/cluster_server_parameter_commands_sharded.js b/jstests/sharding/cluster_server_parameter_commands_sharded.js index 9fecae646eb..3431fa0edcd 100644 --- a/jstests/sharding/cluster_server_parameter_commands_sharded.js +++ b/jstests/sharding/cluster_server_parameter_commands_sharded.js @@ -1,5 +1,5 @@ /** - * Checks that getClusterParameter runs as expected on sharded clusters. + * Checks that set/getClusterParameter runs as expected on sharded clusters. * * @tags: [ * # Requires all nodes to be running the latest binary. @@ -13,31 +13,27 @@ load('jstests/libs/cluster_server_parameter_utils.js'); -// Tests that getClusterParameter works on all nodes of a sharded cluster. -function runShardedTest() { - const options = { - mongos: 1, - config: 1, - shards: 3, - rs: { - nodes: 3, - }, - }; - const st = new ShardingTest(options); +// Tests that set/getClusterParameter works on all nodes of a sharded cluster. +const options = { + mongos: 1, + config: 1, + shards: 3, + rs: { + nodes: 3, + }, +}; +const st = new ShardingTest(options); - // Setup the necessary logging on mongos and the shards. - setupSharded(st); +// Setup the necessary logging on mongos and the shards. +setupSharded(st); - // First, ensure that nonexistent parameters are rejected with the - // appropriate error codes on mongos and all shards. - testInvalidParameters(st); +// First, ensure that incorrect usages of set/getClusterParameter fail appropriately on mongos +// and cluster mongods. +testInvalidClusterParameterCommands(st); - // Then, ensure that getClusterParameter returns the expected values for all valid invocations - // of getClusterParameter. - testValidParameters(st); +// Then, ensure that set/getClusterParameter set and retrieve the expected values on mongos +// and the majority of nodes on all replica sets in the cluster. +testValidClusterParameterCommands(st); - st.stop(); -} - -runShardedTest(); +st.stop(); })(); diff --git a/src/mongo/db/commands/get_cluster_parameter_command.cpp b/src/mongo/db/commands/get_cluster_parameter_command.cpp index 76f6f91c590..87c84d18387 100644 --- a/src/mongo/db/commands/get_cluster_parameter_command.cpp +++ b/src/mongo/db/commands/get_cluster_parameter_command.cpp @@ -34,6 +34,7 @@ #include "mongo/db/auth/authorization_session.h" #include "mongo/db/commands.h" #include "mongo/db/commands/cluster_server_parameter_cmds_gen.h" +#include "mongo/db/repl/replication_coordinator.h" #include "mongo/idl/cluster_server_parameter_gen.h" #include "mongo/logv2/log.h" @@ -69,6 +70,13 @@ public: "featureFlagClusterWideConfig not enabled", gFeatureFlagClusterWideConfig.isEnabled(serverGlobalParams.featureCompatibility)); + // TODO SERVER-65249: This will eventually be made specific to the parameter being set + // so that some parameters will be able to use getClusterParameter even on standalones. + uassert(ErrorCodes::IllegalOperation, + str::stream() << Request::kCommandName << " cannot be run on standalones", + repl::ReplicationCoordinator::get(opCtx)->getReplicationMode() != + repl::ReplicationCoordinator::modeNone); + const stdx::variant<std::string, std::vector<std::string>>& cmdBody = request().getCommandParameter(); ServerParameterSet* clusterParameters = ServerParameterSet::getClusterParameterSet(); @@ -78,7 +86,6 @@ public: // For each parameter, generate a BSON representation of it and retrieve its name. auto makeBSON = [&](ServerParameter* requestedParameter) { BSONObjBuilder bob; - bob.append("_id"_sd, requestedParameter->name()); requestedParameter->append(opCtx, bob, requestedParameter->name()); parameterValues.push_back(bob.obj()); parameterNames.push_back(requestedParameter->name()); diff --git a/src/mongo/db/commands/set_cluster_parameter_command.cpp b/src/mongo/db/commands/set_cluster_parameter_command.cpp index c4472f1ae12..ce752e33266 100644 --- a/src/mongo/db/commands/set_cluster_parameter_command.cpp +++ b/src/mongo/db/commands/set_cluster_parameter_command.cpp @@ -35,6 +35,7 @@ #include "mongo/db/commands.h" #include "mongo/db/commands/cluster_server_parameter_cmds_gen.h" #include "mongo/db/commands/set_cluster_parameter_invocation.h" +#include "mongo/db/repl/replication_coordinator.h" #include "mongo/idl/cluster_server_parameter_gen.h" #include "mongo/logv2/log.h" @@ -67,11 +68,21 @@ public: using InvocationBase::InvocationBase; void typedRun(OperationContext* opCtx) { - uassert(ErrorCodes::IllegalOperation, "Cannot set cluster parameter, gFeatureFlagClusterWideConfig is not enabled", gFeatureFlagClusterWideConfig.isEnabledAndIgnoreFCV()); + uassert(ErrorCodes::ErrorCodes::NotImplemented, + "setClusterParameter can only run on mongos in sharded clusters", + (serverGlobalParams.clusterRole == ClusterRole::None)); + + // TODO SERVER-65249: This will eventually be made specific to the parameter being set + // so that some parameters will be able to use setClusterParameter even on standalones. + uassert(ErrorCodes::IllegalOperation, + str::stream() << Request::kCommandName << " cannot be run on standalones", + repl::ReplicationCoordinator::get(opCtx)->getReplicationMode() != + repl::ReplicationCoordinator::modeNone); + std::unique_ptr<ServerParameterService> parameterService = std::make_unique<ClusterParameterService>(); diff --git a/src/mongo/db/commands/set_cluster_parameter_invocation.cpp b/src/mongo/db/commands/set_cluster_parameter_invocation.cpp index 8d29147d282..b021b800e12 100644 --- a/src/mongo/db/commands/set_cluster_parameter_invocation.cpp +++ b/src/mongo/db/commands/set_cluster_parameter_invocation.cpp @@ -61,11 +61,10 @@ bool SetClusterParameterInvocation::invoke(OperationContext* opCtx, "Cluster parameter value must be an object", BSONType::Object == commandElement.type()); - LogicalTime clusterTime = - paramTime ? LogicalTime(*paramTime) : _dbService.getUpdateClusterTime(opCtx); + Timestamp clusterTime = paramTime ? *paramTime : _dbService.getUpdateClusterTime(opCtx); BSONObjBuilder updateBuilder; - updateBuilder << "_id" << parameterName << "clusterParameterTime" << clusterTime.toBSON(); + updateBuilder << "_id" << parameterName << "clusterParameterTime" << clusterTime; updateBuilder.appendElements(commandElement.Obj()); BSONObj query = BSON("_id" << parameterName); @@ -78,9 +77,9 @@ bool SetClusterParameterInvocation::invoke(OperationContext* opCtx, return uassertStatusOK(_dbService.updateParameterOnDisk(opCtx, query, update, writeConcern)); } -LogicalTime ClusterParameterDBClientService::getUpdateClusterTime(OperationContext* opCtx) { +Timestamp ClusterParameterDBClientService::getUpdateClusterTime(OperationContext* opCtx) { VectorClock::VectorTime vt = VectorClock::get(opCtx)->getTime(); - return vt.clusterTime(); + return vt.clusterTime().asTimestamp(); } StatusWith<bool> ClusterParameterDBClientService::updateParameterOnDisk( @@ -94,6 +93,9 @@ StatusWith<bool> ClusterParameterDBClientService::updateParameterOnDisk( set.append("$set", update); set.doneFast(); + const auto writeConcernObj = + BSON(WriteConcernOptions::kWriteConcernField << writeConcern.toBSON()); + try { _dbClient.runCommand( NamespaceString::kConfigDb.toString(), @@ -109,7 +111,7 @@ StatusWith<bool> ClusterParameterDBClientService::updateParameterOnDisk( return entry; }()}); - return updateOp.toBSON(writeConcern.toBSON()); + return updateOp.toBSON(writeConcernObj); }(), res); } catch (const DBException& ex) { diff --git a/src/mongo/db/commands/set_cluster_parameter_invocation.h b/src/mongo/db/commands/set_cluster_parameter_invocation.h index d6ff09f0d47..aeb9cc1fde7 100644 --- a/src/mongo/db/commands/set_cluster_parameter_invocation.h +++ b/src/mongo/db/commands/set_cluster_parameter_invocation.h @@ -52,7 +52,7 @@ public: BSONObj query, BSONObj update, const WriteConcernOptions&) = 0; - virtual LogicalTime getUpdateClusterTime(OperationContext*) = 0; + virtual Timestamp getUpdateClusterTime(OperationContext*) = 0; virtual ~DBClientService() = default; }; @@ -63,7 +63,7 @@ public: BSONObj query, BSONObj update, const WriteConcernOptions&) override; - LogicalTime getUpdateClusterTime(OperationContext*) override; + Timestamp getUpdateClusterTime(OperationContext*) override; private: DBDirectClient& _dbClient; diff --git a/src/mongo/db/commands/set_cluster_parameter_invocation_test.cpp b/src/mongo/db/commands/set_cluster_parameter_invocation_test.cpp index 895b094d47d..e1e641c019e 100644 --- a/src/mongo/db/commands/set_cluster_parameter_invocation_test.cpp +++ b/src/mongo/db/commands/set_cluster_parameter_invocation_test.cpp @@ -104,9 +104,9 @@ public: return updateParameterOnDiskMockImpl(cmd, info); } - LogicalTime getUpdateClusterTime(OperationContext*) override { + Timestamp getUpdateClusterTime(OperationContext*) override { LogicalTime lt; - return lt; + return lt.asTimestamp(); } private: diff --git a/src/mongo/idl/server_parameter_specialized_test.cpp b/src/mongo/idl/server_parameter_specialized_test.cpp index 611af89335f..36a28fc7bf5 100644 --- a/src/mongo/idl/server_parameter_specialized_test.cpp +++ b/src/mongo/idl/server_parameter_specialized_test.cpp @@ -433,9 +433,8 @@ TEST(SpecializedServerParameter, withValidate) { void SpecializedClusterServerParameter::append(OperationContext*, BSONObjBuilder& builder, const std::string& name) { - BSONObjBuilder subObjBuilder = builder.subobjStart(name); - _data.serialize(&subObjBuilder); - subObjBuilder.done(); + builder.append("_id"_sd, name); + builder.appendElementsUnique(_data.toBSON()); } Status SpecializedClusterServerParameter::set(const BSONElement& newValueElement) { @@ -488,6 +487,7 @@ TEST(SpecializedServerParameter, clusterServerParameter) { data.setClusterParameterTime(updateTime); data.setIntData(50); data.setStrData("hello"); + data.setId(kSpecializedCSPName); data.serialize(&builder); ASSERT_OK(specializedCsp->set(builder.asTempObj())); @@ -500,8 +500,9 @@ TEST(SpecializedServerParameter, clusterServerParameter) { // Assert that the parameter can be appended to a builder. builder.resetToEmpty(); specializedCsp->append(nullptr, builder, kSpecializedCSPName.toString()); - auto obj = builder.asTempObj()["specializedCluster"_sd].Obj(); - ASSERT_EQ(obj.nFields(), 3); + auto obj = builder.asTempObj(); + ASSERT_EQ(obj.nFields(), 4); + ASSERT_EQ(obj["_id"_sd].String(), kSpecializedCSPName); ASSERT_EQ(obj["clusterParameterTime"_sd].timestamp(), updateTime.asTimestamp()); ASSERT_EQ(obj["strData"_sd].String(), "hello"); ASSERT_EQ(obj["intData"_sd].Int(), 50); @@ -520,8 +521,9 @@ TEST(SpecializedServerParameter, clusterServerParameter) { builder.resetToEmpty(); ASSERT_OK(specializedCsp->reset()); specializedCsp->append(nullptr, builder, kSpecializedCSPName.toString()); - obj = builder.asTempObj()["specializedCluster"_sd].Obj(); - ASSERT_EQ(obj.nFields(), 3); + obj = builder.asTempObj(); + ASSERT_EQ(obj.nFields(), 4); + ASSERT_EQ(obj["_id"_sd].String(), kSpecializedCSPName); ASSERT_EQ(obj["clusterParameterTime"_sd].timestamp(), LogicalTime().asTimestamp()); ASSERT_EQ(obj["strData"_sd].String(), "default"); ASSERT_EQ(obj["intData"_sd].Int(), 30); diff --git a/src/mongo/idl/server_parameter_specialized_test.h b/src/mongo/idl/server_parameter_specialized_test.h index 00047c14e90..d21d36175e1 100644 --- a/src/mongo/idl/server_parameter_specialized_test.h +++ b/src/mongo/idl/server_parameter_specialized_test.h @@ -82,6 +82,11 @@ public: _intData = intData; } + void setId(StringData id) { + stdx::lock_guard<Latch> lg(_mutex); + _id = id.toString(); + } + void parse(const BSONObj& updatedObj) { stdx::lock_guard<Latch> lg(_mutex); _clusterParameterTime = LogicalTime(updatedObj["clusterParameterTime"].timestamp()); @@ -91,19 +96,30 @@ public: const void serialize(BSONObjBuilder* builder) const { stdx::lock_guard<Latch> lg(_mutex); + if (_id.is_initialized()) { + builder->append("_id"_sd, _id.get()); + } builder->append("clusterParameterTime"_sd, _clusterParameterTime.asTimestamp()); builder->append("strData"_sd, _strData); builder->append("intData"_sd, _intData); } + BSONObj toBSON() const { + BSONObjBuilder builder; + serialize(&builder); + return builder.obj(); + } + void reset() { stdx::lock_guard<Latch> lg(_mutex); _clusterParameterTime = LogicalTime(); _strData = "default"; _intData = 30; + _id = boost::none; } private: + boost::optional<std::string> _id; LogicalTime _clusterParameterTime; std::string _strData; std::int32_t _intData; diff --git a/src/mongo/idl/server_parameter_with_storage.h b/src/mongo/idl/server_parameter_with_storage.h index e116dee1078..c90280ed60c 100644 --- a/src/mongo/idl/server_parameter_with_storage.h +++ b/src/mongo/idl/server_parameter_with_storage.h @@ -350,7 +350,8 @@ public: if (isRedact()) { b.append(name, "###"); } else if constexpr (paramType == SPT::kClusterWide) { - getValue().serialize(&b); + b.append("_id"_sd, name); + b.appendElementsUnique(getValue().toBSON()); } else { b.append(name, getValue()); } diff --git a/src/mongo/s/commands/cluster_get_cluster_parameter_cmd.cpp b/src/mongo/s/commands/cluster_get_cluster_parameter_cmd.cpp index 7f5022ab989..71bda72dfaf 100644 --- a/src/mongo/s/commands/cluster_get_cluster_parameter_cmd.cpp +++ b/src/mongo/s/commands/cluster_get_cluster_parameter_cmd.cpp @@ -147,6 +147,7 @@ public: defaultParameterNames.reserve(requestedParameterNames.size() - onDiskParameterNames.size()); std::sort(onDiskParameterNames.begin(), onDiskParameterNames.end()); + std::sort(requestedParameterNames.begin(), requestedParameterNames.end()); std::set_difference(requestedParameterNames.begin(), requestedParameterNames.end(), onDiskParameterNames.begin(), @@ -156,7 +157,6 @@ public: for (const auto& defaultParameterName : defaultParameterNames) { auto defaultParameter = clusterParameters->get(defaultParameterName); BSONObjBuilder bob; - bob.append("_id"_sd, defaultParameterName); defaultParameter->append(opCtx, bob, defaultParameterName); retrievedParameters.push_back(bob.obj()); } |