From 48f850a02c5622805ab3513dca6c838e73bb030b Mon Sep 17 00:00:00 2001 From: Andrei Pavel Date: Thu, 28 Nov 2024 10:22:54 +0200 Subject: [PATCH] [#3652] Remove the config history completely - 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(). --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 6 +- src/bin/dhcp4/tests/config_parser_unittest.cc | 1 - src/bin/dhcp6/ctrl_dhcp6_srv.cc | 6 +- src/bin/dhcp6/tests/config_parser_unittest.cc | 1 - src/lib/dhcpsrv/cfgmgr.cc | 106 ++++-------------- src/lib/dhcpsrv/cfgmgr.h | 83 ++------------ src/lib/dhcpsrv/tests/cfgmgr_unittest.cc | 61 +--------- 7 files changed, 41 insertions(+), 223 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 8a37e75d9e..4d56eb2fe8 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -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()); @@ -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 @@ -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)); diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 0527188065..06d3eac287 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -751,7 +751,6 @@ class Dhcp4ParserTest : public LogContentTest { "\"dhcp-ddns\": { \"enable-updates\" : false }, " "\"option-def\": [ ], " "\"option-data\": [ ] }"; - CfgMgr::instance().rollback(); static_cast(executeConfiguration(config, "reset configuration database")); // The default setting is to listen on all interfaces. In order to diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 4e031e3edc..40a0db6bdc 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -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()); @@ -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 @@ -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)); diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 08384d1add..015c697152 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -881,7 +881,6 @@ class Dhcp6ParserTest : public LogContentTest { "\"dhcp-ddns\": { \"enable-updates\" : false }, " "\"option-def\": [ ], " "\"option-data\": [ ] }"; - CfgMgr::instance().rollback(); static_cast(executeConfiguration(config, "reset configuration database")); // The default setting is to listen on all interfaces. In order to diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc index cdd3c307ea..7c437168e1 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/src/lib/dhcpsrv/cfgmgr.cc @@ -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() { @@ -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. @@ -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. @@ -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 @@ -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 diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index d5492ed29b..bfec788807 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/src/lib/dhcpsrv/cfgmgr.h @@ -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 @@ -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 @@ -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. /// @@ -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. /// @@ -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 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. /// diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index e6caa3ae55..e066d9b1c0 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc @@ -424,8 +424,8 @@ TEST_F(CfgMgrTest, d2ClientConfig) { ASSERT_NO_THROW(CfgMgr::instance().setD2ClientConfig(original_config)); } -// This test verifies that the configuration staging, commit and rollback works -// as expected. +// This test verifies that the configuration staging works as expected, including +// committing it. TEST_F(CfgMgrTest, staging) { CfgMgr& cfg_mgr = CfgMgr::instance(); // Initially, the current configuration is a default one. We are going @@ -505,8 +505,8 @@ TEST_F(CfgMgrTest, staging) { // The modified staging configuration should have one logger configured. ASSERT_EQ(1, config->getLoggingInfo().size()); - // Rollback should remove a staging configuration, including the logger. - ASSERT_NO_THROW(cfg_mgr.rollback()); + // Remove the staging configuration, including the logger. + ASSERT_NO_THROW(cfg_mgr.clearStagingConfiguration()); // Make sure that the logger is not set. This is an indication that the // rollback worked. @@ -515,59 +515,6 @@ TEST_F(CfgMgrTest, staging) { EXPECT_EQ(0, config->getLoggingInfo().size()); } -// This test verifies that it is possible to revert to an old configuration. -TEST_F(CfgMgrTest, revert) { - CfgMgr& cfg_mgr = CfgMgr::instance(); - // Let's create 5 unique configurations: differing by a debug level in the - // range of 10 to 14. - for (int i = 0; i < 5; ++i) { - SrvConfigPtr config = cfg_mgr.getStagingCfg(); - LoggingInfo logging_info; - logging_info.debuglevel_ = i + 10; - config->addLoggingInfo(logging_info); - cfg_mgr.commit(); - } - - // Now we have 6 configurations with: - // - debuglevel = 99 (a default one) - // - debuglevel = 10 - // - debuglevel = 11 - // - debuglevel = 12 - // - debuglevel = 13 - // - debuglevel = 14 (current) - - // Hence, the maximum index of the configuration to revert is 5 (which - // points to the configuration with debuglevel = 99). For the index greater - // than 5 we should get an exception. - ASSERT_THROW(cfg_mgr.revert(6), isc::OutOfRange); - // Value of 0 also doesn't make sense. - ASSERT_THROW(cfg_mgr.revert(0), isc::OutOfRange); - - // Return early because the checks that follow simply are not supported by - // CONFIG_LIST_SIZE == 0 at the time of writing. - return; - - // We should be able to revert to configuration with debuglevel = 10. - ASSERT_NO_THROW(cfg_mgr.revert(4)); - // And this configuration should be now the current one and the debuglevel - // of this configuration is 10. - EXPECT_EQ(10, cfg_mgr.getCurrentCfg()->getLoggingInfo()[0].debuglevel_); - EXPECT_NE(cfg_mgr.getCurrentCfg()->getSequence(), 1); - - // The new set of configuration is now as follows: - // - debuglevel = 99 - // - debuglevel = 10 - // - debuglevel = 11 - // - debuglevel = 12 - // - debuglevel = 13 - // - debuglevel = 14 - // - debuglevel = 10 (current) - // So, reverting to configuration having index 3 means that the debug level - // of the current configuration will become 12. - ASSERT_NO_THROW(cfg_mgr.revert(3)); - EXPECT_EQ(12, cfg_mgr.getCurrentCfg()->getLoggingInfo()[0].debuglevel_); -} - // This test verifies that the address family can be set and obtained // from the configuration manager. TEST_F(CfgMgrTest, family) {