Merge lp:~jtv/maas/import-from-configured-mirror into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Tell the import scripts, when run as a celery task, what archives to use.

This ties in with Raphaël's ongoing work: he's changing the UI to manage these same settings. I cribbed the settings' names from lp:~rvb/maas/mirror-settings-re-2

Archive locations are passed to the script through environment variables. The scripts already obeys these variables. The impact on the scripts themselves is altogether quite minimal, but one name in maas-import-ephemerals needed changing for uniformity. I added and documented some compatibility code.

URLs that have not been configured yet fall back on the defaults that are encoded in the import scripts. Missing archive locations are not passed through.

Tests pass for this branch even though it does not incorporate Raphaël's branches, which haven't landed yet. That's because my tests configure the archive settings themselves. In a separate branch, which relies on the synthesis of this branch and Raphaël's, I will add an integration test to establish that the two sets of changes operate on the same configuration items.

My test code repeats make_archive_url. It might be worth abstracting a single make_url somewhere that's available to all apps. But then the sensible step would be to sanitize all code that constructs URLs. A job best left to a separate branch!

Jeroen

[Raphaël Badin (rvb)]

Looks good!

[0]

96 + config = {
97 + name: Config.objects.get_config(name)
98 + for name in config_parameters}
99 + task_kwargs = {
100 + name: value
101 + for name, value in config.items()
102 + if value is not None}

I don't see why we need to filter out None values…? Also, there is probably no need for a second dict comprehension.

[1]

233 + archive_settings = {
234 + variable: value
235 + for variable, value in env.iteritems()
236 + if variable.endswith('_ARCHIVE')}

and

99 + task_kwargs = {
100 + name: value
101 + for name, value in config.items()
102 + if value is not None}

A word about style here. You may disagree but I'd be in favor of formatting that kind of statement this way, mainly because I think it's more readable

    task_kwargs = {
        name: value
        for name, value in config.items()
        if value is not None
    }

With your version I get the feeling that the "if value is not None" statement is inside the loop, which isn't true.

[2]

144 + archive_options = {
145 + arg: value
146 + for arg, value in kwargs.items()
147 + if arg.endswith('_archive')}

How about (and this is not only about style ;))

    archive_options = {
        arg: value
        for arg, value in kwargs.items()
        if arg in archives
    }

[3]

> My test code repeats make_archive_url. It might be worth abstracting a single make_url somewhere that's available
> to all apps. But then the sensible step would be to sanitize all code that constructs URLs. A job best left to a
> separate branch!

My code in lp:~rvb/maas/mirror-settings-re-2 could benefit from that too!

review: Approve

[Jeroen T. Vermeulen (jtv)]

> [0]
>
> 96 + config = {
> 97 + name: Config.objects.get_config(name)
> 98 + for name in config_parameters}
> 99 + task_kwargs = {
> 100 + name: value
> 101 + for name, value in config.items()
> 102 + if value is not None}
>
> I don't see why we need to filter out None values…? Also, there is probably
> no need for a second dict comprehension.

True, it can be done with just one. I thought it'd be hard to read, but it's not so bad so I changed it to just one.

I filtered out the None values because I didn't feel like making the “pass None for default” part of the task's signature. Normally it'd be fine but since a Celery task is a fairly special function-call boundary, I wanted to leave it some room for change. I'm not sure it really helps, but it lets the task specify usefully specify a default other than None, and it helped decouple introduction of the new parameters from introduction of the new config items.

> [1]
>
> 233 + archive_settings = {
> 234 + variable: value
> 235 + for variable, value in env.iteritems()
> 236 + if variable.endswith('_ARCHIVE')}
>
> and
>
> 99 + task_kwargs = {
> 100 + name: value
> 101 + for name, value in config.items()
> 102 + if value is not None}
>
> A word about style here. You may disagree but I'd be in favor of formatting
> that kind of statement this way, mainly because I think it's more readable
>
> task_kwargs = {
> name: value
> for name, value in config.items()
> if value is not None
> }
>
> With your version I get the feeling that the "if value is not None" statement
> is inside the loop, which isn't true.

My editor insists the indentation is the only right way, and I'm reluctant to second-guess it...

The practice of closing a comprehension on the same line goes way back in Launchpad history. I used to do it on a separate line but I think we decided against it eventually. May have been because of controversy over how far a closing bracket (or by extension, brace) should be indented.

> [2]
>
> 144 + archive_options = {
> 145 + arg: value
> 146 + for arg, value in kwargs.items()
> 147 + if arg.endswith('_archive')}
>
> How about (and this is not only about style ;))
>
> archive_options = {
> arg: value
> for arg, value in kwargs.items()
> if arg in archives
> }

I'll do you one better!

    archive_options = {arg: kwargs.get('arg') for arg in archives}

> [3]
>
> > My test code repeats make_archive_url. It might be worth abstracting a
> single make_url somewhere that's available
> > to all apps. But then the sensible step would be to sanitize all code that
> constructs URLs. A job best left to a
> > separate branch!
>
> My code in lp:~rvb/maas/mirror-settings-re-2 could benefit from that too!

A nice little cleanup for when we have time. *Cough* *cough*. :)

Jeroen

[Julian Edwards (julian-edwards)]

On Thursday 08 November 2012 16:07:29 you wrote:
> + config_parameters = {
> + 'http_proxy',
> + 'main_archive',
> + 'ports_archive',
> + 'cloud_images_archive',
> + }

How many places do we refer to these strings now? About time for some
encapsulation or at least a constant, as this is a recipe for disaster.

Especially when you start doing this in tests:

+ archives = {
+ 'main_archive': make_archive_url('main'),
+ 'ports_archive': make_archive_url('ports'),
+ 'cloud_images_archive': make_archive_url('cloud_images'),
+ }
+ for key, value in archives.items():
+ Config.objects.set_config(key, value)

review: Needs Fixing

=== modified file 'etc/maas/import_ephemerals'
--- etc/maas/import_ephemerals	2012-08-15 12:54:49 +0000
+++ etc/maas/import_ephemerals	2012-11-08 17:32:21 +0000
@@ -1,7 +1,14 @@
 ## get default settings from import_pxe_files
 [ ! -f /etc/maas/import_pxe_files ] || . /etc/maas/import_pxe_files
 
-#REMOTE_IMAGES_MIRROR="https://cloud-images.ubuntu.com"
+# CLOUD_IMAGES_ARCHIVE sets the URL where the script can download Ubuntu
+# cloud images.  This used to be called REMOTE_IMAGES_MIRROR, and the old
+# name is still supported for compatibility with older installations.
+if [ -n "$REMOTE_IMAGES_MIRROR" ]; then
+    CLOUD_IMAGES_ARCHIVE="$REMOTE_IMAGES_MIRROR"
+fi
+#CLOUD_IMAGES_ARCHIVE="https://cloud-images.ubuntu.com"
+
 #TARGET_NAME_PREFIX="iqn.2004-05.com.ubuntu:maas:"
 #DATA_DIR="/var/lib/maas/ephemeral"
 #RELEASES="precise"
 
=== modified file 'scripts/maas-import-ephemerals'
--- scripts/maas-import-ephemerals	2012-11-08 06:34:48 +0000
+++ scripts/maas-import-ephemerals	2012-11-08 17:32:21 +0000
@@ -20,7 +20,10 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 VERBOSITY=0
-REMOTE_IMAGES_MIRROR=${REMOTE_IMAGES_MIRROR:-https://maas.ubuntu.com/images}
+
+# Mirror to load cloud images from.  When the cluster controller runs the
+# import scripts, it provides a setting from the server side.
+CLOUD_IMAGES_ARCHIVE=${CLOUD_IMAGES_ARCHIVE:-https://maas.ubuntu.com/images}
 
 # iSCSI targets configuration file.
 SYS_TGT_CONF="/etc/tgt/targets.conf"
@@ -89,10 +92,10 @@
 
 
 query_remote() {
-    # query /query data at REMOTE_IMAGES_MIRROR
+    # query /query data at CLOUD_IMAGES_ARCHIVE
     # returns 7 values prefixed with 'r_'
     local iarch=$1 irelease=$2 istream=$3 out=""
-    local burl="${REMOTE_IMAGES_MIRROR}/query"
+    local burl="${CLOUD_IMAGES_ARCHIVE}/query"
     local url="$burl/$irelease/$istream/${STREAM}-dl.current.txt"
     local target="$TEMP_D/query/$release.$stream"
     mkdir -p -- "$TEMP_D/query"
@@ -168,7 +171,7 @@
     local wd="$1" exdir="" tarball=""
     shift
     local release=$1 stream=$2 label=$3 serial=$4 arch=$5 url=$6 name=$7
-    local furl="$REMOTE_IMAGES_MIRROR/$url"
+    local furl="$CLOUD_IMAGES_ARCHIVE/$url"
 
     mkdir -p "$wd"
     cat > "$wd/info" <<EOF
@@ -365,7 +368,7 @@
         query_local "$arch" "$release" "$BUILD_NAME" ||
             fail "failed to query local for $release/$arch"
         query_remote "$arch" "$release" "$BUILD_NAME" ||
-            fail "remote query of $REMOTE_IMAGES_MIRROR failed"
+            fail "remote query of $CLOUD_IMAGES_ARCHIVE failed"
 
         info="rel: $r_release, arch: $arch: name: $r_name"
         debug 2 "$info"
 
=== modified file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files	2012-11-08 06:34:48 +0000
+++ scripts/maas-import-pxe-files	2012-11-08 17:32:21 +0000
@@ -20,7 +20,8 @@
 local_settings="$(pwd)/$settings"
 [ -r $local_settings ] && . $local_settings
 
-# Download locations for Ubuntu releases.
+# Download locations for Ubuntu releases.  When the cluster controller runs
+# the import scripts, it provides settings from the server side.
 MAIN_ARCHIVE=${MAIN_ARCHIVE:-http://archive.ubuntu.com/ubuntu/}
 PORTS_ARCHIVE=${PORTS_ARCHIVE:-http://ports.ubuntu.com/ubuntu-ports/}
 
 
=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-11-08 06:34:48 +0000
+++ src/maasserver/models/nodegroup.py	2012-11-08 17:32:21 +0000
@@ -239,7 +239,16 @@
         """
         # Avoid circular imports.
         from maasserver.models import Config
-        task_kwargs = dict(http_proxy=Config.objects.get_config('http_proxy'))
+        config_parameters = {
+            'http_proxy',
+            'main_archive',
+            'ports_archive',
+            'cloud_images_archive',
+        }
+        task_kwargs = {
+            name: Config.objects.get_config(name)
+            for name in config_parameters
+                if Config.objects.get_config(name) is not None}
         import_boot_images.apply_async(queue=self.uuid, kwargs=task_kwargs)
 
     def add_dhcp_host_maps(self, new_leases):
 
=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-11-07 10:39:19 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-11-08 17:32:21 +0000
@@ -265,6 +265,14 @@
         self.assertItemsEqual(calls, recorder.apply_async.call_args_list)
 
 
+def make_archive_url(name):
+    """Create a fake archive URL."""
+    return "http://%s.example.com/%s/" % (
+        factory.make_name(name),
+        factory.make_name('path'),
+        )
+
+
 class TestNodeGroup(TestCase):
 
     resources = (
@@ -373,6 +381,24 @@
         recorder.assert_called_once_with(
             ['sudo', '-n', 'maas-import-pxe-files'], env=expected_env)
 
+    def test_import_boot_images_selects_archive_locations_from_config(self):
+        recorder = self.patch(nodegroup_module, 'import_boot_images')
+        nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
+
+        archives = {
+            'main_archive': make_archive_url('main'),
+            'ports_archive': make_archive_url('ports'),
+            'cloud_images_archive': make_archive_url('cloud_images'),
+        }
+        for key, value in archives.items():
+            Config.objects.set_config(key, value)
+
+        nodegroup.import_boot_images()
+
+        kwargs = recorder.apply_async.call_args[1]['kwargs']
+        archive_options = {arg: kwargs.get(arg) for arg in archives}
+        self.assertEqual(archives, archive_options)
+
     def test_import_boot_images_sent_to_nodegroup_queue(self):
         recorder = self.patch(nodegroup_module, 'import_boot_images', Mock())
         nodegroup = factory.make_node_group()
 
=== modified file 'src/maasserver/tests/test_views_settings.py'
--- src/maasserver/tests/test_views_settings.py	2012-11-08 10:40:48 +0000
+++ src/maasserver/tests/test_views_settings.py	2012-11-08 17:32:21 +0000
@@ -38,7 +38,10 @@
     AdminLoggedInTestCase,
     LoggedInTestCase,
     )
-from mock import call
+from mock import (
+    ANY,
+    call,
+    )
 
 
 class SettingsTest(AdminLoggedInTestCase):
@@ -248,7 +251,7 @@
             reverse('settings'), {'import_all_boot_images': 1})
         self.assertEqual(httplib.FOUND, response.status_code)
         calls = [
-           call(queue=nodegroup.work_queue, kwargs={'http_proxy': None})
+           call(queue=nodegroup.work_queue, kwargs=ANY)
            for nodegroup in accepted_nodegroups
         ]
         self.assertItemsEqual(calls, recorder.apply_async.call_args_list)
 
=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-11-08 06:34:48 +0000
+++ src/provisioningserver/tasks.py	2012-11-08 17:32:21 +0000
@@ -376,9 +376,16 @@
 # =====================================================================
 
 @task
-def import_boot_images(http_proxy=None):
+def import_boot_images(http_proxy=None, main_archive=None, ports_archive=None,
+                       cloud_images_archive=None):
     env = dict(os.environ)
     if http_proxy is not None:
         env['http_proxy'] = http_proxy
         env['https_proxy'] = http_proxy
+    if main_archive is not None:
+        env['MAIN_ARCHIVE'] = main_archive
+    if ports_archive is not None:
+        env['PORTS_ARCHIVE'] = ports_archive
+    if cloud_images_archive is not None:
+        env['CLOUD_IMAGES_ARCHIVE'] = cloud_images_archive
     check_call(['sudo', '-n', 'maas-import-pxe-files'], env=env)
 
=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-11-08 06:34:48 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-11-08 17:32:21 +0000
@@ -540,6 +540,11 @@
 
 class TestImportPxeFiles(PservTestCase):
 
+    def make_archive_url(self, name=None):
+        if name is None:
+            name = factory.make_name('archive')
+        return 'http://%s.example.com/%s' % (name, factory.make_name('path'))
+
     def test_import_boot_images(self):
         recorder = self.patch(tasks, 'check_call', Mock())
         import_boot_images()
@@ -560,3 +565,21 @@
         expected_env = dict(os.environ, http_proxy=proxy, https_proxy=proxy)
         recorder.assert_called_once_with(
             ['sudo', '-n', 'maas-import-pxe-files'], env=expected_env)
+
+    def test_import_boot_images_sets_archive_locations(self):
+        self.patch(tasks, 'check_call')
+        archives = {
+            'main_archive': self.make_archive_url('main'),
+            'ports_archive': self.make_archive_url('ports'),
+            'cloud_images_archive': self.make_archive_url('cloud-images'),
+        }
+        expected_settings = {
+            parameter.upper(): value
+            for parameter, value in archives.items()}
+        import_boot_images(**archives)
+        env = tasks.check_call.call_args[1]['env']
+        archive_settings = {
+            variable: value
+            for variable, value in env.iteritems()
+                if variable.endswith('_ARCHIVE')}
+        self.assertEqual(expected_settings, archive_settings)