launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/tweak-import-script-error into lp:maas.

Commit message:
Don't set a persistent error for the import script on startup if there already is an error for it.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/tweak-import-script-error/+merge/127048

Discussed with Julian.  We have two different error messages for the situation where the region controller has no knowledge of any available boot images:
 #1: We haven't had word about any boot images being downloaded.
 #2: We've been told that there are no boot images.

The former error, unless it goes away soon, points to a setup problem, or a communication problem between the master worker and the API.  The latter means that the download script hasn't installed any images (yet), and if it keeps showing up, the script is probably breaking.

When the region controller starts up, it checks for boot images and if there are none, it shows error #1.  But now that these error messages are kept in the database, we can do better.  If a persistent error is already registered for the import script, it could be either #1 or #2 (or who knows, something else) and the difference may be significant to the user.  So in that case, region-controller startup should not overwrite the message.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/tweak-import-script-error/+merge/127048
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/tweak-import-script-error into lp:maas.

=== modified file 'src/maasserver/components.py'
--- src/maasserver/components.py	2012-09-28 16:56:02 +0000
+++ src/maasserver/components.py	2012-09-28 18:51:23 +0000
@@ -12,11 +12,13 @@
 __metaclass__ = type
 __all__ = [
     "discard_persistent_error",
+    "get_persistent_error",
     "get_persistent_errors",
     "register_persistent_error",
     ]
 
 from maasserver.models import ComponentError
+from maasserver.utils.orm import get_one
 
 
 def discard_persistent_error(component):
@@ -37,6 +39,15 @@
     ComponentError.objects.create(component=component, error=error_message)
 
 
+def get_persistent_error(component):
+    """Return persistent error for `component`, or None."""
+    err = get_one(ComponentError.objects.filter(component=component))
+    if err is None:
+        return None
+    else:
+        return err.error
+
+
 def get_persistent_errors():
     """Return list of current persistent error messages."""
     return sorted(err.error for err in ComponentError.objects.all())

=== modified file 'src/maasserver/start_up.py'
--- src/maasserver/start_up.py	2012-09-28 16:56:02 +0000
+++ src/maasserver/start_up.py	2012-09-28 18:51:23 +0000
@@ -18,7 +18,10 @@
 from textwrap import dedent
 
 from lockfile import FileLock
-from maasserver.components import register_persistent_error
+from maasserver.components import (
+    get_persistent_error,
+    register_persistent_error,
+    )
 from maasserver.dns import write_full_dns_config
 from maasserver.enum import COMPONENT
 from maasserver.maasavahi import setup_maas_avahi_service
@@ -58,6 +61,24 @@
         lock.release()
 
 
+def update_import_script_error():
+    import_script = COMPONENT.IMPORT_PXE_FILES
+    have_boot_images = BootImage.objects.all().exists()
+    if not have_boot_images and get_persistent_error(import_script) is None:
+        warning = dedent("""\
+            No boot images have been registered yet.  This may mean that the
+            maas-import-pxe-files script has not been run yet, or that it
+            failed.  Alternatively, there may be a communication problem
+            between the master cluster manager and the MAAS server.
+
+            Try running maas-import-pxe-files manually.  If it succeeds, this
+            message should disappear within 5 minutes.  If it does not, check
+            the master cluster manager's logs for signs that it was unable to
+            report to the MAAS API.
+            """)
+        register_persistent_error(import_script, warning)
+
+
 def inner_start_up():
     # Publish the MAAS server existence over Avahi.
     setup_maas_avahi_service()
@@ -72,16 +93,4 @@
     NodeGroup.objects.refresh_workers()
 
     # Check whether we have boot images yet.
-    if not BootImage.objects.all().exists():
-        warning = dedent("""\
-            No boot images have been registered yet.  This may mean that the
-            maas-import-pxe-files script has not been run yet, or that it
-            failed.  Alternatively, there may be a communication problem
-            between the master cluster manager and the MAAS server.
-
-            Try running maas-import-pxe-files manually.  If it succeeds, this
-            message should disappear within 5 minutes.  If it does not, check
-            the master cluster manager's logs for signs that it was unable to
-            report to the MAAS API.
-            """)
-        register_persistent_error(COMPONENT.IMPORT_PXE_FILES, warning)
+    update_import_script_error()

=== modified file 'src/maasserver/tests/test_components.py'
--- src/maasserver/tests/test_components.py	2012-09-28 16:56:02 +0000
+++ src/maasserver/tests/test_components.py	2012-09-28 18:51:23 +0000
@@ -17,6 +17,7 @@
 
 from maasserver.components import (
     discard_persistent_error,
+    get_persistent_error,
     get_persistent_errors,
     register_persistent_error,
     )
@@ -78,3 +79,12 @@
             errors.append(error_message)
             components.append(component)
         self.assertItemsEqual(errors, get_persistent_errors())
+
+    def test_get_persistent_error_returns_None_if_no_error(self):
+        self.assertIsNone(get_persistent_error(factory.make_name('component')))
+
+    def test_get_persistent_error_returns_component_error(self):
+        component = factory.make_name('component')
+        error = factory.make_name('error')
+        register_persistent_error(component, error)
+        self.assertEqual(error, get_persistent_error(component))

=== modified file 'src/maasserver/tests/test_start_up.py'
--- src/maasserver/tests/test_start_up.py	2012-09-28 16:56:02 +0000
+++ src/maasserver/tests/test_start_up.py	2012-09-28 18:51:23 +0000
@@ -22,6 +22,10 @@
     LockTimeout,
     )
 from maasserver import start_up
+from maasserver.components import (
+    discard_persistent_error,
+    register_persistent_error,
+    )
 from maasserver.enum import COMPONENT
 from maasserver.models import (
     BootImage,
@@ -109,6 +113,7 @@
         # the master worker is having trouble reporting its images.  And
         # so start_up registers a persistent warning about this.
         BootImage.objects.all().delete()
+        discard_persistent_error(COMPONENT.IMPORT_PXE_FILES)
         recorder = self.patch(start_up, 'register_persistent_error')
 
         start_up.start_up()
@@ -128,3 +133,19 @@
         self.assertNotIn(
             COMPONENT.IMPORT_PXE_FILES,
             [args[0][0] for args in recorder.call_args_list])
+
+    def test_start_up_does_not_warn_if_already_warning(self):
+        # If there already is a warning about missing boot images, it is
+        # based on more precise knowledge of whether we ever heard from
+        # the region worker at all.  It will not be replaced by a less
+        # knowledgeable warning.
+        BootImage.objects.all().delete()
+        register_persistent_error(
+            COMPONENT.IMPORT_PXE_FILES, factory.getRandomString())
+        recorder = self.patch(start_up, 'register_persistent_error')
+
+        start_up.start_up()
+
+        self.assertNotIn(
+            COMPONENT.IMPORT_PXE_FILES,
+            [args[0][0] for args in recorder.call_args_list])