Remove mode argument from boot.(prepare|clean_up)_ramdisk

Ideally, the boot interface shouldn't care if it's booting an image for
deploy or rescue. The first step to unwinding this is not passing the
mode argument into the boot interface - for now, we infer it from the
state. Also stop validating whether the boot interface methods have a
mode argument, as this is totally broken anyway (due to the decorator on
the method). The rest of the boot interface's knowledge about deploy vs
rescue can be eliminated in the future, as we shouldn't rework too much
of that during feature freeze.

We fix the bug in validation this way for now, for two reasons:

* This argument really doesn't belong, and it's only been on master for
  six days. Get it out before people use it.
* Library updates are currently frozen; fixing the decorator isn't an
  option right now.

Change-Id: Icdeb870e9fd9cf836ff7ab97624189c8436adaba
Closes-Bug: #1746730

ironic/drivers/base.py

@@ -433,7 +433,7 @@ class BootInterface(BaseInterface):
     interface_type = 'boot'
 
     @abc.abstractmethod
-    def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'):
+    def prepare_ramdisk(self, task, ramdisk_params):
         """Prepares the boot of Ironic ramdisk.
 
         This method prepares the boot of the deploy or rescue ramdisk after
@@ -450,25 +450,17 @@ class BootInterface(BaseInterface):
             Other implementations can make use of ramdisk_params to pass such
             information.  Different implementations of boot interface will
             have different ways of passing parameters to the ramdisk.
-        :param mode: Label indicating a deploy or rescue operation
-            being carried out on the node. Supported values are 'deploy' and
-            'rescue'. Defaults to 'deploy', indicating deploy operation is
-            being carried out.
         :returns: None
         """
 
     @abc.abstractmethod
-    def clean_up_ramdisk(self, task, mode='deploy'):
+    def clean_up_ramdisk(self, task):
         """Cleans up the boot of ironic ramdisk.
 
         This method cleans up the environment that was setup for booting the
         deploy or rescue ramdisk.
 
         :param task: a task from TaskManager.
-        :param mode: Label indicating a deploy or rescue operation
-            was carried out on the node. Supported values are 'deploy' and
-            'rescue'. Defaults to 'deploy', indicating deploy operation was
-            carried out.
         :returns: None
         """
 

ironic/drivers/modules/agent.py

@@ -15,7 +15,6 @@
 from ironic_lib import metrics_utils
 from ironic_lib import utils as il_utils
 from oslo_log import log
-from oslo_utils import reflection
 from oslo_utils import units
 import six.moves.urllib_parse as urlparse
 
@@ -738,7 +737,7 @@ class AgentRescue(base.RescueInterface):
         if CONF.agent.manage_agent_boot:
             ramdisk_opts = deploy_utils.build_agent_options(task.node)
             # prepare_ramdisk will set the boot device
-            task.driver.boot.prepare_ramdisk(task, ramdisk_opts, mode='rescue')
+            task.driver.boot.prepare_ramdisk(task, ramdisk_opts)
         manager_utils.node_power_action(task, states.POWER_ON)
 
         return states.RESCUEWAIT
@@ -775,9 +774,6 @@ class AgentRescue(base.RescueInterface):
             has an invalid value when 'neutron' network is used.
         :raises: MissingParameterValue if node is missing one or more required
             parameters
-        :raises: IncompatibleInterface if 'prepare_ramdisk' and
-            'clean_up_ramdisk' of node's boot interface do not support 'mode'
-            argument.
         """
         node = task.node
         missing_params = []
@@ -787,15 +783,6 @@ class AgentRescue(base.RescueInterface):
             task.driver.network.get_rescuing_network_uuid(task)
 
         if CONF.agent.manage_agent_boot:
-            if ('mode' not in reflection.get_signature(
-                task.driver.boot.prepare_ramdisk).parameters or
-                'mode' not in reflection.get_signature(
-                task.driver.boot.clean_up_ramdisk).parameters):
-                raise exception.IncompatibleInterface(
-                    interface_type='boot',
-                    interface_impl="of 'prepare_ramdisk' and/or "
-                                   "'clean_up_ramdisk' with 'mode' argument",
-                    hardware_type=node.driver)
             # TODO(stendulker): boot.validate() performs validation of
             # provisioning related parameters which is not required during
             # rescue operation.
@@ -833,5 +820,5 @@ class AgentRescue(base.RescueInterface):
         """
         manager_utils.remove_node_rescue_password(task.node, save=True)
         if CONF.agent.manage_agent_boot:
-            task.driver.boot.clean_up_ramdisk(task, mode='rescue')
+            task.driver.boot.clean_up_ramdisk(task)
         task.driver.network.remove_rescuing_network(task)

ironic/drivers/modules/deploy_utils.py

@@ -124,6 +124,11 @@ def get_ironic_api_url():
     return ironic_api
 
 
+def rescue_or_deploy_mode(node):
+    return ('rescue' if node.provision_state in RESCUE_LIKE_STATES
+            else 'deploy')
+
+
 def discovery(portal_address, portal_port):
     """Do iSCSI discovery on portal."""
     utils.execute('iscsiadm',

ironic/drivers/modules/ilo/boot.py

@@ -603,7 +603,7 @@ class IloVirtualMediaBoot(base.BootInterface):
 class IloPXEBoot(pxe.PXEBoot):
 
     @METRICS.timer('IloPXEBoot.prepare_ramdisk')
-    def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'):
+    def prepare_ramdisk(self, task, ramdisk_params):
         """Prepares the boot of Ironic ramdisk using PXE.
 
         This method prepares the boot of the deploy or rescue ramdisk after
@@ -612,10 +612,6 @@ class IloPXEBoot(pxe.PXEBoot):
 
         :param task: a task from TaskManager.
         :param ramdisk_params: the parameters to be passed to the ramdisk.
-        :param mode: Label indicating a deploy or rescue operation
-            being carried out on the node. Supported values are
-            'deploy' and 'rescue'. Defaults to 'deploy', indicating
-            deploy operation is being carried out.
         :returns: None
         :raises: MissingParameterValue, if some information is missing in
             node's driver_info or instance_info.
@@ -629,8 +625,7 @@ class IloPXEBoot(pxe.PXEBoot):
         if task.node.provision_state in (states.DEPLOYING, states.RESCUING):
             prepare_node_for_deploy(task)
 
-        super(IloPXEBoot, self).prepare_ramdisk(task, ramdisk_params,
+        super(IloPXEBoot, self).prepare_ramdisk(task, ramdisk_params)
-                                                mode=mode)
 
     @METRICS.timer('IloPXEBoot.prepare_instance')
     def prepare_instance(self, task):

ironic/drivers/modules/irmc/boot.py

@@ -1040,7 +1040,7 @@ class IRMCPXEBoot(pxe.PXEBoot):
     """iRMC PXE boot."""
 
     @METRICS.timer('IRMCPXEBoot.prepare_ramdisk')
-    def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'):
+    def prepare_ramdisk(self, task, ramdisk_params):
         """Prepares the boot of Ironic ramdisk using PXE.
 
         This method prepares the boot of the deploy kernel/ramdisk after
@@ -1051,10 +1051,6 @@ class IRMCPXEBoot(pxe.PXEBoot):
         :param ramdisk_params: the parameters to be passed to the ramdisk.
             pxe driver passes these parameters as kernel command-line
             arguments.
-        :param mode: Label indicating a deploy or rescue operation
-            being carried out on the node. Supported values are
-            'deploy' and 'rescue'. Defaults to 'deploy', indicating
-            deploy operation is being carried out.
         :returns: None
         :raises: MissingParameterValue, if some information is missing in
             node's driver_info or instance_info.
@@ -1068,8 +1064,7 @@ class IRMCPXEBoot(pxe.PXEBoot):
         if task.node.provision_state == states.DEPLOYING:
             irmc_management.backup_bios_config(task)
 
-        super(IRMCPXEBoot, self).prepare_ramdisk(task, ramdisk_params,
+        super(IRMCPXEBoot, self).prepare_ramdisk(task, ramdisk_params)
-                                                 mode=mode)
 
     @METRICS.timer('IRMCPXEBoot.prepare_instance')
     def prepare_instance(self, task):

ironic/drivers/modules/pxe.py

@@ -234,8 +234,7 @@ def _build_pxe_config_options(task, pxe_info, service=False):
         template.
     """
     node = task.node
-    mode = ('rescue' if node.provision_state in deploy_utils.RESCUE_LIKE_STATES
+    mode = deploy_utils.rescue_or_deploy_mode(node)
-            else 'deploy')
     if service:
         pxe_options = {}
     elif (node.driver_internal_info.get('boot_from_volume') and
@@ -469,7 +468,7 @@ class PXEBoot(base.BootInterface):
         deploy_utils.validate_image_properties(task.context, d_info, props)
 
     @METRICS.timer('PXEBoot.prepare_ramdisk')
-    def prepare_ramdisk(self, task, ramdisk_params, mode='deploy'):
+    def prepare_ramdisk(self, task, ramdisk_params):
         """Prepares the boot of Ironic ramdisk using PXE.
 
         This method prepares the boot of the deploy or rescue kernel/ramdisk
@@ -493,6 +492,7 @@ class PXEBoot(base.BootInterface):
             operation failed on the node.
         """
         node = task.node
+        mode = deploy_utils.rescue_or_deploy_mode(node)
 
         if CONF.pxe.ipxe_enabled:
             # NOTE(mjturek): At this point, the ipxe boot script should
@@ -536,7 +536,7 @@ class PXEBoot(base.BootInterface):
             _cache_ramdisk_kernel(task.context, node, pxe_info)
 
     @METRICS.timer('PXEBoot.clean_up_ramdisk')
-    def clean_up_ramdisk(self, task, mode='deploy'):
+    def clean_up_ramdisk(self, task):
         """Cleans up the boot of ironic ramdisk.
 
         This method cleans up the PXE environment that was setup for booting
@@ -552,6 +552,7 @@ class PXEBoot(base.BootInterface):
         :returns: None
         """
         node = task.node
+        mode = deploy_utils.rescue_or_deploy_mode(node)
         try:
             images_info = _get_image_info(node, mode=mode)
         except exception.MissingParameterValue as e:

ironic/tests/unit/drivers/modules/ilo/test_boot.py

@@ -1102,33 +1102,30 @@ class IloPXEBootTestCase(db_base.DbTestCase):
 
             self.assertFalse(prepare_node_mock.called)
             pxe_prepare_ramdisk_mock.assert_called_once_with(
-                mock.ANY, task, None, mode='deploy')
+                mock.ANY, task, None)
 
     @mock.patch.object(ilo_boot, 'prepare_node_for_deploy', spec_set=True,
                        autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_ramdisk', spec_set=True,
                        autospec=True)
     def _test_prepare_ramdisk_needs_node_prep(self, pxe_prepare_ramdisk_mock,
-                                              prepare_node_mock, prov_state,
+                                              prepare_node_mock, prov_state):
-                                              mode):
         self.node.provision_state = prov_state
         self.node.save()
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=False) as task:
             self.assertIsNone(
-                task.driver.boot.prepare_ramdisk(task, None, mode=mode))
+                task.driver.boot.prepare_ramdisk(task, None))
 
             prepare_node_mock.assert_called_once_with(task)
             pxe_prepare_ramdisk_mock.assert_called_once_with(
-                mock.ANY, task, None, mode=mode)
+                mock.ANY, task, None)
 
     def test_prepare_ramdisk_in_deploying(self):
-        self._test_prepare_ramdisk_needs_node_prep(prov_state=states.DEPLOYING,
+        self._test_prepare_ramdisk_needs_node_prep(prov_state=states.DEPLOYING)
-                                                   mode='deploy')
 
     def test_prepare_ramdisk_in_rescuing(self):
-        self._test_prepare_ramdisk_needs_node_prep(prov_state=states.RESCUING,
+        self._test_prepare_ramdisk_needs_node_prep(prov_state=states.RESCUING)
-                                                   mode='rescue')
 
     @mock.patch.object(deploy_utils, 'is_iscsi_boot',
                        spec_set=True, autospec=True)

ironic/tests/unit/drivers/modules/irmc/test_boot.py

@@ -1230,7 +1230,7 @@ class IRMCPXEBootTestCase(db_base.DbTestCase):
             task.driver.boot.prepare_ramdisk(task, {})
             mock_backup_bios.assert_called_once_with(task)
             mock_parent_prepare.assert_called_once_with(
-                task.driver.boot, task, {}, mode='deploy')
+                task.driver.boot, task, {})
 
     @mock.patch.object(irmc_management, 'backup_bios_config', spec_set=True,
                        autospec=True)
@@ -1245,7 +1245,7 @@ class IRMCPXEBootTestCase(db_base.DbTestCase):
             task.driver.boot.prepare_ramdisk(task, {})
             self.assertFalse(mock_backup_bios.called)
             mock_parent_prepare.assert_called_once_with(
-                task.driver.boot, task, {}, mode='deploy')
+                task.driver.boot, task, {})
 
     @mock.patch.object(irmc_common, 'set_secure_boot_mode', spec_set=True,
                        autospec=True)

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

@@ -16,7 +16,6 @@ import types
 
 import mock
 from oslo_config import cfg
-from oslo_utils import reflection
 
 from ironic.common import dhcp_factory
 from ironic.common import exception
@@ -1281,7 +1280,7 @@ class AgentRescueTestCase(db_base.DbTestCase):
             mock_add_rescue_net.assert_called_once_with(mock.ANY, task)
             mock_build_agent_opts.assert_called_once_with(task.node)
             mock_prepare_ramdisk.assert_called_once_with(
-                mock.ANY, task, {'ipa-api-url': 'fake-api'}, mode='rescue')
+                mock.ANY, task, {'ipa-api-url': 'fake-api'})
             self.assertEqual(states.RESCUEWAIT, result)
 
     @mock.patch.object(flat_network.FlatNetwork, 'add_rescuing_network',
@@ -1329,7 +1328,7 @@ class AgentRescueTestCase(db_base.DbTestCase):
                 [mock.call(task, states.POWER_OFF),
                  mock.call(task, states.POWER_ON)])
             mock_clean_ramdisk.assert_called_once_with(
-                mock.ANY, task, mode='rescue')
+                mock.ANY, task)
             mock_remove_rescue_net.assert_called_once_with(mock.ANY, task)
             mock_conf_tenant_net.assert_called_once_with(mock.ANY, task)
             mock_prepare_instance.assert_called_once_with(mock.ANY, task)
@@ -1463,20 +1462,6 @@ class AgentRescueTestCase(db_base.DbTestCase):
             self.assertFalse(mock_rescuing_net.called)
             mock_boot_validate.assert_called_once_with(mock.ANY, task)
 
-    @mock.patch('ironic.drivers.modules.network.neutron.NeutronNetwork.'
-                'get_rescuing_network_uuid', autospec=True)
-    @mock.patch.object(reflection, 'get_signature', autospec=True)
-    @mock.patch.object(fake.FakeBoot, 'validate', autospec=True)
-    def test_agent_rescue_validate_incompat_exc(self, mock_boot_validate,
-                                                mock_get_signature,
-                                                mock_rescuing_net):
-        mock_get_signature.return_value.parameters = ['task']
-        with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.assertRaises(exception.IncompatibleInterface,
-                              task.driver.rescue.validate, task)
-            self.assertFalse(mock_rescuing_net.called)
-            self.assertFalse(mock_boot_validate.called)
-
     @mock.patch.object(flat_network.FlatNetwork, 'remove_rescuing_network',
                        spec_set=True, autospec=True)
     @mock.patch.object(fake.FakeBoot, 'clean_up_ramdisk', autospec=True)
@@ -1486,7 +1471,7 @@ class AgentRescueTestCase(db_base.DbTestCase):
             task.driver.rescue.clean_up(task)
             self.assertNotIn('rescue_password', task.node.instance_info)
             mock_clean_ramdisk.assert_called_once_with(
-                mock.ANY, task, mode='rescue')
+                mock.ANY, task)
             mock_remove_rescue_net.assert_called_once_with(mock.ANY, task)
 
     @mock.patch.object(flat_network.FlatNetwork, 'remove_rescuing_network',

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

@@ -916,11 +916,10 @@ class PXEBootTestCase(db_base.DbTestCase):
             mock_deploy_img_info.return_value = {
                 'rescue_kernel': 'a',
                 'rescue_ramdisk': 'r'}
-            self.node.provision_state = states.RESCUING
         self.node.save()
         with task_manager.acquire(self.context, self.node.uuid) as task:
             dhcp_opts = pxe_utils.dhcp_options_for_instance(task)
-            task.driver.boot.prepare_ramdisk(task, {'foo': 'bar'}, mode=mode)
+            task.driver.boot.prepare_ramdisk(task, {'foo': 'bar'})
             mock_deploy_img_info.assert_called_once_with(task.node, mode=mode)
             provider_mock.update_dhcp.assert_called_once_with(task, dhcp_opts)
             set_boot_device_mock.assert_called_once_with(task,
@@ -1078,14 +1077,18 @@ class PXEBootTestCase(db_base.DbTestCase):
             image_info = {kernel_label: ['', '/path/to/' + kernel_label],
                           ramdisk_label: ['', '/path/to/' + ramdisk_label]}
             get_image_info_mock.return_value = image_info
-            task.driver.boot.clean_up_ramdisk(task, mode=mode)
+            task.driver.boot.clean_up_ramdisk(task)
             clean_up_pxe_env_mock.assert_called_once_with(task, image_info)
             get_image_info_mock.assert_called_once_with(task.node, mode=mode)
 
     def test_clean_up_ramdisk(self):
+        self.node.provision_state = states.DEPLOYING
+        self.node.save()
         self._test_clean_up_ramdisk()
 
     def test_clean_up_ramdisk_rescue(self):
+        self.node.provision_state = states.RESCUING
+        self.node.save()
         self._test_clean_up_ramdisk(mode='rescue')
 
     @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True)