crimson/admin: let AdminSocket::register_command() return void

instead of returning a seastar::future<>, it can just return void,
as it does not involve asynchronous operations.

also drop servers_tbl_rwlock, as we don't register hooks after OSD
starts, there is no need to protect hooks with a rw lock.

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

diff --git a/src/crimson/admin/admin_socket.cc b/src/crimson/admin/admin_socket.cc
index 136adb41a724f94dc37f8b9768f2191e8836d295..76fe5e15e2f90ba30b46673f286c004ff0957e9c 100644 (file)
--- a/src/crimson/admin/admin_socket.cc
+++ b/src/crimson/admin/admin_socket.cc
@@ -44,25 +44,14 @@ tell_result_t::tell_result_t(std::unique_ptr<Formatter> formatter)
   formatter->flush(out);
 }
 
-seastar::future<>
-AdminSocket::register_command(std::unique_ptr<AdminSocketHook>&& hook)
+void AdminSocket::register_command(std::unique_ptr<AdminSocketHook>&& hook)
 {
-  return seastar::with_lock(servers_tbl_rwlock,
-                           [this, hook = std::move(hook)]() mutable {
-    auto prefix = hook->prefix;
-    auto [it, added] = hooks.emplace(prefix, std::move(hook));
-    //  was this server tag already registered?
-    assert(added);
-    if (added) {
-      logger().info("register_command(): {})", it->first);
-    }
-    return seastar::now();
-  });
+  auto prefix = hook->prefix;
+  auto [it, added] = hooks.emplace(prefix, std::move(hook));
+  assert(added);
+  logger().info("register_command(): {})", it->first);
 }
 
-/*
- * Note: parse_cmd() is executed with servers_tbl_rwlock held as shared
- */
 auto AdminSocket::parse_cmd(const std::vector<std::string>& cmd)
   -> std::variant<parsed_command_t, tell_result_t>
 {
@@ -155,26 +144,23 @@ auto AdminSocket::execute_command(const std::vector<std::string>& cmd,
                                  ceph::bufferlist&& buf)
   -> seastar::future<tell_result_t>
 {
-  return seastar::with_shared(servers_tbl_rwlock,
-                             [cmd, buf=std::move(buf), this]() mutable {
-    auto maybe_parsed = parse_cmd(cmd);
-    if (auto* parsed = std::get_if<parsed_command_t>(&maybe_parsed); parsed) {
-      stringstream os;
-      string desc{parsed->hook.desc};
-      if (!validate_cmd(nullptr, desc, parsed->params, os)) {
-       logger().error("AdminSocket::execute_command: "
-                      "failed to validate '{}': {}", cmd, os.str());
-       ceph::bufferlist out;
-       out.append(os);
-       return seastar::make_ready_future<tell_result_t>(
-          tell_result_t{-EINVAL, "invalid command json", std::move(out)});
-      }
-      return parsed->hook.call(parsed->params, parsed->format, std::move(buf));
-    } else {
-      auto& result = std::get<tell_result_t>(maybe_parsed);
-      return seastar::make_ready_future<tell_result_t>(std::move(result));
+  auto maybe_parsed = parse_cmd(cmd);
+  if (auto* parsed = std::get_if<parsed_command_t>(&maybe_parsed); parsed) {
+    stringstream os;
+    string desc{parsed->hook.desc};
+    if (!validate_cmd(nullptr, desc, parsed->params, os)) {
+      logger().error("AdminSocket::execute_command: "
+                     "failed to validate '{}': {}", cmd, os.str());
+      ceph::bufferlist out;
+      out.append(os);
+      return seastar::make_ready_future<tell_result_t>(
+        tell_result_t{-EINVAL, "invalid command json", std::move(out)});
     }
-  });
+    return parsed->hook.call(parsed->params, parsed->format, std::move(buf));
+  } else {
+    auto& result = std::get<tell_result_t>(maybe_parsed);
+    return seastar::make_ready_future<tell_result_t>(std::move(result));
+  }
 }
 
 // an input_stream consumer that reads buffer into a std::string up to the first
@@ -342,18 +328,16 @@ class HelpHook final : public AdminSocketHook {
                                      std::string_view format,
                                      ceph::bufferlist&&) const final
   {
-    return seastar::with_shared(m_as.servers_tbl_rwlock,
-                               [format, this] {
-      unique_ptr<Formatter> f{Formatter::create(format, "json-pretty", "json-pretty")};
-      f->open_object_section("help");
-      for (const auto& [prefix, hook] : m_as) {
-        if (!hook->help.empty()) {
-          f->dump_string(prefix.data(), hook->help);
-       }
+    unique_ptr<Formatter> f{Formatter::create(format,
+                                             "json-pretty", "json-pretty")};
+    f->open_object_section("help");
+    for (const auto& [prefix, hook] : m_as) {
+      if (!hook->help.empty()) {
+        f->dump_string(prefix.data(), hook->help);
       }
-      f->close_section();
-      return seastar::make_ready_future<tell_result_t>(std::move(f));
-    });
+    }
+    f->close_section();
+    return seastar::make_ready_future<tell_result_t>(std::move(f));
   }
 };
 
@@ -371,20 +355,18 @@ class GetdescsHook final : public AdminSocketHook {
                                      std::string_view format,
                                      ceph::bufferlist&&) const final
   {
-    return seastar::with_shared(m_as.servers_tbl_rwlock, [format, this] {
-      unique_ptr<Formatter> f{Formatter::create(format, "json-pretty", "json-pretty")};
-      int cmdnum = 0;
-      f->open_object_section("command_descriptions");
-      for (const auto& [prefix, hook] : m_as) {
-       auto secname = fmt::format("cmd {:>03}", cmdnum);
-        auto cmd = fmt::format("{} {}", hook->prefix, hook->desc);
-        dump_cmd_and_help_to_json(f.get(), CEPH_FEATURES_ALL, secname,
-                                  cmd, std::string{hook->help});
-        cmdnum++;
-      }
-      f->close_section();
-      return seastar::make_ready_future<tell_result_t>(std::move(f));
-    });
+    unique_ptr<Formatter> f{Formatter::create(format, "json-pretty", "json-pretty")};
+    int cmdnum = 0;
+    f->open_object_section("command_descriptions");
+    for (const auto& [prefix, hook] : m_as) {
+      auto secname = fmt::format("cmd {:>03}", cmdnum);
+      auto cmd = fmt::format("{} {}", hook->prefix, hook->desc);
+      dump_cmd_and_help_to_json(f.get(), CEPH_FEATURES_ALL, secname,
+                               cmd, std::string{hook->help});
+      cmdnum++;
+    }
+    f->close_section();
+    return seastar::make_ready_future<tell_result_t>(std::move(f));
   }
 };
 
@@ -529,21 +511,17 @@ public:
 };
 
 /// the hooks that are served directly by the admin_socket server
-seastar::future<> AdminSocket::register_admin_commands()
+void AdminSocket::register_admin_commands()
 {
-  return seastar::when_all_succeed(
-    register_command(std::make_unique<VersionHook>()),
-    register_command(std::make_unique<GitVersionHook>()),
-    register_command(std::make_unique<HelpHook>(*this)),
-    register_command(std::make_unique<GetdescsHook>(*this)),
-    register_command(std::make_unique<ConfigGetHook>()),
-    register_command(std::make_unique<ConfigSetHook>()),
-    register_command(std::make_unique<ConfigShowHook>()),
-    register_command(std::make_unique<ConfigHelpHook>()),
-    register_command(std::make_unique<InjectArgsHook>())
-  ).then_unpack([] {
-    return seastar::now();
-  });
+  register_command(std::make_unique<VersionHook>());
+  register_command(std::make_unique<GitVersionHook>());
+  register_command(std::make_unique<HelpHook>(*this));
+  register_command(std::make_unique<GetdescsHook>(*this));
+  register_command(std::make_unique<ConfigGetHook>());
+  register_command(std::make_unique<ConfigSetHook>());
+  register_command(std::make_unique<ConfigShowHook>());
+  register_command(std::make_unique<ConfigHelpHook>());
+  register_command(std::make_unique<InjectArgsHook>());
 }
 
 }  // namespace crimson::admin

diff --git a/src/crimson/admin/admin_socket.h b/src/crimson/admin/admin_socket.h
index 81017dd4717ba39b36748561c1da53a87201dcd5..a7540a6071f39271dce16171e6d363b95fafaaea 100644 (file)
--- a/src/crimson/admin/admin_socket.h
+++ b/src/crimson/admin/admin_socket.h
@@ -112,12 +112,12 @@ class AdminSocket : public seastar::enable_lw_shared_from_this<AdminSocket> {
    * A note regarding the help text: if empty, command will not be
    * included in 'help' output.
    */
-  seastar::future<> register_command(std::unique_ptr<AdminSocketHook>&& hook);
+  void register_command(std::unique_ptr<AdminSocketHook>&& hook);
 
   /**
    * Registering the APIs that are served directly by the admin_socket server.
    */
-  seastar::future<> register_admin_commands();
+  void register_admin_commands();
   /**
    * handle a command message by replying an MCommandReply with the same tid
    *
@@ -171,16 +171,10 @@ private:
   std::variant<parsed_command_t, tell_result_t>
   parse_cmd(const std::vector<std::string>& cmd);
 
-  /**
-   *  The servers table is protected by a rw-lock, to be acquired exclusively
-   *  only when registering or removing a server.
-   *  The lock is locked-shared when executing any hook.
-   */
-  mutable seastar::shared_mutex servers_tbl_rwlock;
   using hooks_t = std::map<std::string_view, std::unique_ptr<AdminSocketHook>>;
   hooks_t hooks;
 
- public:
+public:
   /**
    * iterator support
    */
@@ -190,10 +184,6 @@ private:
   hooks_t::const_iterator end() const {
     return hooks.cend();
   }
-
-  friend class AdminSocketTest;
-  friend class HelpHook;
-  friend class GetdescsHook;
 };
 
 }  // namespace crimson::admin

diff --git a/src/crimson/osd/osd.cc b/src/crimson/osd/osd.cc
index d1673acce657aaacfc1a97b879f78a6cde0e6876..17924a9428332550e8eaa2fd878e2fe36618d114 100644 (file)
--- a/src/crimson/osd/osd.cc
+++ b/src/crimson/osd/osd.cc
@@ -459,21 +459,18 @@ seastar::future<> OSD::start_asok_admin()
   auto asok_path = local_conf().get_val<std::string>("admin_socket");
   using namespace crimson::admin;
   return asok->start(asok_path).then([this] {
-    return seastar::when_all_succeed(
-      asok->register_admin_commands(),
-      asok->register_command(make_asok_hook<OsdStatusHook>(std::as_const(*this))),
-      asok->register_command(make_asok_hook<SendBeaconHook>(*this)),
-      asok->register_command(make_asok_hook<FlushPgStatsHook>(*this)),
-      asok->register_command(make_asok_hook<DumpPGStateHistory>(std::as_const(*this))),
-      asok->register_command(make_asok_hook<DumpMetricsHook>()),
-      asok->register_command(make_asok_hook<DumpPerfCountersHook>()),
-      asok->register_command(make_asok_hook<InjectDataErrorHook>(get_shard_services())),
-      asok->register_command(make_asok_hook<InjectMDataErrorHook>(get_shard_services())),
-      // PG commands
-      asok->register_command(make_asok_hook<pg::QueryCommand>(*this)),
-      asok->register_command(make_asok_hook<pg::MarkUnfoundLostCommand>(*this)));
-  }).then_unpack([] {
-    return seastar::now();
+    asok->register_admin_commands();
+    asok->register_command(make_asok_hook<OsdStatusHook>(std::as_const(*this)));
+    asok->register_command(make_asok_hook<SendBeaconHook>(*this));
+    asok->register_command(make_asok_hook<FlushPgStatsHook>(*this));
+    asok->register_command(make_asok_hook<DumpPGStateHistory>(std::as_const(*this)));
+    asok->register_command(make_asok_hook<DumpMetricsHook>());
+    asok->register_command(make_asok_hook<DumpPerfCountersHook>());
+    asok->register_command(make_asok_hook<InjectDataErrorHook>(get_shard_services()));
+    asok->register_command(make_asok_hook<InjectMDataErrorHook>(get_shard_services()));
+    // PG commands
+    asok->register_command(make_asok_hook<pg::QueryCommand>(*this));
+    asok->register_command(make_asok_hook<pg::MarkUnfoundLostCommand>(*this));
   });
 }