airship/armada
Code Issues Proposed changes

Move to semantic diffing of charts

Browse Source 
We were seeing false positives when diffing charts to determine
whether an upgrade was necessary. Previously we were serializing the
charts and values and diffing those, but these serializations often
output things in different and non-deterministic order, hence the
false positives. This removes the ordering concerns by puttings things
in maps instead of lists, and comparing those semantically rather than
via serialization. This also improves the diff output to be easier to
read. It also stops caring about diffs in Chart.yaml.

Change-Id: I4c92c2e7c814178c374623ea52d717bdb9f72b11
This commit is contained in:
Sean Eagan 2018-08-16 17:06:02 -05:00
parent 019b3c142e
commit 5c5ddf8e8c
4 changed files with 141 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
armada/exceptions/armada_exceptions.py
View File

@ -40,3 +40,29 @@ class ProtectedReleaseException(ArmadaException):
'Armada encountered protected release %s in FAILED status' % 'Armada encountered protected release %s in FAILED status' %
reason) reason)
super(ProtectedReleaseException, self).__init__(self._message) 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)

124
armada/handlers/armada.py
View File

@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
import difflib from deepdiff import DeepDiff
import functools import functools
import time import time
import yaml import yaml
@ -377,7 +377,7 @@ class Armada(object):
'cleanup', False) 'cleanup', False)
chartbuilder = ChartBuilder(chart) chartbuilder = ChartBuilder(chart)
protoc_chart = chartbuilder.get_helm_chart() new_chart = chartbuilder.get_helm_chart()
# Begin Chart timeout deadline # Begin Chart timeout deadline
deadline = time.time() + wait_timeout deadline = time.time() + wait_timeout
@ -392,7 +392,7 @@ class Armada(object):
release_name, namespace) release_name, namespace)
# extract the installed chart and installed values from the # extract the installed chart and installed values from the
# latest release so we can compare to the intended state # 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) deployed_releases, release_name)
upgrade = chart.get('upgrade', {}) upgrade = chart.get('upgrade', {})
@ -411,20 +411,26 @@ class Armada(object):
if not self.disable_update_post and upgrade_post: if not self.disable_update_post and upgrade_post:
post_actions = upgrade_post post_actions = upgrade_post
# Show delta for both the chart templates and the chart try:
# values old_values = yaml.safe_load(old_values_string)
# TODO(alanmeadows) account for .files differences except yaml.YAMLError:
# once we support those chart_desc = '{} (previously deployed)'.format(
LOG.info('Checking upgrade chart diffs.') old_chart.metadata.name)
upgrade_diff = self.show_diff(chart, apply_chart, raise armada_exceptions.\
apply_values, InvalidOverrideValuesYamlException(chart_desc)
chartbuilder.dump(), values,
msg)
if not upgrade_diff: LOG.info('Checking for updates to chart release inputs.')
LOG.info("There are no updates found in this chart") diff = self.get_diff(old_chart, old_values, new_chart,
values)
if not diff:
LOG.info("Found no updates to chart release inputs")
continue 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 # TODO(MarshM): Add tiller dry-run before upgrade and
# consider deadline impacts # consider deadline impacts
@ -433,7 +439,7 @@ class Armada(object):
LOG.info('Beginning Upgrade, wait=%s, timeout=%ss', LOG.info('Beginning Upgrade, wait=%s, timeout=%ss',
this_chart_should_wait, timer) this_chart_should_wait, timer)
tiller_result = self.tiller.update_release( tiller_result = self.tiller.update_release(
protoc_chart, new_chart,
release_name, release_name,
namespace, namespace,
pre_actions=pre_actions, pre_actions=pre_actions,
@ -465,7 +471,7 @@ class Armada(object):
LOG.info('Beginning Install, wait=%s, timeout=%ss', LOG.info('Beginning Install, wait=%s, timeout=%ss',
this_chart_should_wait, timer) this_chart_should_wait, timer)
tiller_result = self.tiller.install_release( tiller_result = self.tiller.install_release(
protoc_chart, new_chart,
release_name, release_name,
namespace, namespace,
values=yaml.safe_dump(values), values=yaml.safe_dump(values),
@ -591,49 +597,65 @@ class Armada(object):
LOG.info("Test failed for release: %s", release_name) LOG.info("Test failed for release: %s", release_name)
raise tiller_exceptions.TestFailedException(release_name) raise tiller_exceptions.TestFailedException(release_name)
def show_diff(self, chart, installed_chart, installed_values, target_chart, def get_diff(self, old_chart, old_values, new_chart, new_values):
target_values, msg): '''
'''Produce a unified diff of the installed chart vs our intention''' 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 Release inputs which are relevant are the override values given, and
# nice to clean it up further. Are \\n or \n\n ever valid diffs? the chart content including:
# 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')
source = _sanitize_diff_str(str(installed_chart.SerializeToString())) * default values (values.yaml),
target = _sanitize_diff_str(str(target_chart)) * templates and their content
chart_diff = list(difflib.unified_diff(source, target, n=0)) * 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: :param old_chart: The deployed chart.
LOG.info("Found diff in Chart (%s)", chart_release) :type old_chart: Chart
diff_msg = [] :param old_values: The deployed chart override values.
for line in chart_diff: :type old_values: dict
diff_msg.append(line) :param new_chart: The chart to deploy.
msg['diff'].append({'chart': diff_msg}) :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) def make_release_input(chart, values, desc):
LOG.debug(pretty_diff) # 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) return {
target = _sanitize_diff_str(yaml.safe_dump(target_values)) 'chart': {
values_diff = list(difflib.unified_diff(source, target, n=0)) 'values': default_values,
'files': files,
'templates': templates,
'dependencies': dependencies
},
'values': values
}
if len(values_diff) > 0: old_input = make_release_input(old_chart, old_values,
LOG.info("Found diff in values (%s)", chart_release) 'previously deployed')
diff_msg = [] new_input = make_release_input(new_chart, new_values,
for line in values_diff: 'currently being deployed')
diff_msg.append(line)
msg['diff'].append({'values': diff_msg})
pretty_diff = '\n'.join(diff_msg) return DeepDiff(old_input, new_input, view='tree')
LOG.debug(pretty_diff)
result = (len(chart_diff) > 0) or (len(values_diff) > 0)
return result
def _chart_cleanup(self, prefix, charts, msg): def _chart_cleanup(self, prefix, charts, msg):
LOG.info('Processing chart cleanup to remove unspecified releases.') LOG.info('Processing chart cleanup to remove unspecified releases.')

74
armada/tests/unit/handlers/test_armada.py
View File

@ -317,7 +317,8 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
def _test_sync(self, def _test_sync(self,
known_releases, known_releases,
test_success=True, 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.""" """Test install functionality from the sync() method."""
@mock.patch.object(armada.Armada, 'post_flight_ops') @mock.patch.object(armada.Armada, 'post_flight_ops')
@ -330,7 +331,7 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
# Instantiate Armada object. # Instantiate Armada object.
yaml_documents = list(yaml.safe_load_all(TEST_YAML)) yaml_documents = list(yaml.safe_load_all(TEST_YAML))
armada_obj = armada.Armada(yaml_documents) 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] chart_group = armada_obj.manifest['armada']['chart_groups'][0]
charts = chart_group['chart_group'] charts = chart_group['chart_group']
@ -355,6 +356,9 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
mock_chartbuilder.get_source_path.return_value = None mock_chartbuilder.get_source_path.return_value = None
mock_chartbuilder.get_helm_chart.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() armada_obj.sync()
expected_install_release_calls = [] expected_install_release_calls = []
@ -371,6 +375,7 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
# multiple conditions, so this is enough. # multiple conditions, so this is enough.
this_chart_should_wait = chart['wait']['timeout'] > 0 this_chart_should_wait = chart['wait']['timeout'] > 0
expected_apply = True
if release_name 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( expected_install_release_calls.append(
mock.call( mock.call(
@ -415,26 +420,31 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
break break
if status == const.STATUS_DEPLOYED: if status == const.STATUS_DEPLOYED:
upgrade = chart.get('upgrade', {}) if not diff:
disable_hooks = upgrade.get('no_hooks', False) expected_apply = False
force = upgrade.get('force', False) else:
recreate_pods = upgrade.get('recreate_pods', False) 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( expected_update_release_calls.append(
mock.call( mock.call(
mock_chartbuilder().get_helm_chart(), mock_chartbuilder().get_helm_chart(),
"{}-{}".format( "{}-{}".format(
armada_obj.manifest['armada'] armada_obj.manifest['armada'][
['release_prefix'], chart['release']), 'release_prefix'],
chart['namespace'], chart['release']),
pre_actions={}, chart['namespace'],
post_actions={}, pre_actions={},
disable_hooks=disable_hooks, post_actions={},
force=force, disable_hooks=disable_hooks,
recreate_pods=recreate_pods, force=force,
values=yaml.safe_dump(chart['values']), recreate_pods=recreate_pods,
wait=this_chart_should_wait, values=yaml.safe_dump(chart['values']),
timeout=chart['wait']['timeout'])) wait=this_chart_should_wait,
timeout=chart['wait']['timeout']))
test_chart_override = chart.get('test') test_chart_override = chart.get('test')
# Use old default value when not using newer `test` key # 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( test_cleanup = test_chart_override.get('options', {}).get(
'cleanup', False) 'cleanup', False)
if test_this_chart: if test_this_chart and expected_apply:
expected_test_release_for_success_calls.append( expected_test_release_for_success_calls.append(
mock.call( mock.call(
m_tiller, m_tiller,
@ -505,23 +515,21 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
def test_armada_sync_with_one_deployed_release(self): def test_armada_sync_with_one_deployed_release(self):
c1 = 'armada-test_chart_1' c1 = 'armada-test_chart_1'
known_releases = [[ known_releases = [[c1, None, None, "{}", const.STATUS_DEPLOYED]]
c1, None,
self._get_chart_by_name(c1), None, const.STATUS_DEPLOYED
]]
self._test_sync(known_releases) 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): def test_armada_sync_with_both_deployed_releases(self):
c1 = 'armada-test_chart_1' c1 = 'armada-test_chart_1'
c2 = 'armada-test_chart_2' c2 = 'armada-test_chart_2'
known_releases = [[ known_releases = [[c1, None, None, "{}", const.STATUS_DEPLOYED],
c1, None, [c2, None, None, "{}", const.STATUS_DEPLOYED]]
self._get_chart_by_name(c1), None, const.STATUS_DEPLOYED
], [
c2, None,
self._get_chart_by_name(c2), None, const.STATUS_DEPLOYED
]]
self._test_sync(known_releases) self._test_sync(known_releases)
def test_armada_sync_with_unprotected_releases(self): def test_armada_sync_with_unprotected_releases(self):

1
requirements.txt
View File

@ -1,3 +1,4 @@
deepdiff==3.3.0
gitpython gitpython
grpcio==1.10.0 grpcio==1.10.0
grpcio-tools==1.10.0 grpcio-tools==1.10.0