Mark Wu has posted comments on this change.

Change subject: Allow creating ISO domain on localfs
......................................................................


Patch Set 3: (10 inline comments)

Allon,
I really appreciate for your detailed review and great suggestion.  I will add 
the posix/ISO support in next patch as you suggested.

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddLocalStorageDomainCommand.java
Line 46:                 setStoragePool(storagePool);
Line 47:             }
Line 48: 
Line 49:             if (retVal && getStorageDomain().getStorageDomainType() != 
StorageDomainType.ISO
Line 50:                     && storagePool.getstorage_pool_type() != 
StorageType.LOCALFS) {
Sorry,  I don't understand comments.  My code only allows ISO domain to be 
added to a non localfs DC.  You proposed code allows ISO domain and export 
domain. Please correct me if I am wrong. Thanks.
Line 51:                 
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_IS_NOT_LOCAL);
Line 52:                 retVal = false;
Line 53:             }
Line 54: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageDomainCommand.java
Line 124:             returnValue = false;
Line 125:         }
Line 126:         if (returnValue && getStorageDomain().getStorageDomainType() 
== StorageDomainType.ISO
Line 127:                 && !(getStorageDomain().getStorageType() == 
StorageType.NFS
Line 128:                         || getStorageDomain().getStorageType() == 
StorageType.LOCALFS)) {
Done
Line 129:             
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL);
Line 130:             returnValue = false;
Line 131:         }
Line 132:         if (returnValue && getStorageDomain().getStorageDomainType() 
== StorageDomainType.ImportExport


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
Line 78
Line 79
Line 80
Line 81
Line 82
When I can add a localfs/ISO domain to a NFS DC, it raises an error of 
'.VDS_CANNOT_ADD_LOCAL_STORAGE_TO_NON_LOCAL_H
OST'.   I can't figure out how to tell this connection will be used for ISO 
domain or Data domain here.  So I just simply remove the validation.  It's a 
little bit impolite, but we have already guaranteed localfs/Data can't be added 
to non local host in upper level,  right.  Is it acceptable?


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DisconnectStorageServerConnectionCommand.java
Line 35
Line 36
Line 37
Line 38
Line 39
The same reason as AddStorageServerConnectionCommand. The raised error message 
lead me remove this validation.


....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 542
Line 543
Line 544
Line 545
Line 546
Done


....................................................
Commit Message
Line 3: AuthorDate: 2013-02-28 17:36:26 +0800
Line 4: Commit:     Mark Wu <wu...@linux.vnet.ibm.com>
Line 5: CommitDate: 2013-03-08 16:18:02 +0800
Line 6: 
Line 7: Allow creating ISO domain on localfs
Done
Line 8: 
Line 9: This patch allows creating ISO domain on localfs. Even though localfs
Line 10: can't be shared among the hosts in cluster, it could help in the case
Line 11: of no nfs available. VM can be created on the host which has the ISO


Line 8: 
Line 9: This patch allows creating ISO domain on localfs. Even though localfs
Line 10: can't be shared among the hosts in cluster, it could help in the case
Line 11: of no nfs available. VM can be created on the host which has the ISO
Line 12: domain attached, and then be migrated to any other host in the cluster.
Done
Line 13: 
Line 14: Change-Id: I2a8d3ea8ab4ac10353ec8574287458e8eb63e882


....................................................
File 
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterStorageListModel.java
Line 562:         String localStorgaeDC = null;
Line 563:         for (StorageDomain a : Linq.<StorageDomain> 
Cast(getSelectedItems()))
Line 564:         {
Line 565:             // For local storage - remove; otherwise - detach
Line 566:             if (a.getStorageType() == StorageType.LOCALFS && 
a.getStorageDomainType() != StorageDomainType.ISO)
In my test, NFS ISO domain is not destroyed after detaching.  I think it 
doesn't make sense that forcibly destroy the domain and ISO images on detach.  
We could leave the choice to user.
Line 567:             {
Line 568:                 getpb_remove().add(new 
RemoveStorageDomainParameters(a.getId()));
Line 569:                 localStorgaeDC = a.getStoragePoolName();
Line 570:             }


....................................................
File 
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/NewEditStorageModelBehavior.java
Line 77:                         || 
(!dataCenter.getId().equals(StorageModel.UnassignedDataCenterId) && 
((item.getRole() == StorageDomainType.Data && item.getType() == 
dataCenter.getstorage_pool_type())
Line 78:                                 || (item.getRole() == 
StorageDomainType.ImportExport
Line 79:                                         && item.getType() == 
StorageType.NFS
Line 80:                                         && dataCenter.getstatus() != 
StoragePoolStatus.Uninitialized && isNoStorageAttached) || item.getRole() == 
StorageDomainType.ISO
Line 81:                                 && (item.getType() == StorageType.NFS 
|| item.getType() == StorageType.LOCALFS)
Done
Line 82:                                 && dataCenter.getstatus() != 
StoragePoolStatus.Uninitialized && isNoStorageAttached)) || 
(getModel().getStorage() != null && item.getType() == getModel().getStorage()
Line 83:                         .getStorageType())));
Line 84: 
Line 85:         behavior.OnStorageModelUpdated(item);


....................................................
File 
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
Line 296:         localDataModel.setRole(StorageDomainType.Data);
Line 297:         items.add(localDataModel);
Line 298: 
Line 299:         LocalStorageModel localIsoModel = new LocalStorageModel();
Line 300:         localIsoModel.setRole(StorageDomainType.ISO);
Done
Line 301:         items.add(localIsoModel);
Line 302: 
Line 303:         PosixStorageModel posixDataModel = new PosixStorageModel();
Line 304:         posixDataModel.setRole(StorageDomainType.Data);


--
To view, visit http://gerrit.ovirt.org/12687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2a8d3ea8ab4ac10353ec8574287458e8eb63e882
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wu...@linux.vnet.ibm.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com>
Gerrit-Reviewer: Sharad Mishra <snmis...@linux.vnet.ibm.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to