From d4252c89e990a2f930e05847b042738f61a2bba3 Mon Sep 17 00:00:00 2001 From: John R Barker Date: Wed, 28 Feb 2018 11:33:18 +0000 Subject: [PATCH] nso_config break cycles in dependency sorting (#36828) (#36838) False assumption that values can not have cyclic dependencies. Fix by removing dependency on self and look for cycles, if found remove dependency to get a partial sort done. (cherry picked from commit 042c1115638402f7648ce919c66fabb3489afc27) --- lib/ansible/module_utils/network/nso/nso.py | 41 ++++++++++++++--- .../module_utils/network/nso/test_nso.py | 45 +++++++++++++++++++ 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/lib/ansible/module_utils/network/nso/nso.py b/lib/ansible/module_utils/network/nso/nso.py index 0bb57860d6..a9c89ca01c 100644 --- a/lib/ansible/module_utils/network/nso/nso.py +++ b/lib/ansible/module_utils/network/nso/nso.py @@ -285,6 +285,10 @@ class ValueBuilder(object): self.value = value self.deps = deps + # nodes can depend on themselves + if self.tag_path in self.deps: + self.deps.remove(self.tag_path) + def __lt__(self, rhs): l_len = len(self.path.split('/')) r_len = len(rhs.path.split('/')) @@ -348,12 +352,13 @@ class ValueBuilder(object): @property def values(self): if self._values_dirty: - self._values = self._sort_values(sorted(self._values)) + self._values = ValueBuilder.sort_values(self._values) self._values_dirty = False return self._values - def _sort_values(self, values): + @staticmethod + def sort_values(values): class N(object): def __init__(self, v): self.tmp_mark = False @@ -361,7 +366,32 @@ class ValueBuilder(object): self.v = v sorted_values = [] - nodes = [N(v) for v in values] + nodes = [N(v) for v in sorted(values)] + + def get_node(tag_path): + return next((m for m in nodes + if m.v.tag_path == tag_path), None) + + def is_cycle(n, dep, visited): + visited.add(n.v.tag_path) + if dep in visited: + return True + + dep_n = get_node(dep) + if dep_n is not None: + for sub_dep in dep_n.v.deps: + if is_cycle(dep_n, sub_dep, visited): + return True + + return False + + # check for dependency cycles, remove if detected. sort will + # not be 100% but allows for a best-effort to work around + # issue in NSO. + for n in nodes: + for dep in n.v.deps: + if is_cycle(n, dep, set()): + n.v.deps.remove(dep) def visit(n): if n.tmp_mark: @@ -380,9 +410,10 @@ class ValueBuilder(object): return True - while any((not n.mark for n in nodes)): - n = next((node for node in nodes if not node.mark)) + n = next((n for n in nodes if not n.mark), None) + while n is not None: visit(n) + n = next((n for n in nodes if not n.mark), None) return sorted_values[::-1] diff --git a/test/units/module_utils/network/nso/test_nso.py b/test/units/module_utils/network/nso/test_nso.py index f0faea6cd2..f3bf188be4 100644 --- a/test/units/module_utils/network/nso/test_nso.py +++ b/test/units/module_utils/network/nso/test_nso.py @@ -600,3 +600,48 @@ class TestVerifyVersion(unittest.TestCase): self.assertFalse(nso.verify_version_str('4.4.1', [(4, 6), (4, 5, 1)])) self.assertFalse(nso.verify_version_str('4.4.1.2', [(4, 6), (4, 5, 1)])) self.assertFalse(nso.verify_version_str('4.5.0', [(4, 6), (4, 5, 1)])) + + +class TestValueSort(unittest.TestCase): + def test_sort_parent_depend(self): + values = [ + nso.ValueBuilder.Value('/test/list{entry}', '/test/list', 'CREATE', ['']), + nso.ValueBuilder.Value('/test/list{entry}/description', '/test/list/description', 'TEST', ['']), + nso.ValueBuilder.Value('/test/entry', '/test/entry', 'VALUE', ['/test/list', '/test/list/name']) + ] + + result = [v.path for v in nso.ValueBuilder.sort_values(values)] + + self.assertEquals(['/test/list{entry}', '/test/entry', '/test/list{entry}/description'], result) + + def test_sort_break_direct_cycle(self): + values = [ + nso.ValueBuilder.Value('/test/a', '/test/a', 'VALUE', ['/test/c']), + nso.ValueBuilder.Value('/test/b', '/test/b', 'VALUE', ['/test/a']), + nso.ValueBuilder.Value('/test/c', '/test/c', 'VALUE', ['/test/a']) + ] + + result = [v.path for v in nso.ValueBuilder.sort_values(values)] + + self.assertEquals(['/test/a', '/test/b', '/test/c'], result) + + def test_sort_break_indirect_cycle(self): + values = [ + nso.ValueBuilder.Value('/test/c', '/test/c', 'VALUE', ['/test/a']), + nso.ValueBuilder.Value('/test/a', '/test/a', 'VALUE', ['/test/b']), + nso.ValueBuilder.Value('/test/b', '/test/b', 'VALUE', ['/test/c']) + ] + + result = [v.path for v in nso.ValueBuilder.sort_values(values)] + + self.assertEquals(['/test/a', '/test/c', '/test/b'], result) + + def test_sort_depend_on_self(self): + values = [ + nso.ValueBuilder.Value('/test/a', '/test/a', 'VALUE', ['/test/a']), + nso.ValueBuilder.Value('/test/b', '/test/b', 'VALUE', []) + ] + + result = [v.path for v in nso.ValueBuilder.sort_values(values)] + + self.assertEqual(['/test/a', '/test/b'], result)