[Kimchi-devel] [PATCH V2] Create new storage volume when attaching disk to a guest.
Rodrigo Trujillo
rodrigo.trujillo at linux.vnet.ibm.com
Tue Nov 17 03:36:01 UTC 2015
Comments below
On 11/11/2015 11:43 AM, pvital at linux.vnet.ibm.com wrote:
> From: Paulo Vital <pvital at 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 at 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,
More information about the Kimchi-devel
mailing list