From b708918b85208ce7741209c1ff5e969dc49b0882 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Thu, 1 Jun 2017 14:57:34 +1000 Subject: [PATCH] Remove 'state' argument from later cmd_* calls With I468dbf5134947629f125504513703d6f2cdace59 each node has a reference to the global state object. This means it gets pickled into the node-list, which is loaded for later calls. There is no need to reload the state.json it and pass it for later cmd_* calls, as the nodes can see it via the unpickled self.state Change-Id: I9e2f8910f17599d92ee33e7df8e36d8ed4d44575 --- diskimage_builder/block_device/blockdevice.py | 36 +++++++++---------- .../block_device/level0/localloop.py | 11 +++--- .../block_device/level3/mount.py | 8 ++--- diskimage_builder/block_device/plugin.py | 19 ++++------ .../block_device/tests/plugin/test_a.py | 4 +-- .../block_device/tests/plugin/test_b.py | 11 +++--- .../block_device/tests/test_state.py | 23 ++++++++---- 7 files changed, 57 insertions(+), 55 deletions(-) diff --git a/diskimage_builder/block_device/blockdevice.py b/diskimage_builder/block_device/blockdevice.py index a17d48fb..562dc335 100644 --- a/diskimage_builder/block_device/blockdevice.py +++ b/diskimage_builder/block_device/blockdevice.py @@ -396,36 +396,34 @@ class BlockDevice(object): def cmd_umount(self): """Unmounts the blockdevice and cleanup resources""" - # State should have been created by prior calls; we only need - # the dict. If it is not here, it has been cleaned up already - # (? more details?) - try: - state = BlockDeviceState(self.state_json_file_name) - except BlockDeviceSetupException: + # If the state is not here, cmd_cleanup removed it? Nothing + # more to do? + # XXX: better understand this... + if not os.path.exists(self.node_pickle_file_name): logger.info("State already cleaned - no way to do anything here") return 0 - # Deleting must be done in reverse order call_order = pickle.load(open(self.node_pickle_file_name, 'rb')) reverse_order = reversed(call_order) for node in reverse_order: - node.umount(state) + node.umount() return 0 def cmd_cleanup(self): """Cleanup all remaining relicts - in good case""" - # State should have been created by prior calls; we only need - # the dict - state = BlockDeviceState(self.state_json_file_name) - # Deleting must be done in reverse order - call_order = pickle.load(open(self.node_pickle_file_name, 'rb')) + # Cleanup must be done in reverse order + try: + call_order = pickle.load(open(self.node_pickle_file_name, 'rb')) + except IOError: + raise BlockDeviceSetupException("Pickle file not found") + reverse_order = reversed(call_order) for node in reverse_order: - node.cleanup(state) + node.cleanup() logger.info("Removing temporary state dir [%s]", self.state_dir) shutil.rmtree(self.state_dir) @@ -434,16 +432,16 @@ class BlockDevice(object): def cmd_delete(self): """Cleanup all remaining relicts - in case of an error""" - # State should have been created by prior calls; we only need - # the dict - state = BlockDeviceState(self.state_json_file_name) # Deleting must be done in reverse order - call_order = pickle.load(open(self.node_pickle_file_name, 'rb')) + try: + call_order = pickle.load(open(self.node_pickle_file_name, 'rb')) + except IOError: + raise BlockDeviceSetupException("Pickle file not found") reverse_order = reversed(call_order) for node in reverse_order: - node.delete(state) + node.delete() logger.info("Removing temporary state dir [%s]", self.state_dir) shutil.rmtree(self.state_dir) diff --git a/diskimage_builder/block_device/level0/localloop.py b/diskimage_builder/block_device/level0/localloop.py index 2a66665c..8281c07d 100644 --- a/diskimage_builder/block_device/level0/localloop.py +++ b/diskimage_builder/block_device/level0/localloop.py @@ -119,14 +119,11 @@ class LocalLoopNode(NodeBase): self.name, block_device, self.filename) return - def umount(self, state): - loopdev_detach(state['blockdev'][self.name]['device']) + def umount(self): + loopdev_detach(self.state['blockdev'][self.name]['device']) - def cleanup(self, state): - pass - - def delete(self, state): - image_delete(state['blockdev'][self.name]['image']) + def delete(self): + image_delete(self.state['blockdev'][self.name]['image']) class LocalLoop(PluginBase): diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py index 5b9be63d..f53a76f6 100644 --- a/diskimage_builder/block_device/level3/mount.py +++ b/diskimage_builder/block_device/level3/mount.py @@ -94,12 +94,12 @@ class MountPointNode(NodeBase): self.state['mount_order'] = [] self.state['mount_order'].append(self.mount_point) - def umount(self, state): + def umount(self): logger.info("Called for [%s]", self.name) - exec_sudo(["umount", state['mount'][self.mount_point]['path']]) + exec_sudo(["umount", self.state['mount'][self.mount_point]['path']]) - def delete(self, state): - self.umount(state) + def delete(self): + self.umount() class Mount(PluginBase): diff --git a/diskimage_builder/block_device/plugin.py b/diskimage_builder/block_device/plugin.py index 0efe4e93..1e2e0a5f 100644 --- a/diskimage_builder/block_device/plugin.py +++ b/diskimage_builder/block_device/plugin.py @@ -95,33 +95,29 @@ class NodeBase(object): """ return - def umount(self, state): + def umount(self): """Umount actions - Actions to taken when ``dib-block-device umount`` is called + Actions to taken when ``dib-block-device umount`` is called. + The nodes are called in the reverse order to :func:`create` - :param state: the current state dictionary. This is the - `state` dictionary from :func:`create` before this call is - made. :return: None + """ return - def cleanup(self, state): + def cleanup(self): """Cleanup actions Actions to taken when ``dib-block-device cleanup`` is called. This is the cleanup path in the *success* case. The nodes are called in the reverse order to :func:`create` - :param state: the current state dictionary. This is the - `state` dictionary from :func:`create` before this call is - made. :return: None """ return - def delete(self, state): + def delete(self): """Cleanup actions Actions to taken when ``dib-block-device delete`` is called. @@ -129,9 +125,6 @@ class NodeBase(object): *failure*. The nodes are called in the reverse order to :func:`create` - :param state: the current state dictionary. This is the - `state` dictionary from :func:`create` before this call is - made. :return: None """ return diff --git a/diskimage_builder/block_device/tests/plugin/test_a.py b/diskimage_builder/block_device/tests/plugin/test_a.py index c19152e2..1b98d207 100644 --- a/diskimage_builder/block_device/tests/plugin/test_a.py +++ b/diskimage_builder/block_device/tests/plugin/test_a.py @@ -40,9 +40,9 @@ class TestANode(NodeBase): self.state['test_a']['value2'] = 'bar' return - def umount(self, state): + def umount(self): # Umount is run in reverse. This key should exist from test_b - state['umount'].append('test_a') + self.state['umount'].append('test_a') class TestA(PluginBase): diff --git a/diskimage_builder/block_device/tests/plugin/test_b.py b/diskimage_builder/block_device/tests/plugin/test_b.py index e95ba338..00ac4622 100644 --- a/diskimage_builder/block_device/tests/plugin/test_b.py +++ b/diskimage_builder/block_device/tests/plugin/test_b.py @@ -38,11 +38,14 @@ class TestBNode(NodeBase): self.state['test_b']['value'] = 'baz' return - def umount(self, state): + def umount(self): + # these values should have persisteted from create() + assert self.state['test_b']['value'] == 'baz' + # umount run in reverse. this should run before test_a - assert 'umount' not in state - state['umount'] = [] - state['umount'].append('test_b') + assert 'umount' not in self.state + self.state['umount'] = [] + self.state['umount'].append('test_b') class TestB(PluginBase): diff --git a/diskimage_builder/block_device/tests/test_state.py b/diskimage_builder/block_device/tests/test_state.py index 7f91b191..639a5abd 100644 --- a/diskimage_builder/block_device/tests/test_state.py +++ b/diskimage_builder/block_device/tests/test_state.py @@ -80,7 +80,9 @@ class TestState(TestStateBase): # run umount, which should load the picked nodes and run in # reverse. This will create some state in "test_b" that it - # added to by "test_a" ... ensuring it was run backwards. + # added to by "test_a" ... ensuring it was run backwards. It + # also checks the state was persisted through the pickling + # process. bd_obj.cmd_umount() # Test state going missing between phases @@ -95,20 +97,29 @@ class TestState(TestStateBase): bd_obj.cmd_create() # cmd_create should have persisted this to disk - state_file = os.path.join(self.build_dir.path, - 'states', 'block-device', - 'state.json') + state_file = bd_obj.state_json_file_name self.assertThat(state_file, FileExists()) + pickle_file = bd_obj.node_pickle_file_name + self.assertThat(pickle_file, FileExists()) # simulate the state somehow going missing, and ensure that # later calls notice os.unlink(state_file) + os.unlink(pickle_file) + # This reads from the state dump json file self.assertRaisesRegex(BlockDeviceSetupException, "State dump not found", - bd_obj.cmd_cleanup) + bd_obj.cmd_getval, 'image-path') self.assertRaisesRegex(BlockDeviceSetupException, "State dump not found", bd_obj.cmd_writefstab) + + # this uses the pickled nodes self.assertRaisesRegex(BlockDeviceSetupException, - "State dump not found", + "Pickle file not found", bd_obj.cmd_delete) + self.assertRaisesRegex(BlockDeviceSetupException, + "Pickle file not found", + bd_obj.cmd_cleanup) + + # XXX: figure out unit test for umount