SERVER-74416 Make sure analyzeShardKey and configureQueryAnalyzer command use database and shard versioning

8 files changed, 258 insertions, 55 deletions

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<UUID> validateCollectionOptionsLocal(OperationContext* opCtx,
  */
 StatusWith<UUID> 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<UUID> {
+            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());