Comments below On 11/11/2015 11:43 AM, pvital@linux.vnet.ibm.com wrote:

From: Paulo Vital <pvital@linux.vnet.ibm.com>

Add back-end support to create new storage volume (new virtual disk) when attaching disk to a guest created before.

There are three essential parameters to create the new volume: * vol: Storage volume name of disk image, that should be 'new_vol'. * capacity: The total space which can be used to store new volumes. The unit is bytes. * format: The format of the defined Storage Volume. Only used when creating a storage volume with 'capacity'.

Signed-off-by: Paulo Vital <pvital@linux.vnet.ibm.com> --- src/wok/plugins/kimchi/API.json | 12 ++++++++++++ src/wok/plugins/kimchi/docs/API.md | 6 +++++- src/wok/plugins/kimchi/i18n.py | 1 + src/wok/plugins/kimchi/model/vmstorages.py | 28 +++++++++++++++++++++++++++- src/wok/plugins/kimchi/tests/test_model.py | 25 +++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/src/wok/plugins/kimchi/API.json b/src/wok/plugins/kimchi/API.json index 961f35f..8822520 100644 --- a/src/wok/plugins/kimchi/API.json +++ b/src/wok/plugins/kimchi/API.json @@ -564,6 +564,18 @@ "type": "string", "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", "error": "KCHVMSTOR0003E" + }, + "capacity": { + "description": "The total size (MiB) of the storage volume", + "type": "number", + "minimum": 1, + "error": "KCHVOL0020E" + }, + "format": { + "description": "The format of the volume", + "type": "string", + "pattern": "^(|bochs|cloop|cow|dmg|qcow|qcow2|qed|raw|vmdk|vpc)$", + "error": "KCHVOL0015E" } } }, diff --git a/src/wok/plugins/kimchi/docs/API.md b/src/wok/plugins/kimchi/docs/API.md index 52368b7..d00c2fe 100644 --- a/src/wok/plugins/kimchi/docs/API.md +++ b/src/wok/plugins/kimchi/docs/API.md @@ -205,7 +205,11 @@ Represents a snapshot of the Virtual Machine's primary monitor. * 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. + * vol: Storage volume name of disk image ('new_vol' for create a new volume). According to your code, below, it is possible to create a new disk image only passing CAPACITY Must change here, or the code.

+ * capacity: The total space which can be used to store new volumes. + The unit is bytes. + * format: The format of the defined Storage Volume. Only used when creating + a storage volume with 'capacity'. Add MiB in the capacity description ### Sub-resource: storage **URI:** /plugins/kimchi/vms/*:name*/storages/*:dev* diff --git a/src/wok/plugins/kimchi/i18n.py b/src/wok/plugins/kimchi/i18n.py index 42a5e16..53a0b6b 100644 --- a/src/wok/plugins/kimchi/i18n.py +++ b/src/wok/plugins/kimchi/i18n.py @@ -278,6 +278,7 @@ messages = { "KCHVMSTOR0016E": _("Volume already in use by other virtual machine."), "KCHVMSTOR0017E": _("Only one of path or pool/volume can be specified to add a new virtual machine disk"), "KCHVMSTOR0018E": _("Volume chosen with format %(format)s does not fit in the storage type %(type)s"), + "KCHVMSTOR0019E": _("The format msust be used only when creating a new storage volume with 'capacity'."), Typo: must

"KCHSNAP0001E": _("Virtual machine '%(vm)s' must be stopped before creating a snapshot of it."), "KCHSNAP0002E": _("Unable to create snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"), diff --git a/src/wok/plugins/kimchi/model/vmstorages.py b/src/wok/plugins/kimchi/model/vmstorages.py index 23db0a6..29cdc73 100644 --- a/src/wok/plugins/kimchi/model/vmstorages.py +++ b/src/wok/plugins/kimchi/model/vmstorages.py @@ -22,12 +22,14 @@ from lxml import etree

from wok.exception import InvalidOperation, InvalidParameter, NotFoundError from wok.exception import OperationFailed +from wok.model.tasks import TaskModel from wok.utils import wok_log

from wok.plugins.kimchi.model.config import CapabilitiesModel from wok.plugins.kimchi.model.diskutils import get_disk_used_by from wok.plugins.kimchi.model.diskutils import set_disk_used_by from wok.plugins.kimchi.model.storagevolumes import StorageVolumeModel +from wok.plugins.kimchi.model.storagevolumes import StorageVolumesModel from wok.plugins.kimchi.model.utils import get_vm_config_flag from wok.plugins.kimchi.model.vms import DOM_STATE_MAP, VMModel from wok.plugins.kimchi.osinfo import lookup @@ -51,6 +53,7 @@ class VMStoragesModel(object): self.conn = kargs['conn'] self.objstore = kargs['objstore'] self.caps = CapabilitiesModel(**kargs) + self.task = TaskModel(**kargs)

def _get_available_bus_address(self, bus_type, vm_name): if bus_type not in ['ide']: @@ -85,9 +88,12 @@ class VMStoragesModel(object): if not ('vol' in params) ^ ('path' in params): raise InvalidParameter("KCHVMSTOR0017E")

+ if ('format' in params) and ('capacity' not in params): + raise InvalidParameter("KCHVMSTOR0019E") + dom = VMModel.get_vm(vm_name, self.conn) params['bus'] = _get_device_bus(params['type'], dom) - params['format'] = 'raw' + params['format'] = params.get('format', 'raw')

Is there any reason to default format be different of QCOW2 ? You could check the default value being used in Templates (template.conf)

dev_list = [dev for dev, bus in get_vm_disks(dom).iteritems() if bus == params['bus']] @@ -102,6 +108,26 @@ class VMStoragesModel(object): DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): raise InvalidOperation('KCHVMSTOR0011E')

Here I see a issue. The condition to create a new volume is passing a CAPACITY only. What happens if 'vol' is not passed ? I think you are going to have exception in [1] and [2] I think you should check for vol= new_vol, then check the capacity and issue a error, if capacity not there

+ if params.get('capacity'): + # If 'capacity' is in the parameters a new storage volume will + # be created and allocated to the VM. [1] + if params['vol'] == 'new_vol': + # Set the name of the volume as the same pattern used when + # creating a VM based on Template (VM UUID + disk index). + # Otherwise, use the name provided by the user. + params['vol'] = "%s-%s.img" % (dom.UUIDString(), + params['index']) [2] + vol_info = dict(name=params['vol'], + type=params['type'], + capacity=params['capacity'], + allocation=params['capacity'], + format=params['format'], + ) + storage_volumes = StorageVolumesModel(conn=self.conn, + objstore=self.objstore) + mytask = storage_volumes.create(params['pool'], vol_info) + self.task.wait(mytask['id']) + if params.get('vol'): try: pool = params['pool'] diff --git a/src/wok/plugins/kimchi/tests/test_model.py b/src/wok/plugins/kimchi/tests/test_model.py index fdbe3b4..8cc3f14 100644 --- a/src/wok/plugins/kimchi/tests/test_model.py +++ b/src/wok/plugins/kimchi/tests/test_model.py @@ -406,6 +406,19 @@ class ModelTests(unittest.TestCase): self.assertEquals(expect_bus, disk_info['bus']) return disk

+ def _attach_new_disk(expect_bus=modern_disk_bus): + disk_args = {"type": "disk", + "pool": pool, + "vol": 'new_vol', + "format": 'qcow2', + "capacity": 1024} + new_disk = inst.vmstorages_create(vm_name, disk_args) + + # Check the bus type to be 'virtio' + disk_info = inst.vmstorage_lookup(vm_name, new_disk) + self.assertEquals(expect_bus, disk_info['bus']) + return new_disk + inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: path = os.path.join(TMP_DIR, 'kimchi-images') @@ -466,6 +479,18 @@ class ModelTests(unittest.TestCase): self.assertEquals(u'disk', disk_info['type']) inst.vmstorage_delete(vm_name, disk)

+ # Create a new volume when attaching a disks + inst.vm_start(vm_name) + prev_disk_count = len(inst.vmstorages_get_list(vm_name)) + new_disk = _attach_new_disk() + curr_disk_count = len(inst.vmstorages_get_list(vm_name)) + self.assertEquals(prev_disk_count+1, curr_disk_count) + # The number of disks must be the same after VM off.

Is it necessary to stop the VM to remove the disk ? Can I remove the disk with the vm running ? You could add one more disk and remove it with vm running.

+ inst.vm_poweroff(vm_name) + inst.vmstorage_delete(vm_name, new_disk) + after_disk_count = len(inst.vmstorages_get_list(vm_name)) + self.assertEquals(prev_disk_count, after_disk_count) + # Specifying pool and path at same time will fail disk_args = {"type": "disk", "pool": pool,