Merge lp:~ltrager/maas/lp1554636 into lp:~maas-committers/maas/trunk

[Lee Trager]

Update iSCSI targets on every import (in case last time failed to update).

In LP:1554636 MAAS was serving old images because when the image was updated the TGT target to be replaced was removed. Since the target was cached the old version kept getting served. This modifies import_images to reload the TGT targets every time its run(every 15 minutes). This is a stop gap until we replace TGT with downloading the image over HTTP.

[Mike Pontillo (mpontillo)]

A couple questions:

 - When many images are downloaded, is it expensive to update the targets?
 - When tgt is 'reloaded', how invasive is that operation? (Are existing iSCSI connections interrupted, or do they remain open until unmounted -- at which point they become unavailable?)

review: Needs Information

[Lee Trager (ltrager)]

MAAS has always tried to update the iSCSI targets whenever images are added, removed, or updated. It does this by using the tgt-admin --update command which compares a configuration file to what tgt is currently serving. It adds new targets, removes targets which are no longer in the config file, and reloads files which have changed. If a target is in use tgt-admin --update will not remove or update that target. When this happens the user is left using an outdated image.

What this change does is run tgt-admin --update every time the an import is run. So any target that wasn't able to be updated will be updated on the next import(happens every 15 minutes). The amount of target loads stays the same, as targets are only changed when new images actually come in. For the most part this just causes the tgt-admin to load the config and see that there is nothing to update.

[Mike Pontillo (mpontillo)]

All right. Sounds like this is a good improvement, then. Thanks!

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~ltrager/maas/lp1554636 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://security.ubuntu.com/ubuntu xenial-security InRelease
Hit:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [95.7 kB]
Get:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [92.2 kB]
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [356 kB]
Fetched 544 kB in 0s (985 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1...

[MAAS Lander (maas-lander)]

The attempt to merge lp:~ltrager/maas/lp1554636 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://security.ubuntu.com/ubuntu xenial-security InRelease [94.5 kB]
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [95.7 kB]
Get:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [92.2 kB]
Get:5 http://security.ubuntu.com/ubuntu xenial-security/main Sources [45.1 kB]
Get:6 http://security.ubuntu.com/ubuntu xenial-security/main amd64 Packages [161 kB]
Get:7 http://security.ubuntu.com/ubuntu xenial-security/main Translation-en [66.6 kB]
Get:8 http://security.ubuntu.com/ubuntu xenial-security/universe amd64 Packages [52.4 kB]
Get:9 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main Sources [203 kB]
Get:10 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages [417 kB]
Get:11 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main Translation-en [160 kB]
Get:12 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [357 kB]
Get:13 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe Translation-en [129 kB]
Fetched 1,872 kB in 0s (2,770 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest ver...

[MAAS Lander (maas-lander)]

The attempt to merge lp:~ltrager/maas/lp1554636 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [95.7 kB]
Get:3 http://security.ubuntu.com/ubuntu xenial-security InRelease [94.5 kB]
Get:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [92.2 kB]
Fetched 282 kB in 0s (618 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18-1).
python-netifaces is already the new...

=== modified file 'src/provisioningserver/import_images/boot_resources.py'
--- src/provisioningserver/import_images/boot_resources.py	2016-09-30 18:14:53 +0000
+++ src/provisioningserver/import_images/boot_resources.py	2016-11-02 08:28:19 +0000
@@ -233,6 +233,11 @@
     return BootSources.parse(StringIO(sources_yaml))
 
 
+def update_iscsi_targets(snapshot_path):
+    maaslog.info("Updating boot image iSCSI targets.")
+    update_targets_conf(snapshot_path)
+
+
 def import_images(sources):
     """Import images.  Callable from the command line.
 
@@ -244,6 +249,9 @@
         maaslog.warning("Can't import: region did not provide a source.")
         return False
 
+    with ClusterConfiguration.open() as config:
+        storage = FilePath(config.tftp_root).parent().path
+
     with tempdir('keyrings') as keyrings_path:
         # XXX: Band-aid to ensure that the keyring_data is bytes. Future task:
         # try to figure out why this sometimes happens.
@@ -259,15 +267,15 @@
 
         image_descriptions = download_all_image_descriptions(sources)
         if image_descriptions.is_empty():
+            update_iscsi_targets(os.path.join(storage, 'current'))
             maaslog.warning(
                 "Finished importing boot images, the region does not have "
                 "any boot images available.")
             return False
 
-        with ClusterConfiguration.open() as config:
-            storage = FilePath(config.tftp_root).parent().path
         meta_file_content = image_descriptions.dump_json()
         if meta_contains(storage, meta_file_content):
+            update_iscsi_targets(os.path.join(storage, 'current'))
             maaslog.info(
                 "Finished importing boot images, the region does not "
                 "have any new images.")
@@ -279,6 +287,7 @@
             snapshot_path = download_all_boot_resources(
                 sources, storage, product_mapping)
         except:
+            update_iscsi_targets(os.path.join(storage, 'current'))
             # Cleanup snapshots and cache since download failed.
             maaslog.warning(
                 "Unable to import boot images; cleaning up failed snapshot "
@@ -295,8 +304,8 @@
 
     # If we got here, all went well.  This is now truly the "current" snapshot.
     update_current_symlink(storage, snapshot_path)
-    maaslog.info("Updating boot image iSCSI targets.")
-    update_targets_conf(snapshot_path)
+
+    update_iscsi_targets(snapshot_path)
 
     # Now cleanup the old snapshots and cache.
     maaslog.info('Cleaning up old snapshots and cache.')

=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
--- src/provisioningserver/import_images/tests/test_boot_resources.py	2016-10-14 17:37:37 +0000
+++ src/provisioningserver/import_images/tests/test_boot_resources.py	2016-11-02 08:28:19 +0000
@@ -19,11 +19,15 @@
     Popen,
 )
 from unittest import mock
-from unittest.mock import call
+from unittest.mock import (
+    call,
+    MagicMock,
+)
 
 from maastesting.factory import factory
 from maastesting.matchers import (
     MockAnyCall,
+    MockCalledOnce,
     MockCalledOnceWith,
     MockCalledWith,
     MockCallsMatch,
@@ -545,6 +549,7 @@
             BootImageMapping())
         self.patch_maaslog()
         self.patch(boot_resources, 'RepoWriter')
+        self.patch(boot_resources, 'update_iscsi_targets')
         args = self.make_args(sources_file=sources_fixture.filename)
 
         boot_resources.main(args)
@@ -691,3 +696,151 @@
         boot_resources.import_images(sources)
         self.assertThat(
             fake_write_all_keyrings, MockCalledWith(mock.ANY, sources))
+
+    def test__returns_false_when_no_images(self):
+        # Stop import_images() from actually doing anything.
+        self.patch(boot_resources, 'maaslog')
+        fake_download_all_image_descriptions = self.patch(
+            boot_resources, 'download_all_image_descriptions')
+        fake_download_all_image_descriptions.return_value = MagicMock()
+        fake_update_iscsi_targets = self.patch(
+            boot_resources, 'update_iscsi_targets')
+
+        self.patch(boot_resources, 'write_all_keyrings')
+        sources = [
+            {
+                'keyring_data': self.getUniqueString(),
+                'url': factory.make_name("something"),
+                'selections': [
+                    {
+                        'os': factory.make_name("os"),
+                        'release': factory.make_name("release"),
+                        'arches': [factory.make_name("arch")],
+                        'subarches': [factory.make_name("subarch")],
+                        'labels': [factory.make_name("label")],
+                    },
+                    ],
+            },
+            ],
+        self.assertFalse(boot_resources.import_images(sources))
+        self.assertThat(fake_update_iscsi_targets, MockCalledOnce())
+
+    def test__returns_false_when_no_new_images(self):
+        # Stop import_images() from actually doing anything.
+        self.patch(boot_resources, 'maaslog')
+        fake_download_all_image_descriptions = self.patch(
+            boot_resources, 'download_all_image_descriptions')
+        fake_image_descriptions = MagicMock()
+        fake_image_descriptions.is_empty.return_value = False
+        fake_download_all_image_descriptions.return_value = (
+            fake_image_descriptions)
+        self.patch(boot_resources, 'meta_contains').return_value = True
+        fake_update_iscsi_targets = self.patch(
+            boot_resources, 'update_iscsi_targets')
+
+        self.patch(boot_resources, 'write_all_keyrings')
+        sources = [
+            {
+                'keyring_data': self.getUniqueString(),
+                'url': factory.make_name("something"),
+                'selections': [
+                    {
+                        'os': factory.make_name("os"),
+                        'release': factory.make_name("release"),
+                        'arches': [factory.make_name("arch")],
+                        'subarches': [factory.make_name("subarch")],
+                        'labels': [factory.make_name("label")],
+                    },
+                    ],
+            },
+            ],
+        self.assertFalse(boot_resources.import_images(sources))
+        self.assertThat(fake_update_iscsi_targets, MockCalledOnce())
+
+    def test__cleans_up_on_failure(self):
+        # Stop import_images() from actually doing anything.
+        self.patch(boot_resources, 'maaslog')
+        fake_download_all_image_descriptions = self.patch(
+            boot_resources, 'download_all_image_descriptions')
+        fake_image_descriptions = MagicMock()
+        fake_image_descriptions.is_empty.return_value = False
+        fake_download_all_image_descriptions.return_value = (
+            fake_image_descriptions)
+        self.patch(boot_resources, 'meta_contains').return_value = False
+        self.patch(boot_resources, 'map_products')
+        self.patch(
+            boot_resources, 'download_all_boot_resources'
+            ).side_effect = Exception
+        fake_update_iscsi_targets = self.patch(
+            boot_resources, 'update_iscsi_targets')
+        fake_cleanup_snapshots_and_cache = self.patch(
+            boot_resources, 'cleanup_snapshots_and_cache')
+
+        self.patch(boot_resources, 'write_all_keyrings')
+        sources = [
+            {
+                'keyring_data': self.getUniqueString(),
+                'url': factory.make_name("something"),
+                'selections': [
+                    {
+                        'os': factory.make_name("os"),
+                        'release': factory.make_name("release"),
+                        'arches': [factory.make_name("arch")],
+                        'subarches': [factory.make_name("subarch")],
+                        'labels': [factory.make_name("label")],
+                    },
+                    ],
+            },
+            ],
+        self.assertRaises(
+            Exception, boot_resources.import_images, sources)
+        self.assertThat(fake_update_iscsi_targets, MockCalledOnce())
+        self.assertThat(fake_cleanup_snapshots_and_cache, MockCalledOnce())
+
+    def test__runs_import_and_returns_true(self):
+        # Stop import_images() from actually doing anything.
+        self.patch(boot_resources, 'maaslog')
+        fake_download_all_image_descriptions = self.patch(
+            boot_resources, 'download_all_image_descriptions')
+        fake_image_descriptions = MagicMock()
+        fake_image_descriptions.is_empty.return_value = False
+        fake_download_all_image_descriptions.return_value = (
+            fake_image_descriptions)
+        self.patch(boot_resources, 'meta_contains').return_value = False
+        self.patch(boot_resources, 'map_products')
+        self.patch(boot_resources, 'download_all_boot_resources')
+        fake_write_snapshot_metadata = self.patch(
+            boot_resources, 'write_snapshot_metadata')
+        fake_targets_conf = self.patch(
+            boot_resources, 'write_targets_conf')
+        fake_link_bootloaders = self.patch(boot_resources, 'link_bootloaders')
+        fake_update_current_symlink = self.patch(
+            boot_resources, 'update_current_symlink')
+        fake_update_iscsi_targets = self.patch(
+            boot_resources, 'update_iscsi_targets')
+        fake_cleanup_snapshots_and_cache = self.patch(
+            boot_resources, 'cleanup_snapshots_and_cache')
+
+        self.patch(boot_resources, 'write_all_keyrings')
+        sources = [
+            {
+                'keyring_data': self.getUniqueString(),
+                'url': factory.make_name("something"),
+                'selections': [
+                    {
+                        'os': factory.make_name("os"),
+                        'release': factory.make_name("release"),
+                        'arches': [factory.make_name("arch")],
+                        'subarches': [factory.make_name("subarch")],
+                        'labels': [factory.make_name("label")],
+                    },
+                    ],
+            },
+            ],
+        self.assertTrue(boot_resources.import_images(sources))
+        self.assertThat(fake_write_snapshot_metadata, MockCalledOnce())
+        self.assertThat(fake_targets_conf, MockCalledOnce())
+        self.assertThat(fake_link_bootloaders, MockCalledOnce())
+        self.assertThat(fake_update_current_symlink, MockCalledOnce())
+        self.assertThat(fake_update_iscsi_targets, MockCalledOnce())
+        self.assertThat(fake_cleanup_snapshots_and_cache, MockCalledOnce())