Merge lp:~allenap/maas/remove-temp-dir-after-refresh--bug-1587544 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Ensure that the temporary directory created when refreshing a rack or region controller is removed.

[Blake Rouse (blake-rouse)]

Looks good. Much cleaner and easier to read.

review: Approve

=== modified file 'src/provisioningserver/refresh/__init__.py'
--- src/provisioningserver/refresh/__init__.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/refresh/__init__.py	2016-05-31 17:21:35 +0000
@@ -3,7 +3,6 @@
 
 """Functionality to refresh rack controller hardware and networking details."""
 import os
-import shutil
 import socket
 import stat
 import subprocess
@@ -107,18 +106,27 @@
         'token_key': token_key,
         'token_secret': token_secret,
         'consumer_secret': '',
-        }
-
-    tmpdir = tempfile.mkdtemp(prefix='maas-commission-')
-    scripts_to_run = {
+    }
+    scripts = {
         name: config
         for name, config in NODE_INFO_SCRIPTS.items()
         if config["run_on_controller"]
     }
-    total_scripts = len(scripts_to_run)
+
+    with tempfile.TemporaryDirectory(prefix='maas-commission-') as tmpdir:
+        failed_scripts = runscripts(scripts, url, creds, tmpdir=tmpdir)
+
+    if len(failed_scripts) == 0:
+        signal(url, creds, "OK", "Finished refreshing %s" % system_id)
+    else:
+        signal(url, creds, "FAILED", "Failed refreshing %s" % system_id)
+
+
+def runscripts(scripts, url, creds, tmpdir):
+    total_scripts = len(scripts)
     current_script = 1
     failed_scripts = []
-    for output_name, config in scripts_to_run.items():
+    for output_name, config in scripts.items():
         signal(
             url, creds, "WORKING", "Starting %s [%d/%d]" %
             (config['name'], current_script, total_scripts))
@@ -143,12 +151,4 @@
         if proc.returncode != 0:
             failed_scripts.append(config['name'])
         current_script += 1
-
-    shutil.rmtree(tmpdir)
-    fail_count = len(failed_scripts)
-    if fail_count == 0:
-        signal(
-            url, creds, "OK", "Finished refreshing %s" % system_id)
-    else:
-        signal(
-            url, creds, "FAILED", "Failed refreshing %s" % system_id)
+    return failed_scripts

=== modified file 'src/provisioningserver/refresh/tests/test_refresh.py'
--- src/provisioningserver/refresh/tests/test_refresh.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/refresh/tests/test_refresh.py	2016-05-31 17:21:35 +0000
@@ -7,9 +7,12 @@
 
 from collections import OrderedDict
 import os
+from pathlib import Path
 import random
 import re
+import tempfile
 from textwrap import dedent
+from unittest.mock import sentinel
 
 from maastesting.factory import factory
 from maastesting.matchers import (
@@ -18,6 +21,11 @@
 )
 from maastesting.testcase import MAASTestCase
 from provisioningserver import refresh
+from testtools.matchers import (
+    Contains,
+    DirExists,
+    Not,
+)
 
 
 class TestHelpers(MAASTestCase):
@@ -266,3 +274,37 @@
             'FAILED',
             "Failed refreshing %s" % system_id],
             signal.call_args_list[2][0])
+
+    def test_refresh_clears_up_temporary_directory(self):
+
+        ScriptsBroken = factory.make_exception_type()
+
+        def find_temporary_directories():
+            with tempfile.TemporaryDirectory() as tmpdir:
+                tmpdir = Path(tmpdir).absolute()
+                return {
+                    entry.path for entry in tmpdir.parent.iterdir()
+                    if entry.is_dir() and entry != tmpdir
+                }
+
+        tmpdirs_during = set()
+        tmpdir_during = None
+
+        def runscripts(*args, tmpdir):
+            self.assertThat(tmpdir, DirExists())
+            nonlocal tmpdirs_during, tmpdir_during
+            tmpdirs_during |= find_temporary_directories()
+            tmpdir_during = tmpdir
+            raise ScriptsBroken("Foom")
+
+        self.patch(refresh, "runscripts", runscripts)
+
+        tmpdirs_before = find_temporary_directories()
+        self.assertRaises(
+            ScriptsBroken, refresh.refresh, sentinel.system_id,
+            sentinel.consumer_key, sentinel.token_key, sentinel.token_secret)
+        tmpdirs_after = find_temporary_directories()
+
+        self.assertThat(tmpdirs_before, Not(Contains(tmpdir_during)))
+        self.assertThat(tmpdirs_during, Contains(tmpdir_during))
+        self.assertThat(tmpdirs_after, Not(Contains(tmpdir_during)))