This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit fad7dd457aa5a35dddfa0e6086b3dce9b03d42b7 Author: Andrew Wong <[email protected]> AuthorDate: Fri Jul 16 17:30:12 2021 -0700 [test] deflake dynamic_multi_master-test The test was flaky for a few reasons: - retries to create a table could end up with the table already created, and an AlreadyPresent error was not being caught. This patch treats such errors as benign. - ExternalMiniCluster::WaitForTabletServerCount() could previously fail if the RPCs sent therein failed because the master wasn't yet ready to serve RPCs. This patch first pings the masters, retrying until all masters can be pinged, indicating the health of their RPC layers. - VerifyVoterMastersForCluster() would previously only verify the voters according to the leader master. This wouldn't fly upon restarting the cluster, since lagging masters could fail to start up with a mismatched on-disk Raft config. Without this patch, the test would fail ~1% of the time in debug mode with --stress_cpu_threads=3. With this patch, it passed 1000/1000 times. Change-Id: I67c5ac38701c4283611dc95e17f9f054c90aba99 Reviewed-on: http://gerrit.cloudera.org:8080/17692 Reviewed-by: Alexey Serbin <[email protected]> Tested-by: Kudu Jenkins --- src/kudu/integration-tests/master_hms-itest.cc | 2 +- src/kudu/master/dynamic_multi_master-test.cc | 39 ++++++++++++++++++++++++-- src/kudu/mini-cluster/external_mini_cluster.cc | 18 +++++++++++- src/kudu/mini-cluster/external_mini_cluster.h | 6 ++-- 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/kudu/integration-tests/master_hms-itest.cc b/src/kudu/integration-tests/master_hms-itest.cc index 4dedec2..7f4af33 100644 --- a/src/kudu/integration-tests/master_hms-itest.cc +++ b/src/kudu/integration-tests/master_hms-itest.cc @@ -746,7 +746,7 @@ TEST_F(MasterHmsUpgradeTest, TestConflictingNormalizedNames) { // this situation a fallback to a leader-only API will deterministically fail. Status s = cluster_->Restart(); if (!s.ok()) { - ASSERT_TRUE(s.IsNetworkError()) << s.ToString(); + ASSERT_TRUE(s.IsTimedOut() || s.IsNetworkError()) << s.ToString(); } else { vector<string> tables; s = client_->ListTables(&tables); diff --git a/src/kudu/master/dynamic_multi_master-test.cc b/src/kudu/master/dynamic_multi_master-test.cc index d1e473c..71b278f 100644 --- a/src/kudu/master/dynamic_multi_master-test.cc +++ b/src/kudu/master/dynamic_multi_master-test.cc @@ -197,9 +197,43 @@ Status RunListMasters(ListMastersResponsePB* resp, ExternalMiniCluster* cluster) return RunLeaderMasterRPC(list_masters, cluster); } +// Verify the 'cluster' contains 'num_masters' on all masters, and that they +// are all voters. Returns an error if the expected state is not present. +// +// This should be used instead of VerifyVoterMastersForCluster() if it is +// required that all masters have accepted the config change. E.g. tests that +// restart a cluster after adding a master should verify that all masters agree +// before restarting, in case lagging masters start up with stale configs. +// +// TODO(awong): we should be more robust to starting up with mismatched on-disk +// configs, if we can help it. +Status VerifyVotersOnAllMasters(int num_masters, ExternalMiniCluster* cluster) { + for (int i = 0; i < cluster->num_masters(); i++) { + ListMastersResponsePB resp; + ListMastersRequestPB req; + RpcController rpc; + RETURN_NOT_OK(cluster->master_proxy(i)->ListMasters(req, &resp, &rpc)); + if (num_masters != resp.masters_size()) { + return Status::IllegalState(Substitute("expected $0 masters but got $1", + num_masters, resp.masters_size())); + } + for (const auto& master : resp.masters()) { + if ((master.role() != RaftPeerPB::LEADER && master.role() != RaftPeerPB::FOLLOWER) || + master.member_type() != RaftPeerPB::VOTER || + master.registration().rpc_addresses_size() != 1) { + return Status::IllegalState(Substitute("bad master: $0", SecureShortDebugString(master))); + } + } + } + return Status::OK(); +} + // Verify the ExternalMiniCluster 'cluster' contains 'num_masters' overall and // are all VOTERS. Populates the new master addresses in 'master_hps', if not // nullptr. Returns an error if the expected state is not present. +// +// This should be used instead of VerifyVotersOnAllMasters() if the test does +// not need to wait for all masters to agree on the config. Status VerifyVoterMastersForCluster(int num_masters, vector<HostPort>* master_hps, ExternalMiniCluster* cluster) { ListMastersResponsePB resp; @@ -1645,7 +1679,7 @@ TEST_F(AutoAddMasterTest, TestAddWithOnGoingDdl) { ASSERT_OK(new_master->WaitForCatalogManager()); num_masters++; ASSERT_EVENTUALLY([&] { - ASSERT_OK(VerifyVoterMastersForCluster(num_masters, nullptr, cluster_.get())); + ASSERT_OK(VerifyVotersOnAllMasters(num_masters, cluster_.get())); }); { std::lock_guard<simple_spinlock> l(master_addrs_lock); @@ -1662,7 +1696,8 @@ TEST_F(AutoAddMasterTest, TestAddWithOnGoingDdl) { t.join(); } for (const auto& e : errors) { - if (e.ok() || e.IsTimedOut()) { + // NOTE: the table may exist if the CreateTable call is retried. + if (e.ok() || e.IsTimedOut() || e.IsAlreadyPresent()) { continue; } // TODO(awong): we should relax the need for clients to have the precise diff --git a/src/kudu/mini-cluster/external_mini_cluster.cc b/src/kudu/mini-cluster/external_mini_cluster.cc index cd666f9..63baa74 100644 --- a/src/kudu/mini-cluster/external_mini_cluster.cc +++ b/src/kudu/mini-cluster/external_mini_cluster.cc @@ -730,6 +730,22 @@ Status ExternalMiniCluster::WaitForTabletServerCount(int count, const MonoDelta& return Status::TimedOut(Substitute( "Timed out waiting for $0 TS(s) to register with all masters", count)); } + bool all_masters_reachable = true; + for (const auto& master_idx : masters_to_search) { + master::PingRequestPB req; + master::PingResponsePB resp; + rpc::RpcController rpc; + rpc.set_timeout(remaining); + Status s = master_proxy(master_idx)->Ping(req, &resp, &rpc); + if (!s.ok()) { + all_masters_reachable = false; + break; + } + } + if (!all_masters_reachable) { + SleepFor(MonoDelta::FromMilliseconds(10)); + continue; + } for (auto iter = masters_to_search.begin(); iter != masters_to_search.end();) { master::ListTabletServersRequestPB req; @@ -764,7 +780,7 @@ Status ExternalMiniCluster::WaitForTabletServerCount(int count, const MonoDelta& LOG(INFO) << count << " TS(s) registered with all masters"; return Status::OK(); } - SleepFor(MonoDelta::FromMilliseconds(1)); + SleepFor(MonoDelta::FromMilliseconds(10)); } } diff --git a/src/kudu/mini-cluster/external_mini_cluster.h b/src/kudu/mini-cluster/external_mini_cluster.h index 7c00de0..79d5b04 100644 --- a/src/kudu/mini-cluster/external_mini_cluster.h +++ b/src/kudu/mini-cluster/external_mini_cluster.h @@ -415,9 +415,9 @@ class ExternalMiniCluster : public MiniCluster { // Wait until the number of registered tablet servers reaches the given count // on running masters. Returns Status::TimedOut if the desired count is not - // achieved with the given timeout. - // If 'master_idx' is specified, only examines the given master if it's - // running. Otherwise, checks all running masters. + // achieved or if the masters cannot be reached within the given timeout. If + // 'master_idx' is specified, only examines the given master if it's running. + // Otherwise, checks all running masters. Status WaitForTabletServerCount(int count, const MonoDelta& timeout, int master_idx = -1);
