From a4c351cb47675459200707549d278cc967bd21c6 Mon Sep 17 00:00:00 2001 From: Valerii Kovalchuk Date: Wed, 25 May 2016 18:18:40 +0300 Subject: [PATCH] Import package and dependencies in correct order Package and its dependencies are being imported in random order which allows to import package and then get an error while importing its dependency. This patch uses OrderedDict ordered by topological sort instead of regular dict to fix that. Cyclic requirements are imported in random order. Tests to check order of import are also added. Change-Id: Ia6cb087679b3ae4613ac3d1d19279a9826e537ef Closes-bug: #1585420 Co-Authored-By: ksnihyr --- muranoclient/common/utils.py | 106 ++++++++-- muranoclient/tests/unit/test_utils.py | 194 +++++++++++++++++- muranoclient/v1/shell.py | 9 +- .../requirements-order-19ecc40ca6d34739.yaml | 5 + 4 files changed, 285 insertions(+), 29 deletions(-) create mode 100644 releasenotes/notes/requirements-order-19ecc40ca6d34739.yaml diff --git a/muranoclient/common/utils.py b/muranoclient/common/utils.py index 02dc6347..b68ad948 100644 --- a/muranoclient/common/utils.py +++ b/muranoclient/common/utils.py @@ -388,20 +388,92 @@ class Package(FileWrapperMixin): self._logo = None return self._logo - def requirements(self, base_url, path=None, dep_dict=None): - """Get dict of all requirements + def _get_package_order(self, packages_graph): + """Sorts packages according to dependencies between them - Recursively scan Require section of manifests of all the - dependencies. Returns a dict with FQPNs as keys and respective - Package objects as values + Murano allows cyclic dependencies. It is impossible + to do topological sort for graph with cycles, so at first + graph condensation should be built. + For condensation building Kosaraju's algorithm is used. + Packages in strongly connected components can be situated + in random order to each other. """ - if not dep_dict: - dep_dict = {} - dep_dict[self.manifest['FullName']] = self - if 'Require' in self.manifest: - for dep_name, ver in six.iteritems(self.manifest['Require']): - if dep_name in dep_dict: - continue + def topological_sort(graph, start_node): + order = [] + not_seen = set(graph) + + def dfs(node): + not_seen.discard(node) + for dep_node in graph[node]: + if dep_node in not_seen: + dfs(dep_node) + order.append(node) + + dfs(start_node) + return order + + def transpose_graph(graph): + transposed = collections.defaultdict(list) + for node, deps in six.viewitems(graph): + for dep in deps: + transposed[dep].append(node) + return transposed + + order = topological_sort(packages_graph, self.manifest['FullName']) + order.reverse() + transposed = transpose_graph(packages_graph) + + def top_sort_by_components(graph, component_order): + result = [] + seen = set() + + def dfs(node): + seen.add(node) + result.append(node) + for dep_node in graph[node]: + if dep_node not in seen: + dfs(dep_node) + for item in component_order: + if item not in seen: + dfs(item) + return reversed(result) + return top_sort_by_components(transposed, order) + + def requirements(self, base_url, path=None, dep_dict=None): + """Scans Require section of manifests of all the dependencies. + + Returns a dict with FQPNs as keys and respective Package objects + as values, ordered by topological sort. + + :param base_url: url of packages location + :param path: local path of packages location + :param dep_dict: unused. Left for backward compatibility + """ + + unordered_requirements = {} + requirements_graph = collections.defaultdict(list) + dep_queue = collections.deque([(self.manifest['FullName'], self)]) + while dep_queue: + dep_name, dep_file = dep_queue.popleft() + unordered_requirements[dep_name] = dep_file + direct_deps = Package._get_direct_deps(dep_file, base_url, path) + for name, file in direct_deps: + if name not in unordered_requirements: + dep_queue.append((name, file)) + requirements_graph[dep_name] = [dep[0] for dep in direct_deps] + + ordered_reqs_names = self._get_package_order(requirements_graph) + ordered_reqs_dict = collections.OrderedDict() + for name in ordered_reqs_names: + ordered_reqs_dict[name] = unordered_requirements[name] + + return ordered_reqs_dict + + @staticmethod + def _get_direct_deps(package, base_url, path): + result = [] + if 'Require' in package.manifest: + for dep_name, ver in six.iteritems(package.manifest['Require']): try: req_file = Package.from_location( dep_name, @@ -413,14 +485,10 @@ class Package(FileWrapperMixin): LOG.error("Error {0} occurred while parsing package {1}, " "required by {2} package".format( e, dep_name, - self.manifest['FullName'])) + package.manifest['FullName'])) continue - dep_dict.update(req_file.requirements( - base_url=base_url, - path=path, - dep_dict=dep_dict, - )) - return dep_dict + result.append((req_file.manifest['FullName'], req_file)) + return result def save_image_local(image_spec, base_url, dst): diff --git a/muranoclient/tests/unit/test_utils.py b/muranoclient/tests/unit/test_utils.py index cff0f65a..5ce2e57c 100644 --- a/muranoclient/tests/unit/test_utils.py +++ b/muranoclient/tests/unit/test_utils.py @@ -177,8 +177,99 @@ class PackageTest(testtools.TestCase): {'main_app': app, 'dep_app': mock.ANY, 'dep_of_dep': mock.ANY}, reqs) - @requests_mock.mock() - def test_cyclic_requirements(self, m): + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_requirements_order(self, from_file): + """Test that dependencies are parsed in correct order.""" + + pkg5 = make_pkg({'FullName': 'd4', }) + pkg4 = make_pkg({'FullName': 'd3', 'Require': {'d4': None}, }) + pkg3 = make_pkg({'FullName': 'd2', 'Require': {'d3': None}, }) + pkg2 = make_pkg({'FullName': 'd1', 'Require': {'d3': None}, }) + pkg1 = make_pkg({'FullName': 'M', 'Require': {'d1': None, + 'd2': None, + 'd4': None}, }) + + def side_effect(name): + if 'M' in name: + return utils.Package(utils.File(pkg1)) + if 'd1' in name: + return utils.Package(utils.File(pkg2)) + if 'd2' in name: + return utils.Package(utils.File(pkg3)) + if 'd3' in name: + return utils.Package(utils.File(pkg4)) + if 'd4' in name: + return utils.Package(utils.File(pkg5)) + + from_file.side_effect = side_effect + app = from_file('M') + reqs = app.requirements(base_url=self.base_url) + + def key_position(key): + keys = list(six.iterkeys(reqs)) + return keys.index(key) + + self.assertTrue( + key_position('d4') < key_position('d3') and + key_position('d4') < key_position('M') and + key_position('d3') < key_position('d1') and + key_position('d3') < key_position('d2') < key_position('M') + ) + + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_requirements_order2(self, from_file): + """Test that dependencies are parsed in correct order.""" + + pkg5 = make_pkg({'FullName': 'd4', 'Require': {'d6': None}, }) + pkg4 = make_pkg({'FullName': 'd3', 'Require': {'d4': None}, }) + pkg3 = make_pkg({'FullName': 'd1', 'Require': {'d3': None, + 'd7': None}, }) + pkg2 = make_pkg({'FullName': 'd2', 'Require': {'d3': None}, }) + pkg6 = make_pkg({'FullName': 'd6', }) + pkg7 = make_pkg({'FullName': 'd7', 'Require': {'d8': None}, }) + pkg8 = make_pkg({'FullName': 'd8', }) + + pkg1 = make_pkg({'FullName': 'M', 'Require': {'d1': None, + 'd2': None, + 'd4': None, }, }) + + def side_effect(name): + if 'M' in name: + return utils.Package(utils.File(pkg1)) + if 'd1' in name: + return utils.Package(utils.File(pkg2)) + if 'd2' in name: + return utils.Package(utils.File(pkg3)) + if 'd3' in name: + return utils.Package(utils.File(pkg4)) + if 'd4' in name: + return utils.Package(utils.File(pkg5)) + if 'd6' in name: + return utils.Package(utils.File(pkg6)) + if 'd7' in name: + return utils.Package(utils.File(pkg7)) + if 'd8' in name: + return utils.Package(utils.File(pkg8)) + + from_file.side_effect = side_effect + app = from_file('M') + reqs = app.requirements(base_url=self.base_url) + + def key_position(key): + keys = list(six.iterkeys(reqs)) + return keys.index(key) + + self.assertTrue( + key_position('d6') < key_position('d4') < + key_position('d3') < key_position('d1') and + key_position('d3') < key_position('d2') and + key_position('d1') < key_position('M') and + key_position('d2') < key_position('M') and + key_position('d8') < key_position('d7') < key_position('d1') + ) + + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_cyclic_requirements(self, from_file): """Test that a cyclic dependency would be handled correctly.""" pkg3 = make_pkg({'FullName': 'dep_of_dep', 'Require': { 'main_app': None, 'dep_app': None}, }) @@ -187,16 +278,107 @@ class PackageTest(testtools.TestCase): pkg1 = make_pkg({'FullName': 'main_app', 'Require': { 'dep_app': None, 'dep_of_dep': None}, }) - m.get(self.base_url + '/apps/main_app.zip', body=pkg1) - m.get(self.base_url + '/apps/dep_app.zip', body=pkg2) - m.get(self.base_url + '/apps/dep_of_dep.zip', body=pkg3) - app = utils.Package.fromFile(pkg1) + def side_effect(name): + if 'main_app' in name: + return utils.Package(utils.File(pkg1)) + if 'dep_app' in name: + return utils.Package(utils.File(pkg2)) + if 'dep_of_dep' in name: + return utils.Package(utils.File(pkg3)) + + from_file.side_effect = side_effect + app = from_file('main_app') reqs = app.requirements(base_url=self.base_url) self.assertEqual( {'main_app': app, 'dep_app': mock.ANY, 'dep_of_dep': mock.ANY}, reqs) + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_order_with_cyclic_requirements2(self, from_file): + """Test that dependencies are parsed in correct order.""" + + pkg6 = make_pkg({'FullName': 'd5', 'Require': {'d6': None}, }) + pkg7 = make_pkg({'FullName': 'd6', }) + pkg5 = make_pkg({'FullName': 'd4', 'Require': {'d3': None, + 'd5': None}}) + pkg4 = make_pkg({'FullName': 'd3', 'Require': {'d4': None}, }) + pkg3 = make_pkg({'FullName': 'd2', 'Require': {'d1': None, + 'd5': None, + 'd6': None}, }) + pkg2 = make_pkg({'FullName': 'd1', 'Require': {'d2': None}, }) + pkg1 = make_pkg({'FullName': 'M', 'Require': {'d1': None, + 'd3': None}, }) + + def side_effect(name): + if 'M' in name: + return utils.Package(utils.File(pkg1)) + if 'd1' in name: + return utils.Package(utils.File(pkg2)) + if 'd2' in name: + return utils.Package(utils.File(pkg3)) + if 'd3' in name: + return utils.Package(utils.File(pkg4)) + if 'd4' in name: + return utils.Package(utils.File(pkg5)) + if 'd5' in name: + return utils.Package(utils.File(pkg6)) + if 'd6' in name: + return utils.Package(utils.File(pkg7)) + + from_file.side_effect = side_effect + app = from_file('M') + reqs = app.requirements(base_url=self.base_url) + + def key_position(key): + keys = list(six.iterkeys(reqs)) + return keys.index(key) + + self.assertTrue( + key_position('d5') < key_position('d4') and + key_position('d5') < key_position('d2') and + key_position('d5') < key_position('d3') < key_position('M') and + key_position('d5') < key_position('d1') < key_position('M') + ) + + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_order_with_cyclic_requirements3(self, from_file): + """Test that dependencies are parsed in correct order.""" + + pkg5 = make_pkg({'FullName': 'd4', }) + pkg4 = make_pkg({'FullName': 'd3', 'Require': {'M': None}, }) + pkg3 = make_pkg({'FullName': 'd2', 'Require': {'d3': None, + 'd4': None}, }) + pkg2 = make_pkg({'FullName': 'd1', 'Require': {'d2': None}, }) + pkg1 = make_pkg({'FullName': 'M', 'Require': {'d1': None}, }) + + def side_effect(name): + if 'M' in name: + return utils.Package(utils.File(pkg1)) + if 'd1' in name: + return utils.Package(utils.File(pkg2)) + if 'd2' in name: + return utils.Package(utils.File(pkg3)) + if 'd3' in name: + return utils.Package(utils.File(pkg4)) + if 'd4' in name: + return utils.Package(utils.File(pkg5)) + + from_file.side_effect = side_effect + app = from_file('M') + reqs = app.requirements(base_url=self.base_url) + + def key_position(key): + keys = list(six.iterkeys(reqs)) + return keys.index(key) + + self.assertTrue( + key_position('d4') < key_position('M') and + key_position('d4') < key_position('d1') and + key_position('d4') < key_position('d2') and + key_position('d4') < key_position('d3') + ) + def test_images(self): pkg = make_pkg({}) app = utils.Package.fromFile(pkg) diff --git a/muranoclient/v1/shell.py b/muranoclient/v1/shell.py index ca37d698..55d99761 100644 --- a/muranoclient/v1/shell.py +++ b/muranoclient/v1/shell.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import functools import json import os @@ -651,7 +652,7 @@ def do_package_import(mc, args): if args.categories: data["categories"] = args.categories - total_reqs = {} + total_reqs = collections.OrderedDict() main_packages_names = [] for filename in args.filename: if os.path.isfile(filename): @@ -755,7 +756,7 @@ def do_bundle_import(mc, args): file names, relative to location of the bundle file. Requirements are first searched in the same directory. """ - total_reqs = {} + total_reqs = collections.OrderedDict() for filename in args.filename: local_path = None if os.path.isfile(filename): @@ -875,7 +876,7 @@ def do_bundle_save(mc, args): else: dst = os.getcwd() - total_reqs = {} + total_reqs = collections.OrderedDict() if os.path.isfile(bundle): _file = bundle @@ -941,7 +942,7 @@ def do_package_save(mc, args): "ignoring version.") version = '' - total_reqs = {} + total_reqs = collections.OrderedDict() for package in args.package: _file = utils.to_url( package, diff --git a/releasenotes/notes/requirements-order-19ecc40ca6d34739.yaml b/releasenotes/notes/requirements-order-19ecc40ca6d34739.yaml new file mode 100644 index 00000000..dd6ff446 --- /dev/null +++ b/releasenotes/notes/requirements-order-19ecc40ca6d34739.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Fixed order of packages import - main package can not be imported before + all its dependencies are imported. Cyclic requirements are imported in + random order.