From 3fdd9df983dbd23000007c457f50d125166708f9 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Fri, 26 May 2017 10:32:59 +1000 Subject: [PATCH] Move create_graph into config.py This was suggested in a review comment in I8a5d62a076a5a50597f2f1df3a8615afba6dadb2. It works out quite nicely because the BlockDevice() driver now doesn't need to know anything about stevedore or plugins, and just works on the node list. It also simplifies the unit testing by not having to call create_graph through a BlockDevice object. Change-Id: I98512f6cf42e256d2ea8225a0b496d303bf357b8 --- diskimage_builder/block_device/blockdevice.py | 110 +----------------- diskimage_builder/block_device/config.py | 92 +++++++++++++++ .../block_device/tests/test_config.py | 18 +-- .../block_device/tests/test_mount_order.py | 4 +- 4 files changed, 106 insertions(+), 118 deletions(-) diff --git a/diskimage_builder/block_device/blockdevice.py b/diskimage_builder/block_device/blockdevice.py index 456f4bc8..2e80fd7d 100644 --- a/diskimage_builder/block_device/blockdevice.py +++ b/diskimage_builder/block_device/blockdevice.py @@ -20,16 +20,8 @@ import shutil import sys import yaml -import networkx as nx - -from stevedore import extension - -from diskimage_builder.block_device.config import \ - config_tree_to_graph -from diskimage_builder.block_device.exception import \ - BlockDeviceSetupException -from diskimage_builder.block_device.plugin import NodeBase -from diskimage_builder.block_device.plugin import PluginBase +from diskimage_builder.block_device.config import config_tree_to_graph +from diskimage_builder.block_device.config import create_graph from diskimage_builder.block_device.utils import exec_sudo @@ -147,9 +139,6 @@ class BlockDevice(object): self.params['build-dir'], "states/block-device") self.state_json_file_name \ = os.path.join(self.state_dir, "state.json") - self.plugin_manager = extension.ExtensionManager( - namespace='diskimage_builder.block_device.plugin', - invoke_on_load=False) self.config_json_file_name \ = os.path.join(self.state_dir, "config.json") @@ -168,95 +157,8 @@ class BlockDevice(object): with open(self.state_json_file_name, "w") as fd: json.dump(state, fd) - def create_graph(self, config, default_config): - """Generate configuration digraph - - Generate the configuration digraph from the config - - :param config: graph configuration file - :param default_config: default parameters (from --params) - :return: tuple with the graph object, nodes in call order - """ - # This is the directed graph of nodes: each parse method must - # add the appropriate nodes and edges. - dg = nx.DiGraph() - - for config_entry in config: - # this should have been checked by generate_config - assert len(config_entry) == 1 - - logger.debug("Config entry [%s]" % config_entry) - cfg_obj_name = list(config_entry.keys())[0] - cfg_obj_val = config_entry[cfg_obj_name] - - # Instantiate a "plugin" object, passing it the - # configuration entry - # XXX would a "factory" pattern for plugins, where we make - # a method call on an object stevedore has instantiated be - # better here? - if cfg_obj_name not in self.plugin_manager: - raise BlockDeviceSetupException( - ("Config element [%s] is not implemented" % cfg_obj_name)) - plugin = self.plugin_manager[cfg_obj_name].plugin - assert issubclass(plugin, PluginBase) - cfg_obj = plugin(cfg_obj_val, default_config) - - # Ask the plugin for the nodes it would like to insert - # into the graph. Some plugins, such as partitioning, - # return multiple nodes from one config entry. - nodes = cfg_obj.get_nodes() - assert isinstance(nodes, list) - for node in nodes: - # plugins should return nodes... - assert isinstance(node, NodeBase) - # ensure node names are unique. networkx by default - # just appends the attribute to the node dict for - # existing nodes, which is not what we want. - if node.name in dg.node: - raise BlockDeviceSetupException( - "Duplicate node name: %s" % (node.name)) - logger.debug("Adding %s : %s", node.name, node) - dg.add_node(node.name, obj=node) - - # Now find edges - for name, attr in dg.nodes(data=True): - obj = attr['obj'] - # Unfortunately, we can not determine node edges just from - # the configuration file. It's not always simply the - # "base:" pointer. So ask nodes for a list of nodes they - # want to point to. *mostly* it's just base: ... but - # mounting is different. - # edges_from are the nodes that point to us - # edges_to are the nodes we point to - edges_from, edges_to = obj.get_edges() - logger.debug("Edges for %s: f:%s t:%s", name, - edges_from, edges_to) - for edge_from in edges_from: - if edge_from not in dg.node: - raise BlockDeviceSetupException( - "Edge not defined: %s->%s" % (edge_from, name)) - dg.add_edge(edge_from, name) - for edge_to in edges_to: - if edge_to not in dg.node: - raise BlockDeviceSetupException( - "Edge not defined: %s->%s" % (name, edge_to)) - dg.add_edge(name, edge_to) - - # this can be quite helpful debugging but needs pydotplus. - # run "dotty /tmp/out.dot" - # XXX: maybe an env var that dumps to a tmpdir or something? - # nx.nx_pydot.write_dot(dg, '/tmp/graph_dump.dot') - - # Topological sort (i.e. create a linear array that satisfies - # dependencies) and return the object list - call_order_nodes = nx.topological_sort(dg) - logger.debug("Call order: %s", list(call_order_nodes)) - call_order = [dg.node[n]['obj'] for n in call_order_nodes] - - return dg, call_order - def create(self, result, rollback): - dg, call_order = self.create_graph(self.config, self.params) + dg, call_order = create_graph(self.config, self.params) for node in call_order: node.create(result, rollback) @@ -413,7 +315,7 @@ class BlockDevice(object): return 0 # Deleting must be done in reverse order - dg, call_order = self.create_graph(self.config, self.params) + dg, call_order = create_graph(self.config, self.params) reverse_order = reversed(call_order) if dg is None: @@ -427,7 +329,7 @@ class BlockDevice(object): """Cleanup all remaining relicts - in good case""" # Deleting must be done in reverse order - dg, call_order = self.create_graph(self.config, self.params) + dg, call_order = create_graph(self.config, self.params) reverse_order = reversed(call_order) for node in reverse_order: @@ -442,7 +344,7 @@ class BlockDevice(object): """Cleanup all remaining relicts - in case of an error""" # Deleting must be done in reverse order - dg, call_order = self.create_graph(self.config, self.params) + dg, call_order = create_graph(self.config, self.params) reverse_order = reversed(call_order) for node in reverse_order: diff --git a/diskimage_builder/block_device/config.py b/diskimage_builder/block_device/config.py index 1fef2a0a..61f8b366 100644 --- a/diskimage_builder/block_device/config.py +++ b/diskimage_builder/block_device/config.py @@ -11,11 +11,14 @@ # under the License. import logging +import networkx as nx from stevedore import extension from diskimage_builder.block_device.exception import \ BlockDeviceSetupException +from diskimage_builder.block_device.plugin import NodeBase +from diskimage_builder.block_device.plugin import PluginBase logger = logging.getLogger(__name__) @@ -138,6 +141,95 @@ def config_tree_to_graph(config): return output +def create_graph(config, default_config): + """Generate configuration digraph + + Generate the configuration digraph from the config + + :param config: graph configuration file + :param default_config: default parameters (from --params) + :return: tuple with the graph object (a :class:`nx.Digraph`), + ordered list of :class:`NodeBase` objects + + """ + # This is the directed graph of nodes: each parse method must + # add the appropriate nodes and edges. + dg = nx.DiGraph() + + for config_entry in config: + # this should have been checked by generate_config + assert len(config_entry) == 1 + + logger.debug("Config entry [%s]" % config_entry) + cfg_obj_name = list(config_entry.keys())[0] + cfg_obj_val = config_entry[cfg_obj_name] + + # Instantiate a "plugin" object, passing it the + # configuration entry + # XXX : would a "factory" pattern for plugins, where we + # make a method call on an object stevedore has instantiated + # be better here? + if not is_a_plugin(cfg_obj_name): + raise BlockDeviceSetupException( + ("Config element [%s] is not implemented" % cfg_obj_name)) + plugin = _extensions[cfg_obj_name].plugin + assert issubclass(plugin, PluginBase) + cfg_obj = plugin(cfg_obj_val, default_config) + + # Ask the plugin for the nodes it would like to insert + # into the graph. Some plugins, such as partitioning, + # return multiple nodes from one config entry. + nodes = cfg_obj.get_nodes() + assert isinstance(nodes, list) + for node in nodes: + # plugins should return nodes... + assert isinstance(node, NodeBase) + # ensure node names are unique. networkx by default + # just appends the attribute to the node dict for + # existing nodes, which is not what we want. + if node.name in dg.node: + raise BlockDeviceSetupException( + "Duplicate node name: %s" % (node.name)) + logger.debug("Adding %s : %s", node.name, node) + dg.add_node(node.name, obj=node) + + # Now find edges + for name, attr in dg.nodes(data=True): + obj = attr['obj'] + # Unfortunately, we can not determine node edges just from + # the configuration file. It's not always simply the + # "base:" pointer. So ask nodes for a list of nodes they + # want to point to. *mostly* it's just base: ... but + # mounting is different. + # edges_from are the nodes that point to us + # edges_to are the nodes we point to + edges_from, edges_to = obj.get_edges() + logger.debug("Edges for %s: f:%s t:%s", name, + edges_from, edges_to) + for edge_from in edges_from: + if edge_from not in dg.node: + raise BlockDeviceSetupException( + "Edge not defined: %s->%s" % (edge_from, name)) + dg.add_edge(edge_from, name) + for edge_to in edges_to: + if edge_to not in dg.node: + raise BlockDeviceSetupException( + "Edge not defined: %s->%s" % (name, edge_to)) + dg.add_edge(name, edge_to) + + # this can be quite helpful debugging but needs pydotplus. + # run "dotty /tmp/out.dot" + # XXX: maybe an env var that dumps to a tmpdir or something? + # nx.nx_pydot.write_dot(dg, '/tmp/graph_dump.dot') + + # Topological sort (i.e. create a linear array that satisfies + # dependencies) and return the object list + call_order_nodes = nx.topological_sort(dg) + logger.debug("Call order: %s", list(call_order_nodes)) + call_order = [dg.node[n]['obj'] for n in call_order_nodes] + + return dg, call_order + # # On partitioning: objects # diff --git a/diskimage_builder/block_device/tests/test_config.py b/diskimage_builder/block_device/tests/test_config.py index 911deb70..c2bd0ba9 100644 --- a/diskimage_builder/block_device/tests/test_config.py +++ b/diskimage_builder/block_device/tests/test_config.py @@ -16,10 +16,8 @@ import os import testtools import yaml -from diskimage_builder.block_device.blockdevice \ - import BlockDevice -from diskimage_builder.block_device.config \ - import config_tree_to_graph +from diskimage_builder.block_device.config import config_tree_to_graph +from diskimage_builder.block_device.config import create_graph from diskimage_builder.block_device.exception import \ BlockDeviceSetupException @@ -63,8 +61,6 @@ class TestGraphGeneration(TestConfig): 'mount-base': '/fake', } - self.bd = BlockDevice(self.fake_default_config) - class TestConfigParsing(TestConfig): """Test parsing config file into a graph""" @@ -121,7 +117,7 @@ class TestCreateGraph(TestGraphGeneration): config = self.load_config_file('bad_edge_graph.yaml') self.assertRaisesRegexp(BlockDeviceSetupException, "Edge not defined: this_is_not_a_node", - self.bd.create_graph, + create_graph, config, self.fake_default_config) # Test a graph with bad edge pointing to an invalid node @@ -130,15 +126,14 @@ class TestCreateGraph(TestGraphGeneration): self.assertRaisesRegexp(BlockDeviceSetupException, "Duplicate node name: " "this_is_a_duplicate", - self.bd.create_graph, + create_graph, config, self.fake_default_config) # Test digraph generation from deep_graph config file def test_deep_graph_generator(self): config = self.load_config_file('deep_graph.yaml') - graph, call_order = self.bd.create_graph(config, - self.fake_default_config) + graph, call_order = create_graph(config, self.fake_default_config) call_order_list = [n.name for n in call_order] @@ -155,8 +150,7 @@ class TestCreateGraph(TestGraphGeneration): def test_multiple_partitions_graph_generator(self): config = self.load_config_file('multiple_partitions_graph.yaml') - graph, call_order = self.bd.create_graph(config, - self.fake_default_config) + graph, call_order = create_graph(config, self.fake_default_config) call_order_list = [n.name for n in call_order] # The sort creating call_order_list is unstable. diff --git a/diskimage_builder/block_device/tests/test_mount_order.py b/diskimage_builder/block_device/tests/test_mount_order.py index 8c948e43..86315368 100644 --- a/diskimage_builder/block_device/tests/test_mount_order.py +++ b/diskimage_builder/block_device/tests/test_mount_order.py @@ -15,6 +15,7 @@ import mock import diskimage_builder.block_device.tests.test_config as tc +from diskimage_builder.block_device.config import create_graph from diskimage_builder.block_device.level3.mount import MountPointNode logger = logging.getLogger(__name__) @@ -27,8 +28,7 @@ class TestMountOrder(tc.TestGraphGeneration): config = self.load_config_file('multiple_partitions_graph.yaml') - graph, call_order = self.bd.create_graph(config, - self.fake_default_config) + graph, call_order = create_graph(config, self.fake_default_config) result = {} result['filesys'] = {}