Allon Mureinik has posted comments on this change.
Change subject: Allow creating ISO domain on localfs
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(10 inline comments)
This review is given following the discussion on engine-devel[1], assuming we
are not removing the ISO domain concept quite yet.
IMHO, having ISO domains only on NFS storage is an arbitrary limitation, but an
understandable one ("it only works on NFS because we decided to, and that's
that!").
IMHO, just adding localfs is half a solution, because it leaves the arbitrary
decision that NFS on POSIX domains is not supported.
Why not widen the scope and support /all/ file domains? This seems like a very
small delta to me.
....................................................
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) {
This would allow adding an localfs export doamin.
You should have something like this:
if (retVal && getStorageDomain().getStorageDomainType() ==
StorageDomainType.DATA && storagePool.getstorage_pool_type() !=
StorageType.LOCALFS) {
// return failure, as is now
}
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)) {
If you accept my suggestion to support file domains general, this should be
replaced with getStorageDoamin().getStorageType().isFileDomain()
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
How is this deletion relevant to the patch?
....................................................
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
It seems correct to remove this (useless) hunk of code, but why is it relevant
to this patch?
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 542
Line 543
Line 544
Line 545
Line 546
The same should be done for the other locations of AppErrors
....................................................
Commit Message
Line 3: AuthorDate: 2013-02-28 17:36:26 +0800
Line 4: Commit: Mark Wu <[email protected]>
Line 5: CommitDate: 2013-03-08 16:18:02 +0800
Line 6:
Line 7: Allow creating ISO domain on localfs
Please add the "core:" prefix
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.
This is also a great feature for POCs, all-in-one, etc.
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)
AFAIK, today, we remove NFS ISO domains - shouldn't this act the same with
localfs ISO domains?
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)
If you accept my general suggestion, this should be done with
item.getTime().isFileDomain()
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);
Same for POSIX, if you accept my suggestion
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 <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Sharad Mishra <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
