launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/more-kernel-options-ephemeral into lp:maas with lp:~rvb/maas/more-kernel-options as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/more-kernel-options-ephemeral/+merge/118397

This branch adds the name of the ephemeral to the iscsi_target_name parameters in the kernel command line.

The maas-import-ephemerals script imports ephemeral images and also create an 'info' file which contains the information we need to use in the iscsi_target_name parameter.  The ephemeral images.info are in directory named after the release data (e.g.  /var/lib/maas/ephemeral/precise/ephemeral/i386/20120424/info).

= Pre-imp =

This was mostly discussed with Scott.

We thought of two ways to do this:
a) modify the maas-import-ephemerals so that it would maintain a symbolic link to the last ephemeral import.
b) list the directories for a specific release/architecture, sort them, and take the first one.

I choose solution b) as it doesn't require to change the script, it still lets an admin trick the system (by manually creating a new directory) if one wants to use an old ephemeral image.

= Notes =

I've added the utility parse_config() to parse the ephemeral info file (and I've used it in omshell).

I think we should improve the error handling in get_first_directory() and get_ephemeral_name() to raise a proper error when the import hasn't been run.  But this will happen in a follow-up branch.
-- 
https://code.launchpad.net/~rvb/maas/more-kernel-options-ephemeral/+merge/118397
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/more-kernel-options-ephemeral into lp:maas.

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-07-26 08:10:52 +0000
+++ src/maas/settings.py	2012-08-06 20:59:20 +0000
@@ -296,6 +296,9 @@
 # doing.
 COMMISSIONING_SCRIPT = '/etc/maas/commissioning-user-data'
 
+# The location of the ephemeral images/infos.
+EPHEMERAL_ROOT = "/var/lib/maas/ephemeral"
+
 # The duration, in minutes, after which we consider a commissioning node
 # to have failed and mark it as FAILED_TESTS.
 COMMISSIONING_TIMEOUT = 60

=== modified file 'src/maasserver/kernel_opts.py'
--- src/maasserver/kernel_opts.py	2012-08-06 20:59:20 +0000
+++ src/maasserver/kernel_opts.py	2012-08-06 20:59:20 +0000
@@ -14,9 +14,13 @@
     'compose_kernel_command_line',
     ]
 
+import os
+
+from django.conf import settings
 from maasserver.server_address import get_maas_facing_server_address
 from maasserver.utils import absolute_reverse
 from provisioningserver.pxe.tftppath import compose_image_path
+from provisioningserver.utils import parse_config
 
 
 def compose_initrd_opt(arch, subarch, release, purpose):
@@ -86,9 +90,31 @@
         ]
 
 
+def get_last_directory(root):
+    """Return the last directory from the directories in the given root.
+
+    This is used to get the most recent ephemeral import directory.
+    The ephemeral directories are named after the release date: 20120424,
+    20120424, 20120301, etc. so fetching the last one (sorting by name)
+    returns the most recent.
+    """
+    dirs = [
+        os.path.join(root, directory) for directory in os.listdir(root)]
+    dirs = filter(os.path.isdir, dirs)
+    return sorted(dirs)[-1]
+
+
 def get_ephemeral_name(release, arch):
-    # TODO: do something real here.
-    return "maas-precise-12.04-i386-ephemeral-20120424"
+    """Returns the name of the ephemeral image.
+
+    That information is read from the config file named 'info' in the
+    ephemeral directory e.g:
+    /var/lib/maas/ephemeral/precise/ephemeral/i386/20120424/info
+    """
+    root = os.path.join(settings.EPHEMERAL_ROOT, release, 'ephemeral', arch)
+    filename = os.path.join(get_last_directory(root), 'info')
+    name = parse_config(filename, separator="=")['name']
+    return name
 
 
 def compose_purpose_opts(release, arch, purpose):

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-06 12:23:00 +0000
+++ src/maasserver/tests/test_api.py	2012-08-06 20:59:20 +0000
@@ -34,7 +34,10 @@
 from django.db.models.signals import post_save
 from django.http import QueryDict
 from fixtures import Fixture
-from maasserver import api
+from maasserver import (
+    api,
+    kernel_opts,
+    )
 from maasserver.api import (
     extract_constraints,
     extract_oauth_key,
@@ -77,6 +80,7 @@
     )
 from maasserver.utils import map_enum
 from maastesting.djangotestcase import TransactionTestCase
+from maastesting.fakemethod import FakeMethod
 from maastesting.matchers import ContainsAll
 from metadataserver.models import (
     NodeKey,
@@ -2334,6 +2338,10 @@
         # Some parameters are optional, others are mandatory. The
         # absence of a mandatory parameter always results in a BAD
         # REQUEST response.
+        # Silence `get_ephemeral_name`.
+        self.patch(
+            kernel_opts, 'get_ephemeral_name',
+            FakeMethod(result=factory.getRandomString()))
         expected_response_to_missing_parameter = {
             'arch': httplib.BAD_REQUEST,
             'subarch': httplib.OK,
@@ -2347,6 +2355,10 @@
             expected_response_to_missing_parameter, observed_response)
 
     def test_pxeconfig_appends_enlistment_preseed_url_for_unknown_node(self):
+        # Silence `get_ephemeral_name`.
+        self.patch(
+            kernel_opts, 'get_ephemeral_name',
+            FakeMethod(result=factory.getRandomString()))
         params = self.get_params()
         params['mac'] = factory.getRandomMACAddress()
         response = self.client.get(reverse('pxeconfig'), params)

=== modified file 'src/maasserver/tests/test_kernel_opts.py'
--- src/maasserver/tests/test_kernel_opts.py	2012-08-06 20:59:20 +0000
+++ src/maasserver/tests/test_kernel_opts.py	2012-08-06 20:59:20 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 import httplib
+import os
 
 from django.conf import settings
 from maasserver.api import get_boot_purpose
@@ -21,12 +22,13 @@
     compose_kernel_command_line,
     compose_preseed_opt,
     compose_preseed_url,
+    get_last_directory,
     )
-from maasserver.server_address import get_maas_facing_server_address
 from maasserver.preseed import (
     get_enlist_preseed,
     get_preseed,
     )
+from maasserver.server_address import get_maas_facing_server_address
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maastesting.matchers import ContainsAll
@@ -34,6 +36,19 @@
 from testtools.matchers import StartsWith
 
 
+class TestUtilitiesKernelOpts(TestCase):
+
+    def test_get_last_directory(self):
+        root = self.make_dir()
+        dir1 = os.path.join(root, '20120405')
+        dir2 = os.path.join(root, '20120105')
+        dir3 = os.path.join(root, '20120403')
+        os.makedirs(dir1)
+        os.makedirs(dir2)
+        os.makedirs(dir3)
+        self.assertEqual(dir1, get_last_directory(root))
+
+
 class TestKernelOpts(TestCase):
 
     def test_compose_kernel_command_line_accepts_None_for_unknown_node(self):
@@ -131,17 +146,42 @@
                 factory.make_name('subarch'),
                 purpose=factory.make_name('purpose')))
 
+    def create_ephemeral_info(self, name, arch):
+        """Create a pseudo-real ephemeral info file."""
+        release = 'precise'
+        epheneral_info = """
+            release=precise
+            stream=ephemeral
+            label=release
+            serial=20120424
+            arch=i386
+            name=%s
+            """ % name
+        ephemeral_root = self.make_dir()
+        self.patch(settings, 'EPHEMERAL_ROOT', ephemeral_root)
+        ephemeral_dir = os.path.join(
+            ephemeral_root, release, 'ephemeral', arch,
+            factory.make_name('release'))
+        os.makedirs(ephemeral_dir)
+        factory.make_file(
+            ephemeral_dir, name='info', contents=epheneral_info)
+
     def test_compose_kernel_command_line_inc_purpose_opts_comm_node(self):
         # The result of compose_kernel_command_line includes the purpose
         # options for a "commissioning" node.
+        ephemeral_name = factory.make_name("ephemeral")
+        arch = factory.make_name('arch')
+        self.create_ephemeral_info(ephemeral_name, arch)
         node = factory.make_node()
+        target_name_prefix = "iqn.2004-05.com.ubuntu:maas"
         self.assertThat(
             compose_kernel_command_line(
-                node, factory.make_name('arch'),
+                node, arch,
                 factory.make_name('subarch'),
                 purpose="commissioning"),
             ContainsAll([
-                "iscsi_target_name=iqn.2004-05.com.ubuntu:maas",
+                "iscsi_target_name=%s:%s" % (
+                    target_name_prefix, ephemeral_name),
                 "iscsi_target_port=3260",
                 "iscsi_target_ip=%s" % get_maas_facing_server_address(),
                 ]))

=== modified file 'src/provisioningserver/omshell.py'
--- src/provisioningserver/omshell.py	2012-07-30 23:34:57 +0000
+++ src/provisioningserver/omshell.py	2012-08-06 20:59:20 +0000
@@ -28,6 +28,8 @@
 from tempfile import mkdtemp
 from textwrap import dedent
 
+from provisioningserver.utils import parse_config
+
 
 def call_dnssec_keygen(tmpdir):
     return check_output(
@@ -56,19 +58,12 @@
             raise AssertionError("dnssec-keygen didn't generate anything")
         key_id = key_id.strip()  # Remove trailing newline.
         key_file_name = os.path.join(tmpdir, key_id + '.private')
-        with open(key_file_name, 'rb') as f:
-            key_file = f.read()
-
-        for line in key_file.splitlines():
-            try:
-                field, value = line.split(":")
-            except ValueError:
-                continue
-            if field == "Key":
-                return value.strip()
-
-        raise AssertionError(
-            "Key field not found in output from dnssec-keygen")
+        config = parse_config(key_file_name)
+        if 'Key' in config:
+            return config['Key']
+        else:
+            raise AssertionError(
+                "Key field not found in output from dnssec-keygen")
     finally:
         shutil.rmtree(tmpdir)
 

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-07-23 13:24:17 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-08-06 20:59:20 +0000
@@ -32,6 +32,7 @@
     increment_age,
     incremental_write,
     MainScript,
+    parse_config,
     Safe,
     ShellTemplate,
     )
@@ -105,6 +106,30 @@
             os.stat(self.filename).st_mtime, old_mtime, delta=0.01)
 
 
+class ParseConfigTest(TestCase):
+    """Testing for the method `parse_config`."""
+
+    def test_parse_config_parses_config_file(self):
+        contents = """
+        key1: value1
+        key2  :  value2
+        """
+        file_name = self.make_file(contents=contents)
+        self.assertEqual(
+            {'key1': 'value1', 'key2': 'value2'},
+            parse_config(file_name))
+
+    def test_parse_config_parse_alternate_separator(self):
+        contents = """
+        key1= value1
+        key2   =  value2
+        """
+        file_name = self.make_file(contents=contents)
+        self.assertEqual(
+            {'key1': 'value1', 'key2': 'value2'},
+            parse_config(file_name, separator='='))
+
+
 class TestShellTemplate(TestCase):
     """Test `ShellTemplate`."""
 

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-07-23 21:22:32 +0000
+++ src/provisioningserver/utils.py	2012-08-06 20:59:20 +0000
@@ -16,6 +16,7 @@
     "deferred",
     "incremental_write",
     "MainScript",
+    "parse_config",
     "ShellTemplate",
     "xmlrpc_export",
     ]
@@ -136,6 +137,30 @@
     os.utime(filename, (new_mtime, new_mtime))
 
 
+def parse_config(file_name, separator=":"):
+    """Returns a dictionary out of the provided config file.
+
+    The expected format of the file is:
+    key1:value1
+    key2:value2
+    ...
+
+    We couldn't use python's ConfigParse module here because it insists
+    on having sections.
+    """
+    with open(file_name, 'rb') as f:
+        data = f.read()
+
+    result = {}
+    for line in data.splitlines():
+        try:
+            field, value = line.split(separator)
+            result[field.strip()] = value.strip()
+        except ValueError:
+            continue
+    return result
+
+
 class Safe:
     """An object that is safe to render as-is."""