You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@cloudstack.apache.org by Wei Zhou <w....@leaseweb.com> on 2013/08/02 08:39:40 UTC

Review Request 13223: (CLOUDSTACK-2729) use file lock to prevent concurrent refreshPool/deleteVolume on KVM shared storage pool

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

Review request for cloudstack, edison su and Wido den Hollander.


Bugs: CLOUDSTACK-2729


Repository: cloudstack-git


Description
-------

The storage pool issue (CLOUDSTACK-2729) is because of a bug in libvirt (https://bugzilla.redhat.com/show_bug.cgi?id=977706)
We need to prevent deleting a volume when refreshing the pool.
This patch use a simple file lock to implement it.

PS: I have tested another file lock similar to Read/Write file lock, but it was very unstable.


Diffs
-----

  plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java dae25ae 

Diff: https://reviews.apache.org/r/13223/diff/


Testing
-------

Applied on 4.0.2 and 4.0.1

Testing On 4.0.1 From 20,June
3 nodes, create a VM on each node every 15 minutes. Destroy the VMs 5 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2
 

Testing On 4.0.2 From 01,July
2 nodes, create two VMs on each node every 5 minutes. Destroy the VMs 4 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2 


Thanks,

Wei Zhou

Re: Review Request 13223: (CLOUDSTACK-2729) use file lock to prevent concurrent refreshPool/deleteVolume on KVM shared storage pool

Posted by "Jenkins Cloudstack.org" <hu...@apache.org>.

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


Review 13223 failed the build test : FAILURE
The url of build cloudstack-master-with-patch #104 is : http://jenkins.cloudstack.org/job/cloudstack-master-with-patch/104/

- Jenkins Cloudstack.org


On Aug. 2, 2013, 6:39 a.m., Wei Zhou wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13223/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2013, 6:39 a.m.)
> 
> 
> Review request for cloudstack, edison su and Wido den Hollander.
> 
> 
> Bugs: CLOUDSTACK-2729
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> The storage pool issue (CLOUDSTACK-2729) is because of a bug in libvirt (https://bugzilla.redhat.com/show_bug.cgi?id=977706)
> We need to prevent deleting a volume when refreshing the pool.
> This patch use a simple file lock to implement it.
> 
> PS: I have tested another file lock similar to Read/Write file lock, but it was very unstable.
> 
> 
> Diffs
> -----
> 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java dae25ae 
> 
> Diff: https://reviews.apache.org/r/13223/diff/
> 
> 
> Testing
> -------
> 
> Applied on 4.0.2 and 4.0.1
> 
> Testing On 4.0.1 From 20,June
> 3 nodes, create a VM on each node every 15 minutes. Destroy the VMs 5 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2
>  
> 
> Testing On 4.0.2 From 01,July
> 2 nodes, create two VMs on each node every 5 minutes. Destroy the VMs 4 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2 
> 
> 
> Thanks,
> 
> Wei Zhou
> 
>

Re: Review Request 13223: (CLOUDSTACK-2729) use file lock to prevent concurrent refreshPool/deleteVolume on KVM shared storage pool

Posted by Wei Zhou <w....@leaseweb.com>.


> On Aug. 2, 2013, 11:09 a.m., Wido den Hollander wrote:
> > I haven't tested the patch, but I'm missing some comments.
> > 
> > Can we add clear comments as to WHY this is in the code? Otherwise I'm afraid this will become legacy and in 6 months new committer won't have the slightest idea of why this is in there.

Wido,

Make sense. I will add some comments later.


- Wei


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


On Aug. 2, 2013, 6:39 a.m., Wei Zhou wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13223/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2013, 6:39 a.m.)
> 
> 
> Review request for cloudstack, edison su and Wido den Hollander.
> 
> 
> Bugs: CLOUDSTACK-2729
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> The storage pool issue (CLOUDSTACK-2729) is because of a bug in libvirt (https://bugzilla.redhat.com/show_bug.cgi?id=977706)
> We need to prevent deleting a volume when refreshing the pool.
> This patch use a simple file lock to implement it.
> 
> PS: I have tested another file lock similar to Read/Write file lock, but it was very unstable.
> 
> 
> Diffs
> -----
> 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java dae25ae 
> 
> Diff: https://reviews.apache.org/r/13223/diff/
> 
> 
> Testing
> -------
> 
> Applied on 4.0.2 and 4.0.1
> 
> Testing On 4.0.1 From 20,June
> 3 nodes, create a VM on each node every 15 minutes. Destroy the VMs 5 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2
>  
> 
> Testing On 4.0.2 From 01,July
> 2 nodes, create two VMs on each node every 5 minutes. Destroy the VMs 4 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2 
> 
> 
> Thanks,
> 
> Wei Zhou
> 
>

Re: Review Request 13223: (CLOUDSTACK-2729) use file lock to prevent concurrent refreshPool/deleteVolume on KVM shared storage pool

Posted by Wido den Hollander <wi...@widodh.nl>.

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


I haven't tested the patch, but I'm missing some comments.

Can we add clear comments as to WHY this is in the code? Otherwise I'm afraid this will become legacy and in 6 months new committer won't have the slightest idea of why this is in there.

- Wido den Hollander


On Aug. 2, 2013, 6:39 a.m., Wei Zhou wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13223/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2013, 6:39 a.m.)
> 
> 
> Review request for cloudstack, edison su and Wido den Hollander.
> 
> 
> Bugs: CLOUDSTACK-2729
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> The storage pool issue (CLOUDSTACK-2729) is because of a bug in libvirt (https://bugzilla.redhat.com/show_bug.cgi?id=977706)
> We need to prevent deleting a volume when refreshing the pool.
> This patch use a simple file lock to implement it.
> 
> PS: I have tested another file lock similar to Read/Write file lock, but it was very unstable.
> 
> 
> Diffs
> -----
> 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java dae25ae 
> 
> Diff: https://reviews.apache.org/r/13223/diff/
> 
> 
> Testing
> -------
> 
> Applied on 4.0.2 and 4.0.1
> 
> Testing On 4.0.1 From 20,June
> 3 nodes, create a VM on each node every 15 minutes. Destroy the VMs 5 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2
>  
> 
> Testing On 4.0.2 From 01,July
> 2 nodes, create two VMs on each node every 5 minutes. Destroy the VMs 4 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2 
> 
> 
> Thanks,
> 
> Wei Zhou
> 
>

Re: Review Request 13223: (CLOUDSTACK-2729) use file lock to prevent concurrent refreshPool/deleteVolume on KVM shared storage pool

Posted by edison su <ed...@citrix.com>.

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



plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
<https://reviews.apache.org/r/13223/#comment48610>

    How about use the atomic semantics provided by createNewFile to coordinate between processes? 
    For example: lock(int wait) {
       File lockFile = new File(lock-file-path);
       while(wait) {
       boolean havelock = false;
       if (lockFile.createNewFile()) {
          haveLock = true;
          break;
       }
        Time.sleep(1000);
        wait--;
        }
       return haveLock;
    }
    
    unlock() {
       File lockFile = new File(path-of-lock-file);
       lockFile.delete();
    }
    
    It's simpler version of lockfile: https://github.com/spearce/jgit/blob/master/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java 


- edison su


On Aug. 2, 2013, 1:03 p.m., Wei Zhou wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13223/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2013, 1:03 p.m.)
> 
> 
> Review request for cloudstack, edison su and Wido den Hollander.
> 
> 
> Bugs: CLOUDSTACK-2729
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> The storage pool issue (CLOUDSTACK-2729) is because of a bug in libvirt (https://bugzilla.redhat.com/show_bug.cgi?id=977706)
> We need to prevent deleting a volume when refreshing the pool.
> This patch use a simple file lock to implement it.
> 
> PS: I have tested another file lock similar to Read/Write file lock, but it was very unstable.
> 
> 
> Diffs
> -----
> 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java a9baa52 
> 
> Diff: https://reviews.apache.org/r/13223/diff/
> 
> 
> Testing
> -------
> 
> Applied on 4.0.2 and 4.0.1
> 
> Testing On 4.0.1 From 20,June
> 3 nodes, create a VM on each node every 15 minutes. Destroy the VMs 5 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2
>  
> 
> Testing On 4.0.2 From 01,July
> 2 nodes, create two VMs on each node every 5 minutes. Destroy the VMs 4 minutes later. expunge.inteval = 600 (10 minutes), expunge.worker = 2 
> 
> 
> Thanks,
> 
> Wei Zhou
> 
>