Skip to content

Commit

Permalink
c/leader_balancer/ut: remove duplicate test cases
Browse files Browse the repository at this point in the history
  • Loading branch information
@ztlpn
ztlpn committed Oct 14, 2024
1 parent d06a115 commit 1f0954d
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 269 deletions.
187 changes: 9 additions & 178 deletions src/v/cluster/tests/leader_balancer_constraints_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,184 +33,15 @@
#include <tuple>
#include <vector>

BOOST_AUTO_TEST_CASE(greedy_empty) {
// empty spec, expect no movement
BOOST_REQUIRE(no_movement({}));
}

BOOST_AUTO_TEST_CASE(greedy_balanced) {
// single node is already balanced, expect no movement
BOOST_REQUIRE(no_movement({
{{1, 2, 3}, {}},
}));
BOOST_REQUIRE(no_movement({
{{1}, {2}},
{{2}, {1}},
}));

// not exactly balanced, but as balanced as possible,
// i.e., lead loaded node has 1 less than the most loaded node
BOOST_REQUIRE(no_movement({
{{1, 3}, {2}},
{{2}, {1, 3}},
}));
}

BOOST_AUTO_TEST_CASE(greedy_movement) {
// 10 nodes
// 2 cores x node
// 10 partitions per shard
// r=3 (3 replicas)
auto index = leader_balancer_test_utils::make_cluster_index(10, 2, 10, 3);
auto shard_index = lbt::shard_index(
leader_balancer_test_utils::copy_cluster_index(index));
auto mute_index = lbt::muted_index({}, {});

auto greed = lbt::even_shard_load_constraint(shard_index, mute_index);
BOOST_REQUIRE_EQUAL(greed.error(), 0);

// new groups on shard {2, 0}
auto shard20 = model::broker_shard{model::node_id{2}, 0};
index[shard20][raft::group_id(20)] = index[shard20][raft::group_id(3)];
index[shard20][raft::group_id(21)] = index[shard20][raft::group_id(3)];
index[shard20][raft::group_id(22)] = index[shard20][raft::group_id(3)];

shard_index = lbt::shard_index(
leader_balancer_test_utils::copy_cluster_index(index));
auto greed2 = lbt::even_shard_load_constraint(shard_index, mute_index);
BOOST_REQUIRE_GT(greed2.error(), 0);

// movement should be _from_ the overloaded shard
auto movement = greed2.recommended_reassignment();
BOOST_REQUIRE(movement);
check_valid(shard_index.shards(), *movement);
BOOST_REQUIRE_EQUAL(movement->from, shard20);
}

BOOST_AUTO_TEST_CASE(greedy_obeys_replica) {
// unbalanced, but the overloaded node has no replicas it can
// send its groups to
BOOST_REQUIRE(no_movement({{{1, 2, 3}, {}}, {{4}, {}}, {{}, {4}}}));
}

BOOST_AUTO_TEST_CASE(greedy_simple_movement) {
// simple balancing from {2, 0} leader counts to {1, 1}
BOOST_REQUIRE_EQUAL(
expect_movement(
{
// clang-format off
{{1, 2}, {}},
{{}, {1, 2}}
// clang-format on
},
re(1, 0, 1)),
"");
}

BOOST_AUTO_TEST_CASE(greedy_highest_to_lowest) {
// balancing should occur from the most to least loaded node if that is
// possible
BOOST_REQUIRE_EQUAL(
expect_movement(
{
// clang-format off
{{1, 2}, {-1}},
{{3, 4, 5}, {-1}}, // from 1
{{6, 7}, {-1}},
{{}, {1, 2, 3, 4, 5, 6, 7}} // to 3

// clang-format on
},
re(3, 1, 3)),
"");

// like the previous case but group 3 is not replicated on the to node, so
// we check that group 4 goes instead
BOOST_REQUIRE_EQUAL(
expect_movement(
{
// clang-format off
{{1, 2}, {-1}},
{{3, 4, 5}, {-1}}, // from 1
{{6, 7}, {-1}},
{{}, {1, 2, 4, 5, 6, 7}} // to 3

// clang-format on
},
re(4, 1, 3)),
"");
}

BOOST_AUTO_TEST_CASE(greedy_low_to_lower) {
// balancing can occur even from a shard with less than average load,
// if there is a shard with even lower load and the higher loaded shards
// cannot be rebalanced from because of a lack of replicas for their
// groups on the lower load nodes
BOOST_REQUIRE_EQUAL(
expect_movement(
{
// clang-format off
{{1, 2, 3, 10}, {}},
{{4, 5, 6, 11}, {}},
{{7, 8}, {}}, // from 2
{{}, {8}} // to 3

// clang-format on
},
re(8, 2, 3)),
"");
}

BOOST_AUTO_TEST_CASE(greedy_muted) {
// base spec without high, medium and low (zero) load nodes
auto spec = cluster_spec{
// clang-format off
{{1, 2, 3, 4}, {-1}},
{{5, 6}, {-1}},
{{}, {-1}},
// clang-format on
};

// base case, move from high to low
BOOST_REQUIRE_EQUAL(expect_movement(spec, re(1, 0, 2)), "");

// mute from the "from" node (high), so moves from mid to low
BOOST_REQUIRE_EQUAL(expect_movement(spec, re(5, 1, 2), {0}), "");

// mute from the "to" node (low), so moves from high to mid
BOOST_REQUIRE_EQUAL(expect_movement(spec, re(1, 0, 1), {2}), "");

// mute any 2 nodes and there should be no movement
BOOST_REQUIRE(no_movement(spec, {0, 1}));
BOOST_REQUIRE(no_movement(spec, {0, 2}));
BOOST_REQUIRE(no_movement(spec, {1, 2}));
}

BOOST_AUTO_TEST_CASE(greedy_skip) {
// base spec without high, medium and low load nodes

auto spec = cluster_spec{
// clang-format off
{{1, 2, 3, 4}, {-1}},
{{5, 6}, {-1}},
{{}, {-1}},
// clang-format on
};

// base case, move from high to low
BOOST_REQUIRE_EQUAL(expect_movement(spec, re(1, 0, 2)), "");

// skip group 1, we move group 2 instead from same node
BOOST_REQUIRE_EQUAL(expect_movement(spec, re(2, 0, 2), {}, {1}), "");

// skip all groups on node 0, we move mid to low
BOOST_REQUIRE_EQUAL(
expect_movement(spec, re(5, 1, 2), {}, {1, 2, 3, 4}), "");

// mute node 0 and skip all groups on node 1, no movement
absl::flat_hash_set<raft::group_id> skip{
raft::group_id(5), raft::group_id(6)};
BOOST_REQUIRE(no_movement(spec, {0}, skip));
/**
* @brief Helper to create a reassignment from group, from and to passed as
* integers.
*/
inline reassignment re(unsigned group, int from, int to) {
return {
raft::group_id(group),
model::broker_shard{model::node_id(from)},
model::broker_shard{model::node_id(to)}};
}

BOOST_AUTO_TEST_CASE(even_topic_distribution_empty) {
Expand Down
92 changes: 1 addition & 91 deletions src/v/cluster/tests/leader_balancer_constraints_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ using reassignment = lbt::reassignment;
/**
* @brief Create a greedy balancer from a cluster_spec and set of muted nodes.
*/
static auto from_spec(
inline auto from_spec(
const cluster_spec& spec, const absl::flat_hash_set<int>& muted = {}) {
index_type index;

Expand Down Expand Up @@ -123,93 +123,3 @@ inline auto group_to_topic_from_spec(const group_to_ntp_spec& spec) {

return group_to_topic;
}

/**
* @brief Basic validity checks on a reassignment.
*/
static void check_valid(const index_type& index, const reassignment& movement) {
// from == to is not valid (error should not decrease)
vassert(movement.from != movement.to, "");

auto& from_groups = index.at(movement.from);
// check that the from shard was the leader
vassert(from_groups.contains(movement.group), "");

// check that the to shard is in the replica set
auto& replicas = from_groups.at(movement.group);
vassert(std::count(replicas.begin(), replicas.end(), movement.to) == 1, "");
}

/**
* @brief returns true iff the passed spec had no movement on balance
*/
inline bool no_movement(
const cluster_spec& spec,
const absl::flat_hash_set<int>& muted = {},
const absl::flat_hash_set<raft::group_id>& skip = {}) {
auto [shard_index, muted_index] = from_spec(spec, muted);
cluster::leader_balancer_types::muted_groups_t skip_typed;
for (auto& s : skip) {
skip_typed.add(static_cast<uint64_t>(s));
}
muted_index.update_muted_groups(skip_typed);
auto balancer = lbt::even_shard_load_constraint(shard_index, muted_index);

return !balancer.recommended_reassignment();
}

static bool operator==(const lbt::reassignment& l, const lbt::reassignment& r) {
return l.group == r.group && l.from == r.from && l.to == r.to;
}

static ss::sstring to_string(const reassignment& r) {
return fmt::format("{{group: {} from: {} to: {}}}", r.group, r.from, r.to);
}

/**
* @brief Test helper which checks that the given spec and other parameters
* result in the expected resassignment.
*/
inline ss::sstring expect_movement(
const cluster_spec& spec,
const reassignment& expected_reassignment,
const absl::flat_hash_set<int>& muted = {},
const absl::flat_hash_set<int>& skip = {}) {
auto [shard_index, mute_index] = from_spec(spec, muted);

cluster::leader_balancer_types::muted_groups_t skip_typed;
for (auto& s : skip) {
skip_typed.add(static_cast<uint64_t>(s));
}

mute_index.update_muted_groups(std::move(skip_typed));
auto balancer = lbt::even_shard_load_constraint(shard_index, mute_index);

auto reassignment = balancer.recommended_reassignment();

if (!reassignment) {
return "no reassignment occurred";
}

check_valid(shard_index.shards(), *reassignment);

if (!(*reassignment == expected_reassignment)) {
return fmt::format(
"Reassignment not as expected.\nExpected: {}\nActual: {}\n",
to_string(expected_reassignment),
to_string(*reassignment));
}

return "";
}

/**
* @brief Helper to create a reassignment from group, from and to passed as
* integers.
*/
inline reassignment re(unsigned group, int from, int to) {
return {
raft::group_id(group),
model::broker_shard{model::node_id(from)},
model::broker_shard{model::node_id(to)}};
}

0 comments on commit 1f0954d

Please sign in to comment.