diff --git a/armada/exceptions/armada_exceptions.py b/armada/exceptions/armada_exceptions.py index 9343fd0c..074c98a7 100644 --- a/armada/exceptions/armada_exceptions.py +++ b/armada/exceptions/armada_exceptions.py @@ -40,3 +40,29 @@ class ProtectedReleaseException(ArmadaException): 'Armada encountered protected release %s in FAILED status' % reason) super(ProtectedReleaseException, self).__init__(self._message) + + +class InvalidValuesYamlException(ArmadaException): + ''' + Exception that occurs when Armada encounters invalid values.yaml content in + a helm chart. + ''' + + def __init__(self, chart_description): + self._message = ( + 'Armada encountered invalid values.yaml in helm chart: %s' % + chart_description) + super(InvalidValuesYamlException, self).__init__(self._message) + + +class InvalidOverrideValuesYamlException(ArmadaException): + ''' + Exception that occurs when Armada encounters invalid override yaml in + helm chart. + ''' + + def __init__(self, chart_description): + self._message = ( + 'Armada encountered invalid values.yaml in helm chart: %s' % + chart_description) + super(InvalidValuesYamlException, self).__init__(self._message) diff --git a/armada/handlers/armada.py b/armada/handlers/armada.py index 2c8b1911..ae97c596 100644 --- a/armada/handlers/armada.py +++ b/armada/handlers/armada.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -import difflib +from deepdiff import DeepDiff import functools import time import yaml @@ -377,7 +377,7 @@ class Armada(object): 'cleanup', False) chartbuilder = ChartBuilder(chart) - protoc_chart = chartbuilder.get_helm_chart() + new_chart = chartbuilder.get_helm_chart() # Begin Chart timeout deadline deadline = time.time() + wait_timeout @@ -392,7 +392,7 @@ class Armada(object): release_name, namespace) # extract the installed chart and installed values from the # latest release so we can compare to the intended state - apply_chart, apply_values = self.find_release_chart( + old_chart, old_values_string = self.find_release_chart( deployed_releases, release_name) upgrade = chart.get('upgrade', {}) @@ -411,20 +411,26 @@ class Armada(object): if not self.disable_update_post and upgrade_post: post_actions = upgrade_post - # Show delta for both the chart templates and the chart - # values - # TODO(alanmeadows) account for .files differences - # once we support those - LOG.info('Checking upgrade chart diffs.') - upgrade_diff = self.show_diff(chart, apply_chart, - apply_values, - chartbuilder.dump(), values, - msg) + try: + old_values = yaml.safe_load(old_values_string) + except yaml.YAMLError: + chart_desc = '{} (previously deployed)'.format( + old_chart.metadata.name) + raise armada_exceptions.\ + InvalidOverrideValuesYamlException(chart_desc) - if not upgrade_diff: - LOG.info("There are no updates found in this chart") + LOG.info('Checking for updates to chart release inputs.') + diff = self.get_diff(old_chart, old_values, new_chart, + values) + + if not diff: + LOG.info("Found no updates to chart release inputs") continue + LOG.info("Found updates to chart release inputs") + LOG.debug("%s", diff) + msg['diff'].append({chart['release']: str(diff)}) + # TODO(MarshM): Add tiller dry-run before upgrade and # consider deadline impacts @@ -433,7 +439,7 @@ class Armada(object): LOG.info('Beginning Upgrade, wait=%s, timeout=%ss', this_chart_should_wait, timer) tiller_result = self.tiller.update_release( - protoc_chart, + new_chart, release_name, namespace, pre_actions=pre_actions, @@ -465,7 +471,7 @@ class Armada(object): LOG.info('Beginning Install, wait=%s, timeout=%ss', this_chart_should_wait, timer) tiller_result = self.tiller.install_release( - protoc_chart, + new_chart, release_name, namespace, values=yaml.safe_dump(values), @@ -591,49 +597,65 @@ class Armada(object): LOG.info("Test failed for release: %s", release_name) raise tiller_exceptions.TestFailedException(release_name) - def show_diff(self, chart, installed_chart, installed_values, target_chart, - target_values, msg): - '''Produce a unified diff of the installed chart vs our intention''' + def get_diff(self, old_chart, old_values, new_chart, new_values): + ''' + Get the diff between old and new chart release inputs to determine + whether an upgrade is needed. - # TODO(MarshM) This gives decent output comparing values. Would be - # nice to clean it up further. Are \\n or \n\n ever valid diffs? - # Can these be cleanly converted to dicts, for easier compare? - def _sanitize_diff_str(str): - return str.replace('\\n', '\n').replace('\n\n', '\n').split('\n') + Release inputs which are relevant are the override values given, and + the chart content including: - source = _sanitize_diff_str(str(installed_chart.SerializeToString())) - target = _sanitize_diff_str(str(target_chart)) - chart_diff = list(difflib.unified_diff(source, target, n=0)) + * default values (values.yaml), + * templates and their content + * files and their content + * the above for each chart on which the chart depends transitively. - chart_release = chart.get('release', None) + This excludes Chart.yaml content as that is rarely used by the chart + via ``{{ .Chart }}``, and even when it is does not usually necessitate + an upgrade. - if len(chart_diff) > 0: - LOG.info("Found diff in Chart (%s)", chart_release) - diff_msg = [] - for line in chart_diff: - diff_msg.append(line) - msg['diff'].append({'chart': diff_msg}) + :param old_chart: The deployed chart. + :type old_chart: Chart + :param old_values: The deployed chart override values. + :type old_values: dict + :param new_chart: The chart to deploy. + :type new_chart: Chart + :param new_values: The chart override values to deploy. + :type new_values: dict + :return: Mapping of difference types to sets of those differences. + :rtype: dict + ''' - pretty_diff = '\n'.join(diff_msg) - LOG.debug(pretty_diff) + def make_release_input(chart, values, desc): + # TODO(seaneagan): Should we include `chart.metadata` (Chart.yaml)? + try: + default_values = yaml.safe_load(chart.values.raw) + except yaml.YAMLError: + chart_desc = '{} ({})'.format(chart.metadata.name, desc) + raise armada_exceptions.InvalidValuesYamlException(chart_desc) + files = {f.type_url: f.value for f in chart.files} + templates = {t.name: t.data for t in chart.templates} + dependencies = { + d.metadata.name: make_release_input(d) + for d in chart.dependencies + } - source = _sanitize_diff_str(installed_values) - target = _sanitize_diff_str(yaml.safe_dump(target_values)) - values_diff = list(difflib.unified_diff(source, target, n=0)) + return { + 'chart': { + 'values': default_values, + 'files': files, + 'templates': templates, + 'dependencies': dependencies + }, + 'values': values + } - if len(values_diff) > 0: - LOG.info("Found diff in values (%s)", chart_release) - diff_msg = [] - for line in values_diff: - diff_msg.append(line) - msg['diff'].append({'values': diff_msg}) + old_input = make_release_input(old_chart, old_values, + 'previously deployed') + new_input = make_release_input(new_chart, new_values, + 'currently being deployed') - pretty_diff = '\n'.join(diff_msg) - LOG.debug(pretty_diff) - - result = (len(chart_diff) > 0) or (len(values_diff) > 0) - - return result + return DeepDiff(old_input, new_input, view='tree') 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 c9f6b359..341cd7e1 100644 --- a/armada/tests/unit/handlers/test_armada.py +++ b/armada/tests/unit/handlers/test_armada.py @@ -317,7 +317,8 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): def _test_sync(self, known_releases, test_success=True, - test_failure_to_run=False): + test_failure_to_run=False, + diff={'some_key': {'some diff'}}): """Test install functionality from the sync() method.""" @mock.patch.object(armada.Armada, 'post_flight_ops') @@ -330,7 +331,7 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): # Instantiate Armada object. yaml_documents = list(yaml.safe_load_all(TEST_YAML)) armada_obj = armada.Armada(yaml_documents) - armada_obj.show_diff = mock.Mock() + armada_obj.get_diff = mock.Mock() chart_group = armada_obj.manifest['armada']['chart_groups'][0] charts = chart_group['chart_group'] @@ -355,6 +356,9 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): mock_chartbuilder.get_source_path.return_value = None mock_chartbuilder.get_helm_chart.return_value = None + # Simulate chart diff, upgrade should only happen if non-empty. + armada_obj.get_diff.return_value = diff + armada_obj.sync() expected_install_release_calls = [] @@ -371,6 +375,7 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): # multiple conditions, so this is enough. this_chart_should_wait = chart['wait']['timeout'] > 0 + expected_apply = True if release_name not in [x[0] for x in known_releases]: expected_install_release_calls.append( mock.call( @@ -415,26 +420,31 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): break if status == const.STATUS_DEPLOYED: - upgrade = chart.get('upgrade', {}) - disable_hooks = upgrade.get('no_hooks', False) - force = upgrade.get('force', False) - recreate_pods = upgrade.get('recreate_pods', False) + if not diff: + expected_apply = False + else: + upgrade = chart.get('upgrade', {}) + disable_hooks = upgrade.get('no_hooks', False) + force = upgrade.get('force', False) + recreate_pods = upgrade.get( + 'recreate_pods', False) - expected_update_release_calls.append( - mock.call( - mock_chartbuilder().get_helm_chart(), - "{}-{}".format( - armada_obj.manifest['armada'] - ['release_prefix'], chart['release']), - chart['namespace'], - pre_actions={}, - post_actions={}, - disable_hooks=disable_hooks, - force=force, - recreate_pods=recreate_pods, - values=yaml.safe_dump(chart['values']), - wait=this_chart_should_wait, - timeout=chart['wait']['timeout'])) + expected_update_release_calls.append( + mock.call( + mock_chartbuilder().get_helm_chart(), + "{}-{}".format( + armada_obj.manifest['armada'][ + 'release_prefix'], + chart['release']), + chart['namespace'], + pre_actions={}, + post_actions={}, + disable_hooks=disable_hooks, + force=force, + recreate_pods=recreate_pods, + values=yaml.safe_dump(chart['values']), + wait=this_chart_should_wait, + timeout=chart['wait']['timeout'])) test_chart_override = chart.get('test') # Use old default value when not using newer `test` key @@ -448,7 +458,7 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): test_cleanup = test_chart_override.get('options', {}).get( 'cleanup', False) - if test_this_chart: + if test_this_chart and expected_apply: expected_test_release_for_success_calls.append( mock.call( m_tiller, @@ -505,23 +515,21 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): def test_armada_sync_with_one_deployed_release(self): c1 = 'armada-test_chart_1' - known_releases = [[ - c1, None, - self._get_chart_by_name(c1), None, const.STATUS_DEPLOYED - ]] + known_releases = [[c1, None, None, "{}", const.STATUS_DEPLOYED]] self._test_sync(known_releases) + def test_armada_sync_with_one_deployed_release_no_diff(self): + c1 = 'armada-test_chart_1' + + known_releases = [[c1, None, None, "{}", const.STATUS_DEPLOYED]] + self._test_sync(known_releases, diff=set()) + def test_armada_sync_with_both_deployed_releases(self): c1 = 'armada-test_chart_1' c2 = 'armada-test_chart_2' - known_releases = [[ - c1, None, - self._get_chart_by_name(c1), None, const.STATUS_DEPLOYED - ], [ - c2, None, - self._get_chart_by_name(c2), None, const.STATUS_DEPLOYED - ]] + known_releases = [[c1, None, None, "{}", const.STATUS_DEPLOYED], + [c2, None, None, "{}", const.STATUS_DEPLOYED]] self._test_sync(known_releases) def test_armada_sync_with_unprotected_releases(self): diff --git a/requirements.txt b/requirements.txt index fbcf0794..4056c400 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ +deepdiff==3.3.0 gitpython grpcio==1.10.0 grpcio-tools==1.10.0