Eliraz Levi has posted comments on this change. Change subject: backend: engine resolving host's active interface ......................................................................
Patch Set 6: (9 comments) https://gerrit.ovirt.org/#/c/35895/6/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 163: : : : : : > Why did you removed this check? the log message is not relevant as lastClientInterface is deprecated (feature main purpose). in case host's active nic is null, then nic will be null and the current error message describes it. the current message is not clear enough? Line 33: : import java.util.ArrayList; : import java.util.HashMap; : import java.util.List; : import java.util.Map; : import java.util.concurrent.TimeUnit; : > please put java.* imports first and setup your IDE settings for that Done Line 64: : log.info("The management network '{}' is already configured on host '{}'", : managementNetworkName, : host.getName()); > the message does not match the check. pls separate checking mmanagement net in order to keep previous functionality, the mgmt network should be configured over the host's ip. in case mgmt network exist, we should verify it has the same ip as host's ip. if the ip is different , the mgmt network should be re configured. note - in this case the mgmt creation will fail as engine prohibit the configuration of one network (in this case, mgmt) on 2 different interfaces. Line 100: : private String resolveHostManagementNetworkAddress(String hostManagmentNetworkName){ : for (VdsNetworkInterface iface : host.getInterfaces()){ : if (iface.getNetworkName() != null && iface.getNetworkName().equals(hostManagmentNetworkName)) { : return iface.getAddress(); : } : } : : return null; : } > this looks like findNicByNetworkName functionality. Iguess we already have Done. i added a predicate to do so. https://gerrit.ovirt.org/#/c/35895/6/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java: Line 289: Logger log > Why do that need callee's logger? The local one could be used instead. Done Line 290: // TODO elevi check in case of null > in case of null loopback address will be returned. Is that what we want? why would hostName will be null? Line 290: : String output = null; : InetAddress address = null; : try { : address = InetAddress.getByName(vds.getHostName()); : output = address.getHostAddress().trim(); : } catch (Exception ex) { : log.warn("Fail to resolve host ip by name '{}', Details: '{}' ", vds.getHostName(), ex.toString()); : log.debug("Exception", ex); : } finally { : return output; : } > I'd write it that way: Done https://gerrit.ovirt.org/#/c/35895/6/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 1469: resolveActiveNic > the mmethod name does not describe wjat method does Done Line 1505: setActiveNic > here you might be overriding the value that you already set few instruction Done -- To view, visit https://gerrit.ovirt.org/35895 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64a21595e58358fe9e04ea1de5d3e3115c7bc2c6 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
