Lior Vernia has posted comments on this change. Change subject: backend: engine resolving host's active nic ......................................................................
Patch Set 2: (10 comments) http://gerrit.ovirt.org/#/c/35895/2//COMMIT_MSG Commit Message: Line 7: backend: engine resolving host's active nic Line 8: Line 9: engine will resolve host's active nic by looking Line 10: into vdsGetCapabilities and see which nic is the Line 11: one is being used by ovirtmgmt This isn't accurate - what engine checks is which NIC owns the IP to which the hostname resolves, unrelated (at that moment) to our ovirtmgmt network. Line 12: Line 13: Change-Id: I64a21595e58358fe9e04ea1de5d3e3115c7bc2c6 http://gerrit.ovirt.org/#/c/35895/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/NetworkConfigurator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/NetworkConfigurator.java: Line 58: return; Line 59: } Line 60: Line 61: Map<String, Network> vdsNetworks = Entities.entitiesByName(host.getNetworks()); Line 62: if (vdsNetworks.containsKey(managementNetwork)) { This condition isn't equivalent to what used to be there... It would be equivalent if you also checked that the configured management network had the same IP address as the resolved hostname. Line 63: log.info("The management network '{}' is already configured on host '{}'", Line 64: managementNetwork, Line 65: host.getName()); Line 66: return; Line 161: + " If the interface is a bridge, it should be torn-down manually.", Line 162: host.getActiveNic(), Line 163: host.getName()); Line 164: throw new NetworkConfiguratorException( Line 165: String.format("Invalid interface for management network")); I would keep activeNic as part of the exception message - it might help in debugging. Note that it doesn't have to be null... It can be a bridge, OVS etc... Line 166: } Line 167: Line 168: Network managementNetwork = Line 169: getDbFacade().getNetworkDao() http://gerrit.ovirt.org/#/c/35895/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 566: vds.setSoftwareVersion(AssignStringValue(xmlRpcStruct, VdsProperties.software_version)); Line 567: vds.setBuildName(AssignStringValue(xmlRpcStruct, VdsProperties.build_name)); Line 568: if (xmlRpcStruct.containsKey(VdsProperties.host_os)) { Line 569: vds.setHostOs(GetPackageVersionFormated((Map<String, Object>) xmlRpcStruct.get(VdsProperties.host_os), Line 570: true)); Please note that you format the entire file and not just the lines you work on. This is bad practice, as it appears like you change more rows than you actually are changing. Line 571: } Line 572: if (xmlRpcStruct.containsKey(VdsProperties.packages)) { Line 573: // packages is an array of xmlRpcStruct (that each is a name, ver, Line 574: // release.. of a package) Line 990: } Line 991: String stringValue = (String) input.get(name); Line 992: if (!StringUtils.isEmpty(stringValue)) { // in case the input Line 993: // is decimal and we Line 994: // need int. And here, for example, you're breaking intentional layout... Line 995: stringValue = stringValue.split("[.]", -1)[0]; Line 996: } Line 997: try { Line 998: int intValue = Integer.parseInt(stringValue); Line 1201: addHostVlanDevices(vds, xmlRpcStruct); Line 1202: Line 1203: addHostBondDevices(vds, xmlRpcStruct); Line 1204: Line 1205: //by now, if the host is communicating with engine over a valid interface, the interface will have the host's engine IP And ironically, this row that should be formatted isn't. Line 1206: resolveActiveNic(vds); Line 1207: Line 1208: addHostNetworksAndUpdateInterfaces(vds, xmlRpcStruct); Line 1209: Line 1220: InetAddress mgmt = null; Line 1221: try { Line 1222: mgmt = InetAddress.getByName(vds.getHostName()); Line 1223: Line 1224: } catch (SecurityException ex) { Since there's no difference in how you handle these exceptions, you could group them under a single catch (Exception ex) statement. Line 1225: log.warn("Fail to resolve host ip by name '{}', Details: '{}' ", vds.getHostName(), ex.toString()); Line 1226: log.debug("Exception", ex); Line 1227: return; Line 1228: } catch (UnknownHostException ex) { Line 1229: log.warn("Fail to resolve host ip by name '{}', Details: '{}' ", vds.getHostName(), ex.toString()); Line 1230: log.debug("Exception", ex); Line 1231: return; Line 1232: } Line 1233: Double blank line? Line 1234: Line 1235: String mgmtAdd = mgmt.getHostAddress().trim(); Line 1236: for (VdsNetworkInterface iface : vds.getInterfaces()) { Line 1237: if (iface.getAddress().equals(mgmtAdd)){ Line 1236: for (VdsNetworkInterface iface : vds.getInterfaces()) { Line 1237: if (iface.getAddress().equals(mgmtAdd)){ Line 1238: vds.setActiveNic(iface.getName()); Line 1239: } Line 1240: Unnecessary blank line... Line 1241: } Line 1242: Line 1243: } Line 1244: Line 1238: vds.setActiveNic(iface.getName()); Line 1239: } Line 1240: Line 1241: } Line 1242: Here as well... Line 1243: } Line 1244: Line 1245: private static void addHostNetworksAndUpdateInterfaces(VDS vds, Line 1246: Map<String, Object> xmlRpcStruct) { -- To view, visit http://gerrit.ovirt.org/35895 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64a21595e58358fe9e04ea1de5d3e3115c7bc2c6 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: [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
