Merge lp:~rvb/maas/bug-1061409 into lp:maas/trunk

[Raphaël Badin]

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.

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.

[Jeroen T. Vermeulen (jtv)]

The code and tests look fine. However the log notice comes across as confusing for a reader who doesn't already know how things work. It currently says...

89 + "Unable to parse the DHCP leases file. You might want to install "
90 + "the 'maas-dhcp' package. Note that this is perfectly fine "
91 + "if this cluster is not managing its DHCP server.")

I would switch those last two sentences around, so that the message conveys information in this order:
1. There's no leases file.
2. Which is fine, unless you want MAAS to manage DHCP.
3. If you want MAAS to manage DHCP, the problem is that maas-dhcp hasn't been installed.

Several things need attention in the details of the text, in my view:

 * It doesn't help that the "this is perfectly fine" is ambiguous -- it could refer to the inability to parse the leases file, or to installation of maas-dhcp.

 * Is "Unable to parse the DHCP leases file" really the best information we can give? There's a big difference between "the leases file does not exist" and "the parser had a problem with the contents of the leases file." I think this message is only meant for the former case.

 * Hinting that people may be able to solve their problems by installing maas-dhcp is an open invitation for people to set up accidental rogue DHCP servers.

 * "Note that" doesn't carry a lot of meaning. Better leave it out.

Please fix up before landing!

Jeroen

[Raphaël Badin (rvb)]

[...]
> Several things need attention in the details of the text, in my view:
>
> * It doesn't help that the "this is perfectly fine" is ambiguous -- it could
> refer to the inability to parse the leases file, or to installation of maas-
> dhcp.
>
> * Is "Unable to parse the DHCP leases file" really the best information we
> can give? There's a big difference between "the leases file does not exist"
> and "the parser had a problem with the contents of the leases file." I think
> this message is only meant for the former case.
>
> * Hinting that people may be able to solve their problems by installing maas-
> dhcp is an open invitation for people to set up accidental rogue DHCP servers.
>
> * "Note that" doesn't carry a lot of meaning. Better leave it out.
>
> Please fix up before landing!
>
> Jeroen

You're definitely right… how about:

        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.")

[lp:~rvb/maas/bug-1061409]

1164. By Raphaël Badin on 2012-10-04

Fix typo.

[Raphaël Badin (rvb)]

review: Approve

=== 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 12:26:22 +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 exist.  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 12:26:22 +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())