SERVER-45844 UUID shard key values cause failed chunk migrations

6 files changed, 232 insertions, 11 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..1cc3c0df987
--- /dev/null
+++ b/jstests/multiVersion/chunk_operations_UUID_shard_key.js
@@ -0,0 +1,134 @@
+/*
+ * 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 earlier versions 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("3.6", "3.6");
+const ns36 = setUpChunks(kDbName, kCollName + "-36", kShardKey, kNumDocs);
+
+jsTest.log("Upgrade the cluster to 4.0");
+upgradeCluster("last-stable", "4.0");
+const ns40 = setUpChunks(kDbName, kCollName + "-40", kShardKey, kNumDocs);
+
+jsTest.log("Upgrade the cluster to 4.2");
+upgradeCluster("latest", "4.2");
+const ns42 = setUpChunks(kDbName, kCollName + "-42", 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);
+
+jsTest.log("Perform operations on chunks created when the cluster is in 4.2");
+testChunkOperations(ns42);
+
+st.stop();
+})();
diff --git a/jstests/multiVersion/libs/multi_cluster.js b/jstests/multiVersion/libs/multi_cluster.js
index e611e541c3f..c1c8074986a 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 9174a7008ef..7b325b7213e 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
@@ -189,7 +189,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());
@@ -199,7 +199,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());
@@ -213,7 +213,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());
@@ -224,7 +224,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());
@@ -365,6 +365,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) {
@@ -406,6 +409,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;
@@ -415,7 +425,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);
@@ -428,7 +438,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());
@@ -565,6 +575,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 {
@@ -711,6 +732,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;
@@ -752,6 +780,13 @@ StatusWith<BSONObj> ShardingCatalogManager::commitChunkMigration(
         newControlChunk = origControlChunk.getValue();
         newControlChunk->setVersion(ChunkVersion(
             currentCollectionVersion.majorVersion() + 1, 1, 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.
+        newControlChunk->setName(origControlChunk.getValue().getName());
     }
 
     auto command = makeCommitChunkTransactionCommand(
@@ -792,7 +827,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);
 
@@ -803,8 +838,8 @@ 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)
-                              << ", but found no chunks"};
+                str::stream() << "Tried to find the chunk for namespace " << nss.ns()
+                              << " and min key " << key.toString() << ", but found no chunks"};
     }
 
     return ChunkType::fromConfigBSON(origChunks.front());
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 2ba113cc5f0..6a4e0a72062 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
@@ -400,7 +400,7 @@ TEST_F(MergeChunkTest, NonMatchingEpochsOfChunkAndRequestErrors) {
     ASSERT_EQ(ErrorCodes::StaleEpoch, mergeStatus);
 }
 
-TEST_F(MergeChunkTest, MergeAlreadyHappenedFailsPrecondition) {
+TEST_F(MergeChunkTest, MergeAlreadyHappenedFails) {
     ChunkType chunk;
     chunk.setNS(kNamespace);
 
@@ -433,7 +433,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 cf97e57845f..e26d063a939 100644
--- a/src/mongo/s/catalog/type_chunk.cpp
+++ b/src/mongo/s/catalog/type_chunk.cpp
@@ -208,6 +208,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())
@@ -370,11 +383,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 7b2bb2b189b..29b695f710d 100644
--- a/src/mongo/s/catalog/type_chunk.h
+++ b/src/mongo/s/catalog/type_chunk.h
@@ -218,6 +218,8 @@ public:
 
     std::string getName() const;
 
+    void setName(const std::string& id);
+
     /**
      * Getters and setters.
      */
@@ -289,6 +291,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