From d76f12f4f91a01cab447ba0833816852cba873d3 Mon Sep 17 00:00:00 2001 From: bin yu Date: Wed, 16 Mar 2016 02:16:09 +0000 Subject: [PATCH] Fix VirtualBox cannot set boot device when powered on When the VirtualBox VMs is powered on, Ironic cannot set the boot device. As a result, VirtualBox driver cannot finish deploying local harddisk boot VMs and also set and get Virtualbox VMs's boot devices correctly. Solution: We get boot device from set_boot_device and store the target boot device in driver_internal_info. Before next starting up we set the boot device and clean target_boot_device information, if the target_boot_device is None, we just skip setting boot device. For the get_boot_device call, if the VMs is powered off, we call remotebox to get current boot device. otherwise, we return the target boot device from driver_internal_info(if target_boot_device is not None) or we call remotebox to return the current boot device. Returning target boot device when VMs is powered on will avoid the problem that even users set the target boot device while returning the last boot device. Change-Id: Ice489ba642bf093fe7015aa97e6a92717f676118 Closes-Bug: #1554908 --- ironic/drivers/modules/virtualbox.py | 93 +++++++++++++------ .../unit/drivers/modules/test_virtualbox.py | 77 ++++++++++++--- ...ocalboot-not-working-558a3dec72b5116b.yaml | 11 +++ 3 files changed, 139 insertions(+), 42 deletions(-) create mode 100644 releasenotes/notes/fix-virtualbox-localboot-not-working-558a3dec72b5116b.yaml diff --git a/ironic/drivers/modules/virtualbox.py b/ironic/drivers/modules/virtualbox.py index 57044b0bbf..b58319d3c6 100644 --- a/ironic/drivers/modules/virtualbox.py +++ b/ironic/drivers/modules/virtualbox.py @@ -22,6 +22,7 @@ from ironic.common import boot_devices from ironic.common import exception from ironic.common.i18n import _ from ironic.common.i18n import _LE +from ironic.common.i18n import _LW from ironic.common import states from ironic.common import utils from ironic.conductor import task_manager @@ -76,7 +77,6 @@ COMMON_PROPERTIES.update(OPTIONAL_PROPERTIES) def _strip_virtualbox_from_param_name(param_name): - if param_name.startswith('virtualbox_'): return param_name[11:] else: @@ -181,7 +181,6 @@ def _run_virtualbox_method(node, ironic_method, vm_object_method, class VirtualBoxPower(base.PowerInterface): - def get_properties(self): return COMMON_PROPERTIES @@ -196,6 +195,18 @@ class VirtualBoxPower(base.PowerInterface): """ _parse_driver_info(task.node) + def _apply_boot_device(self, task): + """Get the target boot device and apply on the baremetal machine . + + :param task: a TaskManager instance. + """ + driver_internal_info = task.node.driver_internal_info + device = driver_internal_info.pop('vbox_target_boot_device', None) + if device is not None: + task.driver.management.set_boot_device(task, device) + task.node.driver_internal_info = driver_internal_info + task.node.save() + def get_power_state(self, task): """Gets the current power state. @@ -233,9 +244,15 @@ class VirtualBoxPower(base.PowerInterface): :raises: VirtualBoxOperationFailed, if error encountered from VirtualBox operation. """ + + # We set boot device before power on to avoid the case that user + # shuts down the machine without calling power off method here. For + # instance, soft power off the machine from OS. if target_state == states.POWER_OFF: _run_virtualbox_method(task.node, 'set_power_state', 'stop') + self._apply_boot_device(task) elif target_state == states.POWER_ON: + self._apply_boot_device(task) _run_virtualbox_method(task.node, 'set_power_state', 'start') elif target_state == states.REBOOT: self.reboot(task) @@ -257,11 +274,11 @@ class VirtualBoxPower(base.PowerInterface): VirtualBox operation. """ _run_virtualbox_method(task.node, 'reboot', 'stop') + self._apply_boot_device(task) _run_virtualbox_method(task.node, 'reboot', 'start') class VirtualBoxManagement(base.ManagementInterface): - def get_properties(self): return COMMON_PROPERTIES @@ -288,6 +305,18 @@ class VirtualBoxManagement(base.ManagementInterface): """ return list(IRONIC_TO_VIRTUALBOX_DEVICE_MAPPING.keys()) + def _get_boot_device_from_hardware(self, task): + boot_dev = _run_virtualbox_method(task.node, + 'get_boot_device', 'get_boot_device') + ironic_boot_dev = VIRTUALBOX_TO_IRONIC_DEVICE_MAPPING.get(boot_dev) + persistent = True + if not ironic_boot_dev: + persistent = None + msg = _LW("VirtualBox returned unknown boot " + "device '%(device)s' for node %(node)s") + LOG.warning(msg, {'device': boot_dev, 'node': task.node.uuid}) + return (ironic_boot_dev, persistent) + def get_boot_device(self, task): """Get the current boot device for a node. @@ -302,17 +331,23 @@ class VirtualBoxManagement(base.ManagementInterface): :raises: VirtualBoxOperationFailed, if error encountered from VirtualBox operation. """ - boot_dev = _run_virtualbox_method(task.node, 'get_boot_device', - 'get_boot_device') - persistent = True - ironic_boot_dev = VIRTUALBOX_TO_IRONIC_DEVICE_MAPPING.get(boot_dev, - None) - if not ironic_boot_dev: - persistent = None - msg = _LE("VirtualBox returned unknown boot device '%(device)s' " - "for node %(node)s") - LOG.error(msg, {'device': boot_dev, 'node': task.node.uuid}) - + if task.driver.power.get_power_state(task) == states.POWER_OFF: + ironic_boot_dev, persistent = \ + self._get_boot_device_from_hardware(task) + else: + ironic_boot_dev = task.node. \ + driver_internal_info.get('vbox_target_boot_device') + if ironic_boot_dev is not None: + msg = _LW("As ironic node %(node)s is" + " powered on, we will set to boot" + " from %(device)s before next boot.") + LOG.warning(msg, {'node': task.node.uuid, + 'device': ironic_boot_dev}) + persistent = True + else: + # Maybe the vbox_target_boot_device is cleaned + ironic_boot_dev, persistent = \ + self._get_boot_device_from_hardware(task) return {'boot_device': ironic_boot_dev, 'persistent': persistent} @task_manager.require_exclusive_lock @@ -337,22 +372,24 @@ class VirtualBoxManagement(base.ManagementInterface): raise exception.InvalidParameterValue( _("Invalid boot device %s specified.") % device) - try: + if task.driver.power.get_power_state(task) == states.POWER_OFF: + _run_virtualbox_method(task.node, 'set_boot_device', 'set_boot_device', boot_dev) - except virtualbox_exc.VmInWrongPowerState as exc: - # NOTE(rameshg87): We cannot change the boot device when the vm - # is powered on. This is a VirtualBox limitation. We just log - # the error silently and return because throwing error will cause - # deploys to fail (pxe and agent deploy mechanisms change the boot - # device after completing the deployment, when node is powered on). - # Since this is driver that is meant only for developers, this - # should be okay. Developers will need to set the boot device - # manually after powering off the vm when deployment is complete. - # This will be documented. - LOG.error(_LE("'set_boot_device' failed for node %(node_id)s " - "with error: %(error)s"), - {'node_id': task.node.uuid, 'error': exc}) + else: + LOG.warning(_LW('Node %(node_uuid)s: As VirtualBox do not support ' + 'setting boot device when VM is powered on, we ' + 'will set booting from %(device)s when reboot ' + 'next time.'), + {'node_uuid': task.node.uuid, 'device': device}) + # We should store target boot device in case the + # end user shutoff the baremetal machine from NOVA API. + boot_device_now = self.get_boot_device(task)['boot_device'] + if device != boot_device_now: + driver_internal_info = task.node.driver_internal_info + driver_internal_info['vbox_target_boot_device'] = device + task.node.driver_internal_info = driver_internal_info + task.node.save() def get_sensors_data(self, task): """Get sensors data. diff --git a/ironic/tests/unit/drivers/modules/test_virtualbox.py b/ironic/tests/unit/drivers/modules/test_virtualbox.py index a6ec5b4313..15b195fa14 100644 --- a/ironic/tests/unit/drivers/modules/test_virtualbox.py +++ b/ironic/tests/unit/drivers/modules/test_virtualbox.py @@ -243,23 +243,30 @@ class VirtualBoxPowerTestCase(db_base.DbTestCase): 'set_power_state', 'stop') + @mock.patch.object(virtualbox.VirtualBoxManagement, 'set_boot_device') @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) - def test_set_power_state_on(self, run_method_mock): + def test_set_power_state_on(self, run_method_mock, set_boot_device_mock): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: + task.node.driver_internal_info['vbox_target_boot_device'] = 'pxe' task.driver.power.set_power_state(task, states.POWER_ON) run_method_mock.assert_called_once_with(task.node, 'set_power_state', 'start') + set_boot_device_mock.assert_called_once_with(task, 'pxe') + @mock.patch.object(virtualbox.VirtualBoxManagement, 'set_boot_device') @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) - def test_set_power_state_reboot(self, run_method_mock): + def test_set_power_state_reboot(self, run_method_mock, + mock_set_boot_device): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: + task.node.driver_internal_info['vbox_target_boot_device'] = 'pxe' task.driver.power.set_power_state(task, states.REBOOT) run_method_mock.assert_any_call(task.node, 'reboot', 'stop') + mock_set_boot_device.assert_called_once_with(task, 'pxe') run_method_mock.assert_any_call(task.node, 'reboot', 'start') @@ -317,9 +324,14 @@ class VirtualBoxManagementTestCase(db_base.DbTestCase): self.assertIn(boot_devices.DISK, devices) self.assertIn(boot_devices.CDROM, devices) - @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) - def test_get_boot_device_ok(self, run_method_mock): + @mock.patch.object(virtualbox.VirtualBoxPower, + 'get_power_state', autospec=True) + @mock.patch.object(virtualbox, '_run_virtualbox_method', + autospec=True) + def test_get_boot_device_VM_Poweroff_ok(self, run_method_mock, + get_power_state_mock): run_method_mock.return_value = 'Network' + get_power_state_mock.return_value = states.POWER_OFF with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: ret_val = task.driver.management.get_boot_device(task) @@ -329,6 +341,36 @@ class VirtualBoxManagementTestCase(db_base.DbTestCase): self.assertEqual(boot_devices.PXE, ret_val['boot_device']) self.assertTrue(ret_val['persistent']) + @mock.patch.object(virtualbox.VirtualBoxPower, + 'get_power_state', autospec=True) + def test_get_boot_device_VM_Poweron_ok(self, get_power_state_mock): + get_power_state_mock.return_value = states.POWER_ON + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.node.driver_internal_info['vbox_target_boot_device'] = 'pxe' + ret_val = task.driver.management.get_boot_device(task) + self.assertEqual(boot_devices.PXE, ret_val['boot_device']) + self.assertTrue(ret_val['persistent']) + + @mock.patch.object(virtualbox.VirtualBoxPower, + 'get_power_state', autospec=True) + @mock.patch.object(virtualbox, '_run_virtualbox_method', + autospec=True) + def test_get_boot_device_target_device_none_ok(self, + run_method_mock, + get_power_state_mock): + run_method_mock.return_value = 'Network' + get_power_state_mock.return_value = states.POWER_ON + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.node.driver_internal_info['vbox_target_boot_device'] = None + ret_val = task.driver.management.get_boot_device(task) + run_method_mock.assert_called_once_with(task.node, + 'get_boot_device', + 'get_boot_device') + self.assertEqual(boot_devices.PXE, ret_val['boot_device']) + self.assertTrue(ret_val['persistent']) + @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) def test_get_boot_device_invalid(self, run_method_mock): run_method_mock.return_value = 'invalid-boot-device' @@ -338,25 +380,32 @@ class VirtualBoxManagementTestCase(db_base.DbTestCase): self.assertIsNone(ret_val['boot_device']) self.assertIsNone(ret_val['persistent']) + @mock.patch.object(virtualbox.VirtualBoxPower, + 'get_power_state', autospec=True) @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) - def test_set_boot_device_ok(self, run_method_mock): + def test_set_boot_device_VM_Poweroff_ok(self, run_method_mock, + get_power_state_mock): + get_power_state_mock.return_value = states.POWER_OFF with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.management.set_boot_device(task, boot_devices.PXE) - run_method_mock.assert_called_once_with(task.node, - 'set_boot_device', - 'set_boot_device', - 'Network') + run_method_mock.assert_called_with(task.node, + 'set_boot_device', + 'set_boot_device', + 'Network') - @mock.patch.object(virtualbox, 'LOG', autospec=True) + @mock.patch.object(virtualbox.VirtualBoxPower, + 'get_power_state', autospec=True) @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) - def test_set_boot_device_wrong_power_state(self, run_method_mock, - log_mock): - run_method_mock.side_effect = pyremotevbox_exc.VmInWrongPowerState + def test_set_boot_device_VM_Poweron_ok(self, run_method_mock, + get_power_state_mock): + get_power_state_mock.return_value = states.POWER_ON with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: task.driver.management.set_boot_device(task, boot_devices.PXE) - log_mock.error.assert_called_once_with(mock.ANY, mock.ANY) + self.assertEqual('pxe', + task.node.driver_internal_info + ['vbox_target_boot_device']) @mock.patch.object(virtualbox, '_run_virtualbox_method', autospec=True) def test_set_boot_device_invalid(self, run_method_mock): diff --git a/releasenotes/notes/fix-virtualbox-localboot-not-working-558a3dec72b5116b.yaml b/releasenotes/notes/fix-virtualbox-localboot-not-working-558a3dec72b5116b.yaml new file mode 100644 index 0000000000..cae99ef01d --- /dev/null +++ b/releasenotes/notes/fix-virtualbox-localboot-not-working-558a3dec72b5116b.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - Fixed a VirtualBox issue that Ironic fails + to set VirtualBox VM's boot device + when it is powered on. This bug causes + two problems 1. VirtualBox cannot + deploy VMs in local boot mode. 2. Ironic + fails to set boot device when VirtualBox + VMs is powered on and also fails to get + the correct boot device from Ironic + API call when VMs is powered on.