yahoo-eng-team team mailing list archive
-
yahoo-eng-team team
-
Mailing list archive
-
Message #48570
[Bug 1561900] [NEW] _get_instance_disk_info in libvirt/driver.py need to refactor
Public bug reported:
1. [root@yindesheng nova]# git log
commit e80376e3796cc20a400db423b828a98de6f528e6
Merge: b27b063 cabe2df
Author: Jenkins <jenkins@xxxxxxxxxxxxxxxxxxxx>
Date: Fri Mar 25 00:29:05 2016 +0000
Merge "Include CellMapping in InstanceMappin
2. I am not sure whether this is a bug, but I think this code could be
improved.
def _get_instance_disk_info(self, instance_name, xml,
block_device_info=None):
"""Get the non-volume disk information from the domain xml
:param str instance_name: the name of the instance (domain)
:param str xml: the libvirt domain xml for the instance
:param dict block_device_info: block device info for BDMs
:returns disk_info: list of dicts with keys:
* 'type': the disk type (str)
* 'path': the disk path (str)
* 'virt_disk_size': the virtual disk size (int)
* 'backing_file': backing file of a disk image (str)
* 'disk_size': physical disk size (int)
* 'over_committed_disk_size': virt_disk_size - disk_size or 0
"""
block_device_mapping = driver.block_device_info_get_mapping(
block_device_info)
volume_devices = set()
for vol in block_device_mapping:
disk_dev = vol['mount_device'].rpartition("/")[2]
volume_devices.add(disk_dev)
disk_info = []
doc = etree.fromstring(xml)
disk_nodes = doc.findall('.//devices/disk')
path_nodes = doc.findall('.//devices/disk/source')
driver_nodes = doc.findall('.//devices/disk/driver')
target_nodes = doc.findall('.//devices/disk/target')
for cnt, path_node in enumerate(path_nodes):
disk_type = disk_nodes[cnt].get('type')
path = path_node.get('file') or path_node.get('dev')
target = target_nodes[cnt].attrib['dev']
if not path:
LOG.debug('skipping disk for %s as it does not have a path',
instance_name)
continue
if disk_type not in ['file', 'block']:
LOG.debug('skipping disk because it looks like a volume', path)
continue
if target in volume_devices:
LOG.debug('skipping disk %(path)s (%(target)s) as it is a '
'volume', {'path': path, 'target': target})
continue
# get the real disk size or
# raise a localized error if image is unavailable
if disk_type == 'file':
dk_size = int(os.path.getsize(path))
elif disk_type == 'block' and block_device_info:
dk_size = lvm.get_volume_size(path)
else:
LOG.debug('skipping disk %(path)s (%(target)s) - unable to '
'determine if volume',
{'path': path, 'target': target})
continue
disk_type = driver_nodes[cnt].get('type')
if disk_type == "qcow2":
backing_file = libvirt_utils.get_disk_backing_file(path)
virt_size = disk.get_disk_size(path)
over_commit_size = int(virt_size) - dk_size
else:
backing_file = ""
virt_size = dk_size
over_commit_size = 0
disk_info.append({'type': disk_type,
'path': path,
'virt_disk_size': virt_size,
'backing_file': backing_file,
'disk_size': dk_size,
'over_committed_disk_size': over_commit_size})
return disk_info
3. in some cases, len(disk_nodes) is not equal len(path_nodes), I write a test case.
@mock.patch("os.path.getsize")
def test_get_instance_disk_info_disk_file_is_empty(self, mock_getsize):
instance = objects.Instance(**self.test_instance)
dummyxml = ("<domain type='kvm'><name>instance-0000000a</name>"
"<devices>"
"<disk type='file' device='cdrom'>"
"<driver name='qemu' type='raw' cache='none'/>"
"<target dev='hda' bus='ide'/></disk>"
"<disk type='block' device='disk'>"
"<driver name='qemu' type='qcow2' cache='none'/>"
"<source dev='/dev/mapper/mpathp'/>"
"<target dev='vda' bus='virtio'/></disk>"
"</devices></domain>")
block_device_info = \
{'block_device_mapping':
[{'guest_format': None, 'boot_index': 0,
'mount_device': u'/dev/vda',
'connection_info': {u'driver_volume_type': u'iscsi'},
'disk_bus': u'virtio', 'device_type': u'disk',
'delete_on_termination': False}],
'swap': None, 'ephemerals': [], 'root_device_name': u'/dev/vda'}
domain_mock = mock.Mock()
domain_mock.XMLDesc = mock.Mock()
domain_mock.XMLDesc.return_value = dummyxml
def fake_lookup(instance_name):
if instance_name == instance.name:
return domain_mock
self.create_fake_libvirt_mock(lookupByName=fake_lookup)
mock_getsize.return_value = 1000
drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
info = drvr.get_instance_disk_info(instance,
block_device_info=block_device_info)
info = jsonutils.loads(info)
self.assertEqual([], info)
4. run this unit test, and it will fail.
Traceback (most recent call last):
File "/home/nova/.venv/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
return func(*args, **keywargs)
File "/home/nova/nova/tests/unit/virt/libvirt/test_driver.py", line 8779, in test_get_instance_disk_info_disk_file_is_empty
self.assertEqual([], info)
File "/home/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py", line 362, in assertEqual
self.assertThat(observed, matcher, message)
File "/home/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py", line 447, in assertThat
raise mismatch_error
testtools.matchers._impl.MismatchError: !=:
reference = []
actual = [{u'backing_file': u'',
u'disk_size': 1000,
u'over_committed_disk_size': 0,
u'path': u'/dev/mapper/mpathp',
u'type': u'raw',
u'virt_disk_size': 1000}]
** Affects: nova
Importance: Undecided
Assignee: Rong Han ZTE (hanrong)
Status: New
** Changed in: nova
Assignee: (unassigned) => Rong Han ZTE (hanrong)
--
You received this bug notification because you are a member of Yahoo!
Engineering Team, which is subscribed to OpenStack Compute (nova).
https://bugs.launchpad.net/bugs/1561900
Title:
_get_instance_disk_info in libvirt/driver.py need to refactor
Status in OpenStack Compute (nova):
New
Bug description:
1. [root@yindesheng nova]# git log
commit e80376e3796cc20a400db423b828a98de6f528e6
Merge: b27b063 cabe2df
Author: Jenkins <jenkins@xxxxxxxxxxxxxxxxxxxx>
Date: Fri Mar 25 00:29:05 2016 +0000
Merge "Include CellMapping in InstanceMappin
2. I am not sure whether this is a bug, but I think this code could
be improved.
def _get_instance_disk_info(self, instance_name, xml,
block_device_info=None):
"""Get the non-volume disk information from the domain xml
:param str instance_name: the name of the instance (domain)
:param str xml: the libvirt domain xml for the instance
:param dict block_device_info: block device info for BDMs
:returns disk_info: list of dicts with keys:
* 'type': the disk type (str)
* 'path': the disk path (str)
* 'virt_disk_size': the virtual disk size (int)
* 'backing_file': backing file of a disk image (str)
* 'disk_size': physical disk size (int)
* 'over_committed_disk_size': virt_disk_size - disk_size or 0
"""
block_device_mapping = driver.block_device_info_get_mapping(
block_device_info)
volume_devices = set()
for vol in block_device_mapping:
disk_dev = vol['mount_device'].rpartition("/")[2]
volume_devices.add(disk_dev)
disk_info = []
doc = etree.fromstring(xml)
disk_nodes = doc.findall('.//devices/disk')
path_nodes = doc.findall('.//devices/disk/source')
driver_nodes = doc.findall('.//devices/disk/driver')
target_nodes = doc.findall('.//devices/disk/target')
for cnt, path_node in enumerate(path_nodes):
disk_type = disk_nodes[cnt].get('type')
path = path_node.get('file') or path_node.get('dev')
target = target_nodes[cnt].attrib['dev']
if not path:
LOG.debug('skipping disk for %s as it does not have a path',
instance_name)
continue
if disk_type not in ['file', 'block']:
LOG.debug('skipping disk because it looks like a volume', path)
continue
if target in volume_devices:
LOG.debug('skipping disk %(path)s (%(target)s) as it is a '
'volume', {'path': path, 'target': target})
continue
# get the real disk size or
# raise a localized error if image is unavailable
if disk_type == 'file':
dk_size = int(os.path.getsize(path))
elif disk_type == 'block' and block_device_info:
dk_size = lvm.get_volume_size(path)
else:
LOG.debug('skipping disk %(path)s (%(target)s) - unable to '
'determine if volume',
{'path': path, 'target': target})
continue
disk_type = driver_nodes[cnt].get('type')
if disk_type == "qcow2":
backing_file = libvirt_utils.get_disk_backing_file(path)
virt_size = disk.get_disk_size(path)
over_commit_size = int(virt_size) - dk_size
else:
backing_file = ""
virt_size = dk_size
over_commit_size = 0
disk_info.append({'type': disk_type,
'path': path,
'virt_disk_size': virt_size,
'backing_file': backing_file,
'disk_size': dk_size,
'over_committed_disk_size': over_commit_size})
return disk_info
3. in some cases, len(disk_nodes) is not equal len(path_nodes), I write a test case.
@mock.patch("os.path.getsize")
def test_get_instance_disk_info_disk_file_is_empty(self, mock_getsize):
instance = objects.Instance(**self.test_instance)
dummyxml = ("<domain type='kvm'><name>instance-0000000a</name>"
"<devices>"
"<disk type='file' device='cdrom'>"
"<driver name='qemu' type='raw' cache='none'/>"
"<target dev='hda' bus='ide'/></disk>"
"<disk type='block' device='disk'>"
"<driver name='qemu' type='qcow2' cache='none'/>"
"<source dev='/dev/mapper/mpathp'/>"
"<target dev='vda' bus='virtio'/></disk>"
"</devices></domain>")
block_device_info = \
{'block_device_mapping':
[{'guest_format': None, 'boot_index': 0,
'mount_device': u'/dev/vda',
'connection_info': {u'driver_volume_type': u'iscsi'},
'disk_bus': u'virtio', 'device_type': u'disk',
'delete_on_termination': False}],
'swap': None, 'ephemerals': [], 'root_device_name': u'/dev/vda'}
domain_mock = mock.Mock()
domain_mock.XMLDesc = mock.Mock()
domain_mock.XMLDesc.return_value = dummyxml
def fake_lookup(instance_name):
if instance_name == instance.name:
return domain_mock
self.create_fake_libvirt_mock(lookupByName=fake_lookup)
mock_getsize.return_value = 1000
drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
info = drvr.get_instance_disk_info(instance,
block_device_info=block_device_info)
info = jsonutils.loads(info)
self.assertEqual([], info)
4. run this unit test, and it will fail.
Traceback (most recent call last):
File "/home/nova/.venv/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
return func(*args, **keywargs)
File "/home/nova/nova/tests/unit/virt/libvirt/test_driver.py", line 8779, in test_get_instance_disk_info_disk_file_is_empty
self.assertEqual([], info)
File "/home/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py", line 362, in assertEqual
self.assertThat(observed, matcher, message)
File "/home/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py", line 447, in assertThat
raise mismatch_error
testtools.matchers._impl.MismatchError: !=:
reference = []
actual = [{u'backing_file': u'',
u'disk_size': 1000,
u'over_committed_disk_size': 0,
u'path': u'/dev/mapper/mpathp',
u'type': u'raw',
u'virt_disk_size': 1000}]
To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/1561900/+subscriptions
Follow ups
