launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1025582-task into lp:maas with lp:~jtv/maas/bug-1025582-api as a prerequisite.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1025582-task/+merge/123900

See commit message.  Pre-implementation call with Julian.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1025582-task/+merge/123900
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/bug-1025582-task into lp:maas.

=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py	2012-08-24 08:39:18 +0000
+++ etc/celeryconfig.py	2012-09-12 08:19:26 +0000
@@ -72,4 +72,11 @@
         'task': 'provisioningserver.tasks.upload_dhcp_leases',
         'schedule': timedelta(minutes=1),
     },
+
+    # XXX JeroenVermeulen 2012-09-12, bug=1039366: this task should run
+    # only on the master worker.
+    'report-boot-images': {
+        'task': 'provisioningserver.report_boot_images',
+        'schedule': timedelta(minutes=5),
+    },
 }

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-12 08:19:26 +0000
+++ src/maasserver/tests/test_api.py	2012-09-12 08:19:26 +0000
@@ -106,6 +106,7 @@
     )
 from provisioningserver.kernel_opts import KernelParameters
 from provisioningserver.omshell import Omshell
+from provisioningserver.pxe import tftppath
 from testresources import FixtureResource
 from testtools.matchers import (
     Contains,
@@ -2600,6 +2601,10 @@
 
 class TestBootImagesAPI(APITestCase):
 
+    resources = (
+        ('celery', FixtureResource(CeleryFixture())),
+        )
+
     def report_images(self, images, client=None):
         if client is None:
             client = self.client
@@ -2652,3 +2657,14 @@
         self.assertEqual(
             (httplib.OK, "Images noted."),
             (response.status_code, response.content))
+
+    def test_worker_calls_report_boot_images(self):
+        refresh_worker(NodeGroup.objects.ensure_master())
+        self.patch(MAASClient, 'post')
+        self.patch(tftppath, 'list_boot_images', Mock(return_value=[]))
+
+        tasks.report_boot_images.delay()
+
+        MAASClient.post.assert_called_once_with(
+            reverse('boot_images_handler').lstrip('/'), 'report_boot_images',
+            images=json.dumps([]))

=== modified file 'src/provisioningserver/pxe/tests/test_tftppath.py'
--- src/provisioningserver/pxe/tests/test_tftppath.py	2012-08-30 10:37:26 +0000
+++ src/provisioningserver/pxe/tests/test_tftppath.py	2012-09-12 08:19:26 +0000
@@ -21,6 +21,11 @@
     compose_bootloader_path,
     compose_config_path,
     compose_image_path,
+    drill_down,
+    extend_path,
+    is_visible_subdir,
+    list_boot_images,
+    list_subdirs,
     locate_tftp_path,
     )
 from provisioningserver.testing.config import ConfigFixture
@@ -38,6 +43,28 @@
         self.config = {"tftp": {"root": self.tftproot}}
         self.useFixture(ConfigFixture(self.config))
 
+    def make_boot_image_params(self):
+        """Create a dict of boot-image parameters, as in list_boot_images."""
+        return {
+            'architecture': factory.make_name('architecture'),
+            'subarchitecture': factory.make_name('subarchitecture'),
+            'release': factory.make_name('release'),
+            'purpose': factory.make_name('purpose'),
+        }
+
+    def make_image_dir(self, image_params, tftproot):
+        """Fake a boot image matching `image_params` under `tftproot`."""
+        image_dir = locate_tftp_path(
+            compose_image_path(
+                arch=image_params['architecture'],
+                subarch=image_params['subarchitecture'],
+                release=image_params['release'],
+                purpose=image_params['purpose']),
+            tftproot)
+        os.makedirs(image_dir)
+        factory.make_file(image_dir, 'linux')
+        factory.make_file(image_dir, 'initrd.gz')
+
     def test_compose_config_path_follows_maas_pxe_directory_layout(self):
         name = factory.make_name('config')
         self.assertEqual(
@@ -85,3 +112,110 @@
     def test_locate_tftp_path_returns_root_when_path_is_None(self):
         self.assertEqual(
             self.tftproot, locate_tftp_path(None, tftproot=self.tftproot))
+
+    def test_list_boot_images_copes_with_empty_directory(self):
+        self.assertItemsEqual([], list_boot_images(self.tftproot))
+
+    def test_list_boot_images_copes_with_unexpected_files(self):
+        os.makedirs(os.path.join(self.tftproot, factory.make_name('empty')))
+        factory.make_file(self.tftproot)
+        self.assertItemsEqual([], list_boot_images(self.tftproot))
+
+    def test_list_boot_images_finds_boot_image(self):
+        image = self.make_boot_image_params()
+        self.make_image_dir(image, self.tftproot)
+        self.assertItemsEqual([image], list_boot_images(self.tftproot))
+
+    def test_list_boot_images_enumerates_boot_images(self):
+        images = [self.make_boot_image_params() for counter in range(3)]
+        for image in images:
+            self.make_image_dir(image, self.tftproot)
+        self.assertItemsEqual(images, list_boot_images(self.tftproot))
+
+    def test_is_visible_subdir_ignores_regular_files(self):
+        plain_file = self.make_file()
+        self.assertFalse(
+            is_visible_subdir(
+                os.path.dirname(plain_file), os.path.basename(plain_file)))
+
+    def test_is_visible_subdir_ignores_hidden_directories(self):
+        base_dir = self.make_dir()
+        hidden_dir = factory.make_name('.')
+        os.makedirs(os.path.join(base_dir, hidden_dir))
+        self.assertFalse(is_visible_subdir(base_dir, hidden_dir))
+
+    def test_is_visible_subdir_recognizes_subdirectory(self):
+        base_dir = self.make_dir()
+        subdir = factory.make_name('subdir')
+        os.makedirs(os.path.join(base_dir, subdir))
+        self.assertTrue(is_visible_subdir(base_dir, subdir))
+
+    def test_list_subdirs_lists_empty_directory(self):
+        self.assertItemsEqual([], list_subdirs(self.make_dir()))
+
+    def test_list_subdirs_lists_subdirs(self):
+        base_dir = self.make_dir()
+        factory.make_file(base_dir, factory.make_name('plain-file'))
+        subdir = factory.make_name('subdir')
+        os.makedirs(os.path.join(base_dir, subdir))
+        self.assertItemsEqual([subdir], list_subdirs(base_dir))
+
+    def test_extend_path_finds_path_extensions(self):
+        base_dir = self.make_dir()
+        subdirs = [
+            factory.make_name('subdir-%d' % counter)
+            for counter in range(3)]
+        for subdir in subdirs:
+            os.makedirs(os.path.join(base_dir, subdir))
+        self.assertItemsEqual(
+            [[os.path.basename(base_dir), subdir] for subdir in subdirs],
+            extend_path(
+                os.path.dirname(base_dir), [os.path.basename(base_dir)]))
+
+    def test_extend_path_builds_on_given_paths(self):
+        base_dir = self.make_dir()
+        lower_dir = factory.make_name('lower')
+        subdir = factory.make_name('sub')
+        os.makedirs(os.path.join(base_dir, lower_dir, subdir))
+        self.assertEqual(
+            [[lower_dir, subdir]],
+            extend_path(base_dir, [lower_dir]))
+
+    def test_extend_path_stops_if_no_subdirs_found(self):
+        self.assertItemsEqual([], extend_path(self.make_dir(), []))
+
+    def test_drill_down_follows_directory_tree(self):
+        base_dir = self.make_dir()
+        lower_dir = factory.make_name('lower')
+        os.makedirs(os.path.join(base_dir, lower_dir))
+        subdirs = [
+            factory.make_name('subdir-%d' % counter)
+            for counter in range(3)]
+        for subdir in subdirs:
+            os.makedirs(os.path.join(base_dir, lower_dir, subdir))
+        self.assertItemsEqual(
+            [[lower_dir, subdir] for subdir in subdirs],
+            drill_down(base_dir, [[lower_dir]]))
+
+    def test_drill_down_ignores_subdir_not_in_path(self):
+        base_dir = self.make_dir()
+        irrelevant_dir = factory.make_name('irrelevant')
+        irrelevant_subdir = factory.make_name('subdir')
+        relevant_dir = factory.make_name('relevant')
+        relevant_subdir = factory.make_name('subdir')
+        os.makedirs(os.path.join(base_dir, irrelevant_dir, irrelevant_subdir))
+        os.makedirs(os.path.join(base_dir, relevant_dir, relevant_subdir))
+        self.assertEqual(
+            [[relevant_dir, relevant_subdir]],
+            drill_down(base_dir, [[relevant_dir]]))
+
+    def test_drill_down_drops_paths_that_do_not_go_deep_enough(self):
+        base_dir = self.make_dir()
+        shallow_dir = factory.make_name('shallow')
+        os.makedirs(os.path.join(base_dir, shallow_dir))
+        deep_dir = factory.make_name('deep')
+        subdir = factory.make_name('sub')
+        os.makedirs(os.path.join(base_dir, deep_dir, subdir))
+        self.assertEqual(
+            [[deep_dir, subdir]],
+            drill_down(base_dir, [[shallow_dir], [deep_dir]]))

=== modified file 'src/provisioningserver/pxe/tftppath.py'
--- src/provisioningserver/pxe/tftppath.py	2012-08-30 10:50:31 +0000
+++ src/provisioningserver/pxe/tftppath.py	2012-09-12 08:19:26 +0000
@@ -14,6 +14,7 @@
     'compose_bootloader_path',
     'compose_config_path',
     'compose_image_path',
+    'list_boot_images',
     'locate_tftp_path',
     ]
 
@@ -83,3 +84,85 @@
     if path is None:
         return tftproot
     return os.path.join(tftproot, path.lstrip('/'))
+
+
+def is_visible_subdir(directory, subdir):
+    """Is `subdir` a non-hidden sub-directory of `directory`?"""
+    if subdir.startswith('.'):
+        return False
+    else:
+        return os.path.isdir(os.path.join(directory, subdir))
+
+
+def list_subdirs(directory):
+    """Return a list of non-hidden directories in `directory`."""
+    return [
+        subdir
+        for subdir in os.listdir(directory)
+            if is_visible_subdir(directory, subdir)]
+
+
+def extend_path(directory, path):
+    """Dig one directory level deeper on `os.path.join(directory, *path)`.
+
+    If `path` is a list of consecutive path elements drilling down from
+    `directory`, return a list of sub-directory paths leading one step
+    further down.
+
+    :param directory: Base directory that `path` is relative to.
+    :param path: A path to a subdirectory of `directory`, represented as
+        a list of path elements relative to `directory`.
+    :return: A list of paths that go one sub-directory level further
+        down from `path`.
+    """
+    return [
+        path + [subdir]
+        for subdir in list_subdirs(os.path.join(directory, *path))]
+
+
+def drill_down(directory, paths):
+    """Find the extensions of `paths` one level deeper into the filesystem.
+
+    :param directory: Base directory that each path in `paths` is relative to.
+    :param paths: A list of "path lists."  Each path list is a list of
+        path elements drilling down into the filesystem from `directory`.
+    :return: A list of paths, each of which drills one level deeper down into
+        the filesystem hierarchy than the originals in `paths`.
+    """
+    return sum([extend_path(directory, path) for path in paths], [])
+
+
+def extract_image_params(path):
+    """Represent a list of TFTP path elements as a boot-image dict."""
+    [arch, subarch, release, purpose] = path
+    return {
+        'architecture': arch,
+        'subarchitecture': subarch,
+        'release': release,
+        'purpose': purpose,
+    }
+
+
+def list_boot_images(tftproot):
+    """List the available boot images.
+
+    :param tftproot: TFTP root directory.
+    :return: An iterable of dicts, describing boot images as consumed by
+        the report_boot_images API call.
+    """
+    # The sub-directories directly under tftproot, if they contain
+    # images, represent architectures.
+    potential_archs = list_subdirs(tftproot)
+
+    # Starting point for iteration: paths that contain only the
+    # top-level subdirectory of tftproot, i.e. the architecture name.
+    paths = [[subdir] for subdir in potential_archs]
+
+    # Extend paths deeper into the filesystem, through the levels that
+    # represent sub-architecture, release, and purpose.  Any directory
+    # that doesn't extend this deep isn't a boot image.
+    for level in ['subarch', 'release', 'purpose']:
+        paths = drill_down(tftproot, paths)
+
+    # Each path we find this way should be a boot image.
+    return [extract_image_params(path) for path in paths]

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-09-10 14:27:00 +0000
+++ src/provisioningserver/tasks.py	2012-09-12 08:19:26 +0000
@@ -23,6 +23,7 @@
     'write_full_dns_config',
     ]
 
+import json
 from subprocess import (
     CalledProcessError,
     check_call,
@@ -30,13 +31,21 @@
     Popen,
     )
 
+from apiclient.maas_client import (
+    MAASClient,
+    MAASDispatcher,
+    MAASOAuth,
+    )
 from celery.task import task
 from celeryconfig import DHCP_CONFIG_FILE
 from provisioningserver.auth import (
+    get_recorded_api_credentials,
+    get_recorded_maas_url,
     record_api_credentials,
     record_maas_url,
     record_nodegroup_uuid,
     )
+from provisioningserver.config import Config
 from provisioningserver.dhcp import config
 from provisioningserver.dhcp.leases import upload_leases
 from provisioningserver.dns.config import (
@@ -44,11 +53,13 @@
     execute_rndc_command,
     setup_rndc,
     )
+from provisioningserver.logging import task_logger
 from provisioningserver.omshell import Omshell
 from provisioningserver.power.poweraction import (
     PowerAction,
     PowerActionFail,
     )
+from provisioningserver.pxe import tftppath
 
 # For each item passed to refresh_secrets, a refresh function to give it to.
 refresh_functions = {
@@ -320,3 +331,28 @@
 def restart_dhcp_server():
     """Restart the DHCP server."""
     check_call(['sudo', 'service', 'isc-dhcp-server', 'restart'])
+
+
+# =====================================================================
+# Boot images-related tasks
+# =====================================================================
+
+
+@task
+def report_boot_images():
+    """For master worker only: report available netboot images."""
+    maas_url = get_recorded_maas_url()
+    if maas_url is None:
+        task_logger.info("Not reporting boot images: don't have API URL yet.")
+        return
+    api_credentials = get_recorded_api_credentials()
+    if api_credentials is None:
+        task_logger.info("Not reporting boot images: don't have API key yet.")
+        return
+
+    images = tftppath.list_boot_images(
+        Config.load_from_cache()['tftp']['root'])
+
+    MAASClient(MAASOAuth(*api_credentials), MAASDispatcher(), maas_url).post(
+        'api/1.0/boot-images/', 'report_boot_images',
+        images=json.dumps(images))

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-09-10 14:27:00 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-09-12 08:19:26 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 from datetime import datetime
+import json
 import os
 import random
 from subprocess import (
@@ -21,6 +22,7 @@
     )
 
 from apiclient.creds import convert_tuple_to_string
+from apiclient.maas_client import MAASClient
 from celeryconfig import DHCP_CONFIG_FILE
 from maastesting.celery import CeleryFixture
 from maastesting.factory import factory
@@ -49,6 +51,7 @@
     )
 from provisioningserver.enum import POWER_TYPE
 from provisioningserver.power.poweraction import PowerActionFail
+from provisioningserver.pxe import tftppath
 from provisioningserver.tasks import (
     add_new_dhcp_host_map,
     Omshell,
@@ -56,6 +59,7 @@
     power_on,
     refresh_secrets,
     remove_dhcp_host_map,
+    report_boot_images,
     restart_dhcp_server,
     rndc_command,
     RNDC_COMMAND_MAX_RETRY,
@@ -66,6 +70,7 @@
     write_full_dns_config,
     )
 from provisioningserver.testing import network_infos
+from provisioningserver.testing.config import ConfigFixture
 from provisioningserver.testing.testcase import PservTestCase
 from testresources import FixtureResource
 from testtools.matchers import (
@@ -79,6 +84,15 @@
 arbitrary_mac = "AA:BB:CC:DD:EE:FF"
 
 
+def make_api_credentials():
+    """Create a tuple of fake API credentials."""
+    return (
+        factory.make_name('key'),
+        factory.make_name('token'),
+        factory.make_name('secret'),
+        )
+
+
 class TestRefreshSecrets(PservTestCase):
     """Tests for the `refresh_secrets` task."""
 
@@ -103,11 +117,7 @@
         self.assertEqual(maas_url, auth.get_recorded_maas_url())
 
     def test_updates_api_credentials(self):
-        credentials = (
-            factory.make_name('key'),
-            factory.make_name('token'),
-            factory.make_name('secret'),
-            )
+        credentials = make_api_credentials()
         refresh_secrets(
             api_credentials=convert_tuple_to_string(credentials))
         self.assertEqual(credentials, auth.get_recorded_api_credentials())
@@ -423,3 +433,59 @@
                     FileExists(),
                     FileExists(),
                 )))
+
+
+class TestBootImagesTasks(PservTestCase):
+
+    resources = (
+        ("celery", FixtureResource(CeleryFixture())),
+        )
+
+    def setUp(self):
+        super(TestBootImagesTasks, self).setUp()
+        self.useFixture(ConfigFixture({'tftp': {'root': self.make_dir()}}))
+
+    def make_image_params(self):
+        """Create a dict of parameters describing a boot image."""
+        return {
+            'architecture': factory.make_name('architecture'),
+            'subarchitecture': factory.make_name('subarchitecture'),
+            'release': factory.make_name('release'),
+            'purpose': factory.make_name('purpose'),
+        }
+
+    def set_maas_url(self):
+        auth.record_maas_url(
+            'http://127.0.0.1/%s' % factory.make_name('path'))
+
+    def set_api_credentials(self):
+        auth.record_api_credentials(':'.join(make_api_credentials()))
+
+    def test_sends_boot_images_to_server(self):
+        self.set_maas_url()
+        self.set_api_credentials()
+        image = self.make_image_params()
+        self.patch(tftppath, 'list_boot_images', Mock(return_value=[image]))
+        self.patch(MAASClient, 'post')
+
+        report_boot_images.delay()
+
+        self.assertItemsEqual(
+            [image],
+            json.loads(MAASClient.post.call_args[1]['images']))
+
+    def test_does_nothing_without_maas_url(self):
+        self.set_api_credentials()
+        self.patch(
+            tftppath, 'list_boot_images',
+            Mock(return_value=self.make_image_params()))
+        report_boot_images.delay()
+        self.assertItemsEqual([], tftppath.list_boot_images.call_args_list)
+
+    def test_does_nothing_without_credentials(self):
+        self.set_maas_url()
+        self.patch(
+            tftppath, 'list_boot_images',
+            Mock(return_value=self.make_image_params()))
+        report_boot_images.delay()
+        self.assertItemsEqual([], tftppath.list_boot_images.call_args_list)