> 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 > >
