Merge "Allow nova to guess device if not passed to attach"

13 files changed, 298 insertions, 65 deletions

diff --git a/nova/api/metadata/base.py b/nova/api/metadata/base.py
index aa18eceb0..d9710dc37 100644
--- a/nova/api/metadata/base.py
+++ b/nova/api/metadata/base.py
@@ -45,11 +45,6 @@ flags.DECLARE('dhcp_domain', 'nova.network.manager')
 FLAGS.register_opts(metadata_opts)
 
 
-_DEFAULT_MAPPINGS = {'ami': 'sda1',
-                     'ephemeral0': 'sda2',
-                     'root': block_device.DEFAULT_ROOT_DEV_NAME,
-                     'swap': 'sda3'}
-
 VERSIONS = [
     '1.0',
     '2007-01-19',
@@ -387,50 +382,8 @@ def get_metadata_by_address(address):
 
 
 def _format_instance_mapping(ctxt, instance):
-    root_device_name = instance['root_device_name']
-    if root_device_name is None:
-        return _DEFAULT_MAPPINGS
-
-    mappings = {}
-    mappings['ami'] = block_device.strip_dev(root_device_name)
-    mappings['root'] = root_device_name
-    default_ephemeral_device = instance.get('default_ephemeral_device')
-    if default_ephemeral_device:
-        mappings['ephemeral0'] = default_ephemeral_device
-    default_swap_device = instance.get('default_swap_device')
-    if default_swap_device:
-        mappings['swap'] = default_swap_device
-    ebs_devices = []
-
-    # 'ephemeralN', 'swap' and ebs
-    for bdm in db.block_device_mapping_get_all_by_instance(
-        ctxt, instance['uuid']):
-        if bdm['no_device']:
-            continue
-
-        # ebs volume case
-        if (bdm['volume_id'] or bdm['snapshot_id']):
-            ebs_devices.append(bdm['device_name'])
-            continue
-
-        virtual_name = bdm['virtual_name']
-        if not virtual_name:
-            continue
-
-        if block_device.is_swap_or_ephemeral(virtual_name):
-            mappings[virtual_name] = bdm['device_name']
-
-    # NOTE(yamahata): I'm not sure how ebs device should be numbered.
-    #                 Right now sort by device name for deterministic
-    #                 result.
-    if ebs_devices:
-        nebs = 0
-        ebs_devices.sort()
-        for ebs in ebs_devices:
-            mappings['ebs%d' % nebs] = ebs
-            nebs += 1
-
-    return mappings
+    bdms = db.block_device_mapping_get_all_by_instance(ctxt, instance['uuid'])
+    return block_device.instance_block_mapping(instance, bdms)
 
 
 def ec2_md_print(data):
diff --git a/nova/api/openstack/compute/contrib/volumes.py b/nova/api/openstack/compute/contrib/volumes.py
index e566a95f7..99d713cef 100644
--- a/nova/api/openstack/compute/contrib/volumes.py
+++ b/nova/api/openstack/compute/contrib/volumes.py
@@ -339,7 +339,7 @@ class VolumeAttachmentController(object):
             raise exc.HTTPUnprocessableEntity()
 
         volume_id = body['volumeAttachment']['volumeId']
-        device = body['volumeAttachment']['device']
+        device = body['volumeAttachment'].get('device')
 
         msg = _("Attach volume %(volume_id)s to instance %(server_id)s"
                 " at %(device)s") % locals()
@@ -347,15 +347,17 @@ class VolumeAttachmentController(object):
 
         try:
             instance = self.compute_api.get(context, server_id)
-            self.compute_api.attach_volume(context, instance,
-                                           volume_id, device)
+            device = self.compute_api.attach_volume(context, instance,
+                                                    volume_id, device)
         except exception.NotFound:
             raise exc.HTTPNotFound()
 
         # The attach is async
         attachment = {}
         attachment['id'] = volume_id
+        attachment['serverId'] = server_id
         attachment['volumeId'] = volume_id
+        attachment['device'] = device
 
         # NOTE(justinsb): And now, we have a problem...
         # The attach is async, so there's a window in which we don't see
diff --git a/nova/block_device.py b/nova/block_device.py
index aec981933..fbb935d7c 100644
--- a/nova/block_device.py
+++ b/nova/block_device.py
@@ -19,6 +19,10 @@ import re
 
 
 DEFAULT_ROOT_DEV_NAME = '/dev/sda1'
+_DEFAULT_MAPPINGS = {'ami': 'sda1',
+                     'ephemeral0': 'sda2',
+                     'root': DEFAULT_ROOT_DEV_NAME,
+                     'swap': 'sda3'}
 
 
 def properties_root_device_name(properties):
@@ -81,3 +85,49 @@ def strip_prefix(device_name):
     """ remove both leading /dev/ and xvd or sd or vd """
     device_name = strip_dev(device_name)
     return _pref.sub('', device_name)
+
+
+def instance_block_mapping(instance, bdms):
+    root_device_name = instance['root_device_name']
+    if root_device_name is None:
+        return _DEFAULT_MAPPINGS
+
+    mappings = {}
+    mappings['ami'] = strip_dev(root_device_name)
+    mappings['root'] = root_device_name
+    default_ephemeral_device = instance.get('default_ephemeral_device')
+    if default_ephemeral_device:
+        mappings['ephemeral0'] = default_ephemeral_device
+    default_swap_device = instance.get('default_swap_device')
+    if default_swap_device:
+        mappings['swap'] = default_swap_device
+    ebs_devices = []
+
+    # 'ephemeralN', 'swap' and ebs
+    for bdm in bdms:
+        if bdm['no_device']:
+            continue
+
+        # ebs volume case
+        if (bdm['volume_id'] or bdm['snapshot_id']):
+            ebs_devices.append(bdm['device_name'])
+            continue
+
+        virtual_name = bdm['virtual_name']
+        if not virtual_name:
+            continue
+
+        if is_swap_or_ephemeral(virtual_name):
+            mappings[virtual_name] = bdm['device_name']
+
+    # NOTE(yamahata): I'm not sure how ebs device should be numbered.
+    #                 Right now sort by device name for deterministic
+    #                 result.
+    if ebs_devices:
+        nebs = 0
+        ebs_devices.sort()
+        for ebs in ebs_devices:
+            mappings['ebs%d' % nebs] = ebs
+            nebs += 1
+
+    return mappings
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 2d62c00ed..5961ce4f2 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1739,15 +1739,33 @@ class API(base.Base):
 
     @wrap_check_policy
     @check_instance_lock
-    def attach_volume(self, context, instance, volume_id, device):
+    def attach_volume(self, context, instance, volume_id, device=None):
         """Attach an existing volume to an existing instance."""
-        if not re.match("^/dev/x{0,1}[a-z]d[a-z]+$", device):
+        # NOTE(vish): Fail fast if the device is not going to pass. This
+        #             will need to be removed along with the test if we
+        #             change the logic in the manager for what constitutes
+        #             a valid device.
+        if device and not re.match("^/dev/x{0,1}[a-z]d[a-z]+$", device):
             raise exception.InvalidDevicePath(path=device)
-        volume = self.volume_api.get(context, volume_id)
-        self.volume_api.check_attach(context, volume)
-        self.volume_api.reserve_volume(context, volume)
-        self.compute_rpcapi.attach_volume(context, instance=instance,
-                volume_id=volume_id, mountpoint=device)
+        # NOTE(vish): This is done on the compute host because we want
+        #             to avoid a race where two devices are requested at
+        #             the same time. When db access is removed from
+        #             compute, the bdm will be created here and we will
+        #             have to make sure that they are assigned atomically.
+        device = self.compute_rpcapi.reserve_block_device_name(
+            context, device=device, instance=instance)
+        try:
+            volume = self.volume_api.get(context, volume_id)
+            self.volume_api.check_attach(context, volume)
+            self.volume_api.reserve_volume(context, volume)
+            self.compute_rpcapi.attach_volume(context, instance=instance,
+                    volume_id=volume_id, mountpoint=device)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                self.db.block_device_mapping_destroy_by_instance_and_device(
+                        context, instance['uuid'], device)
+
+        return device
 
     @check_instance_lock
     def _detach_volume(self, context, instance, volume_id):
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 10056137c..75e6f1f8a 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -247,7 +247,7 @@ def _get_image_meta(context, image_ref):
 class ComputeManager(manager.SchedulerDependentManager):
     """Manages the running instances from creation to destruction."""
 
-    RPC_API_VERSION = '1.43'
+    RPC_API_VERSION = '1.44'
 
     def __init__(self, compute_driver=None, *args, **kwargs):
         """Load configuration options and connect to the hypervisor."""
@@ -2025,11 +2025,39 @@ class ComputeManager(manager.SchedulerDependentManager):
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
     @reverts_task_state
+    @wrap_instance_fault
+    def reserve_block_device_name(self, context, instance, device):
+
+        @utils.synchronized(instance['uuid'])
+        def do_reserve():
+            result = compute_utils.get_device_name_for_instance(context,
+                                                                instance,
+                                                                device)
+            # NOTE(vish): create bdm here to avoid race condition
+            values = {'instance_uuid': instance['uuid'],
+                      'device_name': result}
+            self.db.block_device_mapping_create(context, values)
+            return result
+        return do_reserve()
+
+    @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    @reverts_task_state
     @checks_instance_lock
     @wrap_instance_fault
     def attach_volume(self, context, volume_id, mountpoint, instance_uuid=None,
                       instance=None):
         """Attach a volume to an instance."""
+        try:
+            return self._attach_volume(context, volume_id, mountpoint,
+                                       instance_uuid, instance)
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                instance_uuid = instance_uuid or instance.get('uuid')
+                self.db.block_device_mapping_destroy_by_instance_and_device(
+                        context, instance_uuid, mountpoint)
+
+    def _attach_volume(self, context, volume_id, mountpoint, instance_uuid,
+                       instance):
         volume = self.volume_api.get(context, volume_id)
         context = context.elevated()
         if not instance:
@@ -2076,7 +2104,7 @@ class ComputeManager(manager.SchedulerDependentManager):
             'volume_id': volume_id,
             'volume_size': None,
             'no_device': None}
-        self.db.block_device_mapping_create(context, values)
+        self.db.block_device_mapping_update_or_create(context, values)
 
     def _detach_volume(self, context, instance, bdm):
         """Do the actual driver detach using block device mapping."""
diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py
index c81d75356..584335754 100644
--- a/nova/compute/rpcapi.py
+++ b/nova/compute/rpcapi.py
@@ -124,6 +124,7 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy):
                finish_resize(), confirm_resize(), revert_resize() and
                finish_revert_resize()
         1.43 - Add migrate_data to live_migration()
+        1.44 - Adds reserve_block_device_name()
     '''
 
     BASE_RPC_API_VERSION = '1.0'
@@ -479,6 +480,13 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy):
         return self.call(ctxt, self.make_msg('get_host_uptime'), topic,
                 version='1.1')
 
+    def reserve_block_device_name(self, ctxt, instance, device):
+        instance_p = jsonutils.to_primitive(instance)
+        return self.call(ctxt, self.make_msg('reserve_block_device_name',
+                instance=instance_p, device=device),
+                topic=_compute_topic(self.topic, ctxt, None, instance),
+                version='1.44')
+
     def snapshot_instance(self, ctxt, instance, image_id, image_type,
             backup_type, rotation):
         instance_p = jsonutils.to_primitive(instance)
diff --git a/nova/compute/utils.py b/nova/compute/utils.py
index 6d2fb2202..ef967c934 100644
--- a/nova/compute/utils.py
+++ b/nova/compute/utils.py
@@ -16,6 +16,10 @@
 
 """Compute-related Utilities and helpers."""
 
+import re
+import string
+
+from nova import block_device
 from nova import db
 from nova import exception
 from nova import flags
@@ -29,6 +33,54 @@ FLAGS = flags.FLAGS
 LOG = log.getLogger(__name__)
 
 
+def get_device_name_for_instance(context, instance, device):
+    # NOTE(vish): this will generate a unique device name that is not
+    #             in use already. It is a reasonable guess at where
+    #             it will show up in a linux guest, but it may not
+    #             always be correct
+    req_prefix = None
+    req_letters = None
+    if device:
+        try:
+            match = re.match("(^/dev/x{0,1}[a-z]d)([a-z]+)$", device)
+            req_prefix, req_letters = match.groups()
+        except (TypeError, AttributeError, ValueError):
+            raise exception.InvalidDevicePath(path=device)
+    bdms = db.block_device_mapping_get_all_by_instance(context,
+                instance['uuid'])
+    mappings = block_device.instance_block_mapping(instance, bdms)
+    try:
+        match = re.match("(^/dev/x{0,1}[a-z]d)[a-z]+[0-9]*$", mappings['root'])
+        prefix = match.groups()[0]
+    except (TypeError, AttributeError, ValueError):
+        raise exception.InvalidDevicePath(path=mappings['root'])
+    if not req_prefix:
+        req_prefix = prefix
+    letters_list = []
+    for _name, device in mappings.iteritems():
+        letter = block_device.strip_prefix(device)
+        # NOTE(vish): delete numbers in case we have something like
+        #             /dev/sda1
+        letter = re.sub("\d+", "", letter)
+        letters_list.append(letter)
+    used_letters = set(letters_list)
+    if not req_letters:
+        req_letters = _get_unused_letters(used_letters)
+    if req_letters in used_letters:
+        raise exception.DevicePathInUse(path=device)
+    return req_prefix + req_letters
+
+
+def _get_unused_letters(used_letters):
+    doubles = [first + second for second in string.ascii_lowercase
+               for first in string.ascii_lowercase]
+    all_letters = set(list(string.ascii_lowercase) + doubles)
+    letters = list(all_letters - used_letters)
+    # NOTE(vish): prepend ` so all shorter sequences sort first
+    letters.sort(key=lambda x: x.rjust(2, '`'))
+    return letters[0]
+
+
 def notify_usage_exists(context, instance_ref, current_period=False,
                         ignore_missing_network_data=True,
                         system_metadata=None, extra_usage_info=None):
diff --git a/nova/db/api.py b/nova/db/api.py
index ceb361722..d8274b2db 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -1298,9 +1298,16 @@ def block_device_mapping_destroy(context, bdm_id):
     return IMPL.block_device_mapping_destroy(context, bdm_id)
 
 
+def block_device_mapping_destroy_by_instance_and_device(context, instance_uuid,
+                                                        device_name):
+    """Destroy the block device mapping."""
+    return IMPL.block_device_mapping_destroy_by_instance_and_device(
+        context, instance_uuid, device_name)
+
+
 def block_device_mapping_destroy_by_instance_and_volume(context, instance_uuid,
                                                         volume_id):
-    """Destroy the block device mapping or raise if it does not exist."""
+    """Destroy the block device mapping."""
     return IMPL.block_device_mapping_destroy_by_instance_and_volume(
         context, instance_uuid, volume_id)
 
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 85f5d8f48..02b4607c2 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -3463,8 +3463,7 @@ def snapshot_update(context, snapshot_id, values):
 
 
 def _block_device_mapping_get_query(context, session=None):
-    return model_query(context, models.BlockDeviceMapping, session=session,
-                       read_deleted="no")
+    return model_query(context, models.BlockDeviceMapping, session=session)
 
 
 @require_context
@@ -3547,6 +3546,19 @@ def block_device_mapping_destroy_by_instance_and_volume(context, instance_uuid,
                     'updated_at': literal_column('updated_at')})
 
 
+@require_context
+def block_device_mapping_destroy_by_instance_and_device(context, instance_uuid,
+                                                        device_name):
+    session = get_session()
+    with session.begin():
+        _block_device_mapping_get_query(context, session=session).\
+            filter_by(instance_uuid=instance_uuid).\
+            filter_by(device_name=device_name).\
+            update({'deleted': True,
+                    'deleted_at': timeutils.utcnow(),
+                    'updated_at': literal_column('updated_at')})
+
+
 ###################
 
 def _security_group_get_query(context, session=None, read_deleted=None,
diff --git a/nova/exception.py b/nova/exception.py
index 86d3a59b2..6c855ec7e 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -386,6 +386,10 @@ class InvalidDevicePath(Invalid):
     message = _("The supplied device path (%(path)s) is invalid.")
 
 
+class DevicePathInUse(Invalid):
+    message = _("The supplied device path (%(path)s) is in use.")
+
+
 class DeviceIsBusy(Invalid):
     message = _("The supplied device (%(device)s) is busy.")
 
diff --git a/nova/tests/compute/test_compute_utils.py b/nova/tests/compute/test_compute_utils.py
index 40e1947e6..7ae692bb6 100644
--- a/nova/tests/compute/test_compute_utils.py
+++ b/nova/tests/compute/test_compute_utils.py
@@ -17,10 +17,13 @@
 
 """Tests For miscellaneous util methods used with compute."""
 
+import string
+
 from nova.compute import instance_types
 from nova.compute import utils as compute_utils
 from nova import context
 from nova import db
+from nova import exception
 from nova import flags
 from nova.openstack.common import importutils
 from nova.openstack.common import log as logging
@@ -37,6 +40,97 @@ FLAGS = flags.FLAGS
 flags.DECLARE('stub_network', 'nova.compute.manager')
 
 
+class ComputeValidateDeviceTestCase(test.TestCase):
+    def setUp(self):
+        super(ComputeValidateDeviceTestCase, self).setUp()
+        self.context = context.RequestContext('fake', 'fake')
+        self.instance = {
+                'uuid': 'fake',
+                'root_device_name': '/dev/vda',
+                'default_ephemeral_device': '/dev/vdb'
+        }
+
+    def _validate_device(self, device=None):
+        return compute_utils.get_device_name_for_instance(self.context,
+                                                          self.instance,
+                                                          device)
+
+    @staticmethod
+    def _fake_bdm(device):
+        return {
+            'device_name': device,
+            'no_device': None,
+            'volume_id': 'fake',
+            'snapshot_id': None
+        }
+
+    def test_wrap(self):
+        data = []
+        for letter in string.ascii_lowercase[2:]:
+            data.append(self._fake_bdm('/dev/vd' + letter))
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: data)
+        device = self._validate_device()
+        self.assertEqual(device, '/dev/vdaa')
+
+    def test_wrap_plus_one(self):
+        data = []
+        for letter in string.ascii_lowercase[2:]:
+            data.append(self._fake_bdm('/dev/vd' + letter))
+        data.append(self._fake_bdm('/dev/vdaa'))
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: data)
+        device = self._validate_device()
+        self.assertEqual(device, '/dev/vdab')
+
+    def test_later(self):
+        data = [
+            self._fake_bdm('/dev/vdc'),
+            self._fake_bdm('/dev/vdd'),
+            self._fake_bdm('/dev/vde'),
+        ]
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: data)
+        device = self._validate_device()
+        self.assertEqual(device, '/dev/vdf')
+
+    def test_gap(self):
+        data = [
+            self._fake_bdm('/dev/vdc'),
+            self._fake_bdm('/dev/vde'),
+        ]
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: data)
+        device = self._validate_device()
+        self.assertEqual(device, '/dev/vdd')
+
+    def test_no_bdms(self):
+        data = []
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: data)
+        device = self._validate_device()
+        self.assertEqual(device, '/dev/vdc')
+
+    def test_invalid_bdms(self):
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: [])
+        self.instance['root_device_name'] = "baddata"
+        self.assertRaises(exception.InvalidDevicePath,
+                          self._validate_device)
+
+    def test_invalid_device_prefix(self):
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: [])
+        self.assertRaises(exception.InvalidDevicePath,
+                          self._validate_device, '/baddata/vdc')
+
+    def test_device_in_use(self):
+        self.stubs.Set(db, 'block_device_mapping_get_all_by_instance',
+                       lambda context, instance: [])
+        self.assertRaises(exception.DevicePathInUse,
+                          self._validate_device, '/dev/vdb')
+
+
 class UsageInfoTestCase(test.TestCase):
 
     def setUp(self):
diff --git a/nova/tests/compute/test_rpcapi.py b/nova/tests/compute/test_rpcapi.py
index e88cb2096..38849d3ed 100644
--- a/nova/tests/compute/test_rpcapi.py
+++ b/nova/tests/compute/test_rpcapi.py
@@ -232,6 +232,10 @@ class ComputeRpcAPITestCase(test.TestCase):
                 injected_files='files', image_ref='ref',
                 orig_image_ref='orig_ref', version='1.24')
 
+    def test_reserve_block_device_name(self):
+        self._test_compute_api('reserve_block_device_name', 'call',
+                instance=self.fake_instance, device='device', version='1.44')
+
     def refresh_provider_fw_rules(self):
         self._test_compute_api('refresh_provider_fw_rules', 'cast',
                 host='host')
diff --git a/nova/tests/test_metadata.py b/nova/tests/test_metadata.py
index b9b67326c..13be9e056 100644
--- a/nova/tests/test_metadata.py
+++ b/nova/tests/test_metadata.py
@@ -27,6 +27,7 @@ import webob
 
 from nova.api.metadata import base
 from nova.api.metadata import handler
+from nova import block_device
 from nova import db
 from nova.db.sqlalchemy import api
 from nova import exception
@@ -183,7 +184,7 @@ class MetadataTestCase(test.TestCase):
                     'ebs0': '/dev/sdh'}
 
         self.assertEqual(base._format_instance_mapping(ctxt, instance_ref0),
-                         base._DEFAULT_MAPPINGS)
+                         block_device._DEFAULT_MAPPINGS)
         self.assertEqual(base._format_instance_mapping(ctxt, instance_ref1),
                          expected)