From 52bf21989fb9fcd93810a70fdd07671fd99f6221 Mon Sep 17 00:00:00 2001 From: Marshall Margenau Date: Thu, 14 Jun 2018 10:31:52 -0500 Subject: [PATCH] Fix release name bug The release name was being treated as multiple different values to mean the same thing, when paired with the 'release_prefix'. This commit addresses the bug, changing all instances to use the 'release' value instead of 'chart_name' or others. Note: This is an impacting change, in the sense that it will cause more reliable behavior in Armada's Apply processing which could have actual impact while upgrading components installed with a previous version of Armada. Previuosly undeleted FAILED releases may now be deleted, and armada test and delete actions may now run as expected where they didn't run before. Change-Id: I9893e506274e974cdc8826b1812becf9b89a0ab6 --- armada/api/controller/test.py | 6 +-- armada/cli/apply.py | 2 +- armada/cli/delete.py | 6 +-- armada/cli/test.py | 6 +-- armada/handlers/armada.py | 45 ++++++++++++------- armada/handlers/tiller.py | 29 ++---------- armada/tests/unit/handlers/test_armada.py | 8 ++-- armada/tests/unit/utils/test_release.py | 12 ++--- armada/utils/release.py | 6 +-- doc/source/commands/apply.rst | 2 +- .../operations/guide-build-armada-yaml.rst | 10 ++--- 11 files changed, 62 insertions(+), 70 deletions(-) diff --git a/armada/api/controller/test.py b/armada/api/controller/test.py index 3664c784..10a43eef 100644 --- a/armada/api/controller/test.py +++ b/armada/api/controller/test.py @@ -23,7 +23,7 @@ from armada.common import policy from armada import const from armada.handlers.tiller import Tiller from armada.handlers.manifest import Manifest -from armada.utils.release import release_prefix +from armada.utils.release import release_prefixer from armada.utils import validate CONF = cfg.CONF @@ -169,8 +169,8 @@ class TestReleasesManifestController(api.BaseResource): for group in armada_obj.get(const.KEYWORD_ARMADA).get( const.KEYWORD_GROUPS): for ch in group.get(const.KEYWORD_CHARTS): - release_name = release_prefix( - prefix, ch.get('chart').get('chart_name')) + release_name = release_prefixer( + prefix, ch.get('chart').get('release')) if release_name in known_releases: self.logger.info('RUNNING: %s tests', release_name) diff --git a/armada/cli/apply.py b/armada/cli/apply.py index 4aa3bef1..1cb6d4a5 100644 --- a/armada/cli/apply.py +++ b/armada/cli/apply.py @@ -51,7 +51,7 @@ To override a specific value in a Manifest, run: \b $ armada apply examples/simple.yaml \ ---set manifest:simple-armada:release_name="wordpress" +--set manifest:simple-armada:release="wordpress" Or to override several values in a Manifest, reference a values.yaml-formatted file: diff --git a/armada/cli/delete.py b/armada/cli/delete.py index fbe33595..6d4c59b4 100644 --- a/armada/cli/delete.py +++ b/armada/cli/delete.py @@ -21,7 +21,7 @@ from armada.cli import CliAction from armada import const from armada.handlers.manifest import Manifest from armada.handlers.tiller import Tiller -from armada.utils.release import release_prefix +from armada.utils.release import release_prefixer CONF = cfg.CONF @@ -130,8 +130,8 @@ class DeleteChartManifest(CliAction): for group in armada_obj.get(const.KEYWORD_ARMADA).get( const.KEYWORD_GROUPS): for ch in group.get(const.KEYWORD_CHARTS): - release_name = release_prefix( - prefix, ch.get('chart').get('chart_name')) + release_name = release_prefixer( + prefix, ch.get('chart').get('release')) if release_name in known_release_names: target_releases.append(release_name) except yaml.YAMLError as e: diff --git a/armada/cli/test.py b/armada/cli/test.py index 07d0ff86..a4074c53 100644 --- a/armada/cli/test.py +++ b/armada/cli/test.py @@ -21,7 +21,7 @@ from armada.cli import CliAction from armada import const from armada.handlers.manifest import Manifest from armada.handlers.tiller import Tiller -from armada.utils.release import release_prefix +from armada.utils.release import release_prefixer CONF = cfg.CONF @@ -149,8 +149,8 @@ class TestChartManifest(CliAction): for group in armada_obj.get(const.KEYWORD_ARMADA).get( const.KEYWORD_GROUPS): for ch in group.get(const.KEYWORD_CHARTS): - release_name = release_prefix( - prefix, ch.get('chart').get('chart_name')) + release_name = release_prefixer( + prefix, ch.get('chart').get('release')) if release_name in known_release_names: self.logger.info('RUNNING: %s tests', release_name) diff --git a/armada/handlers/armada.py b/armada/handlers/armada.py index 15a79a49..4fee0fdc 100644 --- a/armada/handlers/armada.py +++ b/armada/handlers/armada.py @@ -28,7 +28,7 @@ from armada.handlers.chartbuilder import ChartBuilder from armada.handlers.manifest import Manifest from armada.handlers.override import Override from armada.handlers.tiller import Tiller -from armada.utils.release import release_prefix +from armada.utils.release import release_prefixer from armada.utils import source from armada.utils import validate @@ -107,12 +107,12 @@ class Armada(object): self.documents, target_manifest=target_manifest).get_manifest() - def find_release_chart(self, known_releases, name): + def find_release_chart(self, known_releases, release_name): ''' Find a release given a list of known_releases and a release name ''' - for chart_name, _, chart, values, _ in known_releases: - if chart_name == name: + for release, _, chart, values, _ in known_releases: + if release == release_name: return chart, values def pre_flight_ops(self): @@ -150,17 +150,12 @@ class Armada(object): for release in failed_releases: for group in manifest_data.get(const.KEYWORD_GROUPS, []): for ch in group.get(const.KEYWORD_CHARTS, []): - ch_release_name = release_prefix( - prefix, ch.get('chart', {}).get('chart_name')) + ch_release_name = release_prefixer( + prefix, ch.get('chart', {}).get('release')) if release[0] == ch_release_name: - if self.dry_run: - LOG.info('Skipping purge during `dry-run`, would ' - 'have purged failed release %s before ' - 'deployment,', release[0]) - else: - LOG.info('Purging failed release %s ' - 'before deployment', release[0]) - self.tiller.uninstall_release(release[0]) + LOG.info('Purging failed release %s ' + 'before deployment.', release[0]) + self.tiller.uninstall_release(release[0]) # Clone the chart sources # @@ -282,7 +277,7 @@ class Armada(object): wait_timeout = self.timeout wait_labels = {} - release_name = release_prefix(prefix, release) + release_name = release_prefixer(prefix, release) # Retrieve appropriate timeout value if wait_timeout <= 0: @@ -473,7 +468,7 @@ class Armada(object): self.post_flight_ops() if self.enable_chart_cleanup: - self.tiller.chart_cleanup( + self._chart_cleanup( prefix, self.manifest[const.KEYWORD_ARMADA][const.KEYWORD_GROUPS]) @@ -572,3 +567,21 @@ class Armada(object): result = (len(chart_diff) > 0) or (len(values_diff) > 0) return result + + def _chart_cleanup(self, prefix, charts): + LOG.info('Processing chart cleanup to remove unspecified releases.') + + valid_releases = [] + for gchart in charts: + for chart in gchart.get(const.KEYWORD_CHARTS, []): + valid_releases.append(release_prefixer( + prefix, chart.get('chart', {}).get('release'))) + + actual_releases = [x.name for x in self.tiller.list_releases()] + release_diff = list(set(actual_releases) - set(valid_releases)) + + for release in release_diff: + if release.startswith(prefix): + LOG.info('Purging release %s as part of chart cleanup.', + release) + self.tiller.uninstall_release(release) diff --git a/armada/handlers/tiller.py b/armada/handlers/tiller.py index 37fb9832..4ff094b5 100644 --- a/armada/handlers/tiller.py +++ b/armada/handlers/tiller.py @@ -32,7 +32,6 @@ from oslo_log import log as logging from armada import const from armada.exceptions import tiller_exceptions as ex from armada.handlers.k8s import K8s -from armada.utils.release import release_prefix from armada.utils.release import label_selectors TILLER_VERSION = b'2.7.2' @@ -309,8 +308,10 @@ class Tiller(object): for latest_release in self.list_releases(): try: release = ( - latest_release.name, latest_release.version, - latest_release.chart, latest_release.config.raw, + latest_release.name, + latest_release.version, + latest_release.chart, + latest_release.config.raw, latest_release.info.status.Code.Name( latest_release.info.status.code)) charts.append(release) @@ -568,28 +569,6 @@ class Tiller(object): status = self.get_release_status(release) raise ex.ReleaseException(release, status, 'Delete') - def chart_cleanup(self, prefix, charts): - ''' - :params charts - list of yaml charts - :params known_release - list of releases in tiller - - :result - will remove any chart that is not present in yaml - ''' - - valid_charts = [] - for gchart in charts: - for chart in gchart.get('chart_group'): - valid_charts.append(release_prefix( - prefix, chart.get('chart').get('name'))) - - actual_charts = [x.name for x in self.list_releases()] - chart_diff = list(set(actual_charts) - set(valid_charts)) - - for chart in chart_diff: - if chart.startswith(prefix): - LOG.debug("Release: %s will be removed", chart) - self.uninstall_release(chart) - def delete_resources(self, release_name, resource_name, resource_type, resource_labels, namespace, wait=False, timeout=const.DEFAULT_TILLER_TIMEOUT): diff --git a/armada/tests/unit/handlers/test_armada.py b/armada/tests/unit/handlers/test_armada.py index e2b9962b..0c6e0d53 100644 --- a/armada/tests/unit/handlers/test_armada.py +++ b/armada/tests/unit/handlers/test_armada.py @@ -18,7 +18,7 @@ import yaml from armada import const from armada.handlers import armada from armada.tests.unit import base -from armada.utils.release import release_prefix +from armada.utils.release import release_prefixer TEST_YAML = """ @@ -270,14 +270,14 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): for c in charts: chart = c['chart'] - chart_name = chart['chart_name'] + release = chart['release'] prefix = armada_obj.manifest['armada']['release_prefix'] - release = release_prefix(prefix, chart_name) + release_name = release_prefixer(prefix, release) # Simplified check because the actual code uses logical-or's # multiple conditions, so this is enough. this_chart_should_wait = chart['wait']['timeout'] > 0 - if release not in [x[0] for x in known_releases]: + if release_name not in [x[0] for x in known_releases]: expected_install_release_calls.append( mock.call( mock_chartbuilder().get_helm_chart(), diff --git a/armada/tests/unit/utils/test_release.py b/armada/tests/unit/utils/test_release.py index 805348f5..8e67c329 100644 --- a/armada/tests/unit/utils/test_release.py +++ b/armada/tests/unit/utils/test_release.py @@ -21,18 +21,18 @@ class ReleaseTestCase(unittest.TestCase): def test_release_prefix_pass(self): expected = 'armada-test' - prefix, chart = ('armada', 'test') + prefix, release = ('armada', 'test') - assert rel.release_prefix(prefix, chart) == expected + assert rel.release_prefixer(prefix, release) == expected def test_release_prefix_int_string(self): expected = 'armada-4' - prefix, chart = ('armada', 4) + prefix, release = ('armada', 4) - assert rel.release_prefix(prefix, chart) == expected + assert rel.release_prefixer(prefix, release) == expected def test_release_prefix_int_int(self): expected = '4-4' - prefix, chart = (4, 4) + prefix, release = (4, 4) - assert rel.release_prefix(prefix, chart) == expected + assert rel.release_prefixer(prefix, release) == expected diff --git a/armada/utils/release.py b/armada/utils/release.py index 084292d1..55fb40df 100644 --- a/armada/utils/release.py +++ b/armada/utils/release.py @@ -13,11 +13,11 @@ # limitations under the License. -def release_prefix(prefix, chart): +def release_prefixer(prefix, release): ''' - how to attach prefix to chart + attach prefix to release name ''' - return "{}-{}".format(prefix, chart) + return "{}-{}".format(prefix, release) def label_selectors(labels): diff --git a/doc/source/commands/apply.rst b/doc/source/commands/apply.rst index 16ca8ac3..55ec372b 100644 --- a/doc/source/commands/apply.rst +++ b/doc/source/commands/apply.rst @@ -24,7 +24,7 @@ Commands To override a specific value in a Manifest, run: - $ armada apply examples/simple.yaml --set manifest:simple-armada:release_name="wordpress" + $ armada apply examples/simple.yaml --set manifest:simple-armada:release="wordpress" Or to override several values in a Manifest, reference a values.yaml- formatted file: diff --git a/doc/source/operations/guide-build-armada-yaml.rst b/doc/source/operations/guide-build-armada-yaml.rst index 6102c9d6..fab90291 100644 --- a/doc/source/operations/guide-build-armada-yaml.rst +++ b/doc/source/operations/guide-build-armada-yaml.rst @@ -89,7 +89,7 @@ Chart +=================+==========+=======================================================================================+ | chart\_name | string | name for the chart | +-----------------+----------+---------------------------------------------------------------------------------------+ -| release\_name | string | name of the release | +| release | string | name of the release (Armada will prepend with ``release-prefix`` during processing) | +-----------------+----------+---------------------------------------------------------------------------------------+ | namespace | string | namespace of your chart | +-----------------+----------+---------------------------------------------------------------------------------------+ @@ -172,7 +172,7 @@ Chart Example name: blog-1 data: chart_name: blog-1 - release_name: blog-1 + release: blog-1 namespace: default wait: timeout: 100 @@ -232,7 +232,7 @@ Source Example name: blog-1 data: chart_name: blog-1 - release_name: blog-1 + release: blog-1 namespace: default wait: timeout: 100 @@ -258,7 +258,7 @@ Source Example name: blog-1 data: chart_name: blog-1 - release_name: blog-1 + release: blog-1 namespace: default wait: timeout: 100 @@ -282,7 +282,7 @@ Source Example name: blog-1 data: chart_name: blog-1 - release_name: blog-1 + release: blog-1 namespace: default wait: timeout: 100