SERVER-47745 Make chunk query in ShardingCatalogManager compatible with chunks created in 3.4

(cherry picked from commit 55ea26e1ad7c01038b73551ba483194967567311)

6 files changed, 227 insertions, 10 deletions

diff --git a/jstests/multiVersion/chunk_operations_UUID_shard_key.js b/jstests/multiVersion/chunk_operations_UUID_shard_key.js
new file mode 100644
index 00000000000..1aca8a9e9a9
--- /dev/null
+++ b/jstests/multiVersion/chunk_operations_UUID_shard_key.js
@@ -0,0 +1,128 @@
+/*
+ * The _id of a chunk is the concatenation of its namespace and minbound. The string
+ * representation of the UUID type was changed from "BinData(...)" in 3.4 to "UUID(...)"
+ * in 3.6. This test is to verify that chunks for a sharded collection with a UUID shard key
+ * created in 3.4 and 3.6 can still be moved, split and merged after the cluster is
+ * upgraded to 4.2.
+ */
+(function() {
+    "use strict";
+
+    load('jstests/multiVersion/libs/multi_cluster.js');
+
+    // Checking UUID consistency uses cached connections, which are not valid across restarts.
+    TestData.skipCheckingUUIDsConsistentAcrossCluster = true;
+
+    /*
+     * Creates a sharded collection with collection name 'collName', database name 'dbName', and
+     * shard key 'shardKey'. Inserts 'numDocs' docs with a UUID field into the collection, and
+     * splits the initial chunk into two.
+     */
+    function setUpChunks(dbName, collName, shardKey, numDocs) {
+        const ns = dbName + "." + collName;
+
+        jsTest.log(`Set up sharded collection ${ns} with UUID shard key`);
+        assert.commandWorked(st.s.adminCommand({shardCollection: ns, key: shardKey}));
+
+        jsTest.log(`Insert docs for ${ns}`);
+        // This is necessary as splitFind does not work for empty chunks.
+        let bulk = st.s.getCollection(ns).initializeUnorderedBulkOp();
+        for (let i = 0; i < numDocs; ++i) {
+            bulk.insert({x: UUID()});
+        }
+        assert.commandWorked(bulk.execute());
+
+        jsTest.log(`Split the initial chunk for ${ns}`);
+        assert.commandWorked(st.splitFind(ns, {x: MinKey}));
+        assert.eq(2, st.s.getDB("config").chunks.count({ns: ns}));
+
+        return ns;
+    }
+
+    /*
+     * Upgrades the entire cluster to the given binVersion and waits for config server and shards
+     * to become available and for the replica set monitors on the mongos and each shard to reflect
+     * the state of all shards. Then, runs setFCV to the given FCV version.
+     */
+    function upgradeCluster(binVersion, featureCompatibilityVersion) {
+        st.stopBalancer();
+        st.upgradeCluster(binVersion, {
+            upgradeMongos: true,
+            upgradeConfigs: true,
+            upgradeShards: true,
+            waitUntilStable: true
+        });
+        st.s.adminCommand({setFeatureCompatibilityVersion: featureCompatibilityVersion});
+    }
+
+    /*
+     * Selects the chunk with non-MinKey min to do operations on. Assumes that all chunks are on
+     * shard0 since it is the primary shard. Runs moveChunk, splitChunk and mergeChunk and asserts
+     * the commands work.
+     */
+    function testChunkOperations(ns) {
+        // Make sure there is at least one chunk whose min is not MinKey.
+        const numOriginalChunks = st.s.getDB("config").chunks.count({ns: ns});
+        assert.gt(numOriginalChunks, 1);
+
+        // Select the chunk with max of MaxKey.
+        const chunkToMove = st.s.getDB("config").chunks.findOne(
+            {ns: ns, shard: st.shard0.shardName, min: {$ne: {x: MinKey}}, max: {x: MaxKey}});
+        jsTest.log("chunkToMove " + tojson(chunkToMove));
+
+        jsTest.log("Move the chunk");
+        assert.commandWorked(
+            st.s.adminCommand({moveChunk: ns, find: chunkToMove.min, to: st.shard1.shardName}));
+
+        jsTest.log("Split the chunk");
+        assert.commandWorked(st.splitFind(ns, chunkToMove.min));
+        assert.eq(numOriginalChunks + 1, st.s.getDB("config").chunks.count({ns: ns}));
+
+        jsTest.log("Merge the resulting chunks");
+        assert.commandWorked(
+            st.s.adminCommand({mergeChunks: ns, bounds: [chunkToMove.min, chunkToMove.max]}));
+        assert.eq(numOriginalChunks, st.s.getDB("config").chunks.count({ns: ns}));
+    }
+
+    jsTest.log("Start a \"3.4\" sharded cluster");
+
+    const st = new ShardingTest({
+        shards: 2,
+        mongos: 1,
+        config: 1,
+        other: {
+            mongosOptions: {binVersion: "3.4"},
+            configOptions: {binVersion: "3.4"},
+            shardOptions: {binVersion: "3.4"},
+            rsOptions: {binVersion: "3.4"},
+            rs: true,
+        }
+    });
+    const kDbName = "foo";
+    const kCollName = "bar";
+    const kShardKey = {x: 1};
+    const kNumDocs = 100;
+
+    assert.commandWorked(st.s.adminCommand({enableSharding: kDbName}));
+    st.ensurePrimaryShard(kDbName, st.shard0.shardName);
+    const ns34 = setUpChunks(kDbName, kCollName + "-34", kShardKey, kNumDocs);
+
+    jsTest.log("Upgrade the cluster to 3.6");
+    upgradeCluster("last-stable", "3.6");
+    const ns36 = setUpChunks(kDbName, kCollName + "-36", kShardKey, kNumDocs);
+
+    jsTest.log("Upgrade the cluster to 4.0");
+    upgradeCluster("latest", "4.0");
+    const ns40 = setUpChunks(kDbName, kCollName + "-40", kShardKey, kNumDocs);
+
+    jsTest.log("Perform operations on chunks created when the cluster was in 3.4");
+    testChunkOperations(ns34);
+
+    jsTest.log("Perform operations on chunks created when the cluster is in 3.6");
+    testChunkOperations(ns36);
+
+    jsTest.log("Perform operations on chunks created when the cluster was in 4.0");
+    testChunkOperations(ns40);
+
+    st.stop();
+})();
diff --git a/jstests/multiVersion/libs/multi_cluster.js b/jstests/multiVersion/libs/multi_cluster.js
index 2937d0d4d6d..0cea472ea50 100644
--- a/jstests/multiVersion/libs/multi_cluster.js
+++ b/jstests/multiVersion/libs/multi_cluster.js
@@ -2,6 +2,9 @@
 // MultiVersion utility functions for clusters
 //
 
+load('jstests/multiVersion/libs/multi_rs.js');  // For upgradeSet.
+load('jstests/replsets/rslib.js');              // For awaitRSClientHosts.
+
 /**
  * Restarts the specified binaries in options with the specified binVersion.
  * Note: this does not perform any upgrade operations.
@@ -23,6 +26,8 @@ ShardingTest.prototype.upgradeCluster = function(binVersion, options) {
         options.upgradeConfigs = true;
     if (options.upgradeMongos == undefined)
         options.upgradeMongos = true;
+    if (options.waitUntilStable == undefined)
+        options.waitUntilStable = false;
 
     var upgradedSingleShards = [];
 
@@ -86,6 +91,22 @@ ShardingTest.prototype.upgradeCluster = function(binVersion, options) {
         this.config = this.s.getDB("config");
         this.admin = this.s.getDB("admin");
     }
+
+    if (options.waitUntilStable) {
+        // Wait for the config server and shards to become available.
+        this.configRS.awaitSecondaryNodes();
+        let shardPrimaries = [];
+        for (let rs of this._rs) {
+            rs.test.awaitSecondaryNodes();
+            shardPrimaries.push(rs.test.getPrimary());
+        }
+
+        // Wait for the ReplicaSetMonitor on mongoS and each shard to reflect the state of all
+        // shards.
+        for (let client of[...this._mongos, ...shardPrimaries]) {
+            awaitRSClientHosts(client, shardPrimaries, {ok: true, ismaster: true});
+        }
+    }
 };
 
 ShardingTest.prototype.restartMongoses = function() {
diff --git a/src/mongo/db/s/config/sharding_catalog_manager_chunk_operations.cpp b/src/mongo/db/s/config/sharding_catalog_manager_chunk_operations.cpp
index a7e0be3a172..3133094adb4 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager_chunk_operations.cpp
+++ b/src/mongo/db/s/config/sharding_catalog_manager_chunk_operations.cpp
@@ -196,7 +196,7 @@ BSONObj makeCommitChunkTransactionCommand(const NamespaceString& nss,
         op.append("ns", ChunkType::ConfigNS.ns());
 
         BSONObjBuilder n(op.subobjStart("o"));
-        n.append(ChunkType::name(), ChunkType::genID(nss, migratedChunk.getMin()));
+        n.append(ChunkType::name(), migratedChunk.getName());
         migratedChunk.getVersion().appendLegacyWithField(&n, ChunkType::lastmod());
         n.append(ChunkType::ns(), nss.ns());
         n.append(ChunkType::min(), migratedChunk.getMin());
@@ -206,7 +206,7 @@ BSONObj makeCommitChunkTransactionCommand(const NamespaceString& nss,
         n.done();
 
         BSONObjBuilder q(op.subobjStart("o2"));
-        q.append(ChunkType::name(), ChunkType::genID(nss, migratedChunk.getMin()));
+        q.append(ChunkType::name(), migratedChunk.getName());
         q.done();
 
         updates.append(op.obj());
@@ -220,7 +220,7 @@ BSONObj makeCommitChunkTransactionCommand(const NamespaceString& nss,
         op.append("ns", ChunkType::ConfigNS.ns());
 
         BSONObjBuilder n(op.subobjStart("o"));
-        n.append(ChunkType::name(), ChunkType::genID(nss, controlChunk->getMin()));
+        n.append(ChunkType::name(), controlChunk->getName());
         controlChunk->getVersion().appendLegacyWithField(&n, ChunkType::lastmod());
         n.append(ChunkType::ns(), nss.ns());
         n.append(ChunkType::min(), controlChunk->getMin());
@@ -231,7 +231,7 @@ BSONObj makeCommitChunkTransactionCommand(const NamespaceString& nss,
         n.done();
 
         BSONObjBuilder q(op.subobjStart("o2"));
-        q.append(ChunkType::name(), ChunkType::genID(nss, controlChunk->getMin()));
+        q.append(ChunkType::name(), controlChunk->getName());
         q.done();
 
         updates.append(op.obj());
@@ -376,6 +376,9 @@ Status ShardingCatalogManager::commitChunkSplit(OperationContext* opCtx,
     auto newChunkBounds(splitPoints);
     newChunkBounds.push_back(range.getMax());
 
+    auto shouldTakeOriginalChunkID = true;
+    std::string chunkID;
+
     BSONArrayBuilder updates;
 
     for (const auto& endKey : newChunkBounds) {
@@ -421,6 +424,13 @@ Status ShardingCatalogManager::commitChunkSplit(OperationContext* opCtx,
         // splits only update the 'minor' portion of version
         currentMaxVersion.incMinor();
 
+        // First chunk takes ID of the original chunk and all other chunks get new IDs. This occurs
+        // because we perform an update operation below (with upsert true). Keeping the original ID
+        // ensures we overwrite the old chunk (before the split) without having to perform a delete.
+        chunkID = shouldTakeOriginalChunkID ? origChunk.getValue().getName()
+                                            : ChunkType::genID(nss, startKey);
+        shouldTakeOriginalChunkID = false;
+
         // build an update operation against the chunks collection of the config database
         // with upsert true
         BSONObjBuilder op;
@@ -430,7 +440,7 @@ Status ShardingCatalogManager::commitChunkSplit(OperationContext* opCtx,
 
         // add the modified (new) chunk information as the update object
         BSONObjBuilder n(op.subobjStart("o"));
-        n.append(ChunkType::name(), ChunkType::genID(nss, startKey));
+        n.append(ChunkType::name(), chunkID);
         currentMaxVersion.appendLegacyWithField(&n, ChunkType::lastmod());
         n.append(ChunkType::ns(), nss.ns());
         n.append(ChunkType::min(), startKey);
@@ -447,7 +457,7 @@ Status ShardingCatalogManager::commitChunkSplit(OperationContext* opCtx,
 
         // add the chunk's _id as the query part of the update statement
         BSONObjBuilder q(op.subobjStart("o2"));
-        q.append(ChunkType::name(), ChunkType::genID(nss, startKey));
+        q.append(ChunkType::name(), chunkID);
         q.done();
 
         updates.append(op.obj());
@@ -593,6 +603,17 @@ Status ShardingCatalogManager::commitChunkMerge(OperationContext* opCtx,
     for (size_t i = 1; i < chunkBoundaries.size(); ++i) {
         itChunk.setMin(itChunk.getMax());
 
+        // Read the original chunk from disk to lookup that chunk's _id. Propagate the _id
+        // because in clusters where the shard key is a UUID and the chunk was last written
+        // in v3.4, the chunk entry's _id will be in the "BinData()" form, and calling
+        // ChunkType::genID would try to do the update with the _id in the "UUID()" form
+        // (because UUID::toString changed in v3.6), which wouldn't match any documents.
+        auto itOrigChunk = _findChunkOnConfig(opCtx, nss, itChunk.getMin());
+        if (!itOrigChunk.isOK()) {
+            return itOrigChunk.getStatus();
+        }
+        itChunk.setName(itOrigChunk.getValue().getName());
+
         // Ensure the chunk boundaries are strictly increasing
         if (chunkBoundaries[i].woCompare(itChunk.getMin()) <= 0) {
             return {
@@ -748,6 +769,13 @@ StatusWith<BSONObj> ShardingCatalogManager::commitChunkMigration(
     newMigratedChunk.setVersion(ChunkVersion(
         currentCollectionVersion.majorVersion() + 1, 0, currentCollectionVersion.epoch()));
 
+    // Propagate the _id from the original chunk because in clusters where the
+    // shard key is a UUID and the chunk was last written in v3.4, the chunk entry's
+    // _id will be in the "BinData()" form, and calling ChunkType::genID would try to
+    // do the update with the _id in the "UUID()" form (because UUID::toString changed
+    // in v3.6), which wouldn't match any documents.
+    newMigratedChunk.setName(origChunk.getValue().getName());
+
     // Copy the complete history.
     auto newHistory = origChunk.getValue().getHistory();
     const int kHistorySecs = 10;
@@ -800,6 +828,13 @@ StatusWith<BSONObj> ShardingCatalogManager::commitChunkMigration(
             // FCV 3.6 does not have the history field in the persisted metadata
             newControlChunk->setHistory({});
         }
+
+        // Propagate the _id from the original chunk because in clusters where the
+        // shard key is a UUID and the chunk was last written in v3.4, the chunk entry's
+        // _id will be in the "BinData()" form, and calling ChunkType::genID would try to
+        // do the update with the _id in the "UUID()" form (because UUID::toString changed
+        // in v3.6), which wouldn't match any documents.
+        newControlChunk->setName(origControlChunk.getValue().getName());
     }
 
     auto command = makeCommitChunkTransactionCommand(
@@ -840,7 +875,7 @@ StatusWith<ChunkType> ShardingCatalogManager::_findChunkOnConfig(OperationContex
                                             ReadPreferenceSetting{ReadPreference::PrimaryOnly},
                                             repl::ReadConcernLevel::kLocalReadConcern,
                                             ChunkType::ConfigNS,
-                                            BSON(ChunkType::name << ChunkType::genID(nss, key)),
+                                            BSON(ChunkType::ns(nss.ns()) << ChunkType::min(key)),
                                             BSONObj(),
                                             1);
 
@@ -851,7 +886,9 @@ StatusWith<ChunkType> ShardingCatalogManager::_findChunkOnConfig(OperationContex
     const auto origChunks = std::move(findResponse.getValue().docs);
     if (origChunks.size() != 1) {
         return {ErrorCodes::IncompatibleShardingMetadata,
-                str::stream() << "Tried to find the chunk for '" << ChunkType::genID(nss, key)
+                str::stream() << "Tried to find the chunk for namespace " << nss.ns()
+                              << " and min key "
+                              << key.toString()
                               << ", but found no chunks"};
     }
 
diff --git a/src/mongo/db/s/config/sharding_catalog_manager_merge_chunks_test.cpp b/src/mongo/db/s/config/sharding_catalog_manager_merge_chunks_test.cpp
index 08dc37fccfa..2484319ff5e 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager_merge_chunks_test.cpp
+++ b/src/mongo/db/s/config/sharding_catalog_manager_merge_chunks_test.cpp
@@ -401,7 +401,7 @@ TEST_F(MergeChunkTest, NonMatchingEpochsOfChunkAndRequestErrors) {
     ASSERT_EQ(ErrorCodes::StaleEpoch, mergeStatus);
 }
 
-TEST_F(MergeChunkTest, MergeAlreadyHappenedFailsPrecondition) {
+TEST_F(MergeChunkTest, MergeAlreadyHappenedFails) {
     ChunkType chunk;
     chunk.setNS(kNamespace);
 
@@ -434,7 +434,7 @@ TEST_F(MergeChunkTest, MergeAlreadyHappenedFailsPrecondition) {
 
     Timestamp validAfter{1};
 
-    ASSERT_EQ(ErrorCodes::BadValue,
+    ASSERT_EQ(ErrorCodes::IncompatibleShardingMetadata,
               ShardingCatalogManager::get(operationContext())
                   ->commitChunkMerge(operationContext(),
                                      kNamespace,
diff --git a/src/mongo/s/catalog/type_chunk.cpp b/src/mongo/s/catalog/type_chunk.cpp
index 4965975b8a0..3417050b814 100644
--- a/src/mongo/s/catalog/type_chunk.cpp
+++ b/src/mongo/s/catalog/type_chunk.cpp
@@ -210,6 +210,19 @@ StatusWith<ChunkType> ChunkType::fromConfigBSON(const BSONObj& source) {
     ChunkType chunk;
 
     {
+        std::string chunkID;
+        Status status = bsonExtractStringField(source, name.name(), &chunkID);
+        if (status.isOK()) {
+            chunk._id = chunkID;
+        } else if (status == ErrorCodes::NoSuchKey) {
+            // Ignore NoSuchKey because when chunks are sent in commands they are not required to
+            // include the _id.
+        } else {
+            return status;
+        }
+    }
+
+    {
         std::string chunkNS;
         Status status = bsonExtractStringField(source, ns.name(), &chunkNS);
         if (!status.isOK())
@@ -372,11 +385,20 @@ BSONObj ChunkType::toShardBSON() const {
 }
 
 std::string ChunkType::getName() const {
+    if (_id) {
+        // This chunk already has an id so return it.
+        return *_id;
+    }
+
     invariant(_nss);
     invariant(_min);
     return genID(*_nss, *_min);
 }
 
+void ChunkType::setName(const std::string& id) {
+    _id = id;
+}
+
 void ChunkType::setNS(const NamespaceString& nss) {
     invariant(nss.isValid());
     _nss = nss;
diff --git a/src/mongo/s/catalog/type_chunk.h b/src/mongo/s/catalog/type_chunk.h
index ed15f2ff578..06048736c22 100644
--- a/src/mongo/s/catalog/type_chunk.h
+++ b/src/mongo/s/catalog/type_chunk.h
@@ -219,6 +219,8 @@ public:
 
     std::string getName() const;
 
+    void setName(const std::string& id);
+
     /**
      * Getters and setters.
      */
@@ -290,6 +292,13 @@ public:
 private:
     // Convention: (M)andatory, (O)ptional, (S)pecial; (C)onfig, (S)hard.
 
+    // (O)(C)     _id of the config.chunks doc for this chunk
+    // UUID::toString changed between v3.4 and v3.6, so for collections where the
+    // shard key contains a UUID, getName will produce a different _id string (via
+    // genID) in v3.6 than v3.4. Since _id is immutable, this was added to the
+    // ChunkType API to allow commitChunk{Merge,Migration,Split} to propagate the _id
+    // of a chunk written in v3.4 to the new chunk(s) they write.
+    boost::optional<std::string> _id;
     // (O)(C)     collection this chunk is in
     boost::optional<NamespaceString> _nss;
     // (M)(C)(S)  first key of the range, inclusive