Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-02-21 Thread Saksham Srivastava 

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

Review request for cloudstack, Koushik Das and Prachi Damle.


Bugs: CLOUDSTACK-6151
https://issues.apache.org/jira/browse/CLOUDSTACK-6151


Repository: cloudstack-git


Description
---

Attaching a disk created from local disk offering with tags, to a VM was going 
to wrong local storage pool.
Cause : In LocalStoragePoolAlocator-
List hostPools = _poolHostDao.listByHostId(plan.getHostId());
It return pools by hostId, but nowhere were the tags being compared.

Added new method findLocalStoragePoolsByHostAndTags() that returns stoage pools 
by hostid and tags both.


Diffs
-

  
engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
 59c338e 
  
engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
 d35aa44 
  
engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
 1f61e8b 

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


Testing
---

Tested the folowing scenarios:
attaching local volume with tags
attaching local volume without tags
attaching local volume with different tags
attaching shared volume

Build passes successfully.


Thanks,

Saksham Srivastava

Re: Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-03-03 Thread Saksham Srivastava 

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

(Updated March 3, 2014, 1:57 p.m.)


Review request for cloudstack, Koushik Das and Prachi Damle.


Bugs: CLOUDSTACK-6151
https://issues.apache.org/jira/browse/CLOUDSTACK-6151


Repository: cloudstack-git


Description (updated)
---

Attaching a new disk created from local disk offering with tags, to a VM was 
going to wrong local storage pool.
Cause : In LocalStoragePoolAlocator-
List hostPools = _poolHostDao.listByHostId(plan.getHostId());
It return pools by hostId, but nowhere were the tags being compared.

Added new method findLocalStoragePoolsByHostAndTags() that returns stoage pools 
by hostid and tags both.


Diffs (updated)
-

  
engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
 ea0bad9 
  
engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
 59c338e 
  
engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
 d35aa44 
  
engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
 1f61e8b 
  server/test/resources/createNetworkOffering.xml c6228da 

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


Testing (updated)
---

Tested the folowing scenarios:
attaching local volume with tags
attaching local volume without tags
attaching local volume with different tags
attaching shared volume

Build passes successfully.
Patch applies cleanly.


Thanks,

Saksham Srivastava

Re: Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-03-05 Thread Saksham Srivastava 

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

(Updated March 5, 2014, 8:50 a.m.)


Review request for cloudstack, Koushik Das and Prachi Damle.


Bugs: CLOUDSTACK-6151
https://issues.apache.org/jira/browse/CLOUDSTACK-6151


Repository: cloudstack-git


Description
---

Attaching a new disk created from local disk offering with tags, to a VM was 
going to wrong local storage pool.
Cause : In LocalStoragePoolAlocator-
List hostPools = _poolHostDao.listByHostId(plan.getHostId());
It return pools by hostId, but nowhere were the tags being compared.

Added new method findLocalStoragePoolsByHostAndTags() that returns stoage pools 
by hostid and tags both.


Diffs (updated)
-

  
engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
 ea0bad9 
  
engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
 59c338e 
  
engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
 d35aa44 
  
engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
 1f61e8b 
  server/test/resources/createNetworkOffering.xml c6228da 

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


Testing
---

Tested the folowing scenarios:
attaching local volume with tags
attaching local volume without tags
attaching local volume with different tags
attaching shared volume

Build passes successfully.
Patch applies cleanly.


Thanks,

Saksham Srivastava

Re: Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-03-05 Thread ASF Subversion and Git Services 

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


Commit a8b34c0c4f99067243524813f136b31cb78c5f78 in cloudstack's branch 
refs/heads/4.3-forward from Saksham Srivastava
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=a8b34c0 ]

CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

Signed-off-by: Koushik Das 


- ASF Subversion and Git Services


On March 5, 2014, 8:50 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18352/
> ---
> 
> (Updated March 5, 2014, 8:50 a.m.)
> 
> 
> Review request for cloudstack, Koushik Das and Prachi Damle.
> 
> 
> Bugs: CLOUDSTACK-6151
> https://issues.apache.org/jira/browse/CLOUDSTACK-6151
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Attaching a new disk created from local disk offering with tags, to a VM was 
> going to wrong local storage pool.
> Cause : In LocalStoragePoolAlocator-
> List hostPools = 
> _poolHostDao.listByHostId(plan.getHostId());
> It return pools by hostId, but nowhere were the tags being compared.
> 
> Added new method findLocalStoragePoolsByHostAndTags() that returns stoage 
> pools by hostid and tags both.
> 
> 
> Diffs
> -
> 
>   
> engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
>  ea0bad9 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
>  59c338e 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
>  d35aa44 
>   
> engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
>  1f61e8b 
>   server/test/resources/createNetworkOffering.xml c6228da 
> 
> Diff: https://reviews.apache.org/r/18352/diff/
> 
> 
> Testing
> ---
> 
> Tested the folowing scenarios:
> attaching local volume with tags
> attaching local volume without tags
> attaching local volume with different tags
> attaching shared volume
> 
> Build passes successfully.
> Patch applies cleanly.
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-03-05 Thread Koushik Das 

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

Ship it!


Ship It!

- Koushik Das


On March 5, 2014, 8:50 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18352/
> ---
> 
> (Updated March 5, 2014, 8:50 a.m.)
> 
> 
> Review request for cloudstack, Koushik Das and Prachi Damle.
> 
> 
> Bugs: CLOUDSTACK-6151
> https://issues.apache.org/jira/browse/CLOUDSTACK-6151
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Attaching a new disk created from local disk offering with tags, to a VM was 
> going to wrong local storage pool.
> Cause : In LocalStoragePoolAlocator-
> List hostPools = 
> _poolHostDao.listByHostId(plan.getHostId());
> It return pools by hostId, but nowhere were the tags being compared.
> 
> Added new method findLocalStoragePoolsByHostAndTags() that returns stoage 
> pools by hostid and tags both.
> 
> 
> Diffs
> -
> 
>   
> engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
>  ea0bad9 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
>  59c338e 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
>  d35aa44 
>   
> engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
>  1f61e8b 
>   server/test/resources/createNetworkOffering.xml c6228da 
> 
> Diff: https://reviews.apache.org/r/18352/diff/
> 
> 
> Testing
> ---
> 
> Tested the folowing scenarios:
> attaching local volume with tags
> attaching local volume without tags
> attaching local volume with different tags
> attaching shared volume
> 
> Build passes successfully.
> Patch applies cleanly.
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-02-23 Thread Koushik Das 

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



engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java


If you specify the host then you don't need dc, pod, cluster.



engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java


I would recommend that you use the createSearchBuilder() approach instead 
of hardcoding the sql query. Refer to PrimaryDataStoreDaoImpl() ctor.



engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java


What is the use case for null host id? Make it long instead of Long.


- Koushik Das


On Feb. 21, 2014, 11:36 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18352/
> ---
> 
> (Updated Feb. 21, 2014, 11:36 a.m.)
> 
> 
> Review request for cloudstack, Koushik Das and Prachi Damle.
> 
> 
> Bugs: CLOUDSTACK-6151
> https://issues.apache.org/jira/browse/CLOUDSTACK-6151
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Attaching a disk created from local disk offering with tags, to a VM was 
> going to wrong local storage pool.
> Cause : In LocalStoragePoolAlocator-
> List hostPools = 
> _poolHostDao.listByHostId(plan.getHostId());
> It return pools by hostId, but nowhere were the tags being compared.
> 
> Added new method findLocalStoragePoolsByHostAndTags() that returns stoage 
> pools by hostid and tags both.
> 
> 
> Diffs
> -
> 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
>  59c338e 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
>  d35aa44 
>   
> engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
>  1f61e8b 
> 
> Diff: https://reviews.apache.org/r/18352/diff/
> 
> 
> Testing
> ---
> 
> Tested the folowing scenarios:
> attaching local volume with tags
> attaching local volume without tags
> attaching local volume with different tags
> attaching shared volume
> 
> Build passes successfully.
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>

Re: Review Request 18352: CLOUDSTACK-6151: Local data disk with tag goes to the wrong local storage pool

2014-02-24 Thread Saksham Srivastava 


> On Feb. 24, 2014, 7:10 a.m., Koushik Das wrote:
> > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java,
> >  line 118
> > 
> >
> > If you specify the host then you don't need dc, pod, cluster.

The reason why I added this was to make sure in case the hostId is passed as 
null, we could return tagged storagePools by calling 
findLocalStoragePoolsByTags.
This is more of an extra check that we should return all hosts with matching 
tags even when host is not passed. 
As suggested it may not be required as there are not other cases where one 
would call this utility.


> On Feb. 24, 2014, 7:10 a.m., Koushik Das wrote:
> > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java,
> >  line 68
> > 
> >
> > I would recommend that you use the createSearchBuilder() approach 
> > instead of hardcoding the sql query. Refer to PrimaryDataStoreDaoImpl() 
> > ctor.

To return local storage pools of matching tags and matching host id requires 
join of 3 tables (storage_pools, storage_host_ref and storage_pool_details). 
Further , all the methods that search for tagged pools in this file are 
currently using the same technique of filling the base sql query with custom 
search parameters, I have also gone the same way.
But as you have mentioned I will update the patch using createSearchBuilder() 
method.


> On Feb. 24, 2014, 7:10 a.m., Koushik Das wrote:
> > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java,
> >  line 366
> > 
> >
> > What is the use case for null host id? Make it long instead of Long.

This is just because I was checking for null hostIds, since the check may not 
be required, I will change it to long.


- Saksham


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


On Feb. 21, 2014, 11:36 a.m., Saksham Srivastava wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18352/
> ---
> 
> (Updated Feb. 21, 2014, 11:36 a.m.)
> 
> 
> Review request for cloudstack, Koushik Das and Prachi Damle.
> 
> 
> Bugs: CLOUDSTACK-6151
> https://issues.apache.org/jira/browse/CLOUDSTACK-6151
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> ---
> 
> Attaching a disk created from local disk offering with tags, to a VM was 
> going to wrong local storage pool.
> Cause : In LocalStoragePoolAlocator-
> List hostPools = 
> _poolHostDao.listByHostId(plan.getHostId());
> It return pools by hostId, but nowhere were the tags being compared.
> 
> Added new method findLocalStoragePoolsByHostAndTags() that returns stoage 
> pools by hostid and tags both.
> 
> 
> Diffs
> -
> 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java
>  59c338e 
>   
> engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java
>  d35aa44 
>   
> engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java
>  1f61e8b 
> 
> Diff: https://reviews.apache.org/r/18352/diff/
> 
> 
> Testing
> ---
> 
> Tested the folowing scenarios:
> attaching local volume with tags
> attaching local volume without tags
> attaching local volume with different tags
> attaching shared volume
> 
> Build passes successfully.
> 
> 
> Thanks,
> 
> Saksham Srivastava
> 
>