Ayal Baron has posted comments on this change.
Change subject: engine: connectStorageServer is not sent for inactive domains
before connectStoragePool
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File backend/manager/dbscripts/storages_san_sp.sql
Line 537: LANGUAGE plpgsql;
Line 538:
Line 539:
Line 540:
Line 541: Create or replace FUNCTION
Getstorage_server_connectionsActiveUnknownInactiveByPoolId(v_storage_pool_id
UUID)
The name is misleading. Active / Unknown / Inactive are states of the domains,
not the connections.
Line 542: RETURNS SETOF storage_server_connections
Line 543: AS $procedure$
Line 544: BEGIN
Line 545: RETURN QUERY SELECT distinct storage_server_connections.*
Line 547: LUN_storage_server_connection_map LUN_storage_server_connection_map
Line 548: INNER JOIN LUNs ON LUN_storage_server_connection_map.LUN_id =
LUNs.LUN_id
Line 549: INNER JOIN storage_domains ON LUNs.volume_group_id =
storage_domains.storage
Line 550: INNER JOIN storage_server_connections ON
LUN_storage_server_connection_map.storage_server_connection =
storage_server_connections.id
Line 551: WHERE (storage_domains.storage_pool_id = v_storage_pool_id
and storage_domains.status in(0,3,4))
If anything, logic should be reversed. You want all storage domains whose
status is not maintenance. That way you don't need to update it every time you
add a new status.
However, I'm not sure why you're filtering in the select and not in the code
according to the logic you need.
Line 552: UNION
Line 553: SELECT distinct storage_server_connections.*
Line 554: FROM storage_server_connections
Line 555: INNER JOIN storage_domains ON storage_server_connections.id =
storage_domains.storage
Line 552: UNION
Line 553: SELECT distinct storage_server_connections.*
Line 554: FROM storage_server_connections
Line 555: INNER JOIN storage_domains ON storage_server_connections.id =
storage_domains.storage
Line 556: WHERE (storage_domains.storage_pool_id = v_storage_pool_id and
storage_domains.status in(0,3,4));
same
Line 557: END; $procedure$
Line 558: LANGUAGE plpgsql;
Line 559:
Line 560:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConnectHostToStoragePooServerCommandBase.java
Line 72: List<StorageDomain> exportDomains =
Line 73: getStorageDomainsByStoragePoolId(allDomains,
StorageDomainType.ImportExport);
Line 74:
Line 75: Set<StorageServerConnections> connections = new
HashSet<StorageServerConnections>(
Line 76:
DbFacade.getInstance().getStorageServerConnectionDao().getActiveUnknownInActiveForStoragePool(getStoragePool().getId()));
getStorageDomainsByStoragePoolId is filtering according to the status of the
domains as it was when you selected allDomains above.
What is preventing the status of the domains to change from then until you
filter the domains here in getActiveUnknownInActive again?
e.g. iso domain becomes active between line 72 and 75 and now connection list
will include NFS connections that you will not remove below. If the data
domains are iSCSI my guess is that this will cause an error?
Seems to me that the approach of this method is wrong.
You should just select the connections from the db and then split into lists
according to type (nfs/posix/iscsi/...) and then just connect those.
This way you don't assume (as code currently does) that all data domains are of
the same type, nor do you care about the type of domain (export/data/iso) etc.
If you use a dictionary then you can do this in a single loop without also
caring what connection types exist either
Line 77: if (isoDomains.size() != 0) {
Line 78: _isoType = isoDomains.get(0).getStorageType();
Line 79: Set<StorageServerConnections> isoConnections =
Line 80: new HashSet<StorageServerConnections>(
Line 110: List<StorageDomain> domains = new ArrayList<StorageDomain>();
Line 111: for (StorageDomain s : allDomains) {
Line 112: StorageDomainStatus status = s.getStatus();
Line 113: if (s.getStorageDomainType() == type
Line 114: && (StorageDomainStatus.Active == status ||
StorageDomainStatus.Unknown == status || StorageDomainStatus.InActive ==
status)) {
Here too need to just make sure status != StorageDomainStatus.Maintenance
Line 115: domains.add(s);
Line 116: }
Line 117: }
Line 118: return domains;
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAO.java
Line 37: */
Line 38: List<StorageServerConnections> getAll();
Line 39:
Line 40: /**
Line 41: * Retrieves all Active, Unknown , InActive connections for the
specified storage pool.
Inactive is a word in English, no need for capital 'A'
Line 42: *
Line 43: * @param pool
Line 44: * the storage pool
Line 45: * @return the list of connections
--
To view, visit http://gerrit.ovirt.org/12805
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb3710bb83ef81485715577ae939014ffcae693f
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
