Skip to content

Commit

Permalink
[#3652] Remove the config history completely
Browse files Browse the repository at this point in the history 
- Removed revert() which makes no sense anymore.
- Since rollback() was significantly simplified, renamed it to
  clearStagingConfiguration().
- Removed kludgy ensureCurrentAllocated(). The only new required
  allocations are on constructor and on clear().
  • Loading branch information
@andrei-pavel
andrei-pavel committed Nov 28, 2024
1 parent 62013e9 commit 48f850a
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 223 deletions.
6 changes: 3 additions & 3 deletions src/bin/dhcp4/ctrl_dhcp4_srv.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ ControlledDhcpv4Srv::loadConfigFile(const std::string& file_name) {
} catch (const std::exception& ex) {
// If configuration failed at any stage, we drop the staging
// configuration and continue to use the previous one.
CfgMgr::instance().rollback();
CfgMgr::instance().clearStagingConfiguration();

LOG_ERROR(dhcp4_logger, DHCP4_CONFIG_LOAD_FAIL)
.arg(file_name).arg(ex.what());
Expand Down Expand Up @@ -372,7 +372,7 @@ ControlledDhcpv4Srv::commandConfigSetHandler(const string&,
// We are starting the configuration process so we should remove any
// staging configuration that has been created during previous
// configuration attempts.
CfgMgr::instance().rollback();
CfgMgr::instance().clearStagingConfiguration();

// Parse the logger configuration explicitly into the staging config.
// Note this does not alter the current loggers, they remain in
Expand Down Expand Up @@ -478,7 +478,7 @@ ControlledDhcpv4Srv::commandConfigTestHandler(const string&,
// We are starting the configuration process so we should remove any
// staging configuration that has been created during previous
// configuration attempts.
CfgMgr::instance().rollback();
CfgMgr::instance().clearStagingConfiguration();

// Now we check the server proper.
return (checkConfig(dhcp4));
Expand Down
1 change: 0 additions & 1 deletion src/bin/dhcp4/tests/config_parser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,6 @@ class Dhcp4ParserTest : public LogContentTest {
"\"dhcp-ddns\": { \"enable-updates\" : false }, "
"\"option-def\": [ ], "
"\"option-data\": [ ] }";
CfgMgr::instance().rollback();
static_cast<void>(executeConfiguration(config,
"reset configuration database"));
// The default setting is to listen on all interfaces. In order to
Expand Down
6 changes: 3 additions & 3 deletions src/bin/dhcp6/ctrl_dhcp6_srv.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ ControlledDhcpv6Srv::loadConfigFile(const std::string& file_name) {
} catch (const std::exception& ex) {
// If configuration failed at any stage, we drop the staging
// configuration and continue to use the previous one.
CfgMgr::instance().rollback();
CfgMgr::instance().clearStagingConfiguration();

LOG_ERROR(dhcp6_logger, DHCP6_CONFIG_LOAD_FAIL)
.arg(file_name).arg(ex.what());
Expand Down Expand Up @@ -375,7 +375,7 @@ ControlledDhcpv6Srv::commandConfigSetHandler(const string&,
// We are starting the configuration process so we should remove any
// staging configuration that has been created during previous
// configuration attempts.
CfgMgr::instance().rollback();
CfgMgr::instance().clearStagingConfiguration();

// Parse the logger configuration explicitly into the staging config.
// Note this does not alter the current loggers, they remain in
Expand Down Expand Up @@ -481,7 +481,7 @@ ControlledDhcpv6Srv::commandConfigTestHandler(const string&,
// We are starting the configuration process so we should remove any
// staging configuration that has been created during previous
// configuration attempts.
CfgMgr::instance().rollback();
CfgMgr::instance().clearStagingConfiguration();

// Now we check the server proper.
return (checkConfig(dhcp6));
Expand Down
1 change: 0 additions & 1 deletion src/bin/dhcp6/tests/config_parser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,6 @@ class Dhcp6ParserTest : public LogContentTest {
"\"dhcp-ddns\": { \"enable-updates\" : false }, "
"\"option-def\": [ ], "
"\"option-data\": [ ] }";
CfgMgr::instance().rollback();
static_cast<void>(executeConfiguration(config,
"reset configuration database"));
// The default setting is to listen on all interfaces. In order to
Expand Down
106 changes: 19 additions & 87 deletions src/lib/dhcpsrv/cfgmgr.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ using namespace isc::util;
namespace isc {
namespace dhcp {

// Value 0 effectively disables configuration history. Higher values can lead to proportionally
// increased memory usage which can be extreme in case of sizable configurations.
const size_t CfgMgr::CONFIG_LIST_SIZE = 0;
CfgMgr::CfgMgr()
: datadir_(DHCP_DATA_DIR, true), d2_client_mgr_(new D2ClientMgr()),
configuration_(new SrvConfig()), family_(AF_INET) {
}

CfgMgr&
CfgMgr::instance() {
Expand All @@ -41,7 +42,6 @@ CfgMgr::setDataDir(const std::string& datadir, bool unspecified) {

void
CfgMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
ensureCurrentAllocated();
// Note that D2ClientMgr::setD2Config() actually attempts to apply the
// configuration by stopping its sender and opening a new one and so
// forth per the new configuration.
Expand Down Expand Up @@ -69,40 +69,36 @@ CfgMgr::getD2ClientMgr() {
return (*d2_client_mgr_);
}

void
CfgMgr::ensureCurrentAllocated() {
if (!configuration_) {
configuration_.reset(new SrvConfig());
configs_.push_back(configuration_);
}
}

void
CfgMgr::clear() {
if (staging_configuration_) {
staging_configuration_.reset();
}
if (configuration_) {
configuration_->removeStatistics();
configuration_.reset();
configuration_.reset(new SrvConfig());
}
configs_.clear();
external_configs_.clear();
D2ClientConfigPtr d2_default_conf(new D2ClientConfig());
setD2ClientConfig(d2_default_conf);
}

void
CfgMgr::commit() {
ensureCurrentAllocated();
CfgMgr::clearStagingConfiguration() {
staging_configuration_.reset();
}

void
CfgMgr::commit() {
// First we need to remove statistics. The new configuration can have fewer
// subnets. Also, it may change subnet-ids. So we need to remove them all
// and add them back.
configuration_->removeStatistics();

bool promoted(false);
if (!configs_.empty() && !configs_.back()->sequenceEquals(*configuration_)) {
if (staging_configuration_ && !configuration_->sequenceEquals(*staging_configuration_)) {
// Promote the staging configuration to the current configuration.
configuration_ = configs_.back();
promoted = true;
configuration_ = staging_configuration_;
staging_configuration_.reset();
}

// Set the last commit timestamp.
Expand All @@ -113,74 +109,20 @@ CfgMgr::commit() {
configuration_->updateStatistics();

configuration_->configureLowerLevelLibraries();

if (promoted) {
// Keep track of the maximum size of the configs history. Remove the oldest elements.
if (configs_.size() > CONFIG_LIST_SIZE) {
SrvConfigList::const_iterator it = configs_.begin();
std::advance(it, configs_.size() - CONFIG_LIST_SIZE);
configs_.erase(configs_.begin(), it);
}
}
}

void
CfgMgr::rollback() {
ensureCurrentAllocated();
if (!configs_.empty() && !configuration_->sequenceEquals(*configs_.back())) {
configs_.pop_back();
}
}

void
CfgMgr::revert(const size_t index) {
ensureCurrentAllocated();
if (index == 0) {
isc_throw(isc::OutOfRange, "invalid commit index 0 when reverting"
" to an old configuration");
} else if (configs_.size() <= index) {
isc_throw(isc::OutOfRange, "unable to revert to commit index '"
<< index << "', only '" << (configs_.size() == 0 ? 0 : configs_.size() - 1)
<< "' previous commits available");
}

// Let's rollback an existing configuration to make sure that the last
// configuration on the list is the current one. Note that all remaining
// operations in this function should be exception free so there shouldn't
// be a problem that the revert operation fails and the staging
// configuration is destroyed by this rollback.
rollback();

if (!configs_.empty()) {
// Get the iterator to the current configuration and then advance to the
// desired one.
SrvConfigList::const_reverse_iterator it = configs_.rbegin();
std::advance(it, index);

// Copy the desired configuration to the new staging configuration. The
// staging configuration is re-created here because we rolled back earlier
// in this function.
(*it)->copy(*getStagingCfg());

// Make the staging configuration a current one.
commit();
}
}

SrvConfigPtr
CfgMgr::getCurrentCfg() {
ensureCurrentAllocated();
return (configuration_);
}

SrvConfigPtr
CfgMgr::getStagingCfg() {
ensureCurrentAllocated();
if (configs_.empty() || configuration_->sequenceEquals(*configs_.back())) {
if (!staging_configuration_ || configuration_->sequenceEquals(*staging_configuration_)) {
uint32_t sequence = configuration_->getSequence();
configs_.push_back(SrvConfigPtr(new SrvConfig(++sequence)));
staging_configuration_ = SrvConfigPtr(new SrvConfig(++sequence));
}
return (configs_.back());
return (staging_configuration_);
}

SrvConfigPtr
Expand Down Expand Up @@ -230,15 +172,5 @@ CfgMgr::mergeIntoCfg(const SrvConfigPtr& target_config, const uint32_t seq) {
}
}

CfgMgr::CfgMgr()
: datadir_(DHCP_DATA_DIR, true), d2_client_mgr_(new D2ClientMgr()), family_(AF_INET) {
// DHCP_DATA_DIR must be set set with -DDHCP_DATA_DIR="..." in Makefile.am
// Note: the definition of DHCP_DATA_DIR needs to include quotation marks
// See AM_CPPFLAGS definition in Makefile.am
}

CfgMgr::~CfgMgr() {
}

} // end of isc::dhcp namespace
} // end of isc namespace
83 changes: 12 additions & 71 deletions src/lib/dhcpsrv/cfgmgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,6 @@ class DuplicateListeningIface : public Exception {
/// @ref isc::dhcp::SimpleParser6::deriveParameters.
class CfgMgr : public boost::noncopyable {
public:

/// @brief A number of configurations held by @c CfgMgr.
///
/// @todo Make it configurable.
static const size_t CONFIG_LIST_SIZE;

/// @brief returns a single instance of Configuration Manager
///
/// CfgMgr is a singleton and this method is the only way of
Expand Down Expand Up @@ -127,18 +121,13 @@ class CfgMgr : public boost::noncopyable {
/// The following methods manage the process of preparing a configuration
/// without affecting a currently used configuration and then committing
/// the configuration to replace current configuration atomically.
/// They also allow for keeping a history of previous configurations so
/// as the @c CfgMgr can revert to the historical configuration when
/// required.
///
/// @todo Migrate all configuration parameters to use the model supported
/// by these functions.
///
/// @todo Make the size of the configurations history configurable.
///
//@{

/// @brief Removes current, staging and all previous configurations.
/// @brief Remove current, staging, and external configurations.
///
/// This function removes all configurations, including current,
/// staging and external configurations. It creates a new current
Expand All @@ -147,52 +136,18 @@ class CfgMgr : public boost::noncopyable {
/// This function is exception safe.
void clear();

/// @brief Commits the staging configuration.
///
/// The staging configuration becomes current configuration when this
/// function is called. It removes the oldest configurations held in the
/// history so as the size of the list of configuration does not exceed
/// the @c CONFIG_LIST_SIZE.
/// @brief Remove staging configuration.
///
/// This function is exception safe.
void commit();
void clearStagingConfiguration();

/// @brief Removes staging configuration.
/// @brief Commits the staging configuration.
///
/// This function should be called when there is a staging configuration
/// (likely created in the previous configuration attempt) but the entire
/// new configuration should be created. It removes the existing staging
/// configuration and the next call to @c CfgMgr::getStagingCfg will return a
/// fresh (default) configuration.
/// The staging configuration becomes current configuration when this
/// function is called.
///
/// This function is exception safe.
void rollback();

/// @brief Reverts to one of the previous configurations.
///
/// This function reverts to selected previous configuration. The previous
/// configuration is entirely copied to a new @c SrvConfig instance. This
/// new instance has a unique sequence id (sequence id is not copied). The
/// previous configuration (being copied) is not modified by this operation.
///
/// The configuration to be copied is identified by the index value which
/// is the distance between the current (most recent) and desired
/// configuration. If the index is out of range an exception is thrown.
///
/// @warning Revert operation will rollback any changes to the staging
/// configuration (if it exists).
///
/// @warning This function requires that the entire previous configuration
/// is copied to the new configuration object. This is not working for
/// some of the complex configuration objects, e.g. subnets. Hence, the
/// "revert" operation is not really usable at this point.
///
/// @param index A distance from the current configuration to the
/// past configuration to be reverted. The minimal value is 1 which points
/// to the nearest configuration.
///
/// @throw isc::OutOfRange if the specified index is out of range.
void revert(const size_t index);
void commit();

/// @brief Returns a pointer to the current configuration.
///
Expand Down Expand Up @@ -294,18 +249,9 @@ class CfgMgr : public boost::noncopyable {
CfgMgr();

/// @brief virtual destructor
virtual ~CfgMgr();
virtual ~CfgMgr() = default;

private:

/// @brief Checks if current configuration is created and creates it if needed.
///
/// This private method is called to ensure that the current configuration
/// is created. If current configuration is not set, it creates the
/// default current configuration.
void ensureCurrentAllocated();


/// @brief Merges external configuration with the given sequence number
/// into the specified configuration.
///
Expand All @@ -320,21 +266,16 @@ class CfgMgr : public boost::noncopyable {
/// @brief Manages the DHCP-DDNS client and its configuration.
D2ClientMgrPtr d2_client_mgr_;

/// @brief Server configuration
/// @brief Current server configuration
///
/// This is a structure that will hold all configuration.
/// @todo: migrate all other parameters to that structure.
SrvConfigPtr configuration_;

/// @name Configuration List.
/// @brief Staging server configuration
///
//@{
/// @brief Server configuration list type.
typedef std::list<SrvConfigPtr> SrvConfigList;

/// @brief Container holding all previous and current configurations.
SrvConfigList configs_;
//@}
/// This is a structure that holds configuration until it is applied.
SrvConfigPtr staging_configuration_;

/// @name Map of external configurations.
///
Expand Down
Loading

0 comments on commit 48f850a

Please sign in to comment.