You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Venkata Siva Vijayendra Bhamidipati <vi...@citrix.com> on 2013/06/20 04:42:27 UTC
Review Request: Fix primary datastore NPE/incorrect db entry/exception
propagation for KVM on cloudstack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11984/
-----------------------------------------------------------
Review request for cloudstack, Chip Childers, edison su, and Min Chen.
Description
-------
Patch for fixes for issues detected while working on bug CLOUDSTACK-1510 (https://issues.apache.org/jira/browse/CLOUDSTACK-1510).
This addresses bug CLOUDSTACK-1510.
Diffs
-----
api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java 74eb2b9
engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreLifeCycle.java cb46709
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java 89e22c8
plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java fb37e8f
server/src/com/cloud/storage/StorageManagerImpl.java 20b435c
Diff: https://reviews.apache.org/r/11984/diff/
Testing
-------
Deploy KVM cluster in cloudstack. Attempt to add a primary NFS datastore using an invalid path. NPE is not encountered anymore. If KVM host is down or the cloud-agent on the KVM host is down, the primary datastore (whether valid or otherwise) is not logged to the db's storage_pool table. So invalid datastores do not show up in the GUI when listing the primary datastores available. Also, exception is propagated to GUI.
Thanks,
Venkata Siva Vijayendra Bhamidipati
Re: Review Request 11984: Fix primary datastore NPE/incorrect db
entry/exception propagation for KVM on cloudstack
Posted by edison su <ed...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11984/#review22980
-----------------------------------------------------------
Ship it!
Ship It!
- edison su
On July 2, 2013, 11:09 p.m., Venkata Siva Vijayendra Bhamidipati wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11984/
> -----------------------------------------------------------
>
> (Updated July 2, 2013, 11:09 p.m.)
>
>
> Review request for cloudstack, Chip Childers, edison su, and Min Chen.
>
>
> Bugs: CLOUDSTACK-1510
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Patch for fixes for issues detected while working on bug CLOUDSTACK-1510 (https://issues.apache.org/jira/browse/CLOUDSTACK-1510).
>
>
> Diffs
> -----
>
> api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java f5750b9
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java 89e22c8
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java 2e0ff66
> server/src/com/cloud/storage/StorageManagerImpl.java bb21afb
>
> Diff: https://reviews.apache.org/r/11984/diff/
>
>
> Testing
> -------
>
> Deploy KVM cluster in cloudstack. Attempt to add a primary NFS datastore using an invalid path. NPE is not encountered anymore. If KVM host is down or the cloud-agent on the KVM host is down, the primary datastore (whether valid or otherwise) is not logged to the db's storage_pool table. So invalid datastores do not show up in the GUI when listing the primary datastores available. Also, exception is propagated to GUI.
>
>
> Thanks,
>
> Venkata Siva Vijayendra Bhamidipati
>
>
Re: Review Request 11984: Fix primary datastore NPE/incorrect db
entry/exception propagation for KVM on cloudstack
Posted by Venkata Siva Vijayendra Bhamidipati <vi...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11984/
-----------------------------------------------------------
(Updated July 2, 2013, 11:09 p.m.)
Review request for cloudstack, Chip Childers, edison su, and Min Chen.
Changes
-------
Uploading new diff incorporating Edison's review comments.
Bugs: CLOUDSTACK-1510
Repository: cloudstack-git
Description
-------
Patch for fixes for issues detected while working on bug CLOUDSTACK-1510 (https://issues.apache.org/jira/browse/CLOUDSTACK-1510).
Diffs (updated)
-----
api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java f5750b9
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java 89e22c8
plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java 2e0ff66
server/src/com/cloud/storage/StorageManagerImpl.java bb21afb
Diff: https://reviews.apache.org/r/11984/diff/
Testing
-------
Deploy KVM cluster in cloudstack. Attempt to add a primary NFS datastore using an invalid path. NPE is not encountered anymore. If KVM host is down or the cloud-agent on the KVM host is down, the primary datastore (whether valid or otherwise) is not logged to the db's storage_pool table. So invalid datastores do not show up in the GUI when listing the primary datastores available. Also, exception is propagated to GUI.
Thanks,
Venkata Siva Vijayendra Bhamidipati
Re: Review Request 11984: Fix primary datastore NPE/incorrect db
entry/exception propagation for KVM on cloudstack
Posted by Venkata Siva Vijayendra Bhamidipati <vi...@citrix.com>.
> On June 20, 2013, 10:37 p.m., edison su wrote:
> > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreLifeCycle.java, line 44
> > <https://reviews.apache.org/r/11984/diff/1/?file=308929#file308929line44>
> >
> > If stoagemanager wants to delete datastore, then just call deletedatastore, don't need add another api. In implementation of deletedatastore method, you can add a conditional check, if the datastore's state is not in ready state, then don't need to send DeleteStoragePoolCommand to hypervisor, delete the db entry instead.
Checking for the storage pool state is a better idea, will do that and delete the new API.
> On June 20, 2013, 10:37 p.m., edison su wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java, line 160
> > <https://reviews.apache.org/r/11984/diff/1/?file=308930#file308930line160>
> >
> > Don't need to check exeception error message here, if adding storage pool failed, then just failed, rethrow the exception to upper layer.
I thought checking it would be more granular, but you're right, it's unnecessary here.
> On June 20, 2013, 10:37 p.m., edison su wrote:
> > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java, line 506
> > <https://reviews.apache.org/r/11984/diff/1/?file=308930#file308930line506>
> >
> > How about log the exception stack trace also?
Good idea, will log it.
> On June 20, 2013, 10:37 p.m., edison su wrote:
> > server/src/com/cloud/storage/StorageManagerImpl.java, line 857
> > <https://reviews.apache.org/r/11984/diff/1/?file=308932#file308932line857>
> >
> > Call lifecycle->deletedatastore
Will do that, now that I'll put in the check for the storage pool state.
- Venkata Siva Vijayendra
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11984/#review22205
-----------------------------------------------------------
On June 20, 2013, 2:42 a.m., Venkata Siva Vijayendra Bhamidipati wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11984/
> -----------------------------------------------------------
>
> (Updated June 20, 2013, 2:42 a.m.)
>
>
> Review request for cloudstack, Chip Childers, edison su, and Min Chen.
>
>
> Bugs: CLOUDSTACK-1510
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Patch for fixes for issues detected while working on bug CLOUDSTACK-1510 (https://issues.apache.org/jira/browse/CLOUDSTACK-1510).
>
>
> Diffs
> -----
>
> api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java 74eb2b9
> engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreLifeCycle.java cb46709
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java 89e22c8
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java fb37e8f
> server/src/com/cloud/storage/StorageManagerImpl.java 20b435c
>
> Diff: https://reviews.apache.org/r/11984/diff/
>
>
> Testing
> -------
>
> Deploy KVM cluster in cloudstack. Attempt to add a primary NFS datastore using an invalid path. NPE is not encountered anymore. If KVM host is down or the cloud-agent on the KVM host is down, the primary datastore (whether valid or otherwise) is not logged to the db's storage_pool table. So invalid datastores do not show up in the GUI when listing the primary datastores available. Also, exception is propagated to GUI.
>
>
> Thanks,
>
> Venkata Siva Vijayendra Bhamidipati
>
>
Re: Review Request: Fix primary datastore NPE/incorrect db entry/exception
propagation for KVM on cloudstack
Posted by edison su <ed...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11984/#review22205
-----------------------------------------------------------
engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreLifeCycle.java
<https://reviews.apache.org/r/11984/#comment45627>
If stoagemanager wants to delete datastore, then just call deletedatastore, don't need add another api. In implementation of deletedatastore method, you can add a conditional check, if the datastore's state is not in ready state, then don't need to send DeleteStoragePoolCommand to hypervisor, delete the db entry instead.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
<https://reviews.apache.org/r/11984/#comment45628>
Don't need to check exeception error message here, if adding storage pool failed, then just failed, rethrow the exception to upper layer.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java
<https://reviews.apache.org/r/11984/#comment45629>
How about log the exception stack trace also?
server/src/com/cloud/storage/StorageManagerImpl.java
<https://reviews.apache.org/r/11984/#comment45630>
Call lifecycle->deletedatastore
- edison su
On June 20, 2013, 2:42 a.m., Venkata Siva Vijayendra Bhamidipati wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11984/
> -----------------------------------------------------------
>
> (Updated June 20, 2013, 2:42 a.m.)
>
>
> Review request for cloudstack, Chip Childers, edison su, and Min Chen.
>
>
> Description
> -------
>
> Patch for fixes for issues detected while working on bug CLOUDSTACK-1510 (https://issues.apache.org/jira/browse/CLOUDSTACK-1510).
>
>
> This addresses bug CLOUDSTACK-1510.
>
>
> Diffs
> -----
>
> api/src/org/apache/cloudstack/api/command/admin/storage/CreateStoragePoolCmd.java 74eb2b9
> engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreLifeCycle.java cb46709
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java 89e22c8
> plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java fb37e8f
> server/src/com/cloud/storage/StorageManagerImpl.java 20b435c
>
> Diff: https://reviews.apache.org/r/11984/diff/
>
>
> Testing
> -------
>
> Deploy KVM cluster in cloudstack. Attempt to add a primary NFS datastore using an invalid path. NPE is not encountered anymore. If KVM host is down or the cloud-agent on the KVM host is down, the primary datastore (whether valid or otherwise) is not logged to the db's storage_pool table. So invalid datastores do not show up in the GUI when listing the primary datastores available. Also, exception is propagated to GUI.
>
>
> Thanks,
>
> Venkata Siva Vijayendra Bhamidipati
>
>