Merge "Add caching and cleanup of chart tarballs"

armada/handlers/armada.py

@@ -103,7 +103,7 @@ class Armada(object):
             raise
         self.manifest = Manifest(
             self.documents, target_manifest=target_manifest).get_manifest()
-        self.cloned_dirs = set()
+        self.chart_cache = {}
         self.chart_deploy = ChartDeploy(
             disable_update_pre, disable_update_post, self.dry_run,
             k8s_wait_attempts, k8s_wait_attempt_sleep, timeout, self.tiller)
@@ -119,16 +119,12 @@ class Armada(object):
             raise tiller_exceptions.TillerServicesUnavailableException()
 
         # Clone the chart sources
-        repos = {}
         manifest_data = self.manifest.get(const.KEYWORD_ARMADA, {})
         for group in manifest_data.get(const.KEYWORD_GROUPS, []):
             for ch in group.get(const.KEYWORD_CHARTS, []):
-                self.tag_cloned_repo(ch, repos)
+                self.get_chart(ch)
 
-                for dep in ch.get('chart', {}).get('dependencies', []):
+    def get_chart(self, ch):
-                    self.tag_cloned_repo(dep, repos)
-
-    def tag_cloned_repo(self, ch, repos):
         chart = ch.get('chart', {})
         chart_source = chart.get('source', {})
         location = chart_source.get('location')
@@ -138,25 +134,30 @@ class Armada(object):
         if ct_type == 'local':
             chart['source_dir'] = (location, subpath)
         elif ct_type == 'tar':
-            LOG.info('Downloading tarball from: %s', location)
+            source_key = (ct_type, location)
 
-            if not CONF.certs:
+            if source_key not in self.chart_cache:
-                LOG.warn('Disabling server validation certs to extract charts')
+                LOG.info('Downloading tarball from: %s', location)
-                tarball_dir = source.get_tarball(location, verify=False)
-            else:
-                tarball_dir = source.get_tarball(location, verify=CONF.cert)
 
-            chart['source_dir'] = (tarball_dir, subpath)
+                if not CONF.certs:
+                    LOG.warn(
+                        'Disabling server validation certs to extract charts')
+                    tarball_dir = source.get_tarball(location, verify=False)
+                else:
+                    tarball_dir = source.get_tarball(
+                        location, verify=CONF.cert)
+                self.chart_cache[source_key] = tarball_dir
+            chart['source_dir'] = (self.chart_cache.get(source_key), subpath)
         elif ct_type == 'git':
             reference = chart_source.get('reference', 'master')
-            repo_branch = (location, reference)
+            source_key = (ct_type, location, reference)
 
-            if repo_branch not in repos:
+            if source_key not in self.chart_cache:
                 auth_method = chart_source.get('auth_method')
                 proxy_server = chart_source.get('proxy_server')
 
                 logstr = 'Cloning repo: {} from branch: {}'.format(
-                    *repo_branch)
+                    location, reference)
                 if proxy_server:
                     logstr += ' proxy: {}'.format(proxy_server)
                 if auth_method:
@@ -164,19 +165,20 @@ class Armada(object):
                 LOG.info(logstr)
 
                 repo_dir = source.git_clone(
-                    *repo_branch,
+                    location,
+                    reference,
                     proxy_server=proxy_server,
                     auth_method=auth_method)
-                self.cloned_dirs.add(repo_dir)
 
-                repos[repo_branch] = repo_dir
+                self.chart_cache[source_key] = repo_dir
-                chart['source_dir'] = (repo_dir, subpath)
+            chart['source_dir'] = (self.chart_cache.get(source_key), subpath)
-            else:
-                chart['source_dir'] = (repos.get(repo_branch), subpath)
         else:
             chart_name = chart.get('chart_name')
             raise source_exceptions.ChartSourceException(ct_type, chart_name)
 
+        for dep in ch.get('chart', {}).get('dependencies', []):
+            self.get_chart(dep)
+
     def sync(self):
         '''
         Synchronize Helm with the Armada Config(s)
@@ -293,9 +295,9 @@ class Armada(object):
         LOG.info("Performing post-flight operations.")
 
         # Delete temp dirs used for deployment
-        for cloned_dir in self.cloned_dirs:
+        for chart_dir in self.chart_cache.values():
-            LOG.debug('Removing cloned temp directory: %s', cloned_dir)
+            LOG.debug('Removing temp chart directory: %s', chart_dir)
-            source.source_cleanup(cloned_dir)
+            source.source_cleanup(chart_dir)
 
     def _chart_cleanup(self, prefix, charts, msg):
         LOG.info('Processing chart cleanup to remove unspecified releases.')

armada/tests/unit/handlers/test_armada.py

@@ -152,6 +152,7 @@ CHART_SOURCES = [('git://github.com/dummy/armada', 'chart_1'),
                  ('/tmp/dummy/armada', 'chart_4')]
 
 
+# TODO(seaneagan): Add unit tests with dependencies, including transitive.
 class ArmadaHandlerTestCase(base.ArmadaTestCase):
 
     def _test_pre_flight_ops(self, armada_obj):

armada/tests/unit/utils/test_source.py

@@ -15,7 +15,6 @@
 import os
 import shutil
 
-import fixtures
 import mock
 import testtools
 
@@ -156,23 +155,6 @@ class GitTestCase(base.ArmadaTestCase):
         source.source_cleanup(git_path)
         mock_log.warning.assert_not_called()
 
-    @test_utils.attr(type=['negative'])
-    @mock.patch.object(source, 'LOG')
-    @mock.patch('armada.utils.source.shutil')
-    def test_source_cleanup_fake_git_path(self, mock_shutil, mock_log):
-        # Verify that passing in a fake path does nothing but log a warning.
-        # Don't want anyone using the function to delete random directories.
-        fake_path = self.useFixture(fixtures.TempDir()).path
-        self.addCleanup(shutil.rmtree, fake_path)
-        source.source_cleanup(fake_path)
-
-        mock_shutil.rmtree.assert_not_called()
-        self.assertTrue(mock_log.warning.called)
-        actual_call = mock_log.warning.mock_calls[0][1][:2]
-        self.assertEqual(
-            ('%s is not a valid git repository. Details: %s', fake_path),
-            actual_call)
-
     @test_utils.attr(type=['negative'])
     @mock.patch.object(source, 'LOG')
     @mock.patch('armada.utils.source.shutil')
@@ -187,9 +169,8 @@ class GitTestCase(base.ArmadaTestCase):
         mock_shutil.rmtree.assert_not_called()
         self.assertTrue(mock_log.warning.called)
         actual_call = mock_log.warning.mock_calls[0][1]
-        self.assertEqual(
+        self.assertEqual(('Could not find the chart path %s to delete.', path),
-            ('Could not delete the path %s. Is it a git repository?', path),
+                         actual_call)
-            actual_call)
 
     @testtools.skipUnless(base.is_connected(),
                           'git clone requires network connectivity.')

armada/utils/source.py

@@ -153,27 +153,19 @@ def extract_tarball(tarball_path):
     return temp_dir
 
 
-def source_cleanup(git_path):
+def source_cleanup(chart_path):
-    '''Clean up the git repository that was created by ``git_clone`` above.
+    '''Clean up the chart path that was created above.
 
-    Removes the ``git_path`` repository and all associated files if they
+    Removes the ``chart_path`` and all associated files if they
     exist.
 
-    :param str git_path: The git repository to delete.
+    :param str chart_path: The chart path to delete.
     '''
-    if os.path.exists(git_path):
+    if os.path.exists(chart_path):
         try:
-            # Internally validates whether the path points to an actual repo.
+            shutil.rmtree(chart_path)
-            Repo(git_path)
+        except OSError as e:
-        except git_exc.InvalidGitRepositoryError as e:
+            LOG.warning('Could not delete the path %s. Details: %s',
-            LOG.warning('%s is not a valid git repository. Details: %s',
+                        chart_path, e)
-                        git_path, e)
-        else:
-            try:
-                shutil.rmtree(git_path)
-            except OSError as e:
-                LOG.warning('Could not delete the path %s. Details: %s',
-                            git_path, e)
     else:
-        LOG.warning('Could not delete the path %s. Is it a git repository?',
+        LOG.warning('Could not find the chart path %s to delete.', chart_path)
-                    git_path)