From cbb8ed33e13f2d9a2e3742b994949999c5379297 Mon Sep 17 00:00:00 2001 From: Sean Eagan Date: Thu, 25 Oct 2018 15:08:21 -0500 Subject: [PATCH] Add caching and cleanup of chart tarballs Caching and cleanup of git repository chart sources was previously implemented. This adds these features for tarball sources as well. This also implements transitive chart dependency sourcing. Previously only a single level of dependencies were being downloaded, which would lead to an error when multiple dependency levels exist. Change-Id: I988e473a6ea29331e036d26c3ec7269374e0188f --- armada/handlers/armada.py | 54 ++++++++++++----------- armada/tests/unit/handlers/test_armada.py | 1 + armada/tests/unit/utils/test_source.py | 23 +--------- armada/utils/source.py | 28 +++++------- 4 files changed, 41 insertions(+), 65 deletions(-) diff --git a/armada/handlers/armada.py b/armada/handlers/armada.py index fa6afcd2..a95401ea 100644 --- a/armada/handlers/armada.py +++ b/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', []): - self.tag_cloned_repo(dep, repos) - - def tag_cloned_repo(self, ch, repos): + def get_chart(self, ch): 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: - 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) + if source_key not in self.chart_cache: + LOG.info('Downloading tarball from: %s', location) - 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 - chart['source_dir'] = (repo_dir, subpath) - else: - chart['source_dir'] = (repos.get(repo_branch), subpath) + self.chart_cache[source_key] = repo_dir + chart['source_dir'] = (self.chart_cache.get(source_key), 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: - LOG.debug('Removing cloned temp directory: %s', cloned_dir) - source.source_cleanup(cloned_dir) + for chart_dir in self.chart_cache.values(): + LOG.debug('Removing temp chart directory: %s', chart_dir) + source.source_cleanup(chart_dir) def _chart_cleanup(self, prefix, charts, msg): LOG.info('Processing chart cleanup to remove unspecified releases.') diff --git a/armada/tests/unit/handlers/test_armada.py b/armada/tests/unit/handlers/test_armada.py index bfecb309..4dacd02a 100644 --- a/armada/tests/unit/handlers/test_armada.py +++ b/armada/tests/unit/handlers/test_armada.py @@ -151,6 +151,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): diff --git a/armada/tests/unit/utils/test_source.py b/armada/tests/unit/utils/test_source.py index 21fd6308..78682d79 100644 --- a/armada/tests/unit/utils/test_source.py +++ b/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( - ('Could not delete the path %s. Is it a git repository?', path), - actual_call) + self.assertEqual(('Could not find the chart path %s to delete.', path), + actual_call) @testtools.skipUnless(base.is_connected(), 'git clone requires network connectivity.') diff --git a/armada/utils/source.py b/armada/utils/source.py index 727884c6..510c3d38 100644 --- a/armada/utils/source.py +++ b/armada/utils/source.py @@ -153,27 +153,19 @@ def extract_tarball(tarball_path): return temp_dir -def source_cleanup(git_path): - '''Clean up the git repository that was created by ``git_clone`` above. +def source_cleanup(chart_path): + '''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. - Repo(git_path) - except git_exc.InvalidGitRepositoryError as e: - LOG.warning('%s is not a valid git repository. Details: %s', - 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) + shutil.rmtree(chart_path) + except OSError as e: + LOG.warning('Could not delete the path %s. Details: %s', + chart_path, e) else: - LOG.warning('Could not delete the path %s. Is it a git repository?', - git_path) + LOG.warning('Could not find the chart path %s to delete.', chart_path)