openstack
/
nova
Code Issues Proposed changes

Allow nova to guess device if not passed to attach 

partial fix for bug 1004328

Only the xen hypervisor actually respects the device name that
is passed in attach_volume. For other hypervisors it makes much
more sense to automatically generate a unique name.

This patch generates a non-conflicting device name if one is not
passed to attach_volume. It also validates the passed in volume
name to make sure another device isn't already attached there.

A corresponding change to novaclient and horizon will greatly
improve the user experience of attaching a volume.

It moves some common code out of metadata/base so that it can
be used to get a list of block devices. The code was functionally
tested as well and block device name generation works properly.

This adds a new method to the rpcapi to validate a device name. It
also adds server_id to the volumes extension, since it was omitted
by mistake.

The next step is to modify the libvirt driver to match the serial
number of the device to the volume uuid so that the volume can
always be found at /dev/disk/by-id/virtio-<uuid>.

DocImpact

Change-Id: I0b9454fc50a5c93b4aea38545dcee98f68d7e511
This commit is contained in:
Vishvananda Ishaya 2012-08-06 12:17:43 -07:00
parent d141e64de9
commit e44751162b
13 changed files with 298 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
nova/api/metadata/base.py
View File

@ -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):

8
nova/api/openstack/compute/contrib/volumes.py
View File

@ -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

50
nova/block_device.py
View File

@ -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

32
nova/compute/api.py
View File

@ -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):

32
nova/compute/manager.py
View File

@ -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."""
@ -2023,6 +2023,23 @@ class ComputeManager(manager.SchedulerDependentManager):
self.volume_api.attach(context, volume, instance_uuid, mountpoint)
return connection_info
@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
@ -2030,6 +2047,17 @@ class ComputeManager(manager.SchedulerDependentManager):
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."""

8
nova/compute/rpcapi.py
View File

@ -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)

52
nova/compute/utils.py
View File

@ -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):

9
nova/db/api.py
View File

@ -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)

16
nova/db/sqlalchemy/api.py
View File

@ -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,

4
nova/exception.py
View File

@ -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.")

94
nova/tests/compute/test_compute_utils.py
View File

@ -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):

4
nova/tests/compute/test_rpcapi.py
View File

@ -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')

3
nova/tests/test_metadata.py
View File

@ -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)