Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review79012 --- Thank you for submitting your CloudStack contribution through review board. After discussion on the dev@cloudstack.apache.org the community decided to close down review board and start accepting contributiong through GitHub pull requests. We have been using GH PR for several months now and the process is better than review board. We will keep Review Board open for another week to give you time to migrate your patch to a github PR if you wish. After that time, your patch will no longer be viewable (even though it will not be deleted). Please consider submitting a pull request. Great instructions are available at: https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md Thank you very much for your time and your contribution to Apache CloudStack, we hope that using this new process will encourage you to do more. - Sebastien Goasguen On Feb. 19, 2014, 8:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 8:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Feb. 19, 2014, 1:35 p.m., Wido den Hollander wrote: It seems good to me. Applies cleanly to master and builds just fine. Code-wise it's simple but effective, should allow us to support Gluster. Wido den Hollander wrote: I just merged it into master and pushed. So gluster is in master right now! Niels, can I ask you to test it all again? Just to make sure the code all works like you intended. Niels de Vos wrote: Thanks Wido! This seems to be working OK for me. Note that the UI modification (https://reviews.apache.org/r/15933/) have not been reviewed/merged yet. Without these, it's rather difficult for users to configure Primary Storage on Gluster. Also, I've got asked about the dependencies and configuration. I'll add that here for now, and I'll try figure out how to get it added to the documentation: In /etc/glusterfs/glusterd.vol, allow unprivileged ports to contact the 'management' volume to get the volume configuration: option rpc-auth-allow-insecure on After changing the glusterd.vol file, restart the glusterd service to apply the changes. Per volume, allow unprivileged ports to access the brick processes (glusterfsd): # gluster volume set volname server.allow-insecure on # gluster volume stop volume # gluster volume start volume Per volume make sure that the kvm user (uid=36) and kvm group (gid=36) can access the images on the volume: # gluster volume set volname storage.owner-uid 36 # gluster volume set volname storage.owner-gid 36 Other dependencies: * libvirt version 1.0.1 (gluster protocol/network disk support) * qemu version 1.3 (gluster block backend support) Note that RHEL-6.5 and derived distributions contain backports that add sufficient functionality too. Niels de Vos wrote: Some further testing showed that there can be some issues with starting virtual machines which have disks on Gluster. https://reviews.apache.org/r/18412/ contains a solution for that. Wido, could you check that this is in 4.4, 4.5 and master and if not commit it ? trying to clean review board a bit. thanks - Sebastien --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review34859 --- On Feb. 19, 2014, 8:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 8:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
If you get a moment, please look at resize-root branch and see if it works for Gluster. I didn't see any code specific to creating volumes in your patch, mostly just pool stuff, so hopefully it's covered. Also, Wido, as mentioned on the bug for resize-root I'd prefer it if you could review the changes made for RBD support. We basically just override the size for new root volumes with the size of the volume object passed rather than assuming template size, if it's greater than the template size. On Sun, Feb 23, 2014 at 8:23 AM, Niels de Vos nde...@redhat.com wrote: On Feb. 19, 2014, 2:35 p.m., Wido den Hollander wrote: It seems good to me. Applies cleanly to master and builds just fine. Code-wise it's simple but effective, should allow us to support Gluster. Wido den Hollander wrote: I just merged it into master and pushed. So gluster is in master right now! Niels, can I ask you to test it all again? Just to make sure the code all works like you intended. Niels de Vos wrote: Thanks Wido! This seems to be working OK for me. Note that the UI modification (https://reviews.apache.org/r/15933/) have not been reviewed/merged yet. Without these, it's rather difficult for users to configure Primary Storage on Gluster. Also, I've got asked about the dependencies and configuration. I'll add that here for now, and I'll try figure out how to get it added to the documentation: In /etc/glusterfs/glusterd.vol, allow unprivileged ports to contact the 'management' volume to get the volume configuration: option rpc-auth-allow-insecure on After changing the glusterd.vol file, restart the glusterd service to apply the changes. Per volume, allow unprivileged ports to access the brick processes (glusterfsd): # gluster volume set volname server.allow-insecure on # gluster volume stop volume # gluster volume start volume Per volume make sure that the kvm user (uid=36) and kvm group (gid=36) can access the images on the volume: # gluster volume set volname storage.owner-uid 36 # gluster volume set volname storage.owner-gid 36 Other dependencies: * libvirt version 1.0.1 (gluster protocol/network disk support) * qemu version 1.3 (gluster block backend support) Note that RHEL-6.5 and derived distributions contain backports that add sufficient functionality too. Some further testing showed that there can be some issues with starting virtual machines which have disks on Gluster. https://reviews.apache.org/r/18412/ contains a solution for that. - Niels --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review34859 --- On Feb. 19, 2014, 9:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 9:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Feb. 19, 2014, 2:35 p.m., Wido den Hollander wrote: It seems good to me. Applies cleanly to master and builds just fine. Code-wise it's simple but effective, should allow us to support Gluster. Wido den Hollander wrote: I just merged it into master and pushed. So gluster is in master right now! Niels, can I ask you to test it all again? Just to make sure the code all works like you intended. Niels de Vos wrote: Thanks Wido! This seems to be working OK for me. Note that the UI modification (https://reviews.apache.org/r/15933/) have not been reviewed/merged yet. Without these, it's rather difficult for users to configure Primary Storage on Gluster. Also, I've got asked about the dependencies and configuration. I'll add that here for now, and I'll try figure out how to get it added to the documentation: In /etc/glusterfs/glusterd.vol, allow unprivileged ports to contact the 'management' volume to get the volume configuration: option rpc-auth-allow-insecure on After changing the glusterd.vol file, restart the glusterd service to apply the changes. Per volume, allow unprivileged ports to access the brick processes (glusterfsd): # gluster volume set volname server.allow-insecure on # gluster volume stop volume # gluster volume start volume Per volume make sure that the kvm user (uid=36) and kvm group (gid=36) can access the images on the volume: # gluster volume set volname storage.owner-uid 36 # gluster volume set volname storage.owner-gid 36 Other dependencies: * libvirt version 1.0.1 (gluster protocol/network disk support) * qemu version 1.3 (gluster block backend support) Note that RHEL-6.5 and derived distributions contain backports that add sufficient functionality too. Some further testing showed that there can be some issues with starting virtual machines which have disks on Gluster. https://reviews.apache.org/r/18412/ contains a solution for that. - Niels --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review34859 --- On Feb. 19, 2014, 9:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 9:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Feb. 19, 2014, 2:35 p.m., Wido den Hollander wrote: It seems good to me. Applies cleanly to master and builds just fine. Code-wise it's simple but effective, should allow us to support Gluster. Wido den Hollander wrote: I just merged it into master and pushed. So gluster is in master right now! Niels, can I ask you to test it all again? Just to make sure the code all works like you intended. Thanks Wido! This seems to be working OK for me. Note that the UI modification (https://reviews.apache.org/r/15933/) have not been reviewed/merged yet. Without these, it's rather difficult for users to configure Primary Storage on Gluster. Also, I've got asked about the dependencies and configuration. I'll add that here for now, and I'll try figure out how to get it added to the documentation: In /etc/glusterfs/glusterd.vol, allow unprivileged ports to contact the 'management' volume to get the volume configuration: option rpc-auth-allow-insecure on After changing the glusterd.vol file, restart the glusterd service to apply the changes. Per volume, allow unprivileged ports to access the brick processes (glusterfsd): # gluster volume set volname server.allow-insecure on # gluster volume stop volume # gluster volume start volume Per volume make sure that the kvm user (uid=36) and kvm group (gid=36) can access the images on the volume: # gluster volume set volname storage.owner-uid 36 # gluster volume set volname storage.owner-gid 36 Other dependencies: * libvirt version 1.0.1 (gluster protocol/network disk support) * qemu version 1.3 (gluster block backend support) Note that RHEL-6.5 and derived distributions contain backports that add sufficient functionality too. - Niels --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review34859 --- On Feb. 19, 2014, 9:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 9:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Feb. 19, 2014, 1:35 p.m., Wido den Hollander wrote: It seems good to me. Applies cleanly to master and builds just fine. Code-wise it's simple but effective, should allow us to support Gluster. I just merged it into master and pushed. So gluster is in master right now! Niels, can I ask you to test it all again? Just to make sure the code all works like you intended. - Wido --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review34859 --- On Feb. 19, 2014, 8:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 8:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 9:24 a.m.) Review request for cloudstack. Changes --- Rebased to recent master (4.4) and addressed all (?) review comments. 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 (updated) - api/src/com/cloud/storage/Storage.java ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review34859 --- Ship it! It seems good to me. Applies cleanly to master and builds just fine. Code-wise it's simple but effective, should allow us to support Gluster. - Wido den Hollander On Feb. 19, 2014, 8:24 a.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Feb. 19, 2014, 8:24 a.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 ff83dfc plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java d63b643 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java dbe5d4b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java a6186f6 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ff75d61 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java 8cdecd8 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a5f33eb plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java b90d5fc 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review33922 --- My sincere apologies Niels! I completely missed the second version of your patch! I just took a look at it again and it seems pretty straight forward. Most of the RBD code did a lot of work for you, so it's fairly easy to have GlusterFS in CS. I tried applying the patch to the master branch and that failed. It seems that you wrote the patch against the 4.2 branch, correct? Could you try to rebase it again master? If it then applies we might be able to get GlusterFS into 4.4! :) - Wido den Hollander On Jan. 14, 2014, 3:54 p.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Jan. 14, 2014, 3:54 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/resource/LibvirtStoragePoolXMLParser.java a707a0b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 6aaabc5 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java aaefc16 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Feb. 7, 2014, 10:33 a.m., Wido den Hollander wrote: My sincere apologies Niels! I completely missed the second version of your patch! I just took a look at it again and it seems pretty straight forward. Most of the RBD code did a lot of work for you, so it's fairly easy to have GlusterFS in CS. I tried applying the patch to the master branch and that failed. It seems that you wrote the patch against the 4.2 branch, correct? Could you try to rebase it again master? If it then applies we might be able to get GlusterFS into 4.4! :) Sure, no problem! I've done a rebase against 4.3 already, moving to master or 4.4 should not take too much time. - Niels --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review33922 --- On Jan. 14, 2014, 4:54 p.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Jan. 14, 2014, 4:54 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/resource/LibvirtStoragePoolXMLParser.java a707a0b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 6aaabc5 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java aaefc16 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
Awesome. Thanks, guys! On Feb 7, 2014 5:15 AM, Niels de Vos nde...@redhat.com wrote: On Feb. 7, 2014, 10:33 a.m., Wido den Hollander wrote: My sincere apologies Niels! I completely missed the second version of your patch! I just took a look at it again and it seems pretty straight forward. Most of the RBD code did a lot of work for you, so it's fairly easy to have GlusterFS in CS. I tried applying the patch to the master branch and that failed. It seems that you wrote the patch against the 4.2 branch, correct? Could you try to rebase it again master? If it then applies we might be able to get GlusterFS into 4.4! :) Sure, no problem! I've done a rebase against 4.3 already, moving to master or 4.4 should not take too much time. - Niels --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review33922 --- On Jan. 14, 2014, 4:54 p.m., Niels de Vos wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Jan. 14, 2014, 4:54 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/resource/LibvirtStoragePoolXMLParser.java a707a0b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 6aaabc5 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java aaefc16 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- (Updated Jan. 14, 2014, 4:54 p.m.) Review request for cloudstack. Changes --- Addressed all/most of the previous comments. This change also extend the previous version of the patch, and adds support for QEMU+libgfapi native communication. Mounting the the Gluster volume is only used to perform management tasks on the storage pool (create, delete disks, etc.). Comments much appreciated, thanks, Niels 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 (updated) - 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/resource/LibvirtStoragePoolXMLParser.java a707a0b plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java 6aaabc5 plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java aaefc16 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Dec. 4, 2013, 4: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. Reminder - Hi, This review has been pending for long. Please do update the patch so it may be shipped soon. Thanks! - Amogh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review29591 --- On Dec. 1, 2013, 2: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, 2: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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
On Dec. 4, 2013, 5:17 p.m., John Burwell wrote: Thanks John! I'll have a go at impoving the patch and will update it (hopefully) soon. - 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
What's the minimum libvirt version that supports glusterfs? On Dec 5, 2013 6:54 AM, Niels de Vos nde...@redhat.com wrote: On Dec. 4, 2013, 5:17 p.m., John Burwell wrote: Thanks John! I'll have a go at impoving the patch and will update it (hopefully) soon. - 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
Re: Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15932/#review29591
---
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java
https://reviews.apache.org/r/15932/#comment57191
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());
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java
https://reviews.apache.org/r/15932/#comment56987
By coding standards, please remove the tab characters.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java
https://reviews.apache.org/r/15932/#comment57192
Remove the string concatenations from the StringBuilder appends to avoid
needless string allocation that StringBuilder is intended to avoid.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
https://reviews.apache.org/r/15932/#comment57193
The log level of this message should be a WARN to let operators know that
the state of the underlying storage may be out of sync with the CloudStack
configuration.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
https://reviews.apache.org/r/15932/#comment57194
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.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
https://reviews.apache.org/r/15932/#comment57195
Lines 518 and 519 can be collapsed to the following:
s_logger.error(Failed to create mount., e).
Also, please include some state information (e.g. mount name, point, etc)
in the error message so that an operator can correlate the error with the
CloudStack configuration and/or the underlying storage device.
plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java
https://reviews.apache.org/r/15932/#comment57196
Magic value. This default port value should be captured somewhere in the
Gluster driver as a constant.
- John Burwell
On Dec. 1, 2013, 9:07 a.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, 9:07 a.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
Review Request 15932: Add support for Primary Storage on Gluster using the libvirt backend
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/ --- 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
