You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Marcus Sorensen <sh...@gmail.com> on 2012/11/15 06:05:28 UTC
Re: Review Request: resize volume initial implementation
I've been sitting on this for awhile now, just wondering how to proceed.
It's actually a bit intimidating with all of the 'refactoring' going on,
wondering what's worth working on or going forward with and what's not.
Assuming this implementation generally looks good, is it worth merging
somewhere, and where? Does a new API call like this warrant its own branch
so that others can help work on it (for example the Xen specific pieces are
still missing here), or should it be put in a feature branch? Is that what
javelin is or is that only for specific features?
To me this seems like a fairly innocuous change, considering that most of
the functions are new, and the only existing code it touches is in adding
definitions for those functions and a few things like states. I'm just not
sure what the procedure is, especially as things seem to be in the process
of a major overhaul.
On Fri, Oct 5, 2012 at 5:07 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7099/
> Review request for cloudstack.
> By Marcus Sorensen.
>
> *Updated Oct. 5, 2012, 11:07 p.m.*
> Changes
>
> adjusted size input to be more like other disk size inputs (accepts integer gigabytes only).
>
> Description
>
> Initial implementation of resize volumes. Works only for KVM but can be easily used to add in other hypervisors. Works with local,sharedmountpoint,NFS,and CLVM storage. This is a significant chunk of code and my first attempt at a new API call so please let me know if there are any issues with where/how things are done.
>
> This KVM implementation includes a host script "resizevolume.sh" because of several current limitations. 1) we don't seem to have java bindings for virStorageVolResize() or virDomainBlockResize(), and even if we did, virStorageVolResize() doesn't work for logical volume pools. It will presumably be awhile before that's patched and available on current distros.
>
> New API call is 'resizeVolume', with parameters:
>
> 'id' for volume id
>
> 'size' for new size, accepts things like 10G, 10240M, 10485760K, 10737418240B or 10737418240
>
> 'shrinkok' this one is a boolean that confirms the user is ok with the volume shrinking. I did this because it seems reasonable that someone might want to give back storage, and since it's potentially dangerous (users need to free up the end of the block device that they want to give back) there needs to be some sort of signoff. This can be disabled/removed if others think it's too much of a liability. The code checks size twice, comparing the requested size once against what we think the volume size is per database, and once again comparing the actual qcow2/lv size against the requested size, both times ensuring that shrinkok is true before continuing.
>
> If the resize succeeds, but libvirt fails to live update qemu of the new size (whether due to bug, non-virtio disks, or something else), there's currently no indication other than that the API call returns the new size as seen from libvirt, which itself should be an indication that a powercycle is needed if the API call succeeds, the size is what was requested, and the host isn't seeing the new size. Perhaps a field should be added, like 'rebootrequired:true' to make it easy.
>
> One thing I haven't tackled at all is how to handle the service offering fields. If someone has a service offering with a static 5GB discription like the default storage offerings have, that won't change. Suggestions welcome. Should we update the service offering to custom, or could that mess up other things like tags?
>
> Testing
>
> Tested CLVM,NFS,local,sharedmountpoint, qcow2 and lvm formats
>
> create test volumes on above listed pools, attach to VM instance
>
> within instance, format as ext4, populate with files, grow, resize filesystem: pass
>
> within instance, format as ext4, populate with files, shrink filesystem, shrink volume, unmount, fsck, remount: pass
>
> try passing bad arguments to API call fails as expected
>
> try resizing as wrong user fails as expected
>
> force resizevolume.sh to fail through various means bubbles the error up as expected, resets the volume state to Ready
>
>
>
>
> Diffs (updated)
>
> - api/src/com/cloud/agent/api/storage/ResizeVolumeAnswer.java (e69de29)
> - api/src/com/cloud/agent/api/storage/ResizeVolumeCommand.java
> (e69de29)
> - api/src/com/cloud/api/ApiConstants.java (067ddf7)
> - api/src/com/cloud/api/commands/ResizeVolumeCmd.java (e69de29)
> - api/src/com/cloud/event/EventTypes.java (e84a403)
> - api/src/com/cloud/storage/StorageService.java (4fb3b55)
> - api/src/com/cloud/storage/Volume.java (6e8e48e)
> - client/tomcatconf/commands.properties.in (e233694)
> - plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
> (f6a6494)
> - scripts/storage/qcow2/resizevolume.sh (e69de29)
> - server/src/com/cloud/storage/StorageManagerImpl.java (fc6fb5b)
>
> View Diff <https://reviews.apache.org/r/7099/diff/>
>
RE: Review Request: resize volume initial implementation
Posted by Koushik Das <ko...@citrix.com>.
Hi Marcus,
What is the latest on this? Is this checked into master branch? I am looking at the feasibility to do this on Vmware and XS. So if there is a consensus on the commands and interfaces was planning to reuse them.
Some points to consider
- Should resize volume be done based on a new disk offering with the new size etc.? Or is it better to have a custom disk offering where the user can specify a size?
- I feel it is safer to only allow increasing the size.
- Also there will be an additional step required to be performed from within the guest vm for extending the partition, this will be a manual step and needs to be documented.
- Need to put some validations like if the volume has some snapshots then resize won't be allowed.
- If based on the new size the volume cannot be fitted into primary storage, should the operation fail with error or should there be a migration? The capacities need to be properly updated if the resize goes through so that the allocator can take it into account for all subsequent vm deployments
Thanks,
Koushik
-----Original Message-----
From: Marcus Sorensen [mailto:shadowsor@gmail.com]
Sent: Thursday, November 15, 2012 10:35 AM
To: Marcus Sorensen; cloudstack
Subject: Re: Review Request: resize volume initial implementation
I've been sitting on this for awhile now, just wondering how to proceed.
It's actually a bit intimidating with all of the 'refactoring' going on, wondering what's worth working on or going forward with and what's not.
Assuming this implementation generally looks good, is it worth merging somewhere, and where? Does a new API call like this warrant its own branch so that others can help work on it (for example the Xen specific pieces are still missing here), or should it be put in a feature branch? Is that what javelin is or is that only for specific features?
To me this seems like a fairly innocuous change, considering that most of the functions are new, and the only existing code it touches is in adding definitions for those functions and a few things like states. I'm just not sure what the procedure is, especially as things seem to be in the process of a major overhaul.
On Fri, Oct 5, 2012 at 5:07 PM, Marcus Sorensen <sh...@gmail.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7099/
> Review request for cloudstack.
> By Marcus Sorensen.
>
> *Updated Oct. 5, 2012, 11:07 p.m.*
> Changes
>
> adjusted size input to be more like other disk size inputs (accepts integer gigabytes only).
>
> Description
>
> Initial implementation of resize volumes. Works only for KVM but can be easily used to add in other hypervisors. Works with local,sharedmountpoint,NFS,and CLVM storage. This is a significant chunk of code and my first attempt at a new API call so please let me know if there are any issues with where/how things are done.
>
> This KVM implementation includes a host script "resizevolume.sh" because of several current limitations. 1) we don't seem to have java bindings for virStorageVolResize() or virDomainBlockResize(), and even if we did, virStorageVolResize() doesn't work for logical volume pools. It will presumably be awhile before that's patched and available on current distros.
>
> New API call is 'resizeVolume', with parameters:
>
> 'id' for volume id
>
> 'size' for new size, accepts things like 10G, 10240M, 10485760K,
> 10737418240B or 10737418240
>
> 'shrinkok' this one is a boolean that confirms the user is ok with the volume shrinking. I did this because it seems reasonable that someone might want to give back storage, and since it's potentially dangerous (users need to free up the end of the block device that they want to give back) there needs to be some sort of signoff. This can be disabled/removed if others think it's too much of a liability. The code checks size twice, comparing the requested size once against what we think the volume size is per database, and once again comparing the actual qcow2/lv size against the requested size, both times ensuring that shrinkok is true before continuing.
>
> If the resize succeeds, but libvirt fails to live update qemu of the new size (whether due to bug, non-virtio disks, or something else), there's currently no indication other than that the API call returns the new size as seen from libvirt, which itself should be an indication that a powercycle is needed if the API call succeeds, the size is what was requested, and the host isn't seeing the new size. Perhaps a field should be added, like 'rebootrequired:true' to make it easy.
>
> One thing I haven't tackled at all is how to handle the service offering fields. If someone has a service offering with a static 5GB discription like the default storage offerings have, that won't change. Suggestions welcome. Should we update the service offering to custom, or could that mess up other things like tags?
>
> Testing
>
> Tested CLVM,NFS,local,sharedmountpoint, qcow2 and lvm formats
>
> create test volumes on above listed pools, attach to VM instance
>
> within instance, format as ext4, populate with files, grow, resize
> filesystem: pass
>
> within instance, format as ext4, populate with files, shrink
> filesystem, shrink volume, unmount, fsck, remount: pass
>
> try passing bad arguments to API call fails as expected
>
> try resizing as wrong user fails as expected
>
> force resizevolume.sh to fail through various means bubbles the error
> up as expected, resets the volume state to Ready
>
>
>
>
> Diffs (updated)
>
> - api/src/com/cloud/agent/api/storage/ResizeVolumeAnswer.java (e69de29)
> - api/src/com/cloud/agent/api/storage/ResizeVolumeCommand.java
> (e69de29)
> - api/src/com/cloud/api/ApiConstants.java (067ddf7)
> - api/src/com/cloud/api/commands/ResizeVolumeCmd.java (e69de29)
> - api/src/com/cloud/event/EventTypes.java (e84a403)
> - api/src/com/cloud/storage/StorageService.java (4fb3b55)
> - api/src/com/cloud/storage/Volume.java (6e8e48e)
> - client/tomcatconf/commands.properties.in (e233694)
> - plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
> (f6a6494)
> - scripts/storage/qcow2/resizevolume.sh (e69de29)
> - server/src/com/cloud/storage/StorageManagerImpl.java (fc6fb5b)
>
> View Diff <https://reviews.apache.org/r/7099/diff/>
>