Move to semantic diffing of charts
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
parent
019b3c142e
commit
5c5ddf8e8c
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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.')
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
deepdiff==3.3.0
|
||||
gitpython
|
||||
grpcio==1.10.0
|
||||
grpcio-tools==1.10.0
|
||||
|
|
|
|||
Loading…
Reference in New Issue