Liron Ar has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 11: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 62: return
failCanDoAction(VdcBllMessages.VDS_EMPTY_NAME_OR_ID);
Line 63: }
Line 64:
Line 65: Guid storagePoolId = getParameters().getStoragePoolId();
Line 66: if(storagePoolId == null || storagePoolId.equals(Guid.Empty) )
{
the pool id in the parameters has a default of Guid.Empty - it doesn't mean
that it doesn't exist..perhaps that's incorrect message.
I don't think that this check is correct, we can add domain which isn't part of
pool for example, so don't we should be able to edit one as well?
Line 67: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST);
Line 68: }
Line 69:
Line 70: // Check if connection exists by id - otherwise there's
nothing to update
Line 92: if(domains == null) {
Line 93: domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 94: }
Line 95: if(domains.isEmpty()) {
Line 96: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST);
please see related comment in getExc.. method - IMO the storage domain id that
we are trying to edit should be passed in the parameters.
Line 97: }
Line 98: else if(domains.size() == 1) {
Line 99: setStorageDomain(domains.get(0));
Line 100: }
Line 112: return super.canDoAction();
Line 113: }
Line 114:
Line 115:
Line 116: protected String createDomainNamesList(List<StorageDomain>
domains) {
I'd suggest to replace this method with StringUtils.join
Line 117: //Build domain names list to display in the error
Line 118: StringBuilder domainNames = new StringBuilder();
Line 119: for(StorageDomain domain : domains) {
Line 120: domainNames.append(domain.getStorageName());
Line 137: StoragePoolIsoMap map = getStoragePoolIsoMap();
Line 138:
Line 139: changeStorageDomainStatusInTransaction(map,
StorageDomainStatus.Locked);
Line 140: //connect to the new path
Line 141: boolean hasConnectStorageSucceeded = connectToStorage();
I think that we might have serious issue here - we have domain with guid
"55555" for example, we copied it to another mount and try to edit, how can we
verify that another host doesn't still see the domain in it's old path?
for example in a case in which disconnectStorageServer failed., it can cause to
some serious issues which i guess are undefined (domain with same uuid in two
different paths - which can be hazardous.
Line 142: VDSReturnValue returnValueUpdatedStorageDomain = null;
Line 143:
Line 144: if(!hasConnectStorageSucceeded) {
Line 145: setSucceeded(false);
Line 195: protected void executeInNewTransaction(TransactionMethod<?>
method) {
Line 196: TransactionSupport.executeInNewTransaction(method);
Line 197: }
Line 198:
Line 199: protected boolean connectToStorage() {
perhaps we can use here ConnectStorageToVdsCommand instead.
Line 200: ConnectStorageServerVDSCommandParameters
newConnectionParametersForVdsm =
Line 201: createParametersForVdsm(getParameters().getVdsId(),
Line 202: getParameters().getStoragePoolId(),
Line 203:
getParameters().getStorageServerConnection().getstorage_type(),
Line 206: }
Line 207:
Line 208: protected VDSReturnValue updateStatsForDomain() {
Line 209: return runVdsCommand(VDSCommandType.GetStorageDomainStats,
Line 210: new
GetStorageDomainStatsVDSCommandParameters(getVds().getId(),
getStorageDomain().getId()));
The domain isn't activated in vdsm, are we sure about the return value of
GetStorageDomainStats in that kind of situation, also in previous vdsm versions?
Line 211: }
Line 212:
Line 213: protected ConnectStorageServerVDSCommandParameters
createParametersForVdsm(Guid vdsmId,
Line 214: Guid storagePoolId,
Line 243:
Line 244: @Override
Line 245: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 246: Map<String, Pair<String, String>> locks = new HashMap<String,
Pair<String, String>>();
Line 247: domains =
getStorageDomainsByConnId(getParameters().getStorageServerConnection().getid());
1. this line can be exported to a method instead of repeating it.
2. I don't think that we need to perform this load, we are trying to edit a
connection of specific domain, we should acquire the lock based on the domain
id retrieved from parameters.
Line 248: if(!domains.isEmpty() && domains.size() == 1) {
Line 249: setStorageDomain(domains.get(0));
Line 250: locks.put(getStorageDomain().getId().toString(),
LockMessagesMatchUtil.STORAGE);
Line 251: }
--
To view, visit http://gerrit.ovirt.org/12372
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
