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

@@ -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',
+        if task.driver.power.get_power_state(task) == states.POWER_OFF:
-                                          'get_boot_device')
+            ironic_boot_dev, persistent = \
-        persistent = True
+                self._get_boot_device_from_hardware(task)
-        ironic_boot_dev = VIRTUALBOX_TO_IRONIC_DEVICE_MAPPING.get(boot_dev,
+        else:
-                                                                  None)
+            ironic_boot_dev = task.node. \
-        if not ironic_boot_dev:
+                driver_internal_info.get('vbox_target_boot_device')
-            persistent = None
+            if ironic_boot_dev is not None:
-            msg = _LE("VirtualBox returned unknown boot device '%(device)s' "
+                msg = _LW("As ironic node %(node)s is"
-                      "for node %(node)s")
+                          " powered on, we will set to boot"
-            LOG.error(msg, {'device': boot_dev, 'node': task.node.uuid})
+                          " 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:
+        else:
-            # NOTE(rameshg87): We cannot change the boot device when the vm
+            LOG.warning(_LW('Node %(node_uuid)s: As VirtualBox do not support '
-            # is powered on. This is a VirtualBox limitation. We just log
+                            'setting boot device when VM is powered on, we '
-            # the error silently and return because throwing error will cause
+                            'will set booting from %(device)s when reboot '
-            # deploys to fail (pxe and agent deploy mechanisms change the boot
+                            'next time.'),
-            # device after completing the deployment, when node is powered on).
+                        {'node_uuid': task.node.uuid, 'device': device})
-            # Since this is driver that is meant only for developers, this
+            # We should store target boot device in case the
-            # should be okay. Developers will need to set the boot device
+            # end user shutoff the baremetal machine from NOVA API.
-            # manually after powering off the vm when deployment is complete.
+            boot_device_now = self.get_boot_device(task)['boot_device']
-            # This will be documented.
+            if device != boot_device_now:
-            LOG.error(_LE("'set_boot_device' failed for node %(node_id)s "
+                driver_internal_info = task.node.driver_internal_info
-                          "with error: %(error)s"),
+                driver_internal_info['vbox_target_boot_device'] = device
-                      {'node_id': task.node.uuid, 'error': exc})
+                task.node.driver_internal_info = driver_internal_info
+                task.node.save()
 
     def get_sensors_data(self, task):
         """Get sensors data.

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)
+    @mock.patch.object(virtualbox.VirtualBoxPower,
-    def test_get_boot_device_ok(self, run_method_mock):
+                       '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,
+            run_method_mock.assert_called_with(task.node,
-                                                    'set_boot_device',
+                                               'set_boot_device',
-                                                    'set_boot_device',
+                                               'set_boot_device',
-                                                    'Network')
+                                               '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,
+    def test_set_boot_device_VM_Poweron_ok(self, run_method_mock,
-                                               log_mock):
+                                           get_power_state_mock):
-        run_method_mock.side_effect = pyremotevbox_exc.VmInWrongPowerState
+        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):

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.