[3978] Addressed comments

diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
index 1fb22746fd299582960540c5a3fcfaa90f6efaa3..f02438159313d0ff91c016643d57355034a92570 100644 (file)
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc
@@ -72,12 +72,28 @@ ControlledDhcpv4Srv::commandConfigReloadHandler(const string&,
 }
 
 ConstElementPtr
-ControlledDhcpv4Srv::commandLeasesReclaimHandler(const string&, ConstElementPtr) {
-
-
-    server_->alloc_engine_->reclaimExpiredLeases4(0, 0, true);
-    ConstElementPtr answer = isc::config::createAnswer(0,
-                             "Leases successfully reclaimed.");
+ControlledDhcpv4Srv::commandLeasesReclaimHandler(const string& command,
+                                                 ConstElementPtr args) {
+    int status_code = 1;
+    string message;
+
+    // args must be { "remove": <bool> }
+    if (!args) {
+        message = "Missing mandatory 'remove' parameter.";
+    } else {
+        ConstElementPtr remove_name = args->get("remove");
+        if (!remove_name) {
+            message = "Missing mandatory 'remove' parameter.";
+        } else if (remove_name->getType() != Element::boolean) {
+            message = "'remove' parameter expected to be a boolean.";
+        } else {
+            bool remove_lease = remove_name->boolValue();
+            server_->alloc_engine_->reclaimExpiredLeases4(0, 0, remove_lease);
+            status_code = 0;
+            message = "Reclamation of expired leases is complete.";
+        }
+    }
+    ConstElementPtr answer = isc::config::createAnswer(status_code, message);
     return (answer);
 }
 

diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h
index 6bfcdc1da2c32bb2be43e7f1655ab9742478f280..d1abf7e45022e42b46ee47bb357249a1b454867a 100644 (file)
--- a/src/bin/dhcp4/ctrl_dhcp4_srv.h
+++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h
@@ -152,15 +152,19 @@ private:
                                isc::data::ConstElementPtr args);
 
 
-    /// @brief handler for processing 'leases-reclaim' command
+    /// @brief Handler for processing 'leases-reclaim' command
     ///
     /// This handler processes leases-reclaim command, which triggers
-    /// the leases reclamation immediately
+    /// the leases reclamation immediately.
+    /// No limit for processing time or number of processed leases applies.
     ///
     /// @param command (parameter ignored)
-    /// @param args (parameter ignored)
+    /// @param args arguments map { "remove": <bool> }
+    ///        if true a lease is removed when it is reclaimed,
+    ///        if false its state is changed to "expired-reclaimed".
     ///
-    /// @return status of the command
+    /// @return status of the command (should be success unless args
+    ///         was not a Bool Element).
     isc::data::ConstElementPtr
     commandLeasesReclaimHandler(const std::string& command,
                                 isc::data::ConstElementPtr args);

diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
index 961d399ad3b20d88f33bf490749ee6cc64495b02..9cf01e236ec9281f477ad61acd55d0ab3ce040ef 100644 (file)
--- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
@@ -344,33 +344,101 @@ TEST_F(CtrlChannelDhcpv4SrvTest, controlChannelShutdown) {
     EXPECT_EQ("{ \"result\": 0, \"text\": \"Shutting down.\" }",response);
 }
 
-// Thist test verifies that the DHCP server immediately reclaims expired
+// This test verifies that the DHCP server immediately reclaims expired
 // leases on leases-reclaim command
 TEST_F(CtrlChannelDhcpv4SrvTest, controlLeasesReclaim) {
     createUnixChannelServer();
 
-    // Create an expired lease. The lease is expired by 40 seconds ago
+    // Create expired leases. Leases are expired by 40 seconds ago
     // (valid lifetime = 60, cltt = now - 100).
-    HWAddrPtr hwaddr_expired(new HWAddr(HWAddr::fromText("00:01:02:03:04:05")));
-    Lease4Ptr lease_expired(new Lease4(IOAddress("10.0.0.1"), hwaddr_expired,
-                                       ClientIdPtr(), 60, 10, 20,
-                                       time(NULL) - 100, SubnetID(1)));
-
-    // Add lease to the database.
+    HWAddrPtr hwaddr0(new HWAddr(HWAddr::fromText("00:01:02:03:04:05")));
+    Lease4Ptr lease0(new Lease4(IOAddress("10.0.0.1"), hwaddr0,
+                                ClientIdPtr(), 60, 10, 20,
+                                time(NULL) - 100, SubnetID(1)));
+    HWAddrPtr hwaddr1(new HWAddr(HWAddr::fromText("01:02:03:04:05:06")));
+    Lease4Ptr lease1(new Lease4(IOAddress("10.0.0.2"), hwaddr1,
+                                ClientIdPtr(), 60, 10, 20,
+                                time(NULL) - 100, SubnetID(1)));
+
+    // Add leases to the database.
     LeaseMgr& lease_mgr = LeaseMgrFactory().instance();
-    ASSERT_NO_THROW(lease_mgr.addLease(lease_expired));
+    ASSERT_NO_THROW(lease_mgr.addLease(lease0));
+    ASSERT_NO_THROW(lease_mgr.addLease(lease1));
 
-    // Make sure it has been added.
+    // Make sure they have been added.
     ASSERT_TRUE(lease_mgr.getLease4(IOAddress("10.0.0.1")));
+    ASSERT_TRUE(lease_mgr.getLease4(IOAddress("10.0.0.2")));
 
-    // Send the command.
+    // No arguments
     std::string response;
     sendUnixCommand("{ \"command\": \"leases-reclaim\" }", response);
-    EXPECT_EQ("{ \"result\": 0, \"text\": \"Leases successfully reclaimed.\" }", response);
+    EXPECT_EQ("{ \"result\": 1, \"text\": "
+              "\"Missing mandatory 'remove' parameter.\" }", response);
+
+    // Bad argument name
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"reclaim\": true } }", response);
+    EXPECT_EQ("{ \"result\": 1, \"text\": "
+              "\"Missing mandatory 'remove' parameter.\" }", response);
+
+    // Bad remove argument type
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"remove\": \"bogus\" } }", response);
+    EXPECT_EQ("{ \"result\": 1, \"text\": "
+              "\"'remove' parameter expected to be a boolean.\" }", response);
+
+    // Send the command
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"remove\": false } }", response);
+    EXPECT_EQ("{ \"result\": 0, \"text\": "
+              "\"Reclamation of expired leases is complete.\" }", response);
+
+    // Leases should be reclaimed, but not removed
+    ASSERT_NO_THROW(lease0 = lease_mgr.getLease4(IOAddress("10.0.0.1")));
+    ASSERT_NO_THROW(lease1 = lease_mgr.getLease4(IOAddress("10.0.0.2")));
+    ASSERT_TRUE(lease0);
+    ASSERT_TRUE(lease1);
+    EXPECT_TRUE(lease0->stateExpiredReclaimed());
+    EXPECT_TRUE(lease1->stateExpiredReclaimed());
+}
+
+// Thist test verifies that the DHCP server immediately removed expired
+// leases on leases-reclaim command with remove = true
+TEST_F(CtrlChannelDhcpv4SrvTest, controlLeasesReclaimRemove) {
+    createUnixChannelServer();
+
+    // Create expired leases. Leases are expired by 40 seconds ago
+    // (valid lifetime = 60, cltt = now - 100).
+    HWAddrPtr hwaddr0(new HWAddr(HWAddr::fromText("00:01:02:03:04:05")));
+    Lease4Ptr lease0(new Lease4(IOAddress("10.0.0.1"), hwaddr0,
+                                ClientIdPtr(), 60, 10, 20,
+                                time(NULL) - 100, SubnetID(1)));
+    HWAddrPtr hwaddr1(new HWAddr(HWAddr::fromText("01:02:03:04:05:06")));
+    Lease4Ptr lease1(new Lease4(IOAddress("10.0.0.2"), hwaddr1,
+                                ClientIdPtr(), 60, 10, 20,
+                                time(NULL) - 100, SubnetID(1)));
+
+    // Add leases to the database.
+    LeaseMgr& lease_mgr = LeaseMgrFactory().instance();
+    ASSERT_NO_THROW(lease_mgr.addLease(lease0));
+    ASSERT_NO_THROW(lease_mgr.addLease(lease1));
+
+    // Make sure they have been added.
+    ASSERT_TRUE(lease_mgr.getLease4(IOAddress("10.0.0.1")));
+    ASSERT_TRUE(lease_mgr.getLease4(IOAddress("10.0.0.2")));
+
+    // Send the command
+    std::string response;
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"remove\": true } }", response);
+    EXPECT_EQ("{ \"result\": 0, \"text\": "
+              "\"Reclamation of expired leases is complete.\" }", response);
 
-    // Verify that the lease in the database has been processed as expected.
-    ASSERT_NO_THROW(lease_expired = lease_mgr.getLease4(IOAddress("10.0.0.1")));
-    EXPECT_FALSE(lease_expired);
+    // Leases should have been removed.
+    ASSERT_NO_THROW(lease0 = lease_mgr.getLease4(IOAddress("10.0.0.1")));
+    ASSERT_NO_THROW(lease1 = lease_mgr.getLease4(IOAddress("10.0.0.2")));
+    EXPECT_FALSE(lease0);
+    EXPECT_FALSE(lease1);
 }
 
 // Tests that the server properly responds to statistics commands.  Note this

diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
index 8762809705750d1e5fc7dd9e18eca1e0858ce56e..b1fa5e4cfc770804f2c6122c589f56dc667689ef 100644 (file)
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
@@ -70,12 +70,28 @@ ControlledDhcpv6Srv::commandConfigReloadHandler(const string&, ConstElementPtr a
 }
 
 ConstElementPtr
-ControlledDhcpv6Srv::commandLeasesReclaimHandler(const string&, ConstElementPtr) {
-
-
-    server_->alloc_engine_->reclaimExpiredLeases6(0, 0, true);
-    ConstElementPtr answer = isc::config::createAnswer(0,
-                             "Leases successfully reclaimed.");
+ControlledDhcpv6Srv::commandLeasesReclaimHandler(const string& command,
+                                                 ConstElementPtr args) {
+    int status_code = 1;
+    string message;
+
+    // args must be { "remove": <bool> }
+    if (!args) {
+        message = "Missing mandatory 'remove' parameter.";
+    } else {
+        ConstElementPtr remove_name = args->get("remove");
+        if (!remove_name) {
+            message = "Missing mandatory 'remove' parameter.";
+        } else if (remove_name->getType() != Element::boolean) {
+            message = "'remove' parameter expected to be a boolean.";
+        } else {
+            bool remove_lease = remove_name->boolValue();
+            server_->alloc_engine_->reclaimExpiredLeases6(0, 0, remove_lease);
+            status_code = 0;
+            message = "Reclamation of expired leases is complete.";
+        }
+    }
+    ConstElementPtr answer = isc::config::createAnswer(status_code, message);
     return (answer);
 }
 

diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h
index 43b42b64594b64230874a1ce301b4c5675c45108..cc5815519204b9b672301dbf5d18893f49c62348 100644 (file)
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.h
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h
@@ -151,15 +151,19 @@ private:
     commandConfigReloadHandler(const std::string& command,
                                isc::data::ConstElementPtr args);
 
-    /// @brief handler for processing 'leases-reclaim' command
+    /// @brief Handler for processing 'leases-reclaim' command
     ///
     /// This handler processes leases-reclaim command, which triggers
-    /// the leases reclamation immediately
+    /// the leases reclamation immediately.
+    /// No limit for processing time or number of processed leases applies.
     ///
     /// @param command (parameter ignored)
-    /// @param args (parameter ignored)
+    /// @param args arguments map { "remove": <bool> }
+    ///        if true a lease is removed when it is reclaimed,
+    ///        if false its state is changed to "expired-reclaimed".
     ///
-    /// @return status of the command
+    /// @return status of the command (should be success unless args
+    ///         was not a Bool Element).
     isc::data::ConstElementPtr
     commandLeasesReclaimHandler(const std::string& command,
                                 isc::data::ConstElementPtr args);

diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
index 38683a9699e3f7d9785b85941eb6159fddcb684a..967bdb2067c25ecde60a2f0cfb6514d32f1a2d9a 100644 (file)
--- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
@@ -419,35 +419,109 @@ TEST_F(CtrlChannelDhcpv6SrvTest, controlChannelShutdown) {
     EXPECT_EQ("{ \"result\": 0, \"text\": \"Shutting down.\" }",response);
 }
 
-// Thist test verifies that the DHCP server immediately reclaims expired
+// This test verifies that the DHCP server immediately reclaims expired
 // leases on leases-reclaim command
 TEST_F(CtrlChannelDhcpv6SrvTest, controlLeasesReclaim) {
     createUnixChannelServer();
 
-    // Create an expired lease. The lease is expired by 40 seconds ago
+    // Create expired leases. Leases are expired by 40 seconds ago
     // (valid lifetime = 60, cltt = now - 100).
-    DuidPtr duid_expired(new DUID(DUID::fromText("00:01:02:03:04:05:06").getDuid()));
-    Lease6Ptr lease_expired(new Lease6(Lease::TYPE_NA, IOAddress("3000::1"),
-                                       duid_expired, 1, 50, 60, 10, 20, SubnetID(1)));
-    lease_expired->cltt_ = time(NULL) - 100;
-
-    // Add lease to the database.
+    DuidPtr duid0(new DUID(DUID::fromText("00:01:02:03:04:05:06").getDuid()));
+    Lease6Ptr lease0(new Lease6(Lease::TYPE_NA, IOAddress("3000::1"),
+                                duid0, 1, 50, 60, 10, 20, SubnetID(1)));
+    lease0->cltt_ = time(NULL) - 100;
+    DuidPtr duid1(new DUID(DUID::fromText("01:02:03:04:05:06:07").getDuid()));
+    Lease6Ptr lease1(new Lease6(Lease::TYPE_NA, IOAddress("3000::2"),
+                                duid1, 1, 50, 60, 10, 20, SubnetID(1)));
+    lease1->cltt_ = time(NULL) - 100;
+
+    // Add leases to the database.
     LeaseMgr& lease_mgr = LeaseMgrFactory().instance();
-    ASSERT_NO_THROW(lease_mgr.addLease(lease_expired));
+    ASSERT_NO_THROW(lease_mgr.addLease(lease0));
+    ASSERT_NO_THROW(lease_mgr.addLease(lease1));
 
-    // Make sure it has been added.
+    // Make sure they have been added.
     ASSERT_TRUE(lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::1")));
+    ASSERT_TRUE(lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::2")));
 
-    // Send the command.
+    // No arguments
     std::string response;
     sendUnixCommand("{ \"command\": \"leases-reclaim\" }", response);
-    EXPECT_EQ("{ \"result\": 0, \"text\": \"Leases successfully reclaimed.\" }", response);
+    EXPECT_EQ("{ \"result\": 1, \"text\": "
+              "\"Missing mandatory 'remove' parameter.\" }", response);
+
+    // Bad argument name
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"reclaim\": true } }", response);
+    EXPECT_EQ("{ \"result\": 1, \"text\": "
+              "\"Missing mandatory 'remove' parameter.\" }", response);
+
+    // Bad remove argument type
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"remove\": \"bogus\" } }", response);
+    EXPECT_EQ("{ \"result\": 1, \"text\": "
+              "\"'remove' parameter expected to be a boolean.\" }", response);
+
+    // Send the command
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"remove\": false } }", response);
+    EXPECT_EQ("{ \"result\": 0, \"text\": "
+              "\"Reclamation of expired leases is complete.\" }", response);
+
+    // Leases should be reclaimed, but not removed
+    ASSERT_NO_THROW(
+        lease0 = lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::1"))
+    );
+    ASSERT_NO_THROW(
+        lease1 = lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::2"))
+    );
+    ASSERT_TRUE(lease0);
+    ASSERT_TRUE(lease1);
+    EXPECT_TRUE(lease0->stateExpiredReclaimed());
+    EXPECT_TRUE(lease1->stateExpiredReclaimed());
+}
+
+// This test verifies that the DHCP server immediately reclaims expired
+// leases on leases-reclaim command with remove = true
+TEST_F(CtrlChannelDhcpv6SrvTest, controlLeasesReclaimRemove) {
+    createUnixChannelServer();
 
-    // Verify that the lease in the database has been processed as expected.
+    // Create expired leases. Leases are expired by 40 seconds ago
+    // (valid lifetime = 60, cltt = now - 100).
+    DuidPtr duid0(new DUID(DUID::fromText("00:01:02:03:04:05:06").getDuid()));
+    Lease6Ptr lease0(new Lease6(Lease::TYPE_NA, IOAddress("3000::1"),
+                                duid0, 1, 50, 60, 10, 20, SubnetID(1)));
+    lease0->cltt_ = time(NULL) - 100;
+    DuidPtr duid1(new DUID(DUID::fromText("01:02:03:04:05:06:07").getDuid()));
+    Lease6Ptr lease1(new Lease6(Lease::TYPE_NA, IOAddress("3000::2"),
+                                duid1, 1, 50, 60, 10, 20, SubnetID(1)));
+    lease1->cltt_ = time(NULL) - 100;
+
+    // Add leases to the database.
+    LeaseMgr& lease_mgr = LeaseMgrFactory().instance();
+    ASSERT_NO_THROW(lease_mgr.addLease(lease0));
+    ASSERT_NO_THROW(lease_mgr.addLease(lease1));
+
+    // Make sure they have been added.
+    ASSERT_TRUE(lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::1")));
+    ASSERT_TRUE(lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::2")));
+
+    // Send the command
+    std::string response;
+    sendUnixCommand("{ \"command\": \"leases-reclaim\", "
+                    "\"arguments\": { \"remove\": true } }", response);
+    EXPECT_EQ("{ \"result\": 0, \"text\": "
+              "\"Reclamation of expired leases is complete.\" }", response);
+
+    // Leases should have been removed.
+    ASSERT_NO_THROW(
+        lease0 = lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::1"))
+    );
     ASSERT_NO_THROW(
-        lease_expired = lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::1"))
+        lease1 = lease_mgr.getLease6(Lease::TYPE_NA, IOAddress("3000::2"))
     );
-    EXPECT_FALSE(lease_expired);
+    ASSERT_FALSE(lease0);
+    ASSERT_FALSE(lease1);
 }
 
 // Tests that the server properly responds to statistics commands.  Note this