SERVER-26883 Skip sharding zone if no shards are assigned to it

4 files changed, 92 insertions, 67 deletions

diff --git a/jstests/sharding/tag_range.js b/jstests/sharding/tag_range.js
index 8475d853df5..d4fdbb1e183 100644
--- a/jstests/sharding/tag_range.js
+++ b/jstests/sharding/tag_range.js
@@ -1,76 +1,76 @@
 // tests to make sure that tag ranges are added/removed/updated successfully
+(function() {
+    'use strict';
 
-function countTags(num, message) {
-    assert.eq(s.config.tags.count(), num, message);
-}
+    var st = new ShardingTest({shards: 2, mongos: 1});
 
-var s = new ShardingTest({name: "tag_range", shards: 2, mongos: 1});
+    assert.commandWorked(st.s0.adminCommand({enableSharding: 'test'}));
+    st.ensurePrimaryShard('test', 'shard0001');
+    assert.commandWorked(st.s0.adminCommand({shardCollection: 'test.tag_range', key: {_id: 1}}));
 
-// this set up is not required but prevents warnings in the remove
-db = s.getDB("tag_range");
+    function countTags(num, message) {
+        assert.eq(st.config.tags.count(), num, message);
+    }
 
-s.adminCommand({enableSharding: "test"});
-s.ensurePrimaryShard('test', 'shard0001');
-s.adminCommand({shardCollection: "test.tag_range", key: {_id: 1}});
+    assert.eq(1, st.config.chunks.count());
 
-assert.eq(1, s.config.chunks.count());
+    st.addShardTag('shard0000', 'a');
+    st.addShardTag('shard0000', 'b');
 
-sh.addShardTag("shard0000", "a");
-sh.addShardTag("shard0000", "b");
+    // add two ranges, verify the additions
 
-// add two ranges, verify the additions
+    st.addTagRange('test.tag_range', {_id: 5}, {_id: 10}, 'a');
+    st.addTagRange('test.tag_range', {_id: 10}, {_id: 15}, 'b');
 
-sh.addTagRange("test.tag_range", {_id: 5}, {_id: 10}, "a");
-sh.addTagRange("test.tag_range", {_id: 10}, {_id: 15}, "b");
+    countTags(2, 'tag ranges were not successfully added');
 
-countTags(2, "tag ranges were not successfully added");
+    // remove the second range, should be left with one
 
-// remove the second range, should be left with one
+    st.removeTagRange('test.tag_range', {_id: 10}, {_id: 15}, 'b');
 
-sh.removeTagRange("test.tag_range", {_id: 10}, {_id: 15}, "b");
+    countTags(1, 'tag range not removed successfully');
 
-countTags(1, "tag range not removed successfully");
+    // add range min=max, verify the additions
 
-// add range min=max, verify the additions
+    try {
+        st.addTagRange('test.tag_range', {_id: 20}, {_id: 20}, 'a');
+    } catch (e) {
+        countTags(1, 'tag range should not have been added');
+    }
 
-try {
-    sh.addTagRange("test.tag_range", {_id: 20}, {_id: 20}, "a");
-} catch (e) {
-    countTags(1, "tag range should not have been added");
-}
+    // removeTagRange tests for tag ranges that do not exist
 
-// removeTagRange tests for tag ranges that do not exist
+    // Bad namespace
+    st.removeTagRange('badns', {_id: 5}, {_id: 11}, 'a');
+    countTags(1, 'Bad namespace: tag range does not exist');
 
-// Bad namespace
-sh.removeTagRange("badns", {_id: 5}, {_id: 11}, "a");
-countTags(1, "Bad namespace: tag range does not exist");
+    // Bad tag
+    st.removeTagRange('test.tag_range', {_id: 5}, {_id: 11}, 'badtag');
+    countTags(1, 'Bad tag: tag range does not exist');
 
-// Bad tag
-sh.removeTagRange("test.tag_range", {_id: 5}, {_id: 11}, "badtag");
-countTags(1, "Bad tag: tag range does not exist");
+    // Bad min
+    st.removeTagRange('test.tag_range', {_id: 0}, {_id: 11}, 'a');
+    countTags(1, 'Bad min: tag range does not exist');
 
-// Bad min
-sh.removeTagRange("test.tag_range", {_id: 0}, {_id: 11}, "a");
-countTags(1, "Bad min: tag range does not exist");
+    // Bad max
+    st.removeTagRange('test.tag_range', {_id: 5}, {_id: 12}, 'a');
+    countTags(1, 'Bad max: tag range does not exist');
 
-// Bad max
-sh.removeTagRange("test.tag_range", {_id: 5}, {_id: 12}, "a");
-countTags(1, "Bad max: tag range does not exist");
+    // Invalid namesapce
+    st.removeTagRange(35, {_id: 5}, {_id: 11}, 'a');
+    countTags(1, 'Invalid namespace: tag range does not exist');
 
-// Invalid namesapce
-sh.removeTagRange(35, {_id: 5}, {_id: 11}, "a");
-countTags(1, "Invalid namespace: tag range does not exist");
+    // Invalid tag
+    st.removeTagRange('test.tag_range', {_id: 5}, {_id: 11}, 35);
+    countTags(1, 'Invalid tag: tag range does not exist');
 
-// Invalid tag
-sh.removeTagRange("test.tag_range", {_id: 5}, {_id: 11}, 35);
-countTags(1, "Invalid tag: tag range does not exist");
+    // Invalid min
+    st.removeTagRange('test.tag_range', 35, {_id: 11}, 'a');
+    countTags(1, 'Invalid min: tag range does not exist');
 
-// Invalid min
-sh.removeTagRange("test.tag_range", 35, {_id: 11}, "a");
-countTags(1, "Invalid min: tag range does not exist");
+    // Invalid max
+    st.removeTagRange('test.tag_range', {_id: 5}, 35, 'a');
+    countTags(1, 'Invalid max: tag range does not exist');
 
-// Invalid max
-sh.removeTagRange("test.tag_range", {_id: 5}, 35, "a");
-countTags(1, "Invalid max: tag range does not exist");
-
-s.stop();
+    st.stop();
+})();
diff --git a/src/mongo/db/s/balancer/balancer_policy.cpp b/src/mongo/db/s/balancer/balancer_policy.cpp
index 87bc5efd223..206a50257a6 100644
--- a/src/mongo/db/s/balancer/balancer_policy.cpp
+++ b/src/mongo/db/s/balancer/balancer_policy.cpp
@@ -233,7 +233,7 @@ Status BalancerPolicy::isShardSuitableReceiver(const ClusterStatistics::ShardSta
 
     if (!chunkTag.empty() && !stat.shardTags.count(chunkTag)) {
         return {ErrorCodes::IllegalOperation,
-                str::stream() << stat.shardId << " doesn't have right tag"};
+                str::stream() << stat.shardId << " is not in the correct zone " << chunkTag};
     }
 
     return Status::OK();
@@ -369,7 +369,7 @@ vector<MigrateInfo> BalancerPolicy::balance(const ShardStatisticsVector& shardSt
                     continue;
 
                 if (chunk.getJumbo()) {
-                    warning() << "chunk " << redact(chunk.toString()) << " violates tag "
+                    warning() << "Chunk " << redact(chunk.toString()) << " violates zone "
                               << redact(tag) << ", but it is jumbo and cannot be moved";
                     continue;
                 }
@@ -378,7 +378,7 @@ vector<MigrateInfo> BalancerPolicy::balance(const ShardStatisticsVector& shardSt
                     _getLeastLoadedReceiverShard(shardStats, distribution, tag, usedShards);
                 if (!to.isValid()) {
                     if (migrations.empty()) {
-                        warning() << "chunk " << redact(chunk.toString()) << " violates tag "
+                        warning() << "Chunk " << redact(chunk.toString()) << " violates zone "
                                   << redact(tag) << ", but no appropriate recipient found";
                     }
                     continue;
@@ -413,6 +413,18 @@ vector<MigrateInfo> BalancerPolicy::balance(const ShardStatisticsVector& shardSt
             }
         }
 
+        // Skip zones which have no shards assigned to them. This situation is not harmful, but
+        // should not be possible so warn the operator to correct it.
+        if (totalNumberOfShardsWithTag == 0) {
+            if (!tag.empty()) {
+                warning() << "Zone " << redact(tag) << " in collection " << distribution.nss()
+                          << " has no assigned shards and chunks which fall into it cannot be "
+                             "balanced. This should be corrected by either assigning shards to the "
+                             "zone or by deleting it.";
+            }
+            continue;
+        }
+
         // Calculate the ceiling of the optimal number of chunks per shard
         const size_t idealNumberOfChunksPerShardForTag =
             (totalNumberOfChunksWithTag / totalNumberOfShardsWithTag) +
@@ -466,7 +478,7 @@ bool BalancerPolicy::_singleZoneBalance(const ShardStatisticsVector& shardStats,
     const ShardId to = _getLeastLoadedReceiverShard(shardStats, distribution, tag, *usedShards);
     if (!to.isValid()) {
         if (migrations->empty()) {
-            log() << "No available shards to take chunks for tag [" << tag << "]";
+            log() << "No available shards to take chunks for zone [" << tag << "]";
         }
         return false;
     }
diff --git a/src/mongo/db/s/balancer/balancer_policy_test.cpp b/src/mongo/db/s/balancer/balancer_policy_test.cpp
index ea1024a00fb..11fe57e133d 100644
--- a/src/mongo/db/s/balancer/balancer_policy_test.cpp
+++ b/src/mongo/db/s/balancer/balancer_policy_test.cpp
@@ -527,6 +527,18 @@ TEST(BalancerPolicy, BalancerFixesIncorrectTagsInOtherwiseBalancedClusterParalle
     ASSERT_BSONOBJ_EQ(cluster.second[kShardId3][0].getMax(), migrations[1].maxKey);
 }
 
+TEST(BalancerPolicy, BalancerHandlesNoShardsWithTag) {
+    auto cluster = generateCluster(
+        {{ShardStatistics(kShardId0, kNoMaxSize, 5, false, emptyTagSet, emptyShardVersion), 2},
+         {ShardStatistics(kShardId1, kNoMaxSize, 5, false, emptyTagSet, emptyShardVersion), 2}});
+
+    DistributionStatus distribution(kNamespace, cluster.second);
+    ASSERT_OK(
+        distribution.addRangeToZone(ZoneRange(kMinBSONKey, BSON("x" << 7), "NonExistentZone")));
+
+    ASSERT(BalancerPolicy::balance(cluster.first, distribution, false).empty());
+}
+
 TEST(DistributionStatus, AddTagRangeOverlap) {
     DistributionStatus d(kNamespace, ShardToChunksMap{});
 
diff --git a/src/mongo/db/s/migration_source_manager.cpp b/src/mongo/db/s/migration_source_manager.cpp
index bbb40821b2e..73b63ef9a2f 100644
--- a/src/mongo/db/s/migration_source_manager.cpp
+++ b/src/mongo/db/s/migration_source_manager.cpp
@@ -92,7 +92,7 @@ MigrationSourceManager::MigrationSourceManager(OperationContext* txn, MoveChunkR
     // this value does not actually contain the shard version, but the global collection version.
     const ChunkVersion expectedCollectionVersion = oss.getShardVersion(getNss());
 
-    log() << "Starting chunk migration for " << _args.toString()
+    log() << "Starting chunk migration " << redact(_args.toString())
           << " with expected collection version " << expectedCollectionVersion;
 
     // Now that the collection is locked, snapshot the metadata and fetch the latest versions
@@ -245,14 +245,14 @@ Status MigrationSourceManager::enterCriticalSection(OperationContext* txn) {
                         << _collectionMetadata->getCollVersion().toString()
                         << ", but found: "
                         << (metadata ? metadata->getCollVersion().toString()
-                                     : "collection unsharded.")};
+                                     : "unsharded collection.")};
         }
 
         // IMPORTANT: After this line, the critical section is in place and needs to be signaled
         _critSecSignal = std::make_shared<Notification<void>>();
     }
 
-    log() << "Successfully entered critical section for " << _args.toString();
+    log() << "Migration successfully entered critical section";
 
     _state = kCriticalSection;
     scopedGuard.Dismiss();
@@ -301,7 +301,7 @@ Status MigrationSourceManager::commitChunkMetadataOnConfig(OperationContext* txn
         invariant(differentChunk.getMin().woCompare(_args.getMinKey()) != 0);
         controlChunkType = std::move(differentChunk);
     } else {
-        log() << "moveChunk moved last chunk out for collection '" << getNss().ns() << "'";
+        log() << "Moving last chunk for the collection out";
     }
 
     BSONObjBuilder builder;
@@ -337,9 +337,9 @@ Status MigrationSourceManager::commitChunkMetadataOnConfig(OperationContext* txn
         // Need to get the latest optime in case the refresh request goes to a secondary --
         // otherwise the read won't wait for the write that _configsvrCommitChunkMigration may have
         // done
-        warning() << "Error occurred during migration metadata commit. Performing a majority write "
-                     "against the config server to get the latest optime"
-                  << causedBy(redact(migrationCommitStatus));
+        log() << "Error occurred while committing the migration. Performing a majority write "
+                 "against the config server to obtain its latest optime"
+              << causedBy(redact(migrationCommitStatus));
 
         Status status = grid.catalogClient(txn)->logChange(
             txn,
@@ -398,18 +398,19 @@ Status MigrationSourceManager::commitChunkMetadataOnConfig(OperationContext* txn
         }
 
         // Migration succeeded
-        log() << "moveChunk for '" << _args.toString() << "' has updated collection version to '"
-              << refreshedMetadata->getCollVersion() << "'.";
+        log() << "Migration succeeded and updated collection version to "
+              << refreshedMetadata->getCollVersion();
     } else {
         ScopedTransaction scopedXact(txn, MODE_IX);
         AutoGetCollection autoColl(txn, getNss(), MODE_IX, MODE_X);
 
         CollectionShardingState::get(txn, getNss())->refreshMetadata(txn, nullptr);
 
-        log() << "moveChunk failed to refresh metadata for '" << _args.toString()
-              << "'. Metadata was cleared so it will get a full refresh when accessed again"
+        log() << "Failed to refresh metadata after a failed commit attempt. Metadata was cleared "
+                 "so it will get a full refresh when accessed again"
               << causedBy(redact(refreshStatus));
 
+        // We don't know whether migration succeeded or failed
         return {migrationCommitStatus.code(),
                 str::stream() << "Failed to refresh metadata after migration commit due to "
                               << refreshStatus.toString()};