Lior Vernia has posted comments on this change. Change subject: engine: Send net_ifacename to VDSM when connecting iSCSI server ......................................................................
Patch Set 1: Code-Review+1 (3 comments) http://gerrit.ovirt.org/#/c/32383/1/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 76: for (StorageServerConnections conn : conns) { Line 77: // Get list of endpoints (nics or vlans) that will initiate iscsi sessions. Line 78: // Targets are represented by StorageServerConnections object (connection, iqn, port, portal). Line 79: List<VdsNetworkInterface> ifaces = DbFacade.getInstance().getInterfaceDao() Line 80: .getIscsiIfacesByHostIdAndStorageTargetId(vdsId, conn.getid()); I assume this fetches VLAN devices properly, i.e. if the iSCSI bond only uses eth0.101, this won't return eth0 right? Line 81: Line 82: if (!ifaces.isEmpty()) { Line 83: VdsNetworkInterface vdsNetworkInterface = ifaces.remove(0); Line 84: conn.setIface(vdsNetworkInterface.getName()); Line 81: Line 82: if (!ifaces.isEmpty()) { Line 83: VdsNetworkInterface vdsNetworkInterface = ifaces.remove(0); Line 84: conn.setIface(vdsNetworkInterface.getName()); Line 85: conn.setNetIfacename(vdsNetworkInterface.isBridged() ? Since this is used at least twice here, I would extract this logic into a method e.g. getTopLevelDeviceName(VdsNetworkInterface iface). Line 86: vdsNetworkInterface.getNetworkName() : vdsNetworkInterface.getName()); Line 87: Line 88: // Iscsi target is represented by connection object, therefore if this target is approachable Line 89: // from more than one endpoint(initiator) we have to clone this connection per endpoint. http://gerrit.ovirt.org/#/c/32383/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageServerConnections.java: Line 188: public void setIface(String iface) { Line 189: this.iface = iface; Line 190: } Line 191: Line 192: private String netIfacename; Please consider a more meaningful name, e.g. "topLevelNetworkDevice". Line 193: Line 194: public String getNetIfacename() { Line 195: return netIfacename; Line 196: } -- To view, visit http://gerrit.ovirt.org/32383 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3e2f3549fa08741f719eeb8d70209ab298e09da3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amador Pahim <apa...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
