Merge lp:~blake-rouse/maas/fix-1629045 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Cleanup snapshots and cache directory when import fails.

[Andres Rodriguez (andreserl)]

lgtm! Just one comment inline that needs to be addressed beforehand.

review: Approve

=== modified file 'src/provisioningserver/import_images/boot_resources.py'
--- src/provisioningserver/import_images/boot_resources.py	2016-09-11 21:35:53 +0000
+++ src/provisioningserver/import_images/boot_resources.py	2016-09-30 18:15:17 +0000
@@ -275,8 +275,16 @@
 
         product_mapping = map_products(image_descriptions)
 
-        snapshot_path = download_all_boot_resources(
-            sources, storage, product_mapping)
+        try:
+            snapshot_path = download_all_boot_resources(
+                sources, storage, product_mapping)
+        except:
+            # Cleanup snapshots and cache since download failed.
+            maaslog.warning(
+                "Unable to import boot images; cleaning up failed snapshot "
+                "and cache.")
+            cleanup_snapshots_and_cache(storage)
+            raise
 
     maaslog.info("Writing boot image metadata and iSCSI targets.")
     write_snapshot_metadata(snapshot_path, meta_file_content)

=== modified file 'src/provisioningserver/import_images/cleanup.py'
--- src/provisioningserver/import_images/cleanup.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/import_images/cleanup.py	2016-09-30 18:15:17 +0000
@@ -14,7 +14,10 @@
 def list_old_snapshots(storage):
     """List of snapshot directories that are no longer in use."""
     current_dir = os.path.join(storage, 'current')
-    current_snapshot = os.path.basename(os.readlink(current_dir))
+    if os.path.exists(current_dir):
+        current_snapshot = os.path.basename(os.readlink(current_dir))
+    else:
+        current_snapshot = None
     return [
         os.path.join(storage, directory)
         for directory in os.listdir(storage)
@@ -32,11 +35,14 @@
 def list_unused_cache_files(storage):
     """List of cache files that are no longer being referenced by snapshots."""
     cache_dir = os.path.join(storage, 'cache')
-    cache_files = [
-        os.path.join(cache_dir, filename)
-        for filename in os.listdir(cache_dir)
-        if os.path.isfile(os.path.join(cache_dir, filename))
-        ]
+    if os.path.exists(cache_dir):
+        cache_files = [
+            os.path.join(cache_dir, filename)
+            for filename in os.listdir(cache_dir)
+            if os.path.isfile(os.path.join(cache_dir, filename))
+            ]
+    else:
+        cache_files = []
     return [
         cache_file
         for cache_file in cache_files

=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
--- src/provisioningserver/import_images/tests/test_boot_resources.py	2016-08-30 18:45:05 +0000
+++ src/provisioningserver/import_images/tests/test_boot_resources.py	2016-09-30 18:15:17 +0000
@@ -56,6 +56,7 @@
 from testtools.matchers import (
     DirExists,
     FileExists,
+    Not,
 )
 import yaml
 
@@ -484,6 +485,30 @@
             ],
             list(meta_data[osystem][arch][subarch][release][label]))
 
+    def test_failed_run_deletes_snapshot(self):
+        # Patch out things that we don't want running during the test.  Patch
+        # at a low level, so that we exercise all the function calls that a
+        # unit test might not put to the test.
+        self.patch_maaslog()
+        self.patch(boot_resources, 'call_and_check')
+        self.patch(boot_resources, "service_monitor")
+
+        args = self.make_working_args()
+
+        # Cause the import to fail.
+        exception_type = factory.make_exception_type()
+        mock_download = self.patch(
+            boot_resources, "download_all_boot_resources")
+        mock_download.side_effect = exception_type()
+
+        # Run the import code.
+        self.assertRaises(exception_type, boot_resources.main, args)
+
+        # Verify the reuslts.
+        self.assertThat(os.path.join(self.storage, 'cache'), Not(DirExists()))
+        self.assertThat(
+            os.path.join(self.storage, 'current'), Not(DirExists()))
+
     def test_warns_if_no_sources_selected(self):
         self.patch_maaslog()
         sources_fixture = self.useFixture(BootSourcesFixture([]))

=== modified file 'src/provisioningserver/import_images/tests/test_cleanup.py'
--- src/provisioningserver/import_images/tests/test_cleanup.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/import_images/tests/test_cleanup.py	2016-09-30 18:15:17 +0000
@@ -40,6 +40,11 @@
             os.link(cache_path, link_path)
         return cache_path
 
+    def test_list_old_snapshots_returns_all(self):
+        storage = self.make_dir()
+        snapshots = [self.make_snapshot_dir(storage) for _ in range(3)]
+        self.assertItemsEqual(snapshots, cleanup.list_old_snapshots(storage))
+
     def test_list_old_snapshots_returns_all_but_current_directory(self):
         storage = self.make_dir()
         snapshots = [self.make_snapshot_dir(storage) for _ in range(3)]
@@ -62,6 +67,11 @@
             ]
         self.assertEqual([], remaining_snapshots)
 
+    def test_list_unused_cache_files_returns_empty(self):
+        storage = self.make_dir()
+        self.assertItemsEqual(
+            [], cleanup.list_unused_cache_files(storage))
+
     def test_list_unused_cache_files_returns_all_files_nlink_equal_one(self):
         storage = self.make_dir()
         cache_nlink_1 = [self.make_cache_file(storage) for _ in range(3)]