[PATCHv3 0/7] Attach guest disks
From: Royce Lv <lvroyce@linux.vnet.ibm.com> This patch need to be changed with Shaohe's metadata patch, and cannot be merged directly.As metadata patch cannot pass all tests. v1>v3, Choose bus type according to os info in metadata. When adding a disk, specify a pool and vol name, so that kimchi can chek ref_cnt to make sure no others using that volume, also volume format is filled to driver so that qcow2 image will not recognized as raw. Tested: 1. when no bus is specified, kimchi chose 'ide' for cdrom and 'virtio' for disk 2. when bus is specified, kimchi attach disk with specified bus. 3. ide does not support hot plug. 4. Disk size for qcow2 and raw is right. Depend on Sheldon's patch: [PATCH]manually manage the metadata element [PATCH]write the template OS info to vm metadata [PATCH]Add two function to set and get domain xml metadata [PATCH]bug fix: get user and group when vm is living. [PATCH]update test case to set/get user and group when VM is running Royce Lv (7): Guest disks: Update doc to support manage guest disks Guest disks: Update api definition and error reporting Guest disks: Choose proper bus for device Guest disks: Abstract vm disk functions Guest disk: deals with disk attachment Multiple pep8 fixes Guest disks: Update testcase docs/API.md | 12 ++-- src/kimchi/API.json | 37 +++++++--- src/kimchi/i18n.py | 28 ++++---- src/kimchi/mockmodel.py | 18 ++--- src/kimchi/model/storagevolumes.py | 9 +-- src/kimchi/model/vmstorages.py | 138 ++++++++++++++++++------------------- src/kimchi/vmdisks.py | 58 ++++++++++++++++ tests/test_model.py | 75 ++++++++++++++++++++ 8 files changed, 265 insertions(+), 110 deletions(-) create mode 100644 src/kimchi/vmdisks.py -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Update API.md to support manage guest disks Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/API.md b/docs/API.md index 122d8b9..2e2e275 100644 --- a/docs/API.md +++ b/docs/API.md @@ -138,17 +138,21 @@ Represents a snapshot of the Virtual Machine's primary monitor. * **GET**: Retrieve a summarized list of all storages of specified guest * **POST**: Attach a new storage or virtual drive to specified virtual machine. * dev: The name of the storage in the vm. - * type: The type of the storage (currently supports 'cdrom' only). + * type: The type of the storage (currently support 'cdrom' and 'disk'). * path: Path of cdrom iso. + * pool: Storage pool which disk image file locate in. + * vol: Storage volume name of disk image. + * bus: Target bus type for disk to attach. ### Sub-resource: storage **URI:** /vms/*:name*/storages/*:dev* * **GET**: Retrieve storage information * dev: The name of the storage in the vm. - * type: The type of the storage ('cdrom' only for now). - * path: Path of cdrom iso. + * type: The type of the storage (currently support 'cdrom' and 'disk'). + * path: Path of cdrom iso or disk image file. + * bus: Bus type of disk attached. * **PUT**: Update storage information - * path: Path of cdrom iso. Can not be blank. + * path: Path of cdrom iso. Can not be blank. Now just support cdrom type. * **DELETE**: Remove the storage. -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Modify all errors related to cdrom attachment to be compatible with disk attachment, changing error naming and msg reporting. Add parameter 'bus' for disk attachment to give user choice when kimchi cannot decide what bus to use. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/API.json | 37 +++++++++++++++++++++++++++---------- src/kimchi/i18n.py | 28 ++++++++++++++++------------ src/kimchi/mockmodel.py | 10 +++++----- src/kimchi/model/vmstorages.py | 26 ++++++++++++++------------ 4 files changed, 62 insertions(+), 39 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 38e9607..06b0804 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -418,40 +418,57 @@ }, "vmstorages_create": { "type": "object", - "error": "KCHCDROM0012E", + "error": "KCHVMSTOR0012E", "properties": { "dev": { - "description": "The storage (cd-rom) device name", + "description": "The storage device name", "type": "string", - "pattern": "^hd[b-z]$", - "error": "KCHCDROM0001E" + "pattern": "^h|vd[b-z]$", + "error": "KCHVMSTOR0001E" }, "type": { "description": "The storage type", "type": "string", - "pattern": "^cdrom$", + "pattern": "^cdrom|disk$", "required": true, - "error": "KCHCDROM0002E" + "error": "KCHVMSTOR0002E" + }, + "pool": { + "description": "Storage pool name disk image locate in", + "type": "string", + "minLength": 1, + "error": "KCHVMSTOR0012E" + }, + "vol": { + "description": "Storage volume name of disk image", + "type": "string", + "minLength": 1, + "error": "KCHVMSTOR0012E" }, "path": { "description": "Path of iso image file or disk mount point", "type": "string", "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", - "required": true, - "error": "KCHCDROM0003E" + "error": "KCHVMSTOR0003E" + }, + "bus": { + "description": "Target bus type of attached device", + "type": "string", + "pattern": "^scsi|ide|virtio$", + "error": "KCHVMSTOR0005E" } } }, "vmstorage_update": { "type": "object", - "error": "KCHCDROM0013E", + "error": "KCHVMSTOR0013E", "properties": { "path": { "description": "Path of iso image file or disk mount point", "type": "string", "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", "required": true, - "error": "KCHCDROM0003E" + "error": "KCHVMSTOR0003E" } } }, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index e28f689..c3413cd 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -226,18 +226,22 @@ messages = { "KCHUTILS0002E": _("Timeout while running command '%(cmd)s' after %(seconds)s seconds"), "KCHUTILS0003E": _("Unable to choose a virutal machine name"), - "KCHCDROM0001E": _("Invalid CDROM device name"), - "KCHCDROM0002E": _("Invalid storage type. Types supported: 'cdrom'"), - "KCHCDROM0003E": _("The path '%(value)s' is not valid local/remote path for the device"), - "KCHCDROM0004E": _("Device name %(dev_name)s already exists in vm %(vm_name)s"), - "KCHCDROM0007E": _("The storage device %(dev_name)s does not exist in the guest %(vm_name)s"), - "KCHCDROM0008E": _("Error while creating new storage device: %(error)s"), - "KCHCDROM0009E": _("Error while updating storage device: %(error)s"), - "KCHCDROM0010E": _("Error while removing storage device: %(error)s"), - "KCHCDROM0011E": _("Do not support guest CDROM hot plug attachment"), - "KCHCDROM0012E": _("Specify type and path to add a new virtual machine disk"), - "KCHCDROM0013E": _("Specify path to update virtual machine disk"), - "KCHCDROM0014E": _("Controller type %(type)s limitation of %(limit)s devices reached"), + "KCHVMSTOR0001E": _("Invalid vm storage device name"), + "KCHVMSTOR0002E": _("Invalid storage type. Types supported: 'cdrom', 'disk'"), + "KCHVMSTOR0003E": _("The path '%(value)s' is not valid local/remote path for the device"), + "KCHVMSTOR0004E": _("Device name %(dev_name)s already exists in vm %(vm_name)s"), + "KCHVMSTOR0005E": _("Invalid target device bus type, type supported: 'ide', 'scsi', 'virtio'"), + "KCHVMSTOR0006E": _("Just support cdrom path update"), + "KCHVMSTOR0007E": _("The storage device %(dev_name)s does not exist in the guest %(vm_name)s"), + "KCHVMSTOR0008E": _("Error while creating new storage device: %(error)s"), + "KCHVMSTOR0009E": _("Error while updating storage device: %(error)s"), + "KCHVMSTOR0010E": _("Error while removing storage device: %(error)s"), + "KCHVMSTOR0011E": _("Do not support ide device hot plug"), + "KCHVMSTOR0012E": _("Specify type and path or type and pool/volume to add a new virtual machine disk"), + "KCHVMSTOR0013E": _("Specify path to update virtual machine disk"), + "KCHVMSTOR0014E": _("Controller type %(type)s limitation of %(limit)s devices reached"), + "KCHVMSTOR0015E": _("Cannot lookup disk path information by given pool/volume: %(error)s"), + "KCHVMSTOR0016E": _("Volume already been used by other vm"), "KCHREPOS0001E": _("YUM Repository ID must be one word only string."), "KCHREPOS0002E": _("Repository URL must be an http://, ftp:// or file:// URL."), diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index db904aa..fb067a8 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -664,12 +664,12 @@ class MockModel(object): def vmstorages_create(self, vm_name, params): path = params.get('path') if path.startswith('/') and not os.path.exists(path): - raise InvalidParameter("KCHCDROM0003E", {'value': path}) + raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) dom = self._get_vm(vm_name) dev = params.get('dev', None) if dev and dev in self.vmstorages_get_list(vm_name): - return OperationFailed("KCHCDROM0004E", {'dev_name': dev, + return OperationFailed("KCHVMSTOR0004E", {'dev_name': dev, 'vm_name': vm_name}) if not dev: index = len(dom.storagedevices.keys()) + 1 @@ -686,14 +686,14 @@ class MockModel(object): def vmstorage_lookup(self, vm_name, dev_name): dom = self._get_vm(vm_name) if dev_name not in self.vmstorages_get_list(vm_name): - raise NotFoundError("KCHCDROM0007E", {'dev_name': dev_name, + raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, 'vm_name': vm_name}) return dom.storagedevices.get(dev_name).info def vmstorage_delete(self, vm_name, dev_name): dom = self._get_vm(vm_name) if dev_name not in self.vmstorages_get_list(vm_name): - raise NotFoundError("KCHCDROM0007E", {'dev_name': dev_name, + raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, 'vm_name': vm_name}) dom.storagedevices.pop(dev_name) @@ -702,7 +702,7 @@ class MockModel(object): dom = self._get_vm(vm_name) dom.storagedevices[dev_name].info.update(params) except Exception as e: - raise OperationFailed("KCHCDROM0009E", {'error': e.message}) + raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) return dev_name def vmifaces_create(self, vm, params): diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 1cffa84..c31af24 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -89,15 +89,15 @@ def _check_cdrom_path(path): cds = re.findall("drive name:\t\t(.*)", content) if not cds: - raise InvalidParameter("KCHCDROM0003E", {'value': path}) + raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) drives = [os.path.join('/dev', p) for p in cds[0].split('\t')] if path not in drives: - raise InvalidParameter("KCHCDROM0003E", {'value': path}) + raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) src_type = 'block' else: - raise InvalidParameter("KCHCDROM0003E", {'value': path}) + raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) return src_type @@ -122,7 +122,7 @@ class VMStoragesModel(object): valid_id.remove((bus_id, unit_id)) continue if not valid_id: - raise OperationFailed('KCHCDROM0014E', {'type': 'ide', 'limit': 4}) + raise OperationFailed('KCHVMSTOR0014E', {'type': 'ide', 'limit': 4}) else: address = {'controller': controller_id, 'bus': valid_id[0][0], 'unit': valid_id[0][1]} @@ -131,7 +131,7 @@ class VMStoragesModel(object): def create(self, vm_name, params): dom = VMModel.get_vm(vm_name, self.conn) if DOM_STATE_MAP[dom.info()[0]] != 'shutoff': - raise InvalidOperation('KCHCDROM0011E') + raise InvalidOperation('KCHVMSTOR0011E') # Use device name passed or pick next dev_name = params.get('dev', None) @@ -140,7 +140,7 @@ class VMStoragesModel(object): else: devices = self.get_list(vm_name) if dev_name in devices: - raise OperationFailed('KCHCDROM0004E', {'dev_name': dev_name, + raise OperationFailed('KCHVMSTOR0004E', {'dev_name': dev_name, 'vm_name': vm_name}) # Path will never be blank due to API.json verification. @@ -156,7 +156,7 @@ class VMStoragesModel(object): dom = conn.lookupByName(vm_name) dom.attachDeviceFlags(dev_xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) except Exception as e: - raise OperationFailed("KCHCDROM0008E", {'error': e.message}) + raise OperationFailed("KCHVMSTOR0008E", {'error': e.message}) return params['dev'] def _get_storage_device_name(self, vm_name): @@ -190,7 +190,7 @@ class VMStorageModel(object): dom = VMModel.get_vm(vm_name, self.conn) disk = _get_device_xml(dom, dev_name) if disk is None: - raise NotFoundError("KCHCDROM0007E", {'dev_name': dev_name, + raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, 'vm_name': vm_name}) path = "" dev_bus = 'ide' @@ -220,13 +220,13 @@ class VMStorageModel(object): dom = VMModel.get_vm(vm_name, self.conn) disk = _get_device_xml(dom, dev_name) if disk is None: - raise NotFoundError("KCHCDROM0007E", + raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, 'vm_name': vm_name}) dom = VMModel.get_vm(vm_name, self.conn) if DOM_STATE_MAP[dom.info()[0]] != 'shutoff': - raise InvalidOperation('KCHCDROM0011E') + raise InvalidOperation('KCHVMSTOR0011E') try: conn = self.conn.get() @@ -234,18 +234,20 @@ class VMStorageModel(object): dom.detachDeviceFlags(etree.tostring(disk), libvirt.VIR_DOMAIN_AFFECT_CURRENT) except Exception as e: - raise OperationFailed("KCHCDROM0010E", {'error': e.message}) + raise OperationFailed("KCHVMSTOR0010E", {'error': e.message}) def update(self, vm_name, dev_name, params): params['src_type'] = _check_cdrom_path(params['path']) dom = VMModel.get_vm(vm_name, self.conn) dev_info = self.lookup(vm_name, dev_name) + if dev_info['type'] != 'cdrom': + raise InvalidOperation("KCHVMSTOR0006E") dev_info.update(params) xml = _get_storage_xml(dev_info) try: dom.updateDeviceFlags(xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) except Exception as e: - raise OperationFailed("KCHCDROM0009E", {'error': e.message}) + raise OperationFailed("KCHVMSTOR0009E", {'error': e.message}) return dev_name -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When bus type is not passed by user, kimchi choose bus type based on stored os distro and version. And make right hot plug behavior based on bus type. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index c31af24..b265e6a 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -32,9 +32,11 @@ from kimchi.exception import InvalidOperation, InvalidParameter, NotFoundError from kimchi.exception import OperationFailed from kimchi.model.vms import DOM_STATE_MAP, VMModel from kimchi.utils import check_url_path +from kimchi.osinfo import lookup DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', 'block': 'dev'} +HOTPLUG_TYPE = ['scsi', 'virtio'] def _get_device_xml(dom, dev_name): @@ -47,6 +49,14 @@ def _get_device_xml(dom, dev_name): return disk[0] +def _get_device_bus(dev_type, dom): + try: + version, distro = VMModel.vm_get_os_metadata(dom) + except: + version, distro = ('unknown', 'unknown') + return lookup(distro, version)[dev_type+'_bus'] + + def _get_storage_xml(params): src_type = params.get('src_type') disk = E.disk(type=src_type, device=params.get('type')) @@ -65,7 +75,7 @@ def _get_storage_xml(params): source.set(DEV_TYPE_SRC_ATTR_MAP[src_type], params.get('path')) disk.append(source) - disk.append(E.target(dev=params.get('dev'), bus=params.get('bus', 'ide'))) + disk.append(E.target(dev=params.get('dev'), bus=params['bus'])) if params.get('address'): # ide disk target id is always '0' disk.append(E.address( @@ -105,7 +115,9 @@ class VMStoragesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] - def _get_available_ide_address(self, vm_name): + def _get_available_bus_address(self, bus_type, vm_name): + if bus_type not in ['ide']: + return dict() # libvirt limitation of just 1 ide controller # each controller have at most 2 buses and each bus 2 units. dom = VMModel.get_vm(vm_name, self.conn) @@ -130,9 +142,6 @@ class VMStoragesModel(object): def create(self, vm_name, params): dom = VMModel.get_vm(vm_name, self.conn) - if DOM_STATE_MAP[dom.info()[0]] != 'shutoff': - raise InvalidOperation('KCHVMSTOR0011E') - # Use device name passed or pick next dev_name = params.get('dev', None) if dev_name is None: @@ -148,7 +157,13 @@ class VMStoragesModel(object): path = params['path'] params['src_type'] = _check_cdrom_path(path) - params.update(self._get_available_ide_address(vm_name)) + params.setdefault( + 'bus', _get_device_bus(params['type'], dom)) + if (params['bus'] not in HOTPLUG_TYPE + and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): + raise InvalidOperation('KCHVMSTOR0011E') + + params.update(self._get_available_bus_address(params['bus'], vm_name)) # Add device to VM dev_xml = _get_storage_xml(params) try: @@ -218,19 +233,20 @@ class VMStorageModel(object): def delete(self, vm_name, dev_name): # Get storage device xml dom = VMModel.get_vm(vm_name, self.conn) - disk = _get_device_xml(dom, dev_name) - if disk is None: - raise NotFoundError("KCHVMSTOR0007E", - {'dev_name': dev_name, - 'vm_name': vm_name}) + try: + bus_type = self.lookup(vm_name, dev_name)['bus'] + except NotFoundError: + raise dom = VMModel.get_vm(vm_name, self.conn) - if DOM_STATE_MAP[dom.info()[0]] != 'shutoff': + if (bus_type not in HOTPLUG_TYPE and + DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): raise InvalidOperation('KCHVMSTOR0011E') try: conn = self.conn.get() dom = conn.lookupByName(vm_name) + disk = _get_device_xml(dom, dev_name) dom.detachDeviceFlags(etree.tostring(disk), libvirt.VIR_DOMAIN_AFFECT_CURRENT) except Exception as e: -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Vmstorages will refer to storagevolume functions to get storage volume format and reference count, Meanwhile storage volume will refer to vm storages to see if a path is refered by a vm to prevent a disk be used by multiple vm. To prevent circular reference, abstract vm disk list and lookup function to a single module. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 9 +++--- src/kimchi/model/vmstorages.py | 55 +++++------------------------------- src/kimchi/vmdisks.py | 58 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 52 deletions(-) create mode 100644 src/kimchi/vmdisks.py diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 8a568eb..9a56a63 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -28,8 +28,8 @@ from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.isoinfo import IsoImage from kimchi.model.storagepools import StoragePoolModel from kimchi.utils import kimchi_log -from kimchi.model.vms import VMsModel -from kimchi.model.vmstorages import VMStoragesModel, VMStorageModel +from kimchi.model.vms import VMsModel, VMModel +from kimchi.vmdisks import get_vm_disk, get_vm_disk_list VOLUME_TYPE_MAP = {0: 'file', @@ -134,9 +134,10 @@ class StorageVolumeModel(object): # try to find this volume in exsisted vm vms = VMsModel.get_vms(self.conn) for vm in vms: - storages = VMStoragesModel(**args).get_list(vm) + dom = VMModel.get_vm(vm, self.conn) + storages = get_vm_disk_list(dom) for disk in storages: - d_info = VMStorageModel(**args).lookup(vm, disk) + d_info = get_vm_disk(dom, disk) if path == d_info['path']: ref_cnt = ref_cnt + 1 session.store('storagevolume', vol_id, diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index b265e6a..c04d511 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -33,22 +33,12 @@ from kimchi.exception import OperationFailed from kimchi.model.vms import DOM_STATE_MAP, VMModel from kimchi.utils import check_url_path from kimchi.osinfo import lookup +from kimchi.vmdisks import get_device_xml, get_vm_disk, get_vm_disk_list +from kimchi.vmdisks import DEV_TYPE_SRC_ATTR_MAP -DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', - 'block': 'dev'} HOTPLUG_TYPE = ['scsi', 'virtio'] -def _get_device_xml(dom, dev_name): - # Get VM xml and then devices xml - xml = dom.XMLDesc(0) - devices = objectify.fromstring(xml).devices - disk = devices.xpath("./disk/target[@dev='%s']/.." % dev_name) - if not disk: - return None - return disk[0] - - def _get_device_bus(dev_type, dom): try: version, distro = VMModel.vm_get_os_metadata(dom) @@ -114,6 +104,7 @@ def _check_cdrom_path(path): class VMStoragesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore'] def _get_available_bus_address(self, bus_type, vm_name): if bus_type not in ['ide']: @@ -125,7 +116,7 @@ class VMStoragesModel(object): valid_id = [('0', '0'), ('0', '1'), ('1', '0'), ('1', '1')] controller_id = '0' for dev_name in disks: - disk = _get_device_xml(dom, dev_name) + disk = get_device_xml(dom, dev_name) if disk.target.attrib['bus'] == 'ide': controller_id = disk.address.attrib['controller'] bus_id = disk.address.attrib['bus'] @@ -156,7 +147,6 @@ class VMStoragesModel(object): # There is no need to cover this case here. path = params['path'] params['src_type'] = _check_cdrom_path(path) - params.setdefault( 'bus', _get_device_bus(params['type'], dom)) if (params['bus'] not in HOTPLUG_TYPE @@ -187,13 +177,7 @@ class VMStoragesModel(object): def get_list(self, vm_name): dom = VMModel.get_vm(vm_name, self.conn) - xml = dom.XMLDesc(0) - devices = objectify.fromstring(xml).devices - storages = [disk.target.attrib['dev'] - for disk in devices.xpath("./disk[@device='disk']")] - storages += [disk.target.attrib['dev'] - for disk in devices.xpath("./disk[@device='cdrom']")] - return storages + return get_vm_disk_list(dom) class VMStorageModel(object): @@ -203,32 +187,7 @@ class VMStorageModel(object): def lookup(self, vm_name, dev_name): # Retrieve disk xml and format return dict dom = VMModel.get_vm(vm_name, self.conn) - disk = _get_device_xml(dom, dev_name) - if disk is None: - raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, - 'vm_name': vm_name}) - path = "" - dev_bus = 'ide' - try: - source = disk.source - if source is not None: - src_type = disk.attrib['type'] - if src_type == 'network': - host = source.host - path = (source.attrib['protocol'] + '://' + - host.attrib['name'] + ':' + - host.attrib['port'] + source.attrib['name']) - else: - path = source.attrib[DEV_TYPE_SRC_ATTR_MAP[src_type]] - # Retrieve storage bus type - dev_bus = disk.target.attrib['bus'] - except: - pass - dev_type = disk.attrib['device'] - return {'dev': dev_name, - 'type': dev_type, - 'path': path, - 'bus': dev_bus} + return get_vm_disk(dom, dev_name) def delete(self, vm_name, dev_name): # Get storage device xml @@ -246,7 +205,7 @@ class VMStorageModel(object): try: conn = self.conn.get() dom = conn.lookupByName(vm_name) - disk = _get_device_xml(dom, dev_name) + disk = get_device_xml(dom, dev_name) dom.detachDeviceFlags(etree.tostring(disk), libvirt.VIR_DOMAIN_AFFECT_CURRENT) except Exception as e: diff --git a/src/kimchi/vmdisks.py b/src/kimchi/vmdisks.py new file mode 100644 index 0000000..d96a43a --- /dev/null +++ b/src/kimchi/vmdisks.py @@ -0,0 +1,58 @@ +from lxml import objectify + +from kimchi.exception import NotFoundError + +DEV_TYPE_SRC_ATTR_MAP = {'file': 'file', + 'block': 'dev'} + + +def get_device_xml(dom, dev_name): + # Get VM xml and then devices xml + xml = dom.XMLDesc(0) + devices = objectify.fromstring(xml).devices + disk = devices.xpath("./disk/target[@dev='%s']/.." % dev_name) + if not disk: + return None + return disk[0] + + +def get_vm_disk(dom, dev_name): + # Retrieve disk xml and format return dict + disk = get_device_xml(dom, dev_name) + if disk is None: + raise NotFoundError( + "KCHVMSTOR0007E", + {'dev_name': dev_name, 'vm_name': vm_name}) + path = "" + dev_bus = 'ide' + try: + source = disk.source + if source is not None: + src_type = disk.attrib['type'] + if src_type == 'network': + host = source.host + path = (source.attrib['protocol'] + '://' + + host.attrib['name'] + ':' + + host.attrib['port'] + source.attrib['name']) + else: + path = source.attrib[DEV_TYPE_SRC_ATTR_MAP[src_type]] + # Retrieve storage bus type + dev_bus = disk.target.attrib['bus'] + except: + pass + dev_type = disk.attrib['device'] + return {'dev': dev_name, + 'type': dev_type, + 'path': path, + 'format': disk.driver.attrib['type'], + 'bus': dev_bus} + + +def get_vm_disk_list(dom): + xml = dom.XMLDesc(0) + devices = objectify.fromstring(xml).devices + storages = [disk.target.attrib['dev'] + for disk in devices.xpath("./disk[@device='disk']")] + storages += [disk.target.attrib['dev'] + for disk in devices.xpath("./disk[@device='cdrom']")] + return storages -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Disk attachment accept pool and volume, kimchi convert pool/volume information to path information, also check ref count to make sure no other vm is using it. Storage volume format is also checked to make sure guest will recongize it as right format, otherwise disk size will be wrong from guest view. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index c04d511..9e2248b 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -31,6 +31,7 @@ from lxml.builder import E from kimchi.exception import InvalidOperation, InvalidParameter, NotFoundError from kimchi.exception import OperationFailed from kimchi.model.vms import DOM_STATE_MAP, VMModel +from kimchi.model.storagevolumes import StorageVolumeModel from kimchi.utils import check_url_path from kimchi.osinfo import lookup from kimchi.vmdisks import get_device_xml, get_vm_disk, get_vm_disk_list @@ -50,7 +51,7 @@ def _get_device_bus(dev_type, dom): def _get_storage_xml(params): src_type = params.get('src_type') disk = E.disk(type=src_type, device=params.get('type')) - disk.append(E.driver(name='qemu', type='raw')) + disk.append(E.driver(name='qemu', type=params['format'])) # Working with url paths if src_type == 'network': output = urlparse.urlparse(params.get('path')) @@ -75,7 +76,7 @@ def _get_storage_xml(params): return ET.tostring(disk) -def _check_cdrom_path(path): +def _check_path(path): if check_url_path(path): src_type = 'network' # Check if path is a valid local path @@ -145,8 +146,22 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. - path = params['path'] - params['src_type'] = _check_cdrom_path(path) + params['format'] = 'raw' + if params.get('vol'): + try: + pool = params['pool'] + vol_info = StorageVolumeModel( + conn=self.conn, + objstore=self.objstore).lookup(pool, params['vol']) + except KeyError: + raise InvalidParameter("KCHVMSTOR0012E") + except Exception as e: + raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) + if vol_info['ref_cnt'] != 0: + raise InvalidParameter("KCHVMSTOR0016E") + params['format'] = vol_info['format'] + params['path'] = vol_info['path'] + params['src_type'] = _check_path(params['path']) params.setdefault( 'bus', _get_device_bus(params['type'], dom)) if (params['bus'] not in HOTPLUG_TYPE @@ -212,7 +227,7 @@ class VMStorageModel(object): raise OperationFailed("KCHVMSTOR0010E", {'error': e.message}) def update(self, vm_name, dev_name, params): - params['src_type'] = _check_cdrom_path(params['path']) + params['src_type'] = _check_path(params['path']) dom = VMModel.get_vm(vm_name, self.conn) dev_info = self.lookup(vm_name, dev_name) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Fix vmstorages.py and mockmodel.py to comply with pep8. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 14 ++++++++------ src/kimchi/model/vmstorages.py | 8 +++++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index fb067a8..77f6494 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -669,8 +669,8 @@ class MockModel(object): dom = self._get_vm(vm_name) dev = params.get('dev', None) if dev and dev in self.vmstorages_get_list(vm_name): - return OperationFailed("KCHVMSTOR0004E", {'dev_name': dev, - 'vm_name': vm_name}) + return OperationFailed( + "KCHVMSTOR0004E", {'dev_name': dev, 'vm_name': vm_name}) if not dev: index = len(dom.storagedevices.keys()) + 1 params['dev'] = "hd" + string.ascii_lowercase[index] @@ -686,15 +686,17 @@ class MockModel(object): def vmstorage_lookup(self, vm_name, dev_name): dom = self._get_vm(vm_name) if dev_name not in self.vmstorages_get_list(vm_name): - raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, - 'vm_name': vm_name}) + raise NotFoundError( + "KCHVMSTOR0007E", + {'dev_name': dev_name, 'vm_name': vm_name}) return dom.storagedevices.get(dev_name).info def vmstorage_delete(self, vm_name, dev_name): dom = self._get_vm(vm_name) if dev_name not in self.vmstorages_get_list(vm_name): - raise NotFoundError("KCHVMSTOR0007E", {'dev_name': dev_name, - 'vm_name': vm_name}) + raise NotFoundError( + "KCHVMSTOR0007E", + {'dev_name': dev_name, 'vm_name': vm_name}) dom.storagedevices.pop(dev_name) def vmstorage_update(self, vm_name, dev_name, params): diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 9e2248b..ed8843b 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -126,7 +126,8 @@ class VMStoragesModel(object): valid_id.remove((bus_id, unit_id)) continue if not valid_id: - raise OperationFailed('KCHVMSTOR0014E', {'type': 'ide', 'limit': 4}) + raise OperationFailed('KCHVMSTOR0014E', + {'type': 'ide', 'limit': 4}) else: address = {'controller': controller_id, 'bus': valid_id[0][0], 'unit': valid_id[0][1]} @@ -141,8 +142,9 @@ class VMStoragesModel(object): else: devices = self.get_list(vm_name) if dev_name in devices: - raise OperationFailed('KCHVMSTOR0004E', {'dev_name': dev_name, - 'vm_name': vm_name}) + raise OperationFailed( + 'KCHVMSTOR0004E', + {'dev_name': dev_name, 'vm_name': vm_name}) # Path will never be blank due to API.json verification. # There is no need to cover this case here. -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Tested attach and detach disks, according to current vm distro and version, choose proper bus for disk if no bus assigned. Hot plug for virtio disk works while ide disk does not. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/test_model.py b/tests/test_model.py index ab22012..d588600 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -173,6 +173,81 @@ class ModelTests(unittest.TestCase): self.assertEquals("virtio", iface["model"]) @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_vm_disk(self): + def _attach_disk(bus_type=None): + disk_args = {"type": "disk", + "pool": pool, + "vol": vol} + if bus_type: + disk_args['bus'] = bus_type + else: + bus_type = 'virtio' + disk = inst.vmstorages_create(vm_name, disk_args) + storage_list = inst.vmstorages_get_list(vm_name) + self.assertEquals(prev_count + 1, len(storage_list)) + + # Check the bus type to be 'virtio' + disk_info = inst.vmstorage_lookup(vm_name, disk) + self.assertEquals(u'disk', disk_info['type']) + self.assertEquals(vol_path, disk_info['path']) + self.assertEquals(bus_type, disk_info['bus']) + return disk + + inst = model.Model(objstore_loc=self.tmp_store) + with RollbackContext() as rollback: + path = '/tmp/kimchi-images' + pool = 'test-pool' + vol = 'test-volume.img' + vol_path = "%s/%s" % (path, vol) + if not os.path.exists(path): + os.mkdir(path) + + args = {'name': pool, + 'path': path, + 'type': 'dir'} + inst.storagepools_create(args) + rollback.prependDefer(inst.storagepool_delete, pool) + + # Activate the pool before adding any volume + inst.storagepool_activate(pool) + rollback.prependDefer(inst.storagepool_deactivate, pool) + + params = {'name': vol, + 'capacity': 1024, + 'allocation': 512, + 'format': 'qcow2'} + inst.storagevolumes_create(pool, params) + rollback.prependDefer(inst.storagevolume_delete, pool, vol) + + vm_name = 'kimchi-cdrom' + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso} + inst.templates_create(params) + rollback.prependDefer(inst.template_delete, 'test') + params = {'name': vm_name, 'template': '/templates/test'} + inst.vms_create(params) + rollback.prependDefer(inst.vm_delete, vm_name) + + prev_count = len(inst.vmstorages_get_list(vm_name)) + self.assertEquals(1, prev_count) + + # Cold plug and unplug a disk + disk = _attach_disk() + inst.vmstorage_delete(vm_name, disk) + + # Hot plug a disk + inst.vm_start(vm_name) + disk = _attach_disk() + inst.vmstorage_delete(vm_name, disk) + + # Hot plug 'ide' bus disk does not work + self.assertRaises(InvalidOperation, _attach_disk, 'ide') + inst.vm_poweroff(vm_name) + + # Cold plug 'ide' bus disk can work + disk = _attach_disk() + inst.vmstorage_delete(vm_name, disk) + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_cdrom(self): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: -- 1.8.3.2
participants (1)
-
lvroyce＠linux.vnet.ibm.com