From: Kefu Chai <kchai@redhat.com>
Date: Thu, 10 Sep 2020 03:41:27 +0000 (+0800)
Subject: test/crimson: fix racing in testing
X-Git-Tag: v16.1.0~1149^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=fe0f662a936cf53ed8f57d86acf2fcab3cfd3260;p=ceph.git

test/crimson: fix racing in testing

before this change, unittest_seastar_config set the specified option to
the shard id in parallel and expects that the values of the option are
consistent across all shards after setting the value on all shards. but
sharded::invoke_on_all() is executed in parallel, and so does
ConfigProxy::do_change(), so there are actually "n x n" continuations
racing each other. the order of the continuations storing the setting
cannot be determined. so we cannot expected that the last batch of
continuations hitting shareds store the same value.

in this change, only a single `conf.set_val()` call is performed with
a known value. and this value is checked with the values stored on all
shards.

Signed-off-by: Kefu Chai <kchai@redhat.com>
---

diff --git a/src/test/crimson/test_config.cc b/src/test/crimson/test_config.cc
index a8234b2deb83..608aa26942ca 100644
--- a/src/test/crimson/test_config.cc
+++ b/src/test/crimson/test_config.cc
@@ -10,6 +10,7 @@
 using Config = crimson::common::ConfigProxy;
 const std::string test_uint_option = "osd_max_pgls";
 const uint64_t INVALID_VALUE = (uint64_t)(-1);
+const uint64_t EXPECTED_VALUE = 42;
 
 class ConfigObs : public ceph::md_config_obs_impl<Config> {
   uint64_t last_change = INVALID_VALUE;
@@ -63,20 +64,17 @@ static seastar::future<> test_config()
   }).then([] {
     return sharded_cobs.start();
   }).then([] {
-    return crimson::common::sharded_conf().invoke_on_all([](Config& config) {
-      return config.set_val(test_uint_option,
-                            std::to_string(seastar::this_shard_id()));
-    });
+    auto& conf = crimson::common::local_conf();
+    return conf.set_val(test_uint_option, std::to_string(EXPECTED_VALUE));
   }).then([] {
-    auto expected = crimson::common::local_conf().get_val<uint64_t>(test_uint_option);
-    return crimson::common::sharded_conf().invoke_on_all([expected](Config& config) {
-      if (expected != config.get_val<uint64_t>(test_uint_option)) {
+    return crimson::common::sharded_conf().invoke_on_all([](Config& config) {
+      if (config.get_val<uint64_t>(test_uint_option) != EXPECTED_VALUE) {
         throw std::runtime_error("configurations don't match");
       }
-      if (expected != sharded_cobs.local().get_last_change()) {
+      if (sharded_cobs.local().get_last_change() != EXPECTED_VALUE) {
         throw std::runtime_error("last applied changes don't match the latest config");
       }
-      if (seastar::smp::count != sharded_cobs.local().get_num_changes()) {
+      if (sharded_cobs.local().get_num_changes() != 1) {
         throw std::runtime_error("num changes don't match actual changes");
       }
     });