From 03f5fa3484cf001c21d48230590a97fafc1a89ec Mon Sep 17 00:00:00 2001 From: Kaloian Manassiev Date: Wed, 12 Jul 2017 11:50:55 -0400 Subject: SERVER-30058 Balancer policy should not move chunks off shards on 'size exceeded' conditions (cherry picked from commit 411114cb3ec3119bb159b29b8ef65292e4d20de3) --- src/mongo/db/s/balancer/balancer_policy.cpp | 8 +++----- src/mongo/db/s/balancer/balancer_policy_test.cpp | 26 ++++++++++++++++++------ src/mongo/db/s/balancer/cluster_statistics.cpp | 8 -------- src/mongo/db/s/balancer/cluster_statistics.h | 6 ------ 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/mongo/db/s/balancer/balancer_policy.cpp b/src/mongo/db/s/balancer/balancer_policy.cpp index 206a50257a6..348d8cec603 100644 --- a/src/mongo/db/s/balancer/balancer_policy.cpp +++ b/src/mongo/db/s/balancer/balancer_policy.cpp @@ -222,8 +222,7 @@ Status BalancerPolicy::isShardSuitableReceiver(const ClusterStatistics::ShardSta const string& chunkTag) { if (stat.isSizeMaxed()) { return {ErrorCodes::IllegalOperation, - str::stream() << stat.shardId - << " has already reached the maximum total chunk size."}; + str::stream() << stat.shardId << " has reached its maximum storage size."}; } if (stat.isDraining) { @@ -299,11 +298,10 @@ vector BalancerPolicy::balance(const ShardStatisticsVector& shardSt // migrations for the same shard. set usedShards; - // 1) Check for shards, which are in draining mode or are above the size limit and must have - // chunks moved off of them + // 1) Check for shards, which are in draining mode { for (const auto& stat : shardStats) { - if (!stat.isDraining && !stat.isSizeExceeded()) + if (!stat.isDraining) continue; if (usedShards.count(stat.shardId)) diff --git a/src/mongo/db/s/balancer/balancer_policy_test.cpp b/src/mongo/db/s/balancer/balancer_policy_test.cpp index 11fe57e133d..de5ceafaa83 100644 --- a/src/mongo/db/s/balancer/balancer_policy_test.cpp +++ b/src/mongo/db/s/balancer/balancer_policy_test.cpp @@ -408,22 +408,36 @@ TEST(BalancerPolicy, NoBalancingDueToAllNodesEitherDrainingOrMaxedOut) { ASSERT(migrations.empty()); } -TEST(BalancerPolicy, BalancerMovesChunksOffSizeMaxedShards) { +TEST(BalancerPolicy, BalancerRespectsMaxShardSizeOnlyBalanceToNonMaxed) { // Note that maxSize of shard0 is 1, and it is therefore overloaded with currSize = 3. Other // shards have maxSize = 0 = unset. Even though the overloaded shard has the least number of // less chunks, we shouldn't move chunks to that shard. auto cluster = generateCluster( {{ShardStatistics(kShardId0, 1, 3, false, emptyTagSet, emptyShardVersion), 2}, - {ShardStatistics(kShardId1, kNoMaxSize, 4, false, emptyTagSet, emptyShardVersion), 4}, - {ShardStatistics(kShardId2, kNoMaxSize, 6, false, emptyTagSet, emptyShardVersion), 6}}); + {ShardStatistics(kShardId1, kNoMaxSize, 5, false, emptyTagSet, emptyShardVersion), 5}, + {ShardStatistics(kShardId2, kNoMaxSize, 10, false, emptyTagSet, emptyShardVersion), 10}}); const auto migrations(BalancerPolicy::balance( cluster.first, DistributionStatus(kNamespace, cluster.second), false)); ASSERT_EQ(1U, migrations.size()); - ASSERT_EQ(kShardId0, migrations[0].from); + ASSERT_EQ(kShardId2, migrations[0].from); ASSERT_EQ(kShardId1, migrations[0].to); - ASSERT_BSONOBJ_EQ(cluster.second[kShardId0][0].getMin(), migrations[0].minKey); - ASSERT_BSONOBJ_EQ(cluster.second[kShardId0][0].getMax(), migrations[0].maxKey); + ASSERT_BSONOBJ_EQ(cluster.second[kShardId2][0].getMin(), migrations[0].minKey); + ASSERT_BSONOBJ_EQ(cluster.second[kShardId2][0].getMax(), migrations[0].maxKey); +} + +TEST(BalancerPolicy, BalancerRespectsMaxShardSizeWhenAllBalanced) { + // Note that maxSize of shard0 is 1, and it is therefore overloaded with currSize = 4. Other + // shards have maxSize = 0 = unset. We check that being over the maxSize is NOT equivalent to + // draining, we don't want to empty shards for no other reason than they are over this limit. + auto cluster = generateCluster( + {{ShardStatistics(kShardId0, 1, 4, false, emptyTagSet, emptyShardVersion), 4}, + {ShardStatistics(kShardId1, kNoMaxSize, 4, false, emptyTagSet, emptyShardVersion), 4}, + {ShardStatistics(kShardId2, kNoMaxSize, 4, false, emptyTagSet, emptyShardVersion), 4}}); + + const auto migrations(BalancerPolicy::balance( + cluster.first, DistributionStatus(kNamespace, cluster.second), false)); + ASSERT(migrations.empty()); } TEST(BalancerPolicy, BalancerRespectsTagsWhenDraining) { diff --git a/src/mongo/db/s/balancer/cluster_statistics.cpp b/src/mongo/db/s/balancer/cluster_statistics.cpp index 125f11d3551..d51b4ec4a24 100644 --- a/src/mongo/db/s/balancer/cluster_statistics.cpp +++ b/src/mongo/db/s/balancer/cluster_statistics.cpp @@ -60,14 +60,6 @@ bool ClusterStatistics::ShardStatistics::isSizeMaxed() const { return currSizeMB >= maxSizeMB; } -bool ClusterStatistics::ShardStatistics::isSizeExceeded() const { - if (!maxSizeMB || !currSizeMB) { - return false; - } - - return currSizeMB > maxSizeMB; -} - BSONObj ClusterStatistics::ShardStatistics::toBSON() const { BSONObjBuilder builder; builder.append("id", shardId.toString()); diff --git a/src/mongo/db/s/balancer/cluster_statistics.h b/src/mongo/db/s/balancer/cluster_statistics.h index 2717c42b7ee..a8e433062e8 100644 --- a/src/mongo/db/s/balancer/cluster_statistics.h +++ b/src/mongo/db/s/balancer/cluster_statistics.h @@ -70,12 +70,6 @@ public: */ bool isSizeMaxed() const; - /** - * Returns true if a shard must be relieved (if possible) of some of the chunks it hosts - * because it has exceeded its per-shard data size limit. - */ - bool isSizeExceeded() const; - /** * Returns BSON representation of this shard's statistics, for reporting purposes. */ -- cgit v1.2.1