hcq/MaxScale
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Move some Monitor fields to private and protected

Browse Source 
None of the fields should be publicly writable and some should not even
be writable from derived classes.
This commit is contained in:
Esa Korhonen
2019-05-08 18:43:37 +03:00
parent 5077ad62df
commit a9b1e3a442
7 changed files with 100 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
include/maxscale/monitor.hh
View File

@ -280,9 +280,6 @@ public:
Monitor(const std::string& name, const std::string& module);
virtual ~Monitor();
static const int STATUS_FLAG_NOCHECK = 0;
static const int STATUS_FLAG_CHECK = -1;
/**
* Ping or connect to a database. If connection does not exist or ping fails, a new connection
* is created. This will always leave a valid database handle in @c *ppCon, allowing the user
@ -329,6 +326,15 @@ public:
*/
virtual bool configure(const MXS_CONFIG_PARAMETER* params);
/**
* Get text-form settings.
*
* @return Monitor configuration parameters
*/
const MXS_CONFIG_PARAMETER& parameters() const;
int64_t ticks() const;
/**
* Starts the monitor. If the monitor requires polling of the servers, it should create
* a separate monitoring thread.
@ -416,13 +422,6 @@ public:
mutable std::mutex m_lock;
/** Set when admin requests a maintenance status change. */
int check_status_flag = STATUS_FLAG_NOCHECK;
uint64_t m_ticks {0}; /**< Number of performed monitoring intervals */
uint8_t m_journal_hash[SHA_DIGEST_LENGTH]; /**< SHA1 hash of the latest written journal */
MXS_CONFIG_PARAMETER parameters; /**< Configuration parameters */
std::vector<MonitorServer*> m_servers; /**< Monitored servers */
protected:
@ -529,6 +528,8 @@ protected:
*/
bool check_disk_space_this_tick();
bool server_status_request_waiting() const;
/**
* Contains monitor base class settings. Since monitors are stopped before a setting change,
* the items cannot be modified while a monitor is running. No locking required.
@ -552,7 +553,10 @@ protected:
MonitorServer::ConnectionSettings conn_settings;
};
Settings m_settings;
const Settings& settings() const;
/**< Number of monitor ticks ran. Derived classes should increment this whenever completing a tick. */
std::atomic_int64_t m_ticks {0};
private:
@ -583,6 +587,11 @@ private:
int get_data_file_path(char* path) const;
mxb::StopWatch m_disk_space_checked; /**< When was disk space checked the last time */
std::atomic_bool m_status_change_pending {false}; /**< Set when admin requests a status change. */
uint8_t m_journal_hash[SHA_DIGEST_LENGTH]; /**< SHA1 hash of the latest written journal */
MXS_CONFIG_PARAMETER m_parameters; /**< Configuration parameters in text form */
Settings m_settings; /**< Base class settings */
};
/**

2
server/core/config_runtime.cc
View File

@ -2505,7 +2505,7 @@ bool runtime_alter_monitor_from_json(Monitor* monitor, json_t* new_json)
mxb_assert(old_json.get());
const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), MODULE_MONITOR);
auto params = monitor->parameters;
auto params = monitor->parameters();
params.set_multiple(extract_parameters(new_json));
if (is_valid_resource_body(new_json)

65
server/core/monitor.cc
View File

@ -16,6 +16,7 @@
*/
#include <maxscale/monitor.hh>
#include <atomic>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
@ -30,7 +31,6 @@
#include <mutex>
#include <maxscale/alloc.h>
#include <maxbase/atomic.hh>
#include <maxscale/clock.h>
#include <maxscale/json_api.hh>
#include <maxscale/mariadb.hh>
@ -416,7 +416,8 @@ json_t* monitor_parameters_to_json(const Monitor* monitor)
{
json_t* rval = json_object();
const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), MODULE_MONITOR);
config_add_module_params_json(&monitor->parameters,
auto mon_config = monitor->parameters();
config_add_module_params_json(&mon_config,
{CN_TYPE, CN_MODULE, CN_SERVERS},
config_monitor_params,
mod->parameters,
@ -468,7 +469,7 @@ json_t* monitor_json_data(const Monitor* monitor, const char* host)
json_object_set_new(attr, CN_MODULE, json_string(monitor->m_module.c_str()));
json_object_set_new(attr, CN_STATE, json_string(monitor_state_to_string(monitor->state())));
json_object_set_new(attr, CN_TICKS, json_integer(monitor->m_ticks));
json_object_set_new(attr, CN_TICKS, json_integer(monitor->ticks()));
/** Monitor parameters */
json_object_set_new(attr, CN_PARAMETERS, monitor_parameters_to_json(monitor));
@ -578,13 +579,28 @@ bool Monitor::configure(const MXS_CONFIG_PARAMETER* params)
if (!error)
{
// Store module name into parameter storage.
parameters.set(CN_MODULE, m_module);
m_parameters.set(CN_MODULE, m_module);
// Add all config settings to text-mode storage. Needed for serialization.
parameters.set_multiple(*params);
m_parameters.set_multiple(*params);
}
return !error;
}
const MXS_CONFIG_PARAMETER& Monitor::parameters() const
{
return m_parameters;
}
const Monitor::Settings& Monitor::settings() const
{
return m_settings;
}
int64_t Monitor::ticks() const
{
return m_ticks.load(std::memory_order_acquire);
}
Monitor::~Monitor()
{
for (auto server : m_servers)
@ -653,7 +669,7 @@ void Monitor::show(DCB* dcb)
{
dcb_printf(dcb, "Name: %s\n", name());
dcb_printf(dcb, "State: %s\n", monitor_state_to_string(state()));
dcb_printf(dcb, "Times monitored: %lu\n", m_ticks);
dcb_printf(dcb, "Times monitored: %lu\n", ticks());
dcb_printf(dcb, "Sampling interval: %lu milliseconds\n", m_settings.interval);
dcb_printf(dcb, "Connect Timeout: %i seconds\n", m_settings.conn_settings.connect_timeout);
dcb_printf(dcb, "Read Timeout: %i seconds\n", m_settings.conn_settings.read_timeout);
@ -1356,19 +1372,17 @@ void Monitor::check_maintenance_requests()
{
/* In theory, the admin may be modifying the server maintenance status during this function. The overall
* maintenance flag should be read-written atomically to prevent missing a value. */
int flags_changed = atomic_exchange_int(&check_status_flag, Monitor::STATUS_FLAG_NOCHECK);
if (flags_changed != Monitor::STATUS_FLAG_NOCHECK)
bool was_pending = m_status_change_pending.exchange(false, std::memory_order_acq_rel);
if (was_pending)
{
for (auto ptr : m_servers)
{
// The only server status bit the admin may change is the [Maintenance] bit.
int admin_msg = atomic_exchange_int(&ptr->status_request,
MonitorServer::NO_CHANGE);
// The admin can only modify the [Maintenance] and [Drain] bits.
int admin_msg = atomic_exchange_int(&ptr->status_request, MonitorServer::NO_CHANGE);
switch (admin_msg)
{
case MonitorServer::MAINT_ON:
// TODO: Change to writing MONITORED_SERVER->pending status instead once cleanup done.
ptr->server->set_status(SERVER_MAINT);
break;
@ -1692,13 +1706,13 @@ std::vector<MonitorServer*> Monitor::get_monitored_serverlist(const string& key,
{
std::vector<MonitorServer*> monitored_array;
// Check that value exists.
if (!parameters.contains(key))
if (!m_parameters.contains(key))
{
return monitored_array;
}
string name_error;
auto servers = parameters.get_server_list(key, &name_error);
auto servers = m_parameters.get_server_list(key, &name_error);
if (!servers.empty())
{
// All servers in the array must be monitored by the given monitor.
@ -1793,7 +1807,7 @@ bool Monitor::set_server_status(SERVER* srv, int bit, string* errmsg_out)
MXS_WARNING(WRN_REQUEST_OVERWRITTEN);
}
// Also set a flag so the next loop happens sooner.
atomic_store_int(&this->check_status_flag, Monitor::STATUS_FLAG_CHECK);
m_status_change_pending.store(true, std::memory_order_release);
}
}
else
@ -1851,7 +1865,7 @@ bool Monitor::clear_server_status(SERVER* srv, int bit, string* errmsg_out)
MXS_WARNING(WRN_REQUEST_OVERWRITTEN);
}
// Also set a flag so the next loop happens sooner.
atomic_store_int(&this->check_status_flag, Monitor::STATUS_FLAG_CHECK);
m_status_change_pending.store(true, std::memory_order_release);
}
}
else
@ -1898,6 +1912,11 @@ bool Monitor::check_disk_space_this_tick()
return should_update_disk_space;
}
bool Monitor::server_status_request_waiting() const
{
return m_status_change_pending.load(std::memory_order_acquire);
}
MonitorWorker::MonitorWorker(const string& name, const string& module)
: Monitor(name, module)
, m_thread_running(false)
@ -1973,7 +1992,7 @@ bool MonitorWorker::start()
bool started = false;
if (m_checked)
{
m_loop_called = get_time_ms() - m_settings.interval; // Next tick should happen immediately.
m_loop_called = get_time_ms() - settings().interval; // Next tick should happen immediately.
if (!Worker::start())
{
MXS_ERROR("Failed to start worker for monitor '%s'.", name());
@ -2168,7 +2187,7 @@ void MonitorWorkerSimple::tick()
pMs->mon_prev_status = pMs->server->status;
pMs->pending_status = pMs->server->status;
mxs_connect_result_t rval = pMs->ping_or_connect(m_settings.conn_settings);
mxs_connect_result_t rval = pMs->ping_or_connect(settings().conn_settings);
if (connection_is_ok(rval))
{
@ -2289,9 +2308,9 @@ bool MonitorWorker::call_run_one_tick(Worker::Call::action_t action)
{
int64_t now = get_time_ms();
// Enough time has passed,
if ((now - m_loop_called > m_settings.interval)
// or maintenance flag is set,
|| atomic_load_int(&this->check_status_flag) == Monitor::STATUS_FLAG_CHECK
if ((now - m_loop_called > settings().interval)
// or a server status change request is waiting,
|| server_status_request_waiting()
// or a monitor-specific condition is met.
|| immediate_tick_required())
{
@ -2300,7 +2319,7 @@ bool MonitorWorker::call_run_one_tick(Worker::Call::action_t action)
now = get_time_ms();
}
int64_t ms_to_next_call = m_settings.interval - (now - m_loop_called);
int64_t ms_to_next_call = settings().interval - (now - m_loop_called);
// ms_to_next_call will be negative, if the run_one_tick() call took
// longer than one monitor interval.
int64_t delay = ((ms_to_next_call <= 0) || (ms_to_next_call >= base_interval_ms)) ?
@ -2314,7 +2333,7 @@ bool MonitorWorker::call_run_one_tick(Worker::Call::action_t action)
void MonitorWorker::run_one_tick()
{
tick();
mxb::atomic::add(&m_ticks, 1, mxb::atomic::RELAXED);
m_ticks.fetch_add(1, std::memory_order_acq_rel);
}
bool MonitorWorker::immediate_tick_required() const

20
server/core/monitormanager.cc
View File

@ -127,11 +127,11 @@ void MonitorManager::debug_wait_one_tick()
{
mxb_assert(Monitor::is_admin_thread());
using namespace std::chrono;
std::map<Monitor*, uint64_t> ticks;
std::map<Monitor*, int64_t> ticks;
// Get tick values for all monitors
this_unit.foreach_monitor([&ticks](Monitor* mon) {
ticks[mon] = mxb::atomic::load(&mon->m_ticks);
ticks[mon] = mon->ticks();
return true;
});
@ -140,14 +140,12 @@ void MonitorManager::debug_wait_one_tick()
if (mon->state() == MONITOR_STATE_RUNNING)
{
auto start = steady_clock::now();
// A monitor may have been added in between the two foreach-calls (not
// if config changes are
// A monitor may have been added in between the two foreach-calls (not if config changes are
// serialized). Check if entry exists.
if (ticks.count(mon) > 0)
{
auto tick = ticks[mon];
while (mxb::atomic::load(&mon->m_ticks) == tick
&& (steady_clock::now() - start < seconds(60)))
while (mon->ticks() == tick && (steady_clock::now() - start < seconds(60)))
{
std::this_thread::sleep_for(milliseconds(100));
}
@ -351,7 +349,7 @@ bool MonitorManager::create_monitor_config(const Monitor* monitor, const char* f
const MXS_MODULE* mod = get_module(monitor->m_module.c_str(), NULL);
mxb_assert(mod);
string config = generate_config_string(monitor->m_name, monitor->parameters,
string config = generate_config_string(monitor->m_name, monitor->parameters(),
config_monitor_params, mod->parameters);
if (dprintf(file, "%s", config.c_str()) == -1)
@ -412,7 +410,7 @@ bool MonitorManager::reconfigure_monitor(mxs::Monitor* monitor, const MXS_CONFIG
{
mxb_assert(Monitor::is_admin_thread());
// Backup monitor parameters in case configure fails.
auto orig = monitor->parameters;
auto orig = monitor->parameters();
// Stop/start monitor if it's currently running. If monitor was stopped already, this is likely
// managed by the caller.
bool stopstart = (monitor->state() == MONITOR_STATE_RUNNING);
@ -451,7 +449,7 @@ bool MonitorManager::alter_monitor(mxs::Monitor* monitor, const std::string& key
return false;
}
MXS_CONFIG_PARAMETER modified = monitor->parameters;
MXS_CONFIG_PARAMETER modified = monitor->parameters();
modified.set(key, value);
bool success = MonitorManager::reconfigure_monitor(monitor, modified);
@ -570,7 +568,7 @@ bool MonitorManager::add_server_to_monitor(mxs::Monitor* mon, SERVER* server, st
// To keep monitor modifications straightforward, all changes should go through the same
// reconfigure-function. As the function accepts key-value combinations (so that they are easily
// serialized), construct the value here.
MXS_CONFIG_PARAMETER modified_params = mon->parameters;
MXS_CONFIG_PARAMETER modified_params = mon->parameters();
string serverlist = modified_params.get_string(CN_SERVERS);
if (serverlist.empty())
{
@ -615,7 +613,7 @@ bool MonitorManager::remove_server_from_monitor(mxs::Monitor* mon, SERVER* serve
else
{
// Construct the new server list
auto params = mon->parameters;
auto params = mon->parameters();
auto names = config_break_list_string(params.get_string(CN_SERVERS));
names.erase(std::remove(names.begin(), names.end(), server->name()));
std::string servers = mxb::join(names, ",");

12
server/modules/monitor/clustrixmon/clustrixmonitor.cc
View File

@ -375,7 +375,7 @@ bool ClustrixMonitor::choose_dynamic_hub(Clustrix::Softfailed softfailed, std::s
{
ClustrixNode& node = kv.second;
if (node.can_be_used_as_hub(name(), m_settings.conn_settings, softfailed))
if (node.can_be_used_as_hub(name(), settings().conn_settings, softfailed))
{
m_pHub_con = node.release_connection();
m_pHub_server = node.server();
@ -398,7 +398,7 @@ bool ClustrixMonitor::choose_bootstrap_hub(Clustrix::Softfailed softfailed, std:
{
if (ips_checked.find(pMs->server->address) == ips_checked.end())
{
if (Clustrix::ping_or_connect_to_hub(name(), m_settings.conn_settings, softfailed, *pMs))
if (Clustrix::ping_or_connect_to_hub(name(), settings().conn_settings, softfailed, *pMs))
{
m_pHub_con = pMs->con;
m_pHub_server = pMs->server;
@ -433,8 +433,8 @@ bool ClustrixMonitor::refresh_using_persisted_nodes(std::set<string>& ips_checke
if (rv == SQLITE_OK)
{
const std::string& username = m_settings.conn_settings.username;
const std::string& password = m_settings.conn_settings.password;
const std::string& username = settings().conn_settings.username;
const std::string& password = settings().conn_settings.password;
char* zPassword = decrypt_password(password.c_str());
auto it = nodes.begin();
@ -806,7 +806,7 @@ void ClustrixMonitor::check_hub(Clustrix::Softfailed softfailed)
mxb_assert(m_pHub_con);
mxb_assert(m_pHub_server);
if (!Clustrix::ping_or_connect_to_hub(name(), m_settings.conn_settings, softfailed,
if (!Clustrix::ping_or_connect_to_hub(name(), settings().conn_settings, softfailed,
*m_pHub_server, &m_pHub_con))
{
mysql_close(m_pHub_con);
@ -998,7 +998,7 @@ void ClustrixMonitor::initiate_delayed_http_check()
{
mxb_assert(m_delayed_http_check_id == 0);
long max_delay_ms = m_settings.interval / 10;
long max_delay_ms = settings().interval / 10;
long ms = m_http.wait_no_more_than();

6
server/modules/monitor/mariadbmon/mariadbmon.cc
View File

@ -182,8 +182,8 @@ bool MariaDBMonitor::set_replication_credentials(const MXS_CONFIG_PARAMETER* par
else
{
// Ok, neither is set. Use monitor credentials.
repl_user = m_settings.conn_settings.username;
repl_pw = m_settings.conn_settings.password;
repl_user = settings().conn_settings.username;
repl_pw = settings().conn_settings.password;
}
}
@ -395,7 +395,7 @@ void MariaDBMonitor::tick()
// Query all servers for their status.
bool should_update_disk_space = check_disk_space_this_tick();
const auto& conn_settings = m_settings.conn_settings;
const auto& conn_settings = settings().conn_settings;
auto update_task = [should_update_disk_space, conn_settings](MariaDBServer* server) {
server->update_server(should_update_disk_space, conn_settings);

2
server/modules/monitor/mariadbmon/test/test_cycle_find.cc
View File

@ -159,7 +159,7 @@ void MariaDBMonitor::Test::init_servers(int count)
{
// Server contents mostly undefined
auto base_server = Server::create_test_server();
MonitorServer* mon_server = new MonitorServer(base_server, m_monitor->m_settings.disk_space_limits);
MonitorServer* mon_server = new MonitorServer(base_server, m_monitor->settings().disk_space_limits);
MariaDBServer* mariadb_server = new MariaDBServer(mon_server, i - 1, m_use_hostnames, true);
if (m_use_hostnames)