Hi Gabriel, My replies are in-line.
On Wed, Jun 13, 2012 at 4:59 AM, Gabriel Reid <[email protected]>wrote: > Hi, > > I'd like to propose the addition of block-level storage volume management > in LibCloud. In the two environments that I'm familiar with (Amazon EC2 > EBS and > Cloudstack volumes), the way of working is very similar, with the main > operations being: > - create storage volume > - attach storage volume to node > - detach storage volume to node > - destroy storage volume > > There is currently an closed out-of-date JIRA issue dealing with this > issue: > https://issues.apache.org/jira/browse/LIBCLOUD-35. I would propose to use > the > general interface outlined in this issue as a starting point, as shown > below. > > As I'm only familiar with two of the providers that are supported by > LibCloud, > and only in a limited way with both of them, I think it would be very > useful > to have feedback on the interface outlined below, as well as common use > cases > for which this interface may not fit. > > I've already added a portion of the interface below to the Cloudstack > driver > (https://issues.apache.org/jira/browse/LIBCLOUD-208), and would be happy > to > work further on this as well as possibly doing the implementation for EC2 > EBS. > > Any comments or suggestions would be very welcome. > > - Gabriel > > > #------------------------------------------------------------------------------ > > class StorageVolume(object): > """ > A base StorageVolume class to derive from. > """ > > def __init__(self, id, size, driver, extra=None): > self.id = id > self.size = size > self.driver = driver > self.extra = extra > Looks good. > # The following methods could be added to libcloud.compute.base.NodeDriver: > > def create_volume(**kwargs): > """Create a new volume.""" > > @keyword size: Size of volume in gigabytes (required) > @type size: C{int} > > @keyword name: Name of the volume to be created > @type name: C{str} > > @keyword location: Which data center to create a volume in. If > empty, > undefined behavoir will be selected. > (optional) > @type location: L{NodeLocation} > > @keyword snapshot: Name of snapshot from which to create the new > volume. (optional) > @type snapshot: C{str} > > @return: The newly created L{StorageVolume}. > """ > raise NotImplementedError( > 'create_volume not implemented for this driver') > To be consistent with other method names it should just be called "create". Instead of **kwargs we should use actual arguments. All of the arguments besides the location look reasonable to me. I need to check existing providers and see how they handle the location before I can provide more feedback about it. > def destroy_volume(self, volume): > """Destroys a storage volume > > @param volume: Volume to be destroyed > @type volume: L{StorageVolume} > > @return: C{bool} > """ > > raise NotImplementedError( > 'destroy_volume not implemented for this driver') > Looks good. > def attach(self, node, volume, device): > """Attaches volume to node > > @param node: Node to attach volume to > @type node: L{Node} > > @param volume: Volume to attach > @type volume: L{StorageVolume} > > @param device: Where the device is exposed e.g., (/dev/sdb) > @type device: string > > @return: C{bool} > """ > raise NotImplementedError('attach not implemented for this driver') > Looks good. def detach(self, node, volume): > """Detaches a volume from a node > > @param node: Node from which the volume is to be detached > @type node: L{Node} > > @param volume: Volume to be detached > @type volume: L{StorageVolume} > > @returns C{bool} > """ > > raise NotImplementedError('detach not implemented for this driver') > Looks good. Overall the API you proposed looks pretty solid to me. Next step would be implementing this API for at least two providers. When coding a new API we require user to implement it for at least two providers so the implementation is not biased towards a single one. I also closed the old ticket on JIRA. Feel free to open a new one and attach your patch there when you make some progress. - Tomaz
