launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/bug-1061409 into lp:maas.

Commit message:
This branch changes the DHCP lease parsing code so that the DHCP parsing task won't error if the DHCP lease file is not present.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1061409/+merge/127979

This branch changes the DHCP lease parsing code so that the DHCP parsing task won't error if the DHCP lease file is not present.  Instead, it will display a message in the log file: this message is, I'm afraid, the best we can do because we don't know on the cluster controller side if DHCP is enabled for this cluster or not.
-- 
https://code.launchpad.net/~rvb/maas/bug-1061409/+merge/127979
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1061409 into lp:maas.

=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py	2012-09-28 10:16:59 +0000
+++ src/provisioningserver/dhcp/leases.py	2012-10-04 11:22:23 +0000
@@ -32,6 +32,7 @@
     ]
 
 
+import errno
 import json
 from os import (
     fstat,
@@ -67,8 +68,19 @@
 
 
 def get_leases_timestamp():
-    """Return the last modification timestamp of the DHCP leases file."""
-    return stat(get_leases_file()).st_mtime
+    """Return the last modification timestamp of the DHCP leases file.
+
+    None will be returned if the DHCP lease file cannot be found.
+    """
+    try:
+        return stat(get_leases_file()).st_mtime
+    except OSError as exception:
+        # Return None only if the exception is a "No such file or
+        # directory" exception.
+        if exception.errno == errno.ENOENT:
+            return None
+        else:
+            raise
 
 
 def parse_leases_file():
@@ -77,10 +89,19 @@
     :return: A tuple: (timestamp, leases).  The `timestamp` is the last
         modification time of the leases file, and `leases` is a dict
         mapping leased IP addresses to their associated MAC addresses.
+        None will be returned if the DHCP lease file cannot be found.
     """
-    with open(get_leases_file(), 'rb') as leases_file:
-        contents = leases_file.read().decode('utf-8')
-        return fstat(leases_file.fileno()).st_mtime, parse_leases(contents)
+    try:
+        with open(get_leases_file(), 'rb') as leases_file:
+            contents = leases_file.read().decode('utf-8')
+            return fstat(leases_file.fileno()).st_mtime, parse_leases(contents)
+    except IOError as exception:
+        # Return None only if the exception is a "No such file or
+        # directory" exception.
+        if exception.errno == errno.ENOENT:
+            return None
+        else:
+            raise
 
 
 def check_lease_changes():
@@ -93,11 +114,15 @@
 
     if get_leases_timestamp() == previous_leases_time:
         return None
-    timestamp, leases = parse_leases_file()
-    if leases == previous_leases:
-        return None
+    parse_result = parse_leases_file()
+    if parse_result is not None:
+        timestamp, leases = parse_result
+        if leases == previous_leases:
+            return None
+        else:
+            return timestamp, leases
     else:
-        return timestamp, leases
+        return None
 
 
 def record_lease_state(last_change, leases):
@@ -155,8 +180,16 @@
     server restarts, or zone-file update commands getting lost on their
     way to the DNS server.
     """
-    timestamp, leases = parse_leases_file()
-    process_leases(timestamp, leases)
+    parse_result = parse_leases_file()
+    if parse_result:
+        timestamp, leases = parse_result
+        process_leases(timestamp, leases)
+    else:
+        task_logger.info(
+            "The DHCP leases file does not exit.  This is only a problem if "
+            "this cluster controller is managing its DHCP server.  If that's "
+            "the case then you need to install the 'maas-dhcp' package on "
+            "this cluster controller.")
 
 
 def update_leases():

=== modified file 'src/provisioningserver/dhcp/tests/test_leases.py'
--- src/provisioningserver/dhcp/tests/test_leases.py	2012-09-28 10:16:59 +0000
+++ src/provisioningserver/dhcp/tests/test_leases.py	2012-10-04 11:22:23 +0000
@@ -16,6 +16,8 @@
     datetime,
     timedelta,
     )
+import errno
+import os
 from textwrap import dedent
 
 from apiclient.maas_client import MAASClient
@@ -25,6 +27,7 @@
     age_file,
     get_write_time,
     )
+from mock import Mock
 from provisioningserver import cache
 from provisioningserver.auth import (
     MAAS_URL_CACHE_KEY,
@@ -196,6 +199,29 @@
         update_leases()
         self.assertSequenceEqual([], parser.calls)
 
+    def redirect_parser_to_non_existing_file(self):
+        file_name = self.make_file()
+        os.remove(file_name)
+        self.redirect_parser(file_name)
+
+    def test_parse_leases_file_returns_None_if_file_does_not_exist(self):
+        self.redirect_parser_to_non_existing_file()
+        self.assertIsNone(parse_leases_file())
+
+    def test_get_leases_timestamp_returns_None_if_file_does_not_exist(self):
+        self.redirect_parser_to_non_existing_file()
+        self.assertIsNone(parse_leases_file())
+
+    def test_parse_leases_file_errors_if_unexpected_exception(self):
+        exception = IOError(errno.EBUSY, factory.getRandomString())
+        self.patch(leases_module, 'open', Mock(side_effect=exception))
+        self.assertRaises(IOError, parse_leases_file)
+
+    def test_get_leases_timestamp_errors_if_unexpected_exception(self):
+        exception = OSError(errno.EBUSY, factory.getRandomString())
+        self.patch(leases_module, 'open', Mock(side_effect=exception))
+        self.assertRaises(OSError, parse_leases_file)
+
     def test_check_lease_changes_returns_tuple_if_lease_added(self):
         leases = factory.make_random_leases()
         self.set_lease_state(
@@ -206,6 +232,10 @@
             (get_write_time(leases_file), leases),
             check_lease_changes())
 
+    def test_check_lease_returns_None_if_lease_file_does_not_exist(self):
+        self.redirect_parser_to_non_existing_file()
+        self.assertIsNone(check_lease_changes())
+
     def test_check_lease_changes_returns_tuple_if_leases_dropped(self):
         self.set_lease_state(
             datetime.utcnow() - timedelta(seconds=10),
@@ -227,6 +257,10 @@
         self.set_lease_state(datetime.utcnow(), leases.copy())
         self.assertIsNone(check_lease_changes())
 
+    def test_upload_leases_does_not_try_to_process_non_existing_file(self):
+        self.redirect_parser_to_non_existing_file()
+        self.assertIsNone(upload_leases())  # No exception.
+
     def test_update_leases_processes_leases_if_changed(self):
         self.set_lease_state()
         self.patch(leases_module, 'send_leases', FakeMethod())