From: Kefu Chai Date: Sun, 29 Sep 2019 09:28:28 +0000 (+0800) Subject: mgr: use a struct for DaemonKey X-Git-Tag: v15.1.0~1334^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=5aac7eba36be99861eb8726872e339ff55360f47;p=ceph-ci.git mgr: use a struct for DaemonKey instead of using `std::pair<>` for presenting it. there are three advantages: 1. better names for readability: `type` and `name` are better names than `first` and `second` 2. so we can specliaze its behavior by its type: define `operator<<` once, and use it everywhere. no need to worry about ADL to pickup the generic operator of `operator<< (..., pair<>)` anymore. so we can safely use `<< key`, and no need to use `<< key.first << '.' << key.second`. kill the printed form of `osd,1` once and for all, all of them are now "osd.1". 3. consolidate the print/parse in a single place Fixes: https://tracker.ceph.com/issues/42079 Signed-off-by: Kefu Chai --- diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index d9f28761a45..1668248fa3e 100644 --- a/src/mgr/ActivePyModules.cc +++ b/src/mgr/ActivePyModules.cc @@ -29,6 +29,7 @@ #include "PyModuleRegistry.h" #include "ActivePyModules.h" +#include "DaemonKey.h" #include "DaemonServer.h" #define dout_context g_ceph_context @@ -63,23 +64,19 @@ void ActivePyModules::dump_server(const std::string &hostname, f->open_array_section("services"); std::string ceph_version; - for (const auto &i : dmc) { - std::lock_guard l(i.second->lock); - const auto &key = i.first; - const std::string &str_type = key.first; - const std::string &svc_name = key.second; - + for (const auto &[key, state] : dmc) { + std::lock_guard l(state->lock); // TODO: pick the highest version, and make sure that // somewhere else (during health reporting?) we are // indicating to the user if we see mixed versions - auto ver_iter = i.second->metadata.find("ceph_version"); - if (ver_iter != i.second->metadata.end()) { - ceph_version = i.second->metadata.at("ceph_version"); + auto ver_iter = state->metadata.find("ceph_version"); + if (ver_iter != state->metadata.end()) { + ceph_version = state->metadata.at("ceph_version"); } f->open_object_section("service"); - f->dump_string("type", str_type); - f->dump_string("id", svc_name); + f->dump_string("type", key.type); + f->dump_string("id", key.name); f->close_section(); } f->close_section(); @@ -130,7 +127,7 @@ PyObject *ActivePyModules::get_metadata_python( const std::string &svc_type, const std::string &svc_id) { - auto metadata = daemon_state.get(DaemonKey(svc_type, svc_id)); + auto metadata = daemon_state.get(DaemonKey{svc_type, svc_id}); if (metadata == nullptr) { derr << "Requested missing service " << svc_type << "." << svc_id << dendl; Py_RETURN_NONE; @@ -150,7 +147,7 @@ PyObject *ActivePyModules::get_daemon_status_python( const std::string &svc_type, const std::string &svc_id) { - auto metadata = daemon_state.get(DaemonKey(svc_type, svc_id)); + auto metadata = daemon_state.get(DaemonKey{svc_type, svc_id}); if (metadata == nullptr) { derr << "Requested missing service " << svc_type << "." << svc_id << dendl; Py_RETURN_NONE; @@ -243,12 +240,12 @@ PyObject *ActivePyModules::get_python(const std::string &what) auto dmc = daemon_state.get_by_service("osd"); PyEval_RestoreThread(tstate); - for (const auto &i : dmc) { - std::lock_guard l(i.second->lock); - f.open_object_section(i.first.second.c_str()); - f.dump_string("hostname", i.second->hostname); - for (const auto &j : i.second->metadata) { - f.dump_string(j.first.c_str(), j.second); + for (const auto &[key, state] : dmc) { + std::lock_guard l(state->lock); + f.open_object_section(key.name.c_str()); + f.dump_string("hostname", state->hostname); + for (const auto &[name, val] : state->metadata) { + f.dump_string(name.c_str(), val); } f.close_section(); } @@ -681,7 +678,7 @@ PyObject* ActivePyModules::with_perf_counters( PyFormatter f; f.open_array_section(path.c_str()); - auto metadata = daemon_state.get(DaemonKey(svc_name, svc_id)); + auto metadata = daemon_state.get(DaemonKey{svc_name, svc_id}); if (metadata) { std::lock_guard l2(metadata->lock); if (metadata->perf_counters.instances.count(path)) { @@ -775,7 +772,7 @@ PyObject* ActivePyModules::get_perf_schema_python( } else if (svc_id.empty()) { daemons = daemon_state.get_by_service(svc_type); } else { - auto key = DaemonKey(svc_type, svc_id); + auto key = DaemonKey{svc_type, svc_id}; // so that the below can be a loop in all cases auto got = daemon_state.get(key); if (got != nullptr) { @@ -785,13 +782,8 @@ PyObject* ActivePyModules::get_perf_schema_python( PyFormatter f; if (!daemons.empty()) { - for (auto statepair : daemons) { - auto key = statepair.first; - auto state = statepair.second; - - std::ostringstream daemon_name; - daemon_name << key.first << "." << key.second; - f.open_object_section(daemon_name.str().c_str()); + for (auto& [key, state] : daemons) { + f.open_object_section(ceph::to_string(key).c_str()); std::lock_guard l(state->lock); for (auto ctr_inst_iter : state->perf_counters.instances) { diff --git a/src/mgr/CMakeLists.txt b/src/mgr/CMakeLists.txt index 6636e0a6f57..499c700210c 100644 --- a/src/mgr/CMakeLists.txt +++ b/src/mgr/CMakeLists.txt @@ -7,6 +7,7 @@ set(mgr_srcs BaseMgrStandbyModule.cc ClusterState.cc DaemonHealthMetricCollector.cc + DaemonKey.cc DaemonServer.cc DaemonState.cc Gil.cc diff --git a/src/mgr/DaemonHealthMetricCollector.cc b/src/mgr/DaemonHealthMetricCollector.cc index 711ebe60da8..ca864dd5899 100644 --- a/src/mgr/DaemonHealthMetricCollector.cc +++ b/src/mgr/DaemonHealthMetricCollector.cc @@ -4,30 +4,6 @@ #include "include/types.h" #include "DaemonHealthMetricCollector.h" - - -ostream& operator<<(ostream& os, - const DaemonHealthMetricCollector::DaemonKey& daemon) { - return os << daemon.first << "." << daemon.second; -} - -// define operator<<(ostream&, const vector&) after -// ostream& operator<<(ostream&, const DaemonKey&), so that C++'s -// ADL can use the former instead of using the generic one: -// operator<<(ostream&, const std::pair&) -ostream& operator<<( - ostream& os, - const vector& daemons) -{ - os << "["; - for (auto d = daemons.begin(); d != daemons.end(); ++d) { - if (d != daemons.begin()) os << ","; - os << *d; - } - os << "]"; - return os; -} - namespace { class SlowOps final : public DaemonHealthMetricCollector { diff --git a/src/mgr/DaemonHealthMetricCollector.h b/src/mgr/DaemonHealthMetricCollector.h index 42bf905fa9b..558f4e334d3 100644 --- a/src/mgr/DaemonHealthMetricCollector.h +++ b/src/mgr/DaemonHealthMetricCollector.h @@ -4,11 +4,11 @@ #include #include "DaemonHealthMetric.h" +#include "DaemonKey.h" #include "mon/health_check.h" class DaemonHealthMetricCollector { public: - using DaemonKey = std::pair; static std::unique_ptr create(daemon_metric m); void update(const DaemonKey& daemon, const DaemonHealthMetric& metric) { if (_is_relevant(metric.get_type())) { diff --git a/src/mgr/DaemonKey.cc b/src/mgr/DaemonKey.cc new file mode 100644 index 00000000000..5501ac106db --- /dev/null +++ b/src/mgr/DaemonKey.cc @@ -0,0 +1,35 @@ +#include "DaemonKey.h" + +std::pair DaemonKey::parse(const std::string& s) +{ + auto p = s.find('.'); + if (p == s.npos) { + return {{}, false}; + } else { + return {DaemonKey{s.substr(0, p), s.substr(p + 1)}, true}; + } +} + +bool operator<(const DaemonKey& lhs, const DaemonKey& rhs) +{ + if (int cmp = lhs.type.compare(rhs.type); cmp < 0) { + return true; + } else if (cmp > 0) { + return false; + } else { + return lhs.name < rhs.name; + } +} + +std::ostream& operator<<(std::ostream& os, const DaemonKey& key) +{ + return os << key.type << '.' << key.name; +} + +namespace ceph { +std::string to_string(const DaemonKey& key) +{ + return key.type + '.' + key.name; +} +} + diff --git a/src/mgr/DaemonKey.h b/src/mgr/DaemonKey.h new file mode 100644 index 00000000000..92bacd649fb --- /dev/null +++ b/src/mgr/DaemonKey.h @@ -0,0 +1,24 @@ +// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- +// vim: ts=8 sw=2 smarttab + +#pragma once + +#include +#include +#include + +// Unique reference to a daemon within a cluster +struct DaemonKey +{ + std::string type; // service type, like "osd", "mon" + std::string name; // service id / name, like "1", "a" + static std::pair parse(const std::string& s); +}; + +bool operator<(const DaemonKey& lhs, const DaemonKey& rhs); +std::ostream& operator<<(std::ostream& os, const DaemonKey& key); + +namespace ceph { + std::string to_string(const DaemonKey& key); +} + diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index d3b65dd2a2c..915a5c814ae 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -377,25 +377,12 @@ static DaemonKey key_from_service( const std::string& daemon_name) { if (!service_name.empty()) { - return DaemonKey(service_name, daemon_name); + return DaemonKey{service_name, daemon_name}; } else { - return DaemonKey(ceph_entity_type_name(peer_type), daemon_name); + return DaemonKey{ceph_entity_type_name(peer_type), daemon_name}; } } -static bool key_from_string( - const std::string& name, - DaemonKey *out) -{ - auto p = name.find('.'); - if (p == std::string::npos) { - return false; - } - out->first = name.substr(0, p); - out->second = name.substr(p + 1); - return true; -} - bool DaemonServer::handle_open(const ref_t& m) { std::lock_guard l(lock); @@ -498,11 +485,11 @@ bool DaemonServer::handle_report(const ref_t& m) { DaemonKey key; if (!m->service_name.empty()) { - key.first = m->service_name; + key.type = m->service_name; } else { - key.first = ceph_entity_type_name(m->get_connection()->get_peer_type()); + key.type = ceph_entity_type_name(m->get_connection()->get_peer_type()); } - key.second = m->daemon_name; + key.name = m->daemon_name; dout(10) << "from " << m->get_connection() << " " << key << dendl; @@ -533,22 +520,22 @@ bool DaemonServer::handle_report(const ref_t& m) << dendl; // issue metadata request in background if (!daemon_state.is_updating(key) && - (key.first == "osd" || key.first == "mds" || key.first == "mon")) { + (key.type == "osd" || key.type == "mds" || key.type == "mon")) { std::ostringstream oss; auto c = new MetadataUpdate(daemon_state, key); - if (key.first == "osd") { + if (key.type == "osd") { oss << "{\"prefix\": \"osd metadata\", \"id\": " - << key.second<< "}"; + << key.name<< "}"; - } else if (key.first == "mds") { + } else if (key.type == "mds") { c->set_default("addr", stringify(m->get_source_addr())); oss << "{\"prefix\": \"mds metadata\", \"who\": \"" - << key.second << "\"}"; + << key.name << "\"}"; - } else if (key.first == "mon") { + } else if (key.type == "mon") { oss << "{\"prefix\": \"mon metadata\", \"id\": \"" - << key.second << "\"}"; + << key.name << "\"}"; } else { ceph_abort(); } @@ -629,9 +616,7 @@ bool DaemonServer::handle_report(const ref_t& m) // if there are any schema updates, notify the python modules if (!m->declare_types.empty() || !m->undeclare_types.empty()) { - ostringstream oss; - oss << key.first << '.' << key.second; - py_modules.notify_all("perf_schema_update", oss.str()); + py_modules.notify_all("perf_schema_update", ceph::to_string(key)); } if (m->get_connection()->peer_is_osd()) { @@ -930,11 +915,11 @@ bool DaemonServer::_handle_command( f.reset(Formatter::create("json-pretty")); // only include state from services that are in the persisted service map f->open_object_section("service_status"); - for (auto& p : pending_service_map.services) { - f->open_object_section(p.first.c_str()); - for (auto& q : p.second.daemons) { + for (auto& [type, service] : pending_service_map.services) { + f->open_object_section(type.c_str()); + for (auto& q : service.daemons) { f->open_object_section(q.first.c_str()); - DaemonKey key(p.first, q.first); + DaemonKey key{type, q.first}; ceph_assert(daemon_state.exists(key)); auto daemon = daemon_state.get(key); std::lock_guard l(daemon->lock); @@ -1781,13 +1766,13 @@ bool DaemonServer::_handle_command( prefix == "config show-with-defaults") { string who; cmd_getval(g_ceph_context, cmdctx->cmdmap, "who", who); - int r = 0; - auto dot = who.find('.'); - DaemonKey key; - key.first = who.substr(0, dot); - key.second = who.substr(dot + 1); + auto [key, valid] = DaemonKey::parse(who); + if (!valid) { + ss << "invalid daemon name: use ."; + cmdctx->reply(-EINVAL, ss); + return true; + } DaemonStatePtr daemon = daemon_state.get(key); - string name; if (!daemon) { ss << "no config state for daemon " << who; cmdctx->reply(-ENOENT, ss); @@ -1796,6 +1781,8 @@ bool DaemonServer::_handle_command( std::lock_guard l(daemon->lock); + int r = 0; + string name; if (cmd_getval(g_ceph_context, cmdctx->cmdmap, "key", name)) { auto p = daemon->config.find(name); if (p != daemon->config.end() && @@ -1999,8 +1986,7 @@ bool DaemonServer::_handle_command( } else if (prefix == "device ls-by-daemon") { string who; cmd_getval(g_ceph_context, cmdctx->cmdmap, "who", who); - DaemonKey k; - if (!key_from_string(who, &k)) { + if (auto [k, valid] = DaemonKey::parse(who); !valid) { ss << who << " is not a valid daemon name"; r = -EINVAL; } else { @@ -2306,7 +2292,7 @@ void DaemonServer::_prune_pending_service_map() while (p != pending_service_map.services.end()) { auto q = p->second.daemons.begin(); while (q != p->second.daemons.end()) { - DaemonKey key(p->first, q->first); + DaemonKey key{p->first, q->first}; if (!daemon_state.exists(key)) { derr << "missing key " << key << dendl; ++q; @@ -2403,7 +2389,7 @@ void DaemonServer::send_report() if (acc == accumulated.end()) { auto collector = DaemonHealthMetricCollector::create(metric.get_type()); if (!collector) { - derr << __func__ << " " << key.first << "." << key.second + derr << __func__ << " " << key << " sent me an unknown health metric: " << std::hex << static_cast(metric.get_type()) << std::dec << dendl; @@ -2733,11 +2719,11 @@ void DaemonServer::got_service_map() }); // cull missing daemons, populate new ones - for (auto& p : pending_service_map.services) { + for (auto& [type, service] : pending_service_map.services) { std::set names; - for (auto& q : p.second.daemons) { + for (auto& q : service.daemons) { names.insert(q.first); - DaemonKey key(p.first, q.first); + DaemonKey key{type, q.first}; if (!daemon_state.exists(key)) { auto daemon = std::make_shared(daemon_state.types); daemon->key = key; @@ -2747,7 +2733,7 @@ void DaemonServer::got_service_map() dout(10) << "added missing " << key << dendl; } } - daemon_state.cull(p.first, names); + daemon_state.cull(type, names); } } @@ -2761,11 +2747,11 @@ void DaemonServer::got_mgr_map() auto c = new MetadataUpdate(daemon_state, key); // FIXME remove post-nautilus: include 'id' for luminous mons oss << "{\"prefix\": \"mgr metadata\", \"who\": \"" - << key.second << "\", \"id\": \"" << key.second << "\"}"; + << key.name << "\", \"id\": \"" << key.name << "\"}"; monc->start_mon_command({oss.str()}, {}, &c->outbl, &c->outs, c); }; if (mgrmap.active_name.size()) { - DaemonKey key("mgr", mgrmap.active_name); + DaemonKey key{"mgr", mgrmap.active_name}; have.insert(mgrmap.active_name); if (!daemon_state.exists(key) && !daemon_state.is_updating(key)) { md_update(key); @@ -2773,7 +2759,7 @@ void DaemonServer::got_mgr_map() } } for (auto& i : mgrmap.standbys) { - DaemonKey key("mgr", i.second.name); + DaemonKey key{"mgr", i.second.name}; have.insert(i.second.name); if (!daemon_state.exists(key) && !daemon_state.is_updating(key)) { md_update(key); diff --git a/src/mgr/DaemonState.cc b/src/mgr/DaemonState.cc index 3e3a30aeeb8..de519a2b921 100644 --- a/src/mgr/DaemonState.cc +++ b/src/mgr/DaemonState.cc @@ -13,6 +13,8 @@ #include "DaemonState.h" +#include + #include "MgrSession.h" #include "include/stringify.h" #include "common/Formatter.h" @@ -103,7 +105,7 @@ void DeviceState::dump(Formatter *f) const f->close_section(); f->open_array_section("daemons"); for (auto& i : daemons) { - f->dump_string("daemon", to_string(i)); + f->dump_stream("daemon") << i; } f->close_section(); if (life_expectancy.first != utime_t()) { @@ -120,11 +122,9 @@ void DeviceState::print(ostream& out) const for (auto& i : devnames) { out << "attachment " << i.first << ":" << i.second << "\n"; } - set d; - for (auto& j : daemons) { - d.insert(to_string(j)); - } - out << "daemons " << d << "\n"; + std::copy(std::begin(daemons), std::end(daemons), + std::experimental::make_ostream_joiner(out, ",")); + out << '\n'; if (life_expectancy.first != utime_t()) { out << "life_expectancy " << life_expectancy.first << " to " << life_expectancy.second @@ -188,9 +188,9 @@ DaemonStateCollection DaemonStateIndex::get_by_service( DaemonStateCollection result; - for (const auto &i : all) { - if (i.first.first == svc) { - result[i.first] = i.second; + for (const auto& [key, state] : all) { + if (key.type == svc) { + result[key] = state; } } @@ -251,10 +251,10 @@ void DaemonStateIndex::cull(const std::string& svc_name, auto end = all.end(); for (auto &i = begin; i != end; ++i) { const auto& daemon_key = i->first; - if (daemon_key.first != svc_name) + if (daemon_key.type != svc_name) break; - if (names_exist.count(daemon_key.second) == 0) { - victims.push_back(daemon_key.second); + if (names_exist.count(daemon_key.name) == 0) { + victims.push_back(daemon_key.name); } } diff --git a/src/mgr/DaemonState.h b/src/mgr/DaemonState.h index 356dcc6b111..3a0c13b712e 100644 --- a/src/mgr/DaemonState.h +++ b/src/mgr/DaemonState.h @@ -27,18 +27,12 @@ // For PerfCounterType #include "messages/MMgrReport.h" +#include "DaemonKey.h" namespace ceph { class Formatter; } -// Unique reference to a daemon within a cluster -typedef std::pair DaemonKey; - -static inline std::string to_string(const DaemonKey& dk) { - return dk.first + "." + dk.second; -} - // An instance of a performance counter type, within // a particular daemon. class PerfCounterInstance diff --git a/src/mgr/Mgr.cc b/src/mgr/Mgr.cc index f3730d813d2..27f74f2b06e 100644 --- a/src/mgr/Mgr.cc +++ b/src/mgr/Mgr.cc @@ -69,31 +69,27 @@ void MetadataUpdate::finish(int r) { daemon_state.clear_updating(key); if (r == 0) { - if (key.first == "mds" || key.first == "osd" || - key.first == "mgr" || key.first == "mon") { + if (key.type == "mds" || key.type == "osd" || + key.type == "mgr" || key.type == "mon") { json_spirit::mValue json_result; bool read_ok = json_spirit::read( outbl.to_str(), json_result); if (!read_ok) { - dout(1) << "mon returned invalid JSON for " - << key.first << "." << key.second << dendl; + dout(1) << "mon returned invalid JSON for " << key << dendl; return; } if (json_result.type() != json_spirit::obj_type) { - dout(1) << "mon returned valid JSON " - << key.first << "." << key.second + dout(1) << "mon returned valid JSON " << key << " but not an object: '" << outbl.to_str() << "'" << dendl; return; } - dout(4) << "mon returned valid metadata JSON for " - << key.first << "." << key.second << dendl; + dout(4) << "mon returned valid metadata JSON for " << key << dendl; json_spirit::mObject daemon_meta = json_result.get_obj(); // Skip daemon who doesn't have hostname yet if (daemon_meta.count("hostname") == 0) { - dout(1) << "Skipping incomplete metadata entry for " - << key.first << "." << key.second << dendl; + dout(1) << "Skipping incomplete metadata entry for " << key << dendl; return; } @@ -107,9 +103,9 @@ void MetadataUpdate::finish(int r) DaemonStatePtr state; if (daemon_state.exists(key)) { state = daemon_state.get(key); - if (key.first == "mds" || key.first == "mgr" || key.first == "mon") { + if (key.type == "mds" || key.type == "mgr" || key.type == "mon") { daemon_meta.erase("name"); - } else if (key.first == "osd") { + } else if (key.type == "osd") { daemon_meta.erase("id"); } daemon_meta.erase("hostname"); @@ -124,9 +120,9 @@ void MetadataUpdate::finish(int r) state->key = key; state->hostname = daemon_meta.at("hostname").get_str(); - if (key.first == "mds" || key.first == "mgr" || key.first == "mon") { + if (key.type == "mds" || key.type == "mgr" || key.type == "mon") { daemon_meta.erase("name"); - } else if (key.first == "osd") { + } else if (key.type == "osd") { daemon_meta.erase("id"); } daemon_meta.erase("hostname"); @@ -143,9 +139,8 @@ void MetadataUpdate::finish(int r) ceph_abort(); } } else { - dout(1) << "mon failed to return metadata for " - << key.first << "." << key.second << ": " - << cpp_strerror(r) << dendl; + dout(1) << "mon failed to return metadata for " << key + << ": " << cpp_strerror(r) << dendl; } } @@ -333,8 +328,8 @@ void Mgr::load_all_metadata() } DaemonStatePtr dm = std::make_shared(daemon_state.types); - dm->key = DaemonKey("mds", - daemon_meta.at("name").get_str()); + dm->key = DaemonKey{"mds", + daemon_meta.at("name").get_str()}; dm->hostname = daemon_meta.at("hostname").get_str(); daemon_meta.erase("name"); @@ -355,8 +350,8 @@ void Mgr::load_all_metadata() } DaemonStatePtr dm = std::make_shared(daemon_state.types); - dm->key = DaemonKey("mon", - daemon_meta.at("name").get_str()); + dm->key = DaemonKey{"mon", + daemon_meta.at("name").get_str()}; dm->hostname = daemon_meta.at("hostname").get_str(); daemon_meta.erase("name"); @@ -380,8 +375,8 @@ void Mgr::load_all_metadata() dout(4) << osd_metadata.at("hostname").get_str() << dendl; DaemonStatePtr dm = std::make_shared(daemon_state.types); - dm->key = DaemonKey("osd", - stringify(osd_metadata.at("id").get_int())); + dm->key = DaemonKey{"osd", + stringify(osd_metadata.at("id").get_int())}; dm->hostname = osd_metadata.at("hostname").get_str(); osd_metadata.erase("id"); @@ -440,7 +435,7 @@ void Mgr::handle_osd_map() // Consider whether to update the daemon metadata (new/restarted daemon) bool update_meta = false; - const auto k = DaemonKey("osd", stringify(osd_id)); + const auto k = DaemonKey{"osd", std::to_string(osd_id)}; if (daemon_state.is_updating(k)) { continue; } @@ -585,7 +580,7 @@ void Mgr::handle_fs_map(ref_t m) // Remember which MDS exists so that we can cull any that don't names_exist.insert(info.name); - const auto k = DaemonKey("mds", info.name); + const auto k = DaemonKey{"mds", info.name}; if (daemon_state.is_updating(k)) { continue; }