Merge "Improve baremetal driver error handling"

nova/tests/baremetal/test_driver.py

@@ -159,7 +159,7 @@ class BareMetalDriverWithDBTestCase(bm_db_base.BMDBTestCase):
         db.bm_node_update(self.context, self.node['id'],
                 {'id': 9876})
 
-        self.assertRaises(exception.InstanceNotFound,
+        self.assertRaises(exception.NovaException,
                 self.driver.spawn, **self.spawn_params)
 
         row = db.bm_node_get(self.context, 9876)

nova/tests/utils.py

@@ -81,6 +81,7 @@ def get_test_network_info(count=1):
     fake_ip = '0.0.0.0/0'
     fake_ip_2 = '0.0.0.1/0'
     fake_ip_3 = '0.0.0.1/0'
+    fake_netmask = '255.255.255.255'
     fake_vlan = 100
     fake_bridge_interface = 'eth0'
     network = {'bridge': fake,
@@ -91,11 +92,13 @@ def get_test_network_info(count=1):
                'injected': False}
     mapping = {'mac': fake,
                'dhcp_server': fake,
+               'dns': ['fake1', 'fake2'],
                'gateway': fake,
                'gateway_v6': fake,
-               'ips': [{'ip': fake_ip}, {'ip': fake_ip}]}
+               'ips': [{'ip': fake_ip, 'netmask': fake_netmask},
+                       {'ip': fake_ip, 'netmask': fake_netmask}]}
     if ipv6:
-        mapping['ip6s'] = [{'ip': fake_ip},
+        mapping['ip6s'] = [{'ip': fake_ip, 'netmask': fake_netmask},
                            {'ip': fake_ip_2},
                            {'ip': fake_ip_3}]
     return [(network, mapping) for x in xrange(0, count)]

nova/virt/baremetal/base.py

@@ -21,27 +21,26 @@ from nova.virt.baremetal import baremetal_states
 
 class NodeDriver(object):
 
-    def define_vars(self, instance, network_info, block_device_info):
+    def __init__(self):
+        pass
+
+    def cache_images(self, context, node, instance, **kwargs):
         raise NotImplementedError()
 
-    def create_image(self, var, context, image_meta, node, instance,
+    def destroy_images(self, context, node, instance):
-                     injected_files=None, admin_password=None):
         raise NotImplementedError()
 
-    def destroy_images(self, var, context, node, instance):
+    def activate_bootloader(self, context, node, instance):
         raise NotImplementedError()
 
-    def activate_bootloader(self, var, context, node, instance, image_meta):
+    def deactivate_bootloader(self, context, node, instance):
         raise NotImplementedError()
 
-    def deactivate_bootloader(self, var, context, node, instance):
+    def activate_node(self, context, node, instance):
-        raise NotImplementedError()
-
-    def activate_node(self, var, context, node, instance):
         """For operations after power on."""
         raise NotImplementedError()
 
-    def deactivate_node(self, var, context, node, instance):
+    def deactivate_node(self, context, node, instance):
         """For operations before power off."""
         raise NotImplementedError()
 
@@ -52,16 +51,20 @@ class NodeDriver(object):
 class PowerManager(object):
 
     def __init__(self, **kwargs):
+        self.state = baremetal_states.DELETED
         pass
 
     def activate_node(self):
-        return baremetal_states.ACTIVE
+        self.state = baremetal_states.ACTIVE
+        return self.state
 
     def reboot_node(self):
-        return baremetal_states.ACTIVE
+        self.state = baremetal_states.ACTIVE
+        return self.state
 
     def deactivate_node(self):
-        return baremetal_states.DELETED
+        self.state = baremetal_states.DELETED
+        return self.state
 
     def is_power_on(self):
         """Returns True or False according as the node's power state"""

nova/virt/baremetal/db/api.py

@@ -98,6 +98,10 @@ def bm_node_update(context, bm_node_id, values):
     return IMPL.bm_node_update(context, bm_node_id, values)
 
 
+def bm_node_set_uuid_safe(context, bm_node_id, uuid):
+    return IMPL.bm_node_set_uuid_safe(context, bm_node_id, uuid)
+
+
 def bm_pxe_ip_create(context, address, server_address):
     return IMPL.bm_pxe_ip_create(context, address, server_address)
 

nova/virt/baremetal/db/sqlalchemy/api.py

@@ -150,6 +150,37 @@ def bm_node_update(context, bm_node_id, values):
             update(values)
 
 
+@require_admin_context
+def bm_node_set_uuid_safe(context, bm_node_id, values):
+    """Associate an instance to a node safely
+
+    Associate an instance to a node only if that node is not yet assocated.
+    Allow the caller to set any other fields they require in the same
+    operation. For example, this is used to set the node's task_state to
+    BUILDING at the beginning of driver.spawn().
+
+    """
+    if 'instance_uuid' not in values:
+        raise exception.NovaException(_(
+            "instance_uuid must be supplied to bm_node_set_uuid_safe"))
+
+    session = get_session()
+    with session.begin():
+        query = model_query(context, models.BareMetalNode,
+                                session=session, read_deleted="no").\
+                        filter_by(id=bm_node_id)
+
+        count = query.filter_by(instance_uuid=None).\
+                        update(values, synchronize_session=False)
+        if count != 1:
+            raise exception.NovaException(_(
+                "Failed to associate instance %(uuid)s to baremetal node "
+                "%(id)s.") % {'id': bm_node_id,
+                              'uuid': values['instance_uuid']})
+        ref = query.first()
+    return ref
+
+
 @require_admin_context
 def bm_node_destroy(context, bm_node_id):
     model_query(context, models.BareMetalNode).\

nova/virt/baremetal/driver.py

@@ -56,7 +56,7 @@ opts = [
                default='nova.virt.baremetal.pxe.PXE',
                help='Baremetal driver back-end (pxe or tilera)'),
     cfg.StrOpt('power_manager',
-               default='nova.virt.baremetal.ipmi.Ipmi',
+               default='nova.virt.baremetal.ipmi.IPMI',
                help='Baremetal power management method'),
     cfg.StrOpt('tftp_root',
                default='/tftpboot',
@@ -93,14 +93,16 @@ def _get_baremetal_node_by_instance_uuid(instance_uuid):
     return node
 
 
-def _update_baremetal_state(context, node, instance, state):
+def _update_state(context, node, instance, state):
-    instance_uuid = None
+    """Update the node state in baremetal DB
-    if instance:
+
-        instance_uuid = instance['uuid']
+    If instance is not supplied, reset the instance_uuid field for this node.
-    db.bm_node_update(context, node['id'],
+
-        {'instance_uuid': instance_uuid,
+    """
-        'task_state': state,
+    values = {'task_state': state}
-        })
+    if not instance:
+        values['instance_uuid'] = None
+    db.bm_node_update(context, node['id'], values)
 
 
 def get_power_manager(**kwargs):
@@ -174,57 +176,70 @@ class BareMetalDriver(driver.ComputeDriver):
 
     def spawn(self, context, instance, image_meta, injected_files,
               admin_password, network_info=None, block_device_info=None):
-        nodename = instance.get('node')
-        if not nodename:
-            raise exception.NovaException(_("Baremetal node id not supplied"
-                                            " to driver"))
-        node = db.bm_node_get(context, nodename)
-        if node['instance_uuid']:
-            raise exception.NovaException(_("Baremetal node %s already"
-                                            " in use") % nodename)
 
-        # TODO(deva): split this huge try: block into manageable parts
+        node_id = instance.get('node')
+        if not node_id:
+            raise exception.NovaException(_(
+                    "Baremetal node id not supplied to driver"))
+
+        # NOTE(deva): this db method will raise an exception if the node is
+        #             already in use. We call it here to ensure no one else
+        #             allocates this node before we begin provisioning it.
+        node = db.bm_node_set_uuid_safe(context, node_id,
+                    {'instance_uuid': instance['uuid'],
+                     'task_state': baremetal_states.BUILDING})
+        pm = get_power_manager(node=node, instance=instance)
+
         try:
-            _update_baremetal_state(context, node, instance,
-                                    baremetal_states.BUILDING)
-
-            var = self.driver.define_vars(instance, network_info,
-                                                   block_device_info)
-
             self._plug_vifs(instance, network_info, context=context)
 
-            self.firewall_driver.setup_basic_filtering(instance, network_info)
+            self.firewall_driver.setup_basic_filtering(
-            self.firewall_driver.prepare_instance_filter(instance,
+                    instance, network_info)
-                                                          network_info)
+            self.firewall_driver.prepare_instance_filter(
+                    instance, network_info)
+            self.firewall_driver.apply_instance_filter(
+                    instance, network_info)
 
-            self.driver.create_image(var, context, image_meta, node,
+            block_device_mapping = driver.\
-                                              instance,
+                    block_device_info_get_mapping(block_device_info)
-                                              injected_files=injected_files,
-                                              admin_password=admin_password)
-            self.driver.activate_bootloader(var, context, node,
-                                                     instance, image_meta)
-            pm = get_power_manager(node=node, instance=instance)
-            state = pm.activate_node()
-
-            _update_baremetal_state(context, node, instance, state)
-
-            self.driver.activate_node(var, context, node, instance)
-            self.firewall_driver.apply_instance_filter(instance, network_info)
-
-            block_device_mapping = driver.block_device_info_get_mapping(
-                block_device_info)
             for vol in block_device_mapping:
                 connection_info = vol['connection_info']
                 mountpoint = vol['mount_device']
-                self.attach_volume(connection_info, instance['name'],
+                self.attach_volume(
-                                   mountpoint)
+                        connection_info, instance['name'], mountpoint)
 
-            pm.start_console()
+            try:
+                image_info = self.driver.cache_images(
+                                context, node, instance,
+                                admin_password=admin_password,
+                                image_meta=image_meta,
+                                injected_files=injected_files,
+                                network_info=network_info,
+                            )
+                try:
+                    self.driver.activate_bootloader(context, node, instance)
+                except Exception, e:
+                    self.driver.deactivate_bootloader(context, node, instance)
+                    raise e
+            except Exception, e:
+                self.driver.destroy_images(context, node, instance)
+                raise e
         except Exception, e:
-            # TODO(deva): add tooling that can revert a failed spawn
+            # TODO(deva): do network and volume cleanup here
-            _update_baremetal_state(context, node, instance,
-                                    baremetal_states.ERROR)
             raise e
+        else:
+            # NOTE(deva): pm.activate_node should not raise exceptions.
+            #             We check its success in "finally" block
+            pm.activate_node()
+            pm.start_console()
+        finally:
+            if pm.state != baremetal_states.ACTIVE:
+                pm.state = baremetal_states.ERROR
+            try:
+                _update_state(context, node, instance, pm.state)
+            except exception.DBError, e:
+                LOG.warning(_("Failed to update state record for "
+                              "baremetal node %s") % instance['uuid'])
 
     def reboot(self, instance, network_info, reboot_type,
                block_device_info=None):
@@ -232,7 +247,7 @@ class BareMetalDriver(driver.ComputeDriver):
         ctx = nova_context.get_admin_context()
         pm = get_power_manager(node=node, instance=instance)
         state = pm.reboot_node()
-        _update_baremetal_state(ctx, node, instance, state)
+        _update_state(ctx, node, instance, state)
 
     def destroy(self, instance, network_info, block_device_info=None):
         ctx = nova_context.get_admin_context()
@@ -246,10 +261,7 @@ class BareMetalDriver(driver.ComputeDriver):
                     % instance['uuid'])
             return
 
-        var = self.driver.define_vars(instance, network_info,
+        self.driver.deactivate_node(ctx, node, instance)
-                                               block_device_info)
-
-        self.driver.deactivate_node(var, ctx, node, instance)
 
         pm = get_power_manager(node=node, instance=instance)
 
@@ -267,9 +279,9 @@ class BareMetalDriver(driver.ComputeDriver):
             mountpoint = vol['mount_device']
             self.detach_volume(connection_info, instance['name'], mountpoint)
 
-        self.driver.deactivate_bootloader(var, ctx, node, instance)
+        self.driver.deactivate_bootloader(ctx, node, instance)
 
-        self.driver.destroy_images(var, ctx, node, instance)
+        self.driver.destroy_images(ctx, node, instance)
 
         # stop firewall
         self.firewall_driver.unfilter_instance(instance,
@@ -277,7 +289,7 @@ class BareMetalDriver(driver.ComputeDriver):
 
         self._unplug_vifs(instance, network_info)
 
-        _update_baremetal_state(ctx, node, None, state)
+        _update_state(ctx, node, None, state)
 
     def power_off(self, instance):
         """Power off the specified instance."""

nova/virt/baremetal/fake.py

@@ -21,33 +21,25 @@ from nova.virt.baremetal import base
 from nova.virt.firewall import NoopFirewallDriver
 
 
-def get_baremetal_nodes():
-    return FakeDriver()
-
-
 class FakeDriver(base.NodeDriver):
 
-    def define_vars(self, instance, network_info, block_device_info):
+    def cache_images(self, context, node, instance, **kwargs):
-        return {}
-
-    def create_image(self, var, context, image_meta, node, instance,
-                     injected_files=None, admin_password=None):
         pass
 
-    def destroy_images(self, var, context, node, instance):
+    def destroy_images(self, context, node, instance):
         pass
 
-    def activate_bootloader(self, var, context, node, instance, image_meta):
+    def activate_bootloader(self, context, node, instance):
         pass
 
-    def deactivate_bootloader(self, var, context, node, instance):
+    def deactivate_bootloader(self, context, node, instance):
         pass
 
-    def activate_node(self, var, context, node, instance):
+    def activate_node(self, context, node, instance):
         """For operations after power on."""
         pass
 
-    def deactivate_node(self, var, context, node, instance):
+    def deactivate_node(self, context, node, instance):
         """For operations before power off."""
         pass
 
@@ -57,23 +49,8 @@ class FakeDriver(base.NodeDriver):
 
 class FakePowerManager(base.PowerManager):
 
-    def activate_node(self):
+    def __init__(self, **kwargs):
-        return baremetal_states.ACTIVE
+        super(FakePowerManager, self).__init__(**kwargs)
-
-    def reboot_node(self):
-        return baremetal_states.ACTIVE
-
-    def deactivate_node(self):
-        return baremetal_states.DELETED
-
-    def is_power_on(self):
-        return True
-
-    def start_console(self):
-        pass
-
-    def stop_console(self):
-        pass
 
 
 class FakeFirewallDriver(NoopFirewallDriver):

nova/virt/baremetal/utils.py

@@ -18,9 +18,9 @@
 import os
 
 from nova.openstack.common import log as logging
+from nova.virt.disk import api as disk_api
 from nova.virt.libvirt import utils as libvirt_utils
 
-
 LOG = logging.getLogger(__name__)
 
 
@@ -30,8 +30,31 @@ def cache_image(context, target, image_id, user_id, project_id):
                                   user_id, project_id)
 
 
+def inject_into_image(image, key, net, metadata, admin_password,
+        files, partition, use_cow=False):
+    try:
+        disk_api.inject_data(image, key, net, metadata, admin_password,
+                files, partition, use_cow)
+    except Exception as e:
+        LOG.warn(_("Failed to inject data into image %(image)s. "
+                   "Error: %(e)s") % locals())
+
+
 def unlink_without_raise(path):
     try:
-        libvirt_utils.file_delete(path)
+        os.unlink(path)
     except OSError:
-        LOG.exception(_("failed to unlink %s") % path)
+        LOG.exception(_("Failed to unlink %s") % path)
+
+
+def write_to_file(path, contents):
+    with open(path, 'w') as f:
+        f.write(contents)
+
+
+def create_link_without_raise(source, link):
+    try:
+        os.symlink(source, link)
+    except OSError:
+        LOG.exception(_("Failed to create symlink from %(source)s to %(link)s")
+                % locals())