Liron Ar has posted comments on this change. Change subject: engine: Setup multiple iscsi sessions with the iscsi target ......................................................................
Patch Set 8: (8 comments) partially reviewed http://gerrit.ovirt.org/#/c/23198/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/BaseIscsiBondCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/BaseIscsiBondCommand.java: Line 34: Line 35: protected void connectAllHostsToStorage(List<String> connectionIds) { Line 36: List<StorageServerConnections> connections = getDbFacade().getStorageServerConnectionDao().getByIds(connectionIds); Line 37: List<VDS> hosts = getVdsDAO().getAllForStoragePool(getIscsiBond().getStoragePoolId()); Line 38: you have the method getVdsDAO().getAllForStoragePoolAndStatus Line 39: for (VDS host : hosts) { Line 40: if (host.getStatus() == VDSStatus.Up) { Line 41: List<StorageServerConnections> conns = ISCSIStorageHelper.updateIfaces(connections, host.getId()); Line 42: Line 40: if (host.getStatus() == VDSStatus.Up) { Line 41: List<StorageServerConnections> conns = ISCSIStorageHelper.updateIfaces(connections, host.getId()); Line 42: Line 43: runVdsCommand(VDSCommandType.ConnectStorageServer, Line 44: new StorageServerConnectionManagementVDSParameters(host.getId(), Guid.Empty, StorageType.ISCSI, conns) perhaps execute simultenously Line 45: ); Line 46: } Line 47: } Line 48: } http://gerrit.ovirt.org/#/c/23198/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/EditIscsiBondCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/EditIscsiBondCommand.java: Line 80: private void updateNetworksIds() { Line 81: Set<Guid> beforeChangeNetworkIds = new HashSet<>(getExistingIscsiBond().getNetworkIds()); Line 82: Line 83: for (Guid networkId : getIscsiBond().getNetworkIds()) { Line 84: if (!beforeChangeNetworkIds.remove(networkId)) { why remove? just check if it exists there. Line 85: addedNetworks.add(networkId); Line 86: getDbFacade().getIscsiBondDao().addNetworkToIscsiBond(getExistingIscsiBond().getId(), networkId); Line 87: } Line 88: } Line 95: private void updateConnectionsIds() { Line 96: Set<String> beforeChangeConnectionIds = new HashSet<>(getExistingIscsiBond().getStorageConnectionIds()); Line 97: Line 98: for (String connectionId : getIscsiBond().getStorageConnectionIds()) { Line 99: if (!beforeChangeConnectionIds.remove(connectionId)) { why remove? just check if it exists there. Line 100: addedConnections.add(connectionId); Line 101: getDbFacade().getIscsiBondDao().addStorageConnectionToIscsiBond(getExistingIscsiBond().getId(), connectionId); Line 102: } Line 103: } http://gerrit.ovirt.org/#/c/23198/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java: Line 86: List<VdsNetworkInterface> ifaces = DbFacade.getInstance().getInterfaceDao() Line 87: .getIscsiIfacesByHostIdAndStorageTargetId(vdsId, conn.getid()); Line 88: Line 89: if (!ifaces.isEmpty()) { Line 90: conn.setIface(ifaces.remove(0).getName()); why do you remove it? it won't be part of the loop on line 94 and can cause only to degraded performance Line 91: Line 92: // Iscsi target is represented by connection object, therefore if this target is approachable Line 93: // from more than one endpoint(initiator) we have to clone this connection per endpoint. Line 94: for (VdsNetworkInterface iface : ifaces) { Line 91: Line 92: // Iscsi target is represented by connection object, therefore if this target is approachable Line 93: // from more than one endpoint(initiator) we have to clone this connection per endpoint. Line 94: for (VdsNetworkInterface iface : ifaces) { Line 95: StorageServerConnections newConn = StorageServerConnections.copyOf(conn); i suggest to add a clone which will also take care of the id Line 96: newConn.setid(Guid.newGuid().toString()); Line 97: newConn.setIface(iface.getName()); Line 98: res.add(newConn); Line 99: } http://gerrit.ovirt.org/#/c/23198/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveIscsiBondCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveIscsiBondCommand.java: Line 1 Line 2 Line 3 Line 4 Line 5 ? http://gerrit.ovirt.org/#/c/23198/8/packaging/dbscripts/storages_san_sp.sql File packaging/dbscripts/storages_san_sp.sql: Line 511: RETURNS SETOF storage_server_connections STABLE Line 512: AS $procedure$ Line 513: BEGIN Line 514: RETURN QUERY SELECT * Line 515: FROM storage_server_connections WHERE id = any(string_to_array(v_ids,',')::VARCHAR[]); please use fnSplitterUuid instead. Line 516: END; $procedure$ Line 517: LANGUAGE plpgsql; Line 518: Line 519: -- To view, visit http://gerrit.ovirt.org/23198 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I779f6dd95dfbfc2b74ad7ba3ce2271b7c9ad94db Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amador Pahim <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
