cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niels de Vos" <nde...@redhat.com>
Subject Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
Date Tue, 14 Jan 2014 15:50:17 GMT


> On Dec. 4, 2013, 5:17 p.m., John Burwell wrote:
> >
> 
> Niels de Vos wrote:
>     Thanks John! I'll have a go at impoving the patch and will update it (hopefully)
soon.
> 
> Amogh Vasekar wrote:
>     Reminder - 
>     Hi,
>     This review has been pending for long. Please do update the patch so it may be shipped
soon.
>     Thanks!

Thanks for the reminder. I think all comments have been addressed in the next version of the
patch. I have not rewritten existing parts that could get improved based on the comments (like
StringBuilder). My preference is to keep this patch Gluster specific and not modify unrelated
bits.

Cheers,
Niels


> On Dec. 4, 2013, 5:17 p.m., John Burwell wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java,
line 133
> > <https://reviews.apache.org/r/15932/diff/1/?file=392660#file392660line133>
> >
> >     If the Gluster poolType is netfs, why isn't this rule captured in the enumeration?
 This feels like a leaky abstraction that will be difficult to maintain.
> >     
> >     Also, concatenating strings as part of StringBuilder append cancels out the
performance benefits of StringBuilder.  This code should be converted to the following:
> >     
> >       storagePoolBuilder.append("<pool type='");
> >       storagePoolBuilder.append(_poolType);
> >       storagePoolBuilder.append("'>");
> >      storagePoolBuilder.append(System.lineSeparator());

Currently libvirt can only mount glusterfs with help from the glusterfs-fuse client. In libvirt,
this is configured as a "netfs"-pool with type=glusterfs. In future, libvirt will be able
to use the native GlusterFS protocol (through libgfapi.so), which will result in a different
type of pool.

I do not see how to write the code for a netfs+glusterfs much cleaner.

- StringBuilder changes have been applied.


> On Dec. 4, 2013, 5:17 p.m., John Burwell wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java,
line 362
> > <https://reviews.apache.org/r/15932/diff/1/?file=392661#file392661line362>
> >
> >     Extract this if block to factory method on the poolType enumeration (e.g. toStoragePoolType).
 It should also include a catch all that throws a CloudRuntimeException for an unsupported
type mapping.

I'm not sure how you intend this. It seems very common to use if-else-if statements for this...


> On Dec. 4, 2013, 5:17 p.m., John Burwell wrote:
> > plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java,
line 286
> > <https://reviews.apache.org/r/15932/diff/1/?file=392662#file392662line286>
> >
> >     Magic value.  This default port value should be captured somewhere in the Gluster
driver as a constant.

It follows the same concept as the existing other drivers... I'm not sure where it would be
more suitable to place this.


- Niels


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15932/#review29591
-----------------------------------------------------------


On Dec. 1, 2013, 3:07 p.m., Niels de Vos wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15932/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2013, 3:07 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> The support for Gluster as Primary Storage is mostly based on the
> implementation for NFS. Like NFS, libvirt can address a Gluster environment
> through the 'netfs' pool-type.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/storage/Storage.java 07b6667 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
182cb22 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java
e181cea 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
0760e51 
>   plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java
7555c1e 
> 
> Diff: https://reviews.apache.org/r/15932/diff/
> 
> 
> Testing
> -------
> 
> See http://blog.nixpanic.net/2013/12/using-gluster-as-primary-storage-in.html
> 
> 
> Thanks,
> 
> Niels de Vos
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message