From 400ac8f8cf6938ee5f9e7a754061fa318b8bc46c Mon Sep 17 00:00:00 2001 From: Cheahuychou Mao Date: Thu, 2 Mar 2023 20:50:43 +0000 Subject: SERVER-74416 Make sure analyzeShardKey and configureQueryAnalyzer command use database and shard versioning --- ...lyze_shard_key_database_and_shard_versioning.js | 101 +++++++++++++++++++++ ...configure_query_analyzer_database_versioning.js | 47 ++++++++++ .../analyze_shard_key/most_common_values.js | 4 +- .../sharding/safe_secondary_reads_drop_recreate.js | 30 ++++-- ...eads_single_migration_suspend_range_deletion.js | 2 +- ...condary_reads_single_migration_waitForDelete.js | 26 +++++- src/mongo/s/analyze_shard_key_util.cpp | 92 +++++++++++-------- .../s/commands/cluster_analyze_shard_key_cmd.cpp | 11 ++- 8 files changed, 258 insertions(+), 55 deletions(-) create mode 100644 jstests/sharding/analyze_shard_key/analyze_shard_key_database_and_shard_versioning.js create mode 100644 jstests/sharding/analyze_shard_key/configure_query_analyzer_database_versioning.js diff --git a/jstests/sharding/analyze_shard_key/analyze_shard_key_database_and_shard_versioning.js b/jstests/sharding/analyze_shard_key/analyze_shard_key_database_and_shard_versioning.js new file mode 100644 index 00000000000..f00c1acbad8 --- /dev/null +++ b/jstests/sharding/analyze_shard_key/analyze_shard_key_database_and_shard_versioning.js @@ -0,0 +1,101 @@ +/** + * Tests that the analyzeShardKey command uses database and shard versioning. + * + * @tags: [requires_fcv_70, featureFlagAnalyzeShardKey] + */ +(function() { +"use strict"; + +load("jstests/libs/uuid_util.js"); // for 'extractUUIDFromObject' +load("jstests/sharding/analyze_shard_key/libs/analyze_shard_key_util.js"); + +// The write concern to use when inserting documents into test collections. Waiting for the +// documents to get replicated to all nodes is necessary since the test later runs the +// analyzeShardKey command with readPreference "secondary". +const numNodesPerRS = 2; +const writeConcern = { + w: numNodesPerRS +}; + +const numMostCommonValues = 5; +const st = new ShardingTest({ + mongos: 2, + shards: 2, + rs: { + nodes: numNodesPerRS, + setParameter: { + // The calculation of the read and write distribution metrics involves generating split + // points which requires the shard key to have sufficient cardinality. To avoid needing + // to insert a lot of documents, just skip the calculation. + "failpoint.analyzeShardKeySkipCalcalutingReadWriteDistributionMetrics": + tojson({mode: "alwaysOn"}), + analyzeShardKeyNumMostCommonValues: numMostCommonValues + } + } +}); + +function runTest(readPreference) { + const dbName = "testDb" + extractUUIDFromObject(UUID()); + const collName = "testColl"; + const ns = dbName + "." + collName; + jsTest.log(`Testing analyzeShardKey with ${tojson({dbName, collName, readPreference})}`); + + // Make shard0 the primary shard. + assert.commandWorked(st.s0.adminCommand({enableSharding: dbName})); + st.ensurePrimaryShard(dbName, st.shard0.name); + + const mongos0Coll = st.s0.getCollection(ns); + assert.commandWorked(mongos0Coll.createIndex({x: 1})); + assert.commandWorked(mongos0Coll.insert([{x: -1}, {x: 1}], {writeConcern})); + + const analyzeShardKeyCmdObj = { + analyzeShardKey: ns, + key: {x: 1}, + $readPreference: readPreference + }; + const expectedMetrics = { + numDocs: 2, + isUnique: false, + numDistinctValues: 2, + mostCommonValues: [{value: {x: -1}, frequency: 1}, {value: {x: 1}, frequency: 1}], + numMostCommonValues + }; + + // Run the analyzeShardKey command and verify that the metrics are as expected. + const res0 = assert.commandWorked(st.s1.adminCommand(analyzeShardKeyCmdObj)); + AnalyzeShardKeyUtil.assertKeyCharacteristicsMetrics(res0, expectedMetrics); + + // Make shard1 the primary shard instead by running the movePrimary command against mongos0. + assert.commandWorked(st.s0.adminCommand({movePrimary: dbName, to: st.shard1.name})); + + // Rerun the analyzeShardKey command against mongos1. Since it does not know that the primary + // shard has changed, it would forward the analyzeShardKey command to shard0. Without database + // versioning, no StaleDbVersion error would be thrown and so the analyzeShardKey command would + // run on shard0 instead of on shard1. As a result, the command would fail with a + // NamespaceNotFound error. + const res1 = assert.commandWorked(st.s1.adminCommand(analyzeShardKeyCmdObj)); + AnalyzeShardKeyUtil.assertKeyCharacteristicsMetrics(res1, expectedMetrics); + + // Shard the collection and make it have two chunks: + // shard0: [MinKey, 0] + // shard1: [0, MaxKey] + // Again, run all the commands against mongos0. + assert.commandWorked(st.s0.adminCommand({shardCollection: ns, key: {x: 1}})); + assert.commandWorked(st.s0.adminCommand({split: ns, middle: {x: 0}})); + assert.commandWorked(st.s0.adminCommand( + {moveChunk: ns, find: {x: 1}, to: st.shard0.shardName, _waitForDelete: true})); + + // Rerun the analyzeShardKey command against mongos1. Since it does not know that a migration + // occurred, it would only forward the analyzeShardKey command to shard1 only. Without shard + // versioning, no StaleConfig error would be thrown and so the analyzeShardKey command would run + // only on shard1 instead of on both shard0 and shard1. As a result, the metrics would be + // incorrect. + const res2 = assert.commandWorked(st.s1.adminCommand(analyzeShardKeyCmdObj)); + AnalyzeShardKeyUtil.assertKeyCharacteristicsMetrics(res2, expectedMetrics); +} + +runTest({mode: "primary"}); +runTest({mode: "secondary"}); + +st.stop(); +})(); diff --git a/jstests/sharding/analyze_shard_key/configure_query_analyzer_database_versioning.js b/jstests/sharding/analyze_shard_key/configure_query_analyzer_database_versioning.js new file mode 100644 index 00000000000..f8f0b550e10 --- /dev/null +++ b/jstests/sharding/analyze_shard_key/configure_query_analyzer_database_versioning.js @@ -0,0 +1,47 @@ +/** + * Tests that the configureQueryAnalyzer command uses database versioning when running the + * listCollections command to validate the collection options. + * + * @tags: [requires_fcv_70, featureFlagAnalyzeShardKey] + */ +(function() { +"use strict"; + +load("jstests/sharding/analyze_shard_key/libs/analyze_shard_key_util.js"); + +const st = new ShardingTest({mongos: 1, shards: 2, rs: {nodes: 2}}); + +const dbName = "testDb"; +const collName = "testColl"; +const ns = dbName + "." + collName; + +// Make shard0 the primary shard. +assert.commandWorked(st.s.adminCommand({enableSharding: dbName})); +st.ensurePrimaryShard(dbName, st.shard0.name); + +const mongos0Coll = st.s.getCollection(ns); +assert.commandWorked(mongos0Coll.createIndex({x: 1})); +assert.commandWorked(mongos0Coll.insert([{x: -1}, {x: 1}])); + +const configureCmdObj = { + configureQueryAnalyzer: ns, + mode: "full", + sampleRate: 100 +}; + +// Run the configureQueryAnalyzer command. +assert.commandWorked(st.s.adminCommand(configureCmdObj)); + +// Make shard1 the primary shard instead by running the movePrimary command. +assert.commandWorked(st.s.adminCommand({movePrimary: dbName, to: st.shard1.name})); + +// Rerun the configureQueryAnalyzer command. Since the config server does not know that the primary +// shard has changed, it would send the listCollections command to shard0. Without database +// versioning, the listCollections command would run on shard0 and the configureQueryAnalyzer +// command would fail with a NamespaceNotFound error. With database versioning but without retry +// logic, the listCollections command and the configureQueryAnalyzer command would with a +// StaleDbVersion error. +assert.commandWorked(st.s.adminCommand(configureCmdObj)); + +st.stop(); +})(); diff --git a/jstests/sharding/analyze_shard_key/most_common_values.js b/jstests/sharding/analyze_shard_key/most_common_values.js index 2a2056ee2e5..9663da1d008 100644 --- a/jstests/sharding/analyze_shard_key/most_common_values.js +++ b/jstests/sharding/analyze_shard_key/most_common_values.js @@ -183,9 +183,9 @@ function runTest(conn, {isUnique, isShardedColl, st}) { { const st = new ShardingTest({ - shards: numNodesPerRS, + shards: 2, rs: { - nodes: 2, + nodes: numNodesPerRS, setParameter: { "failpoint.analyzeShardKeySkipCalcalutingReadWriteDistributionMetrics": tojson({mode: "alwaysOn"}), diff --git a/jstests/sharding/safe_secondary_reads_drop_recreate.js b/jstests/sharding/safe_secondary_reads_drop_recreate.js index d4ccc36273b..a1a390538a5 100644 --- a/jstests/sharding/safe_secondary_reads_drop_recreate.js +++ b/jstests/sharding/safe_secondary_reads_drop_recreate.js @@ -36,6 +36,7 @@ let nss = db + "." + coll; let validateTestCase = function(test) { assert(test.setUp && typeof (test.setUp) === "function"); assert(test.command && typeof (test.command) === "object"); + assert(test.runsAgainstAdminDb ? typeof (test.runsAgainstAdminDb) === "boolean" : true); assert(test.checkResults && typeof (test.checkResults) === "function"); assert(test.behavior === "unshardedOnly" || test.behavior === "versioned"); }; @@ -106,7 +107,19 @@ let testCases = { behavior: "versioned" }, analyze: {skip: "primary only"}, - analyzeShardKey: {skip: "does not return user data"}, + analyzeShardKey: { + setUp: function(mongosConn) { + assert.commandWorked(mongosConn.getCollection(nss).insert({x: 1})); + }, + command: {analyzeShardKey: nss, key: {x: 1}}, + runsAgainstAdminDb: true, + checkResults: function(res) { + // Cannot analyze a shard key for an empty collection (the collection has just been + // dropped and recreated). + assert.commandFailedWithCode(res, ErrorCodes.IllegalOperation); + }, + behavior: "versioned" + }, appendOplogNote: {skip: "primary only"}, applyOps: {skip: "primary only"}, authSchemaUpgrade: {skip: "primary only"}, @@ -392,8 +405,9 @@ let scenarios = { st.rs0.getPrimary().getDB('admin').runCommand({_flushRoutingTableCacheUpdates: nss})); st.rs0.awaitReplication(); - let res = staleMongos.getDB(db).runCommand( - Object.assign({}, + let res = staleMongos.getDB(test.runsAgainstAdminDb ? "admin" : db) + .runCommand(Object.assign( + {}, test.command, {$readPreference: {mode: 'secondary'}, readConcern: {'level': 'local'}})); @@ -449,8 +463,9 @@ let scenarios = { st.rs0.getPrimary().getDB('admin').runCommand({_flushRoutingTableCacheUpdates: nss})); st.rs0.awaitReplication(); - let res = staleMongos.getDB(db).runCommand( - Object.assign({}, + let res = staleMongos.getDB(test.runsAgainstAdminDb ? "admin" : db) + .runCommand(Object.assign( + {}, test.command, {$readPreference: {mode: 'secondary'}, readConcern: {'level': 'local'}})); @@ -518,8 +533,9 @@ let scenarios = { writeConcern: {w: 2}, })); - let res = staleMongos.getDB(db).runCommand( - Object.assign({}, + let res = staleMongos.getDB(test.runsAgainstAdminDb ? "admin" : db) + .runCommand(Object.assign( + {}, test.command, {$readPreference: {mode: 'secondary'}, readConcern: {'level': 'local'}})); diff --git a/jstests/sharding/safe_secondary_reads_single_migration_suspend_range_deletion.js b/jstests/sharding/safe_secondary_reads_single_migration_suspend_range_deletion.js index 987237579bd..5df44ea6c4a 100644 --- a/jstests/sharding/safe_secondary_reads_single_migration_suspend_range_deletion.js +++ b/jstests/sharding/safe_secondary_reads_single_migration_suspend_range_deletion.js @@ -122,7 +122,7 @@ let testCases = { behavior: "versioned" }, analyze: {skip: "primary only"}, - analyzeShardKey: {skip: "does not return user data"}, + analyzeShardKey: {skip: "only support readConcern 'local'"}, appendOplogNote: {skip: "primary only"}, applyOps: {skip: "primary only"}, authSchemaUpgrade: {skip: "primary only"}, diff --git a/jstests/sharding/safe_secondary_reads_single_migration_waitForDelete.js b/jstests/sharding/safe_secondary_reads_single_migration_waitForDelete.js index 70290373a5c..33393b14cfa 100644 --- a/jstests/sharding/safe_secondary_reads_single_migration_waitForDelete.js +++ b/jstests/sharding/safe_secondary_reads_single_migration_waitForDelete.js @@ -31,6 +31,7 @@ let nss = db + "." + coll; let validateTestCase = function(test) { assert(test.setUp && typeof (test.setUp) === "function"); assert(test.command && typeof (test.command) === "object"); + assert(test.runsAgainstAdminDb ? typeof (test.runsAgainstAdminDb) === "boolean" : true); assert(test.checkResults && typeof (test.checkResults) === "function"); assert(test.behavior === "unshardedOnly" || test.behavior === "targetsPrimaryUsesConnectionVersioning" || @@ -109,7 +110,23 @@ let testCases = { behavior: "versioned" }, analyze: {skip: "primary only"}, - analyzeShardKey: {skip: "does not return user data"}, + analyzeShardKey: { + setUp: function(mongosConn) { + const docs = []; + for (let i = 1; i <= 1000; i++) { + docs.push({x: i}); + } + assert.commandWorked(mongosConn.getCollection(nss).insert(docs)); + }, + command: {analyzeShardKey: nss, key: {x: 1}}, + runsAgainstAdminDb: true, + checkResults: function(res) { + // The command should work and return correct results. + assert.commandWorked(res); + assert.eq(res.numDocs, 1000, res); + }, + behavior: "versioned" + }, appendOplogNote: {skip: "primary only"}, applyOps: {skip: "primary only"}, authenticate: {skip: "does not return user data"}, @@ -455,9 +472,10 @@ for (let command of commands) { writeConcern: {w: 2}, })); - let res = staleMongos.getDB(db).runCommand(Object.extend( - test.command, {$readPreference: {mode: 'secondary'}, readConcern: {'level': 'local'}})); - + let res = staleMongos.getDB(test.runsAgainstAdminDb ? "admin" : db) + .runCommand(Object.extend( + test.command, + {$readPreference: {mode: 'secondary'}, readConcern: {'level': 'local'}})); test.checkResults(res); // Build the query to identify the operation in the system profiler. diff --git a/src/mongo/s/analyze_shard_key_util.cpp b/src/mongo/s/analyze_shard_key_util.cpp index 21f6297de08..0b9c4fabf98 100644 --- a/src/mongo/s/analyze_shard_key_util.cpp +++ b/src/mongo/s/analyze_shard_key_util.cpp @@ -41,6 +41,7 @@ #include "mongo/s/cluster_commands_helpers.h" #include "mongo/s/configure_query_analyzer_cmd_gen.h" #include "mongo/s/grid.h" +#include "mongo/s/stale_shard_version_helpers.h" #define MONGO_LOGV2_DEFAULT_COMPONENT ::mongo::logv2::LogComponent::kSharding @@ -198,52 +199,65 @@ StatusWith validateCollectionOptionsLocal(OperationContext* opCtx, */ StatusWith validateCollectionOptionsOnPrimaryShard(OperationContext* opCtx, const NamespaceString& nss) { - auto dbInfo = uassertStatusOK(Grid::get(opCtx)->catalogCache()->getDatabase(opCtx, nss.db())); - ListCollections listCollections; listCollections.setDbName(nss.db()); listCollections.setFilter(BSON("name" << nss.coll())); - auto cmdResponse = executeCommandAgainstDatabasePrimary( + auto listCollectionsCmdObj = + CommandHelpers::filterCommandRequestForPassthrough(listCollections.toBSON({})); + + auto catalogCache = Grid::get(opCtx)->catalogCache(); + return shardVersionRetry( opCtx, - nss.db(), - dbInfo, - CommandHelpers::filterCommandRequestForPassthrough(listCollections.toBSON({})), - ReadPreferenceSetting{ReadPreference::PrimaryOnly}, - Shard::RetryPolicy::kIdempotent); - auto remoteResponse = uassertStatusOK(cmdResponse.swResponse); - uassertStatusOK(getStatusFromCommandResult(remoteResponse.data)); - - auto cursorResponse = uassertStatusOK(CursorResponse::parseFromBSON(remoteResponse.data)); - auto firstBatch = cursorResponse.getBatch(); - - if (firstBatch.empty()) { - return Status{ErrorCodes::NamespaceNotFound, - str::stream() << "The namespace does not exist"}; - } - uassert(6915300, - str::stream() << "The namespace corresponds to multiple collections", - firstBatch.size() == 1); + catalogCache, + nss, + "validateCollectionOptionsOnPrimaryShard"_sd, + [&]() -> StatusWith { + auto dbInfo = uassertStatusOK(catalogCache->getDatabaseWithRefresh(opCtx, nss.db())); + auto cmdResponse = executeCommandAgainstDatabasePrimary( + opCtx, + nss.db(), + dbInfo, + listCollectionsCmdObj, + ReadPreferenceSetting{ReadPreference::PrimaryOnly}, + Shard::RetryPolicy::kIdempotent); + auto remoteResponse = uassertStatusOK(cmdResponse.swResponse); + uassertStatusOK(getStatusFromCommandResult(remoteResponse.data)); + + auto cursorResponse = + uassertStatusOK(CursorResponse::parseFromBSON(remoteResponse.data)); + auto firstBatch = cursorResponse.getBatch(); + + if (firstBatch.empty()) { + return Status{ErrorCodes::NamespaceNotFound, + str::stream() << "The namespace does not exist"}; + } + uassert(6915300, + str::stream() << "The namespace corresponds to multiple collections", + firstBatch.size() == 1); - auto listCollRepItem = ListCollectionsReplyItem::parse( - IDLParserContext("ListCollectionsReplyItem"), firstBatch[0]); + auto listCollRepItem = ListCollectionsReplyItem::parse( + IDLParserContext("ListCollectionsReplyItem"), firstBatch[0]); - if (listCollRepItem.getType() == "view") { - return Status{ErrorCodes::CommandNotSupportedOnView, "The namespace corresponds to a view"}; - } - if (auto obj = listCollRepItem.getOptions()) { - auto options = uassertStatusOK(CollectionOptions::parse(*obj)); - if (options.encryptedFieldConfig.has_value()) { - return Status{ErrorCodes::IllegalOperation, - str::stream() << "The collection has queryable encryption enabled"}; - } - } + if (listCollRepItem.getType() == "view") { + return Status{ErrorCodes::CommandNotSupportedOnView, + "The namespace corresponds to a view"}; + } + if (auto obj = listCollRepItem.getOptions()) { + auto options = uassertStatusOK(CollectionOptions::parse(*obj)); + if (options.encryptedFieldConfig.has_value()) { + return Status{ErrorCodes::IllegalOperation, + str::stream() + << "The collection has queryable encryption enabled"}; + } + } - auto info = listCollRepItem.getInfo(); - uassert(6915301, - str::stream() << "The listCollections reply for '" << nss - << "' does not have the 'info' field", - info); - return *info->getUuid(); + auto info = listCollRepItem.getInfo(); + uassert(6915301, + str::stream() << "The listCollections reply for '" << nss + << "' does not have the 'info' field", + info); + return *info->getUuid(); + }); } } // namespace diff --git a/src/mongo/s/commands/cluster_analyze_shard_key_cmd.cpp b/src/mongo/s/commands/cluster_analyze_shard_key_cmd.cpp index 8047fead257..b3d04bfb078 100644 --- a/src/mongo/s/commands/cluster_analyze_shard_key_cmd.cpp +++ b/src/mongo/s/commands/cluster_analyze_shard_key_cmd.cpp @@ -86,8 +86,15 @@ public: } PseudoRandom random{SecureRandom{}.nextInt64()}; - const auto unversionedCmdObj = - CommandHelpers::filterCommandRequestForPassthrough(request().toBSON({})); + + // On secondaries, the database and shard version check is only performed for commands + // that specify a readConcern (that is not "available"). So to opt into the check, + // explicitly set the readConcern to "local". + const auto readConcern = + repl::ReadConcernArgs(repl::ReadConcernLevel::kLocalReadConcern); + const auto unversionedCmdObj = CommandHelpers::filterCommandRequestForPassthrough( + request().toBSON(readConcern.toBSON())); + while (true) { // Select a random shard. invariant(!candidateShardIds.empty()); -- cgit v1.2.1