[PATCH 0/7] Manage guest disks
From: Royce Lv <lvroyce@linux.vnet.ibm.com> This patchset deals with guest disk management, when bus is given, attach/detach behavior is based on bus to hot plug or cold plug. when bus is not given, kimchi will look for distro info in db to decide proper bus for disk. Depend on: [PATCH]Choose available address for ide disk Tested: 1. make check. 2. cold plug with ide bus succeeds. 3. hot plug with ide bus fails. 4. when os version/distro unrecorded, default bus is chosed. 5. when os version/distro recorded, bus is chosed based on distro info. 6. hot plug/unplug for virtio bus succeeds. Royce Lv (7): Guest disks: Update doc to support manage guest disks Guest disks: Update api definition and error reporting fix: store distro and version information when creating vm Guest disks: Choose proper bus for device fix: Change check path function naming Multiple pep8 fixes Guest disks: Update testcase docs/API.md | 10 +++--- src/kimchi/API.json | 24 ++++++++----- src/kimchi/i18n.py | 27 ++++++++------- src/kimchi/mockmodel.py | 18 +++++----- src/kimchi/model/vms.py | 23 +++++++------ src/kimchi/model/vmstorages.py | 78 +++++++++++++++++++++++++++--------------- tests/test_model.py | 62 +++++++++++++++++++++++++++++++++ 7 files changed, 170 insertions(+), 72 deletions(-) -- 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 | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/API.md b/docs/API.md index 143c70c..223d57a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -122,17 +122,17 @@ 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). - * 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. ### 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. * **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
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
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 | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/docs/API.md b/docs/API.md index 143c70c..223d57a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -122,17 +122,17 @@ 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). - * 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.
### 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. * **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.
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 | 24 +++++++++++++++--------- src/kimchi/i18n.py | 27 ++++++++++++++------------- src/kimchi/mockmodel.py | 10 +++++----- src/kimchi/model/vmstorages.py | 26 ++++++++++++++------------ 4 files changed, 48 insertions(+), 39 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 5ca94e3..2e76864 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -384,40 +384,46 @@ }, "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" }, "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 e8491ec..f102166 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -220,19 +220,20 @@ 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"), - "KCHCDROM0006E": _("Can't specify a directory for a CDROM device path"), - "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 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"), "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 bf17975..1ffbeb8 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -647,12 +647,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 @@ -669,14 +669,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) @@ -685,7 +685,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
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
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 | 24 +++++++++++++++--------- src/kimchi/i18n.py | 27 ++++++++++++++------------- src/kimchi/mockmodel.py | 10 +++++----- src/kimchi/model/vmstorages.py | 26 ++++++++++++++------------ 4 files changed, 48 insertions(+), 39 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 5ca94e3..2e76864 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -384,40 +384,46 @@ }, "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" }, "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 e8491ec..f102166 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -220,19 +220,20 @@ 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"), - "KCHCDROM0006E": _("Can't specify a directory for a CDROM device path"), - "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 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"),
"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 bf17975..1ffbeb8 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -647,12 +647,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 @@ -669,14 +669,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)
@@ -685,7 +685,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
From: Royce Lv <lvroyce@linux.vnet.ibm.com> We need to decide which cdrom bus and disk bus to use on different architecture and different guest distros. So record these information for attach disk to decide bus type. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 2425a43..94e7d79 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -178,16 +178,19 @@ class VMsModel(object): t.validate() # Store the icon for displaying later - icon = t.info.get('icon') - if icon: - try: - with self.objstore as session: - session.store('vm', vm_uuid, {'icon': icon}) - except Exception as e: - # It is possible to continue Kimchi executions without store - # vm icon info - kimchi_log.error('Error trying to update database with guest ' - 'icon information due error: %s', e.message) + icon = t.info.get('icon', 'images/icon-vm.png') + distro = t.info.get('os_distro', 'unknown') + version = t.info.get('os_version', 'unknown') + try: + with self.objstore as session: + session.store( + 'vm', vm_uuid, + {'icon': icon, 'distro': distro, 'version': version}) + except Exception as e: + # It is possible to continue Kimchi executions without store + # vm icon info + kimchi_log.error('Error trying to update database with guest ' + 'icon information due error: %s', e.message) # If storagepool is SCSI, volumes will be LUNs and must be passed by # the user from UI or manually. -- 1.8.3.2
On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
We need to decide which cdrom bus and disk bus to use on different architecture and different guest distros. So record these information for attach disk to decide bus type.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 2425a43..94e7d79 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -178,16 +178,19 @@ class VMsModel(object): t.validate()
# Store the icon for displaying later - icon = t.info.get('icon') - if icon: - try: - with self.objstore as session: - session.store('vm', vm_uuid, {'icon': icon}) - except Exception as e: - # It is possible to continue Kimchi executions without store - # vm icon info - kimchi_log.error('Error trying to update database with guest ' - 'icon information due error: %s', e.message) + icon = t.info.get('icon', 'images/icon-vm.png') + distro = t.info.get('os_distro', 'unknown') + version = t.info.get('os_version', 'unknown') + try: + with self.objstore as session: + session.store( + 'vm', vm_uuid, + {'icon': icon, 'distro': distro, 'version': version}) + except Exception as e: + # It is possible to continue Kimchi executions without store + # vm icon info + kimchi_log.error('Error trying to update database with guest ' + 'icon information due error: %s', e.message)
# If storagepool is SCSI, volumes will be LUNs and must be passed by # the user from UI or manually.
The code is good but based in what we discussed in the scrum meeting today I think you will change it to store the bus type on VM metadata, right? As we will do for interface model.
On 2014年04月17日 09:39, Aline Manera wrote:
On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
We need to decide which cdrom bus and disk bus to use on different architecture and different guest distros. So record these information for attach disk to decide bus type.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 2425a43..94e7d79 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -178,16 +178,19 @@ class VMsModel(object): t.validate()
# Store the icon for displaying later - icon = t.info.get('icon') - if icon: - try: - with self.objstore as session: - session.store('vm', vm_uuid, {'icon': icon}) - except Exception as e: - # It is possible to continue Kimchi executions without store - # vm icon info - kimchi_log.error('Error trying to update database with guest ' - 'icon information due error: %s', e.message) + icon = t.info.get('icon', 'images/icon-vm.png') + distro = t.info.get('os_distro', 'unknown') + version = t.info.get('os_version', 'unknown') + try: + with self.objstore as session: + session.store( + 'vm', vm_uuid, + {'icon': icon, 'distro': distro, 'version': version}) + except Exception as e: + # It is possible to continue Kimchi executions without store + # vm icon info + kimchi_log.error('Error trying to update database with guest ' + 'icon information due error: %s', e.message)
# If storagepool is SCSI, volumes will be LUNs and must be passed by # the user from UI or manually.
The code is good but based in what we discussed in the scrum meeting today I think you will change it to store the bus type on VM metadata, right? As we will do for interface model.
ACK
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 | 43 ++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index c31af24..6fdf580 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,16 @@ def _get_device_xml(dom, dev_name): return disk[0] +def _get_device_bus(dev_type, dom, objstore): + try: + with objstore as session: + vm_info = session.get('vm', dom.UUIDString()) + distro, version = (vm_info['distro'], vm_info['version']) + except: + distro, version = ('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 +77,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( @@ -104,8 +116,11 @@ def _check_cdrom_path(path): class VMStoragesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore'] - 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 +145,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 +160,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, self.objstore)) + 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 +236,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
On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
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 | 43 ++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index c31af24..6fdf580 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,16 @@ def _get_device_xml(dom, dev_name): return disk[0]
+def _get_device_bus(dev_type, dom, objstore): + try: + with objstore as session: + vm_info = session.get('vm', dom.UUIDString()) + distro, version = (vm_info['distro'], vm_info['version']) + except: + distro, version = ('unknown', 'unknown') + return lookup(distro, version)[dev_type+'_bus'] + +
The same I commented in previous patch about store bus type in VM metadata
def _get_storage_xml(params): src_type = params.get('src_type') disk = E.disk(type=src_type, device=params.get('type')) @@ -65,7 +77,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( @@ -104,8 +116,11 @@ def _check_cdrom_path(path): class VMStoragesModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore']
- 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 +145,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 +160,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, self.objstore)) + 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 +236,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
Please, raise the proper error or fallback to a default bus_type
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:
From: Royce Lv <lvroyce@linux.vnet.ibm.com> We just check cdrom path previously, change the naming to be compatible with disks. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 6fdf580..4ef8ab0 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -87,7 +87,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 @@ -158,7 +158,7 @@ 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['src_type'] = _check_path(path) params.setdefault( 'bus', _get_device_bus(params['type'], dom, self.objstore)) @@ -256,7 +256,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
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
We just check cdrom path previously, change the naming to be compatible with disks.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 6fdf580..4ef8ab0 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -87,7 +87,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 @@ -158,7 +158,7 @@ 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['src_type'] = _check_path(path)
params.setdefault( 'bus', _get_device_bus(params['type'], dom, self.objstore)) @@ -256,7 +256,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)
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 | 13 ++++++++----- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 1ffbeb8..097d3dd 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -652,8 +652,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] @@ -669,15 +669,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 4ef8ab0..54e0229 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -137,7 +137,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]} @@ -152,8 +153,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. @@ -208,8 +210,9 @@ class VMStorageModel(object): 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}) + raise NotFoundError( + "KCHVMSTOR0007E", + {'dev_name': dev_name, 'vm_name': vm_name}) path = "" dev_bus = 'ide' try: -- 1.8.3.2
On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
Fix vmstorages.py and mockmodel.py to comply with pep8.
The code looks good just a comment vmstorages.py is not listed in PEP8_WHITELIST - maybe you can add it there too
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 14 ++++++++------ src/kimchi/model/vmstorages.py | 13 ++++++++----- 2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 1ffbeb8..097d3dd 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -652,8 +652,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] @@ -669,15 +669,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 4ef8ab0..54e0229 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -137,7 +137,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]} @@ -152,8 +153,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. @@ -208,8 +210,9 @@ class VMStorageModel(object): 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}) + raise NotFoundError( + "KCHVMSTOR0007E", + {'dev_name': dev_name, 'vm_name': vm_name}) path = "" dev_bus = 'ide' try:
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 | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/test_model.py b/tests/test_model.py index 3041196..a5d2a06 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -167,6 +167,68 @@ 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", + "path": disk_path} + 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(disk_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: + 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) + + # dummy .iso files + disk_path = '/tmp/existent.qcow2' + disk_path2 = '/tmp/existent2.qcow2' + open(disk_path, 'w').close() + rollback.prependDefer(os.remove, disk_path) + open(disk_path2, 'w').close() + + # 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() + + # update path is not supported for disk + self.assertRaises( + InvalidOperation, inst.vmstorage_update, + vm_name, disk, {'path': disk_path2}) + 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
On 04/13/2014 05:16 AM, lvroyce@linux.vnet.ibm.com wrote:
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 | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
diff --git a/tests/test_model.py b/tests/test_model.py index 3041196..a5d2a06 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -167,6 +167,68 @@ 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", + "path": disk_path} + 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(disk_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: + 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) + + # dummy .iso files + disk_path = '/tmp/existent.qcow2' + disk_path2 = '/tmp/existent2.qcow2' + open(disk_path, 'w').close() + rollback.prependDefer(os.remove, disk_path) + open(disk_path2, 'w').close() +
Add rollback to delete disk_path2 ?
+ # 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() + + # update path is not supported for disk + self.assertRaises( + InvalidOperation, inst.vmstorage_update, + vm_name, disk, {'path': disk_path2}) + 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:
On 04/14/2014 11:37 AM, Aline Manera wrote:
Applied. Thanks.
Regards,
Aline Manera
Sorry, it was not applied. Seems my script failed...
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (3)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com -
Royce Lv