Sergey Gotliv has posted comments on this change. Change subject: engine: Setup multiple iscsi sessions with the iscsi target ......................................................................
Patch Set 8: (8 comments) 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 Done 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 Good point! 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? This comment belongs to another patch. All ids that left in the set after removal should be removed from the database, that's why I remove it. 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. Same as above. 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 caus That's the point, it should not be a part of the loop in the line 94. I have 1 connection and 2 nics => I need to use the same connection with the both of them, so I have to clone connection 1 time. I have 1 connection and 1 nic => I don't need to clone that connection because everything is perfect. 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 I assume that author of "copyOf" meant to create a "clone" method. 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 > ? Done 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. It seems like fnSplitterUuid returns UUID, I need a varchar. 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
