[3534] Basic implementation of staging and rolling back configurations.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -149,8 +149,9 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) {
     // safe and we really don't want to emit exceptions to whoever called this
     // method. Instead, catch an exception and create appropriate answer.
     try {
-        CfgMgr::instance().getConfiguration()->cfg_iface_
-            .openSockets(srv->getPort(), getInstance()->useBroadcast());
+        CfgMgr::instance().getCurrent()->getCfgIface()
+            .openSockets(CfgIface::V4, srv->getPort(),
+                         getInstance()->useBroadcast());
 
     } catch (std::exception& ex) {
         err << "failed to open sockets after server reconfiguration: "

src/bin/dhcp4/json_config_parser.cc

@@ -443,7 +443,7 @@ namespace dhcp {
         parser = new Uint32Parser(config_id,
                                  globalContext()->uint32_values_);
     } else if (config_id.compare("interfaces") == 0) {
-        parser = new InterfaceListConfigParser(config_id);
+        parser = new InterfaceListConfigParser(config_id, globalContext());
     } else if (config_id.compare("subnet4") == 0) {
         parser = new Subnets4ListConfigParser(config_id);
     } else if (config_id.compare("option-data") == 0) {
@@ -619,9 +619,8 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 subnet_parser->commit();
             }
 
-            if (iface_parser) {
-                iface_parser->commit();
-            }
+            // No need to commit interface names as this is handled by the
+            // CfgMgr::commit() function.
 
             // Apply global options
             commitGlobalOptions();
@@ -649,6 +648,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     // Rollback changes as the configuration parsing failed.
     if (rollback) {
         globalContext().reset(new ParserContext(original_context));
+        CfgMgr::instance().rollback();
         return (answer);
     }
 

src/bin/dhcp4/kea_controller.cc

@@ -41,6 +41,11 @@ void configure(const std::string& file_name) {
     // This is a configuration backend implementation that reads the
     // configuration from a JSON file.
 
+    // We are starting the configuration process so we should remove any
+    // staging configuration that has been created during previous
+    // configuration attempts.
+    CfgMgr::instance().rollback();
+
     isc::data::ConstElementPtr json;
     isc::data::ConstElementPtr dhcp4;
     isc::data::ConstElementPtr logger;
@@ -66,7 +71,7 @@ void configure(const std::string& file_name) {
         // If there's no logging element, we'll just pass NULL pointer,
         // which will be handled by configureLogger().
         Daemon::configureLogger(json->get("Logging"),
-                                CfgMgr::instance().getConfiguration(),
+                                CfgMgr::instance().getStaging(),
                                 ControlledDhcpv4Srv::getInstance()->getVerbose());
 
         // Get Dhcp4 component from the config

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -2917,7 +2917,8 @@ TEST_F(Dhcp4ParserTest, selectedInterfaces) {
     ASSERT_TRUE(status);
     checkResult(status, 0);
 
-    CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000);
+    CfgMgr::instance().getStaging()->
+        getCfgIface().openSockets(CfgIface::V4, 10000);
 
     // eth0 and eth1 were explicitly selected. eth2 was not.
     EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET));
@@ -2952,7 +2953,8 @@ TEST_F(Dhcp4ParserTest, allInterfaces) {
     ASSERT_TRUE(status);
     checkResult(status, 0);
 
-    CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000);
+    CfgMgr::instance().getStaging()->
+        getCfgIface().openSockets(CfgIface::V4, 10000);
 
     // All interfaces should be now active.
     ASSERT_TRUE(test_config.socketOpen("eth0", AF_INET));

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -45,7 +45,7 @@ Dhcpv4SrvTest::Dhcpv4SrvTest()
     pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), IOAddress("192.0.2.110")));
     subnet_->addPool(pool_);
 
-    CfgMgr::instance().getConfiguration()->cfg_iface_.reset();
+    CfgMgr::instance().clear();
     CfgMgr::instance().deleteSubnets4();
     CfgMgr::instance().addSubnet4(subnet_);
 
@@ -58,7 +58,7 @@ Dhcpv4SrvTest::Dhcpv4SrvTest()
 Dhcpv4SrvTest::~Dhcpv4SrvTest() {
 
     // Make sure that we revert to default value
-    CfgMgr::instance().getConfiguration()->cfg_iface_.reset();
+    CfgMgr::instance().clear();
     CfgMgr::instance().echoClientId(true);
 }
 

src/bin/dhcp6/ctrl_dhcp6_srv.cc

@@ -144,8 +144,8 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) {
     // safe and we really don't want to emit exceptions to the callback caller.
     // Instead, catch an exception and create appropriate answer.
     try {
-        CfgMgr::instance().getConfiguration()->cfg_iface_
-            .openSockets(srv->getPort());
+        CfgMgr::instance().getConfiguration()->getCfgIface()
+            .openSockets(CfgIface::V6, srv->getPort());
 
     } catch (const std::exception& ex) {
         std::ostringstream err;

src/bin/dhcp6/dhcp6_srv.cc

@@ -150,12 +150,6 @@ Dhcpv6Srv::Dhcpv6Srv(uint16_t port)
         // Instantiate allocation engine
         alloc_engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100));
 
-        // We have to point out to the CfgMgr that the we are in the IPv6
-        // domain, so as the IPv6 sockets are opened rather than IPv4 sockets
-        // which are the default.
-        CfgMgr::instance().getConfiguration()
-            ->cfg_iface_.setFamily(CfgIface::V6);
-
         /// @todo call loadLibraries() when handling configuration changes
 
     } catch (const std::exception &e) {

src/bin/dhcp6/json_config_parser.cc

@@ -662,7 +662,7 @@ namespace dhcp {
         parser = new Uint32Parser(config_id,
                                  globalContext()->uint32_values_);
     } else if (config_id.compare("interfaces") == 0) {
-        parser = new InterfaceListConfigParser(config_id);
+        parser = new InterfaceListConfigParser(config_id, globalContext());
     } else if (config_id.compare("subnet6") == 0) {
         parser = new Subnets6ListConfigParser(config_id);
     } else if (config_id.compare("option-data") == 0) {
@@ -821,9 +821,8 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
                 subnet_parser->commit();
             }
 
-            if (iface_parser) {
-                iface_parser->commit();
-            }
+            // No need to commit interface names as this is handled by the
+            // CfgMgr::commit() function.
 
             // This occurs last as if it succeeds, there is no easy way to
             // revert it.  As a result, the failure to commit a subsequent
@@ -850,6 +849,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
     // Rollback changes as the configuration parsing failed.
     if (rollback) {
         globalContext().reset(new ParserContext(original_context));
+        CfgMgr::instance().rollback();
         return (answer);
     }
 

src/bin/dhcp6/kea_controller.cc

@@ -45,6 +45,11 @@ void configure(const std::string& file_name) {
     // This is a configuration backend implementation that reads the
     // configuration from a JSON file.
 
+    // We are starting the configuration process so we should remove any
+    // staging configuration that has been created during previous
+    // configuration attempts.
+    CfgMgr::instance().rollback();
+
     isc::data::ConstElementPtr json;
     isc::data::ConstElementPtr dhcp6;
     isc::data::ConstElementPtr logger;

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -359,9 +359,7 @@ public:
         // properly test interface configuration we disable listening on
         // all interfaces before each test and later check that this setting
         // has been overriden by the configuration used in the test.
-        CfgMgr::instance().getConfiguration()->cfg_iface_.reset();
-        CfgMgr::instance().getConfiguration()->
-            cfg_iface_.setFamily(CfgIface::V6);
+        CfgMgr::instance().clear();
         // Create fresh context.
         globalContext()->copyContext(ParserContext(Option::V6));
     }
@@ -3057,7 +3055,8 @@ TEST_F(Dhcp6ParserTest, selectedInterfaces) {
     // as the pool does not belong to that subnet
     checkResult(status, 0);
 
-    CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000);
+    CfgMgr::instance().getStaging()->
+        getCfgIface().openSockets(CfgIface::V6, 10000);
 
     // eth0 and eth1 were explicitly selected. eth2 was not.
     EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET6));
@@ -3075,7 +3074,7 @@ TEST_F(Dhcp6ParserTest, allInterfaces) {
     ConstElementPtr status;
 
     // This configuration specifies two interfaces on which server should listen
-    // bu also includes keyword 'all'. This keyword switches server into the
+    // but also includes '*'. This keyword switches server into the
     // mode when it listens on all interfaces regardless of what interface names
     // were specified in the "interfaces" parameter.
     string config = "{ \"interfaces\": [ \"eth0\", \"eth1\", \"*\" ],"
@@ -3090,7 +3089,8 @@ TEST_F(Dhcp6ParserTest, allInterfaces) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
     checkResult(status, 0);
 
-    CfgMgr::instance().getConfiguration()->cfg_iface_.openSockets(10000);
+    CfgMgr::instance().getStaging()->
+        getCfgIface().openSockets(CfgIface::V6, 10000);
 
     // All interfaces should be now active.
     EXPECT_TRUE(test_config.socketOpen("eth0", AF_INET6));

src/lib/dhcpsrv/cfg_iface.cc

@@ -25,24 +25,24 @@ namespace dhcp {
 
 const char* CfgIface::ALL_IFACES_KEYWORD = "*";
 
-CfgIface::CfgIface(Family family)
-    : family_(family),
-      wildcard_used_(false) {
+CfgIface::CfgIface()
+    : wildcard_used_(false) {
 }
 
 void
-CfgIface::closeSockets() {
+CfgIface::closeSockets() const {
     IfaceMgr::instance().closeSockets();
 }
 
 void
-CfgIface::openSockets(const uint16_t port, const bool use_bcast) {
+CfgIface::openSockets(const Family& family, const uint16_t port,
+                      const bool use_bcast) const {
     // If wildcard interface '*' was not specified, set all interfaces to
     // inactive state. We will later enable them selectively using the
     // interface names specified by the user. If wildcard interface was
     // specified, mark all interfaces active. In all cases, mark loopback
     // inactive.
-    setState(!wildcard_used_, true);
+    setState(family, !wildcard_used_, true);
     // Remove selection of unicast addresses from all interfaces.
     IfaceMgr::instance().clearUnicasts();
     // If there is no wildcard interface specified, we will have to iterate
@@ -61,7 +61,7 @@ CfgIface::openSockets(const uint16_t port, const bool use_bcast) {
                           << *iface_name << "' as this interface doesn't"
                           " exist");
 
-            } else if (getFamily() == V4) {
+            } else if (family == V4) {
                 iface->inactive4_ = false;
 
             } else {
@@ -71,7 +71,7 @@ CfgIface::openSockets(const uint16_t port, const bool use_bcast) {
     }
 
     // Select unicast sockets. It works only for V6. Ignore for V4.
-    if (getFamily() == V6) {
+    if (family == V6) {
         for (UnicastMap::const_iterator unicast = unicast_map_.begin();
              unicast != unicast_map_.end(); ++unicast) {
             Iface* iface = IfaceMgr::instance().getIface(unicast->first);
@@ -95,7 +95,7 @@ CfgIface::openSockets(const uint16_t port, const bool use_bcast) {
     IfaceMgrErrorMsgCallback error_callback =
         boost::bind(&CfgIface::socketOpenErrorHandler, _1);
     bool sopen;
-    if (getFamily() == V4) {
+    if (family == V4) {
         sopen = IfaceMgr::instance().openSockets4(port, use_bcast,
                                                   error_callback);
     } else {
@@ -117,12 +117,13 @@ CfgIface::reset() {
 }
 
 void
-CfgIface::setState(const bool inactive, const bool loopback_inactive) {
+CfgIface::setState(const Family& family, const bool inactive,
+                   const bool loopback_inactive) const {
     IfaceMgr::IfaceCollection ifaces = IfaceMgr::instance().getIfaces();
     for (IfaceMgr::IfaceCollection::iterator iface = ifaces.begin();
          iface != ifaces.end(); ++iface) {
         Iface* iface_ptr = IfaceMgr::instance().getIface(iface->getName());
-        if (getFamily() == V4) {
+        if (family == V4) {
             iface_ptr->inactive4_ = iface_ptr->flag_loopback_ ?
                 loopback_inactive : inactive;
         } else {
@@ -138,7 +139,7 @@ CfgIface::socketOpenErrorHandler(const std::string& errmsg) {
 }
 
 void
-CfgIface::use(const std::string& iface_name) {
+CfgIface::use(const Family& family, const std::string& iface_name) {
     // The interface name specified may have two formats, e.g.:
     // - eth0
     // - eth0/2001:db8:1::1.
@@ -184,7 +185,7 @@ CfgIface::use(const std::string& iface_name) {
 
         }
 
-    } else if (getFamily() == V4) {
+    } else if (family == V4) {
         isc_throw(InvalidIfaceName, "unicast addresses in the format of: "
                   "iface-name/unicast-addr_stress can only be specified for"
                   " IPv6 addr_stress family");

src/lib/dhcpsrv/cfg_iface.h

@@ -81,17 +81,10 @@ public:
     };
 
     /// @brief Constructor.
-    ///
-    /// @param family Protocol family (default is V4).
-    CfgIface(Family family = V4);
+    CfgIface();
 
     /// @brief Convenience function which closes all open sockets.
-    void closeSockets();
-
-    /// @brief Returns protocol family used by the @c CfgIface.
-    Family getFamily() const {
-        return (family_);
-    }
+    void closeSockets() const;
 
     /// @brief Tries to open sockets on selected interfaces.
     ///
@@ -100,24 +93,19 @@ public:
     /// documentation for details how to specify interfaces and unicast
     /// addresses to bind the sockets to.
     ///
+    /// @param family Address family (v4 or v6).
     /// @param port Port number to be used to bind sockets to.
     /// @param use_bcast A boolean flag which indicates if the broadcast
     /// traffic should be received through the socket. This parameter is
     /// ignored for IPv6.
-    void openSockets(const uint16_t port, const bool use_bcast = true);
+    void openSockets(const Family& family, const uint16_t port,
+                     const bool use_bcast = true) const;
 
     /// @brief Puts the interface configuration into default state.
     ///
     /// This function removes interface names from the set.
     void reset();
 
-    /// @brief Sets protocol family.
-    ///
-    /// @param family New family value (V4 or V6).
-    void setFamily(Family family) {
-        family_ = family;
-    }
-
     /// @brief Select interface to be used to receive DHCP traffic.
     ///
     /// This function controls the selection of the interface on which the
@@ -137,6 +125,7 @@ public:
     /// not allowed when specifying a unicast address. For example:
     /// */2001:db8:1::1 is not allowed.
     ///
+    /// @param family Address family (v4 or v6).
     /// @param iface_name Explicit interface name, a wildcard name (*) of
     /// the interface(s) or the pair of iterface/unicast-address to be used
     /// to receive DHCP traffic.
@@ -148,7 +137,7 @@ public:
     /// @throw DuplicateIfaceName If the interface is already selected, i.e.
     /// @throw IOError when specified unicast address is invalid.
     /// @c CfgIface::use has been already called for this interface.
-    void use(const std::string& iface_name);
+    void use(const Family& family, const std::string& iface_name);
 
 private:
 
@@ -157,12 +146,14 @@ private:
     /// This function selects all interfaces to receive DHCP traffic or
     /// deselects all interfaces so as none of them receives a DHCP traffic.
     ///
+    /// @param family Address family (v4 or v6).
     /// @param inactive A boolean value which indicates if all interfaces
     /// (except loopback) should be selected or deselected.
     /// @param loopback_inactive A boolean value which indicates if loopback
     /// interface should be selected or deselected.
     /// should be deselected/inactive (true) or selected/active (false).
-    void setState(const bool inactive, const bool loopback_inactive);
+    void setState(const Family& family, const bool inactive,
+                  const bool loopback_inactive) const;
 
     /// @brief Error handler for executed when opening a socket fail.
     ///

src/lib/dhcpsrv/cfgmgr.cc

@@ -357,17 +357,63 @@ CfgMgr::getD2ClientMgr() {
     return (d2_client_mgr_);
 }
 
+void
+CfgMgr::ensureCurrentAllocated() {
+    if (!configuration_ || configs_.empty()) {
+        configuration_.reset(new Configuration());
+        configs_.push_back(configuration_);
+    }
+}
+
+void
+CfgMgr::clear() {
+    configs_.clear();
+    ensureCurrentAllocated();
+}
+
+void
+CfgMgr::commit() {
+    if (!configs_.empty() && configs_.back() != configuration_) {
+        configuration_ = configs_.back();
+    }
+}
+
+void
+CfgMgr::rollback() {
+    ensureCurrentAllocated();
+    if (!configuration_->sequenceEquals(*configs_.back())) {
+        configs_.pop_back();
+    }
+}
+
 ConfigurationPtr
 CfgMgr::getConfiguration() {
     return (configuration_);
 }
 
+ConstConfigurationPtr
+CfgMgr::getCurrent() {
+    ensureCurrentAllocated();
+    return (configuration_);
+}
+
+ConfigurationPtr
+CfgMgr::getStaging() {
+    ensureCurrentAllocated();
+    if (configuration_->sequenceEquals(*configs_.back())) {
+        uint32_t sequence = configuration_->getSequence();
+        configs_.push_back(ConfigurationPtr(new Configuration(++sequence)));
+    }
+    return (configs_.back());
+}
+
 CfgMgr::CfgMgr()
     : datadir_(DHCP_DATA_DIR), echo_v4_client_id_(true),
-      d2_client_mgr_(), configuration_(new Configuration()) {
+      d2_client_mgr_() {
     // 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
+    ensureCurrentAllocated();
 }
 
 CfgMgr::~CfgMgr() {

src/lib/dhcpsrv/cfgmgr.h

@@ -373,12 +373,21 @@ public:
     /// @return a reference to the DHCP-DDNS manager.
     D2ClientMgr& getD2ClientMgr();
 
+    void clear();
+
+    void commit();
+
+    void rollback();
 
     /// @brief Returns the current configuration.
     ///
     /// @return a pointer to the current configuration.
     ConfigurationPtr getConfiguration();
 
+    ConstConfigurationPtr getCurrent();
+
+    ConfigurationPtr getStaging();
+
 protected:
 
     /// @brief Protected constructor.
@@ -410,6 +419,8 @@ protected:
 
 private:
 
+    void ensureCurrentAllocated();
+
     /// @brief Checks that the IPv4 subnet with the given id already exists.
     ///
     /// @param subnet Subnet for which this function will check if the other
@@ -453,6 +464,16 @@ private:
     /// @todo: maybe this should be a vector<Configuration>, so we could keep
     ///        previous configurations and do a rollback if needed?
     ConfigurationPtr configuration_;
+
+    /// @name Configuration List.
+    ///
+    //@{
+    /// @brief Configuration list type.
+    typedef std::list<ConfigurationPtr> ConfigurationList;
+
+    /// @brief Container holding all previous and current configurations.
+    ConfigurationList configs_;
+    //@}
 };
 
 } // namespace isc::dhcp

src/lib/dhcpsrv/configuration.cc

@@ -19,6 +19,14 @@
 namespace isc {
 namespace dhcp {
 
+Configuration::Configuration()
+    : sequence_(0) {
+}
+
+Configuration::Configuration(uint32_t sequence)
+    : sequence_(sequence) {
+}
+
 std::string
 Configuration::getConfigSummary(const uint32_t selection) const {
     std::ostringstream s;
@@ -60,5 +68,10 @@ Configuration::getConfigSummary(const uint32_t selection) const {
     return (summary);
 }
 
+bool
+Configuration::sequenceEquals(const Configuration& other) {
+    return (getSequence() == other.getSequence());
+}
+
 }
 }

src/lib/dhcpsrv/configuration.h

@@ -83,8 +83,8 @@ typedef std::vector<isc::dhcp::LoggingInfo> LoggingInfoStorage;
 /// @brief Specifies current DHCP configuration
 ///
 /// @todo Migrate all other configuration parameters from cfgmgr.h here
-struct Configuration {
-
+class Configuration {
+public:
     /// @name Constants for selection of parameters returned by @c getConfigSummary
     ///
     //@{
@@ -113,11 +113,15 @@ struct Configuration {
     /// @brief logging specific information
     LoggingInfoStorage logging_info_;
 
-    /// @brief Interface configuration.
+    /// @brief Default constructor.
     ///
-    /// Used to select interfaces on which the DHCP server will listen to
-    /// queries.
-    CfgIface cfg_iface_;
+    /// This constructor sets configuration sequence number to 0.
+    Configuration();
+
+    /// @brief Constructor.
+    ///
+    /// Sets arbitrary configuration sequence number.
+    Configuration(uint32_t sequence);
 
     /// @brief Returns summary of the configuration in the textual format.
     ///
@@ -138,11 +142,64 @@ struct Configuration {
     ///
     /// @return Summary of the configuration in the textual format.
     std::string getConfigSummary(const uint32_t selection) const;
+
+    /// @brief Returns configuration sequence number.
+    uint32_t getSequence() const {
+        return (sequence_);
+    }
+
+    /// @brief Compares configuration sequence with other sequence.
+    ///
+    /// This method compares sequence numbers of two configurations for
+    /// equality. The sequence numbers are meant to be unique, so if
+    /// they are equal it means that they point to the same configuration.
+    ///
+    /// @param other Configuration which sequence number should be
+    /// compared with the sequence number of this configuration.
+    ///
+    /// @return true if sequence numbers are equal.
+    bool sequenceEquals(const Configuration& other);
+
+    /// @brief Returns object which represents selection of interfaces.
+    ///
+    /// This function returns a reference to the object which represents the
+    /// set of interfaces being used to receive DHCP traffic.
+    ///
+    /// @return Object representing selection of interfaces.
+    const CfgIface& getCfgIface() const {
+        return (cfg_iface_);
+    }
+
+    /// @brief Sets the object representing selection of interfaces.
+    ///
+    /// @param cfg_iface Object representing selection of interfaces.
+    void setCfgIface(const CfgIface& cfg_iface) {
+        cfg_iface_ = cfg_iface;
+    }
+
+private:
+
+    /// @brief Sequence number identifying the configuration.
+    uint32_t sequence_;
+
+    /// @brief Interface configuration.
+    ///
+    /// Used to select interfaces on which the DHCP server will listen to
+    /// queries.
+    CfgIface cfg_iface_;
+
 };
 
-/// @brief pointer to the configuration
+/// @name Pointers to the @c Configuration object.
+///
+//@{
+/// @brief Non-const pointer to the @ Configuration.
 typedef boost::shared_ptr<Configuration> ConfigurationPtr;
 
+/// @brief Const pointer to the @c Configuration.
+typedef boost::shared_ptr<const Configuration> ConstConfigurationPtr;
+//@}
+
 } // namespace isc::dhcp
 } // namespace isc
 

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -179,8 +179,9 @@ template <> void ValueParser<std::string>::build(ConstElementPtr value) {
 // ******************** InterfaceListConfigParser *************************
 
 InterfaceListConfigParser::
-InterfaceListConfigParser(const std::string& param_name)