BFV Deploy skip minor logging, logic, and test fixes

This is a follow-up to I62e622a2b053f685c2da42ca5106bdb9bdd22dc6
addressing some minor logging, test fixes/additions that were requested
as well as correcting a minor logic discrepency between iscsi and agent
deploy preparation methods as it makes no sense to re-plug networks in
the iscsi deployment preparation method if we are not writing an image
since we will ultimately skip deployment.

Additionally, as a result of review feedback, the FlatNetwork mocks
were updated to be more consistent.

Change-Id: If6bd7e34ea92add15c3c7d8b94a2efbaf882d6ff
Partial-Bug: #1559691

ironic/drivers/modules/agent.py

@@ -338,6 +338,9 @@ class AgentDeploy(AgentDeployMixin, base.DeployInterface):
             # NOTE(TheJulia): There is no reason to validate
             # image properties if we will not be writing an image
             # in a boot from volume case. As such, return to the caller.
+            LOG.debug('Skipping complete deployment interface validation '
+                      'for node %s as it is set to boot from a remote '
+                      'volume.', node.uuid)
             return
 
         params = {}

ironic/drivers/modules/deploy_utils.py

@@ -1297,6 +1297,9 @@ def tear_down_storage_configuration(task):
     # this is dangerous if IPA is not handling the cleaning.
     for volume in task.volume_targets:
         volume.destroy()
+        LOG.info('Successfully deleted volume target %(target)s. '
+                 'The node associated with the target was %(node)s.',
+                 {'target': volume.uuid, 'node': task.node.uuid})
 
     node = task.node
     driver_internal_info = node.driver_internal_info

ironic/drivers/modules/iscsi_deploy.py

@@ -427,6 +427,9 @@ class ISCSIDeploy(AgentDeployMixin, base.DeployInterface):
         # Edit early if we are not writing a volume as the validate
         # tasks evaluate root device hints.
         if not task.driver.storage.should_write_image(task):
+            LOG.debug('Skipping complete deployment interface validation '
+                      'for node %s as it is set to boot from a remote '
+                      'volume.', node.uuid)
             return
 
         # TODO(rameshg87): iscsi_ilo driver uses this method. Remove
@@ -521,8 +524,9 @@ class ISCSIDeploy(AgentDeployMixin, base.DeployInterface):
                 manager_utils.node_power_action(task, states.POWER_OFF)
                 # NOTE(vdrok): in case of rebuild, we have tenant network
                 # already configured, unbind tenant ports if present
-                task.driver.network.unconfigure_tenant_networks(task)
-                task.driver.network.add_provisioning_network(task)
+                if task.driver.storage.should_write_image(task):
+                    task.driver.network.unconfigure_tenant_networks(task)
+                    task.driver.network.add_provisioning_network(task)
                 task.driver.storage.attach_volumes(task)
 
             deploy_opts = deploy_utils.build_agent_options(node)

ironic/tests/unit/drivers/modules/test_agent.py

@@ -29,6 +29,7 @@ from ironic.drivers.modules import agent_base_vendor
 from ironic.drivers.modules import agent_client
 from ironic.drivers.modules import deploy_utils
 from ironic.drivers.modules import fake
+from ironic.drivers.modules.network import flat as flat_network
 from ironic.drivers.modules import pxe
 from ironic.drivers.modules.storage import noop as noop_storage
 from ironic.drivers import utils as driver_utils
@@ -300,8 +301,9 @@ class TestAgentDeploy(db_base.DbTestCase):
 
     @mock.patch.object(noop_storage.NoopStorage, 'detach_volumes',
                        autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'unconfigure_tenant_networks', autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'unconfigure_tenant_networks',
+                       spec_set=True, autospec=True)
     @mock.patch('ironic.conductor.utils.node_power_action', autospec=True)
     def test_tear_down(self, power_mock,
                        unconfigure_tenant_nets_mock,
@@ -328,10 +330,11 @@ class TestAgentDeploy(db_base.DbTestCase):
     @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk')
     @mock.patch.object(deploy_utils, 'build_agent_options')
     @mock.patch.object(deploy_utils, 'build_instance_info_for_deploy')
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'unconfigure_tenant_networks', spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'unconfigure_tenant_networks',
+                       spec_set=True, autospec=True)
     def test_prepare(
             self, unconfigure_tenant_net_mock, add_provisioning_net_mock,
             build_instance_info_mock, build_options_mock,
@@ -382,8 +385,8 @@ class TestAgentDeploy(db_base.DbTestCase):
     @mock.patch.object(noop_storage.NoopStorage, 'attach_volumes',
                        autospec=True)
     @mock.patch.object(deploy_utils, 'populate_storage_driver_internal_info')
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_instance')
     @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk')
     @mock.patch.object(deploy_utils, 'build_agent_options')
@@ -407,8 +410,46 @@ class TestAgentDeploy(db_base.DbTestCase):
             self.assertTrue(storage_driver_info_mock.called)
             self.assertFalse(storage_attach_volumes_mock.called)
 
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', autospec=True)
+    @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
+                       autospec=True)
+    @mock.patch.object(noop_storage.NoopStorage, 'attach_volumes',
+                       autospec=True)
+    @mock.patch.object(deploy_utils, 'populate_storage_driver_internal_info',
+                       autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'unconfigure_tenant_networks',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', autospec=True)
+    @mock.patch.object(deploy_utils, 'build_agent_options', autospec=True)
+    @mock.patch.object(deploy_utils, 'build_instance_info_for_deploy',
+                       autospec=True)
+    def test_prepare_storage_write_false(
+            self, build_instance_info_mock, build_options_mock,
+            pxe_prepare_ramdisk_mock, pxe_prepare_instance_mock,
+            remove_tenant_net_mock, add_provisioning_net_mock,
+            storage_driver_info_mock, storage_attach_volumes_mock,
+            should_write_image_mock):
+        should_write_image_mock.return_value = False
+        with task_manager.acquire(
+                self.context, self.node['uuid'], shared=False) as task:
+            task.node.provision_state = states.DEPLOYING
+
+            self.driver.prepare(task)
+
+            self.assertFalse(build_instance_info_mock.called)
+            self.assertFalse(build_options_mock.called)
+            self.assertFalse(pxe_prepare_ramdisk_mock.called)
+            self.assertFalse(pxe_prepare_instance_mock.called)
+            self.assertFalse(add_provisioning_net_mock.called)
+            self.assertTrue(storage_driver_info_mock.called)
+            self.assertTrue(storage_attach_volumes_mock.called)
+            self.assertEqual(2, should_write_image_mock.call_count)
+
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk')
     @mock.patch.object(deploy_utils, 'build_agent_options')
     @mock.patch.object(deploy_utils, 'build_instance_info_for_deploy')
@@ -426,8 +467,8 @@ class TestAgentDeploy(db_base.DbTestCase):
             self.assertFalse(pxe_prepare_ramdisk_mock.called)
             self.assertFalse(add_provisioning_net_mock.called)
 
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk')
     @mock.patch.object(deploy_utils, 'build_agent_options')
     @mock.patch.object(deploy_utils, 'build_instance_info_for_deploy')

ironic/tests/unit/drivers/modules/test_iscsi_deploy.py

@@ -36,6 +36,7 @@ from ironic.drivers.modules import agent_base_vendor
 from ironic.drivers.modules import agent_client
 from ironic.drivers.modules import deploy_utils
 from ironic.drivers.modules import iscsi_deploy
+from ironic.drivers.modules.network import flat as flat_network
 from ironic.drivers.modules import pxe
 from ironic.drivers.modules.storage import noop as noop_storage
 from ironic.drivers import utils as driver_utils
@@ -593,8 +594,8 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
                        autospec=True)
     @mock.patch.object(deploy_utils, 'populate_storage_driver_internal_info',
                        autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True)
     def test_prepare_node_active(self, prepare_instance_mock,
                                  add_provisioning_net_mock,
@@ -611,8 +612,8 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
             storage_driver_info_mock.assert_called_once_with(task)
         self.assertFalse(storage_attach_volumes_mock.called)
 
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True)
     def test_prepare_node_adopting(self, prepare_instance_mock,
                                    add_provisioning_net_mock):
@@ -631,10 +632,11 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
                        autospec=True)
     @mock.patch.object(deploy_utils, 'build_agent_options', autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'add_provisioning_network', spec_set=True, autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'unconfigure_tenant_networks', spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'unconfigure_tenant_networks',
+                       spec_set=True, autospec=True)
     def test_prepare_node_deploying(
             self, unconfigure_tenant_net_mock, add_provisioning_net_mock,
             mock_prepare_ramdisk, mock_agent_options,
@@ -654,6 +656,41 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
             storage_attach_volumes_mock.assert_called_once_with(
                 task.driver.storage, task)
 
+    @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
+                       autospec=True)
+    @mock.patch.object(noop_storage.NoopStorage, 'attach_volumes',
+                       autospec=True)
+    @mock.patch.object(deploy_utils, 'populate_storage_driver_internal_info',
+                       autospec=True)
+    @mock.patch.object(deploy_utils, 'build_agent_options', autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork, 'add_provisioning_network',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'unconfigure_tenant_networks',
+                       spec_set=True, autospec=True)
+    def test_prepare_node_deploying_storage_should_write_false(
+            self, unconfigure_tenant_net_mock, add_provisioning_net_mock,
+            mock_prepare_ramdisk, mock_agent_options,
+            storage_driver_info_mock, storage_attach_volumes_mock,
+            storage_should_write_mock):
+        storage_should_write_mock.return_value = False
+        mock_agent_options.return_value = {'c': 'd'}
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            task.node.provision_state = states.DEPLOYING
+
+            task.driver.deploy.prepare(task)
+
+            mock_agent_options.assert_called_once_with(task.node)
+            mock_prepare_ramdisk.assert_called_once_with(
+                task.driver.boot, task, {'c': 'd'})
+            self.assertFalse(add_provisioning_net_mock.called)
+            self.assertFalse(unconfigure_tenant_net_mock.called)
+            storage_driver_info_mock.assert_called_once_with(task)
+            storage_attach_volumes_mock.assert_called_once_with(
+                task.driver.storage, task)
+            self.assertEqual(1, storage_should_write_mock.call_count)
+
     @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
     @mock.patch.object(iscsi_deploy, 'check_image_size', autospec=True)
     @mock.patch.object(iscsi_deploy, 'cache_instance_image', autospec=True)
@@ -670,10 +707,12 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
 
     @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
                        autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'configure_tenant_networks', spec_set=True, autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'remove_provisioning_network', spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'configure_tenant_networks',
+                       spec_set=True, autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'remove_provisioning_network',
+                       spec_set=True, autospec=True)
     @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
     @mock.patch.object(iscsi_deploy, 'check_image_size', autospec=True)
     @mock.patch.object(iscsi_deploy, 'cache_instance_image', autospec=True)
@@ -699,8 +738,9 @@ class ISCSIDeployTestCase(db_base.DbTestCase):
 
     @mock.patch.object(noop_storage.NoopStorage, 'detach_volumes',
                        autospec=True)
-    @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
-                'unconfigure_tenant_networks', autospec=True)
+    @mock.patch.object(flat_network.FlatNetwork,
+                       'unconfigure_tenant_networks',
+                       spec_set=True, autospec=True)
     @mock.patch.object(manager_utils, 'node_power_action', autospec=True)
     def test_tear_down(self, node_power_action_mock,
                        unconfigure_tenant_nets_mock,

ironic/tests/unit/drivers/modules/test_pxe.py

@@ -1092,11 +1092,11 @@ class PXEBootTestCase(db_base.DbTestCase):
             self.assertFalse(switch_pxe_config_mock.called)
             self.assertFalse(set_boot_device_mock.called)
 
-    @mock.patch('os.path.isfile', return_value=False, autospec=True)
+    @mock.patch('os.path.isfile', lambda filename: False)
     @mock.patch.object(pxe_utils, 'create_pxe_config', autospec=True)
-    @mock.patch.object(deploy_utils, 'is_iscsi_boot', autospec=True)
+    @mock.patch.object(deploy_utils, 'is_iscsi_boot', lambda task: True)
     @mock.patch.object(noop_storage.NoopStorage, 'should_write_image',
-                       autospec=True)
+                       lambda task: False)
     @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)
     @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True)
     @mock.patch.object(dhcp_factory, 'DHCPFactory', autospec=True)
@@ -1105,13 +1105,10 @@ class PXEBootTestCase(db_base.DbTestCase):
     def test_prepare_instance_netboot_iscsi(
             self, get_image_info_mock, cache_mock,
             dhcp_factory_mock, switch_pxe_config_mock,
-            set_boot_device_mock, should_write_image_mock,
-            is_iscsi_boot_mock, create_pxe_config_mock, isfile_mock):
+            set_boot_device_mock, create_pxe_config_mock):
         http_url = 'http://192.1.2.3:1234'
         self.config(ipxe_enabled=True, group='pxe')
         self.config(http_url=http_url, group='deploy')
-        is_iscsi_boot_mock.return_value = True
-        should_write_image_mock.return_valule = False
         provider_mock = mock.MagicMock()
         dhcp_factory_mock.return_value = provider_mock
         vol_id = uuidutils.generate_uuid()