Alona Kaplan has posted comments on this change. Change subject: engine: event on interface status change ......................................................................
Patch Set 2: (11 comments) http://gerrit.ovirt.org/#/c/26104/2/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 217: VDS_ACTIVATE_FAILED=Failed to activate Host ${VdsName}.(User: ${UserName}). Line 218: VDS_ACTIVATE_FAILED_ASYNC=Failed to autorecover Host ${VdsName}. Line 219: HOST_REFRESHED_CAPABILITIES=Successfully refreshed the capabilities of host ${VdsName}. Line 220: HOST_REFRESH_CAPABILITIES_FAILED=Failed to refresh the capabilities of host ${VdsName}. Line 221: HOST_IFACE_DOWN=Interface '${InterfaceName}' on host ${VdsName}, changed state to down > s/HOST_IFACE_DOWN/HOST_INTERFACE_STATE_DOWN Done Line 222: HOST_IFACE_UP=Interface '${InterfaceName}' on host ${VdsName}, changed state to up Line 223: HOST_BOND_DOWN=Bond '${InterfaceName}' on host ${VdsName}, changed state to down Line 224: HOST_BOND_UP=Bond '${InterfaceName}' on host ${VdsName}, changed state to up Line 225: VDS_DETECTED=Status of host ${VdsName} was set to ${HostStatus}. Line 218: VDS_ACTIVATE_FAILED_ASYNC=Failed to autorecover Host ${VdsName}. Line 219: HOST_REFRESHED_CAPABILITIES=Successfully refreshed the capabilities of host ${VdsName}. Line 220: HOST_REFRESH_CAPABILITIES_FAILED=Failed to refresh the capabilities of host ${VdsName}. Line 221: HOST_IFACE_DOWN=Interface '${InterfaceName}' on host ${VdsName}, changed state to down Line 222: HOST_IFACE_UP=Interface '${InterfaceName}' on host ${VdsName}, changed state to up > s/HOST_IFACE_UP/HOST_INTERFACE_STATE_UP Done Line 223: HOST_BOND_DOWN=Bond '${InterfaceName}' on host ${VdsName}, changed state to down Line 224: HOST_BOND_UP=Bond '${InterfaceName}' on host ${VdsName}, changed state to up Line 225: VDS_DETECTED=Status of host ${VdsName} was set to ${HostStatus}. Line 226: VDS_FAILURE=Host ${VdsName} is non responsive. Line 219: HOST_REFRESHED_CAPABILITIES=Successfully refreshed the capabilities of host ${VdsName}. Line 220: HOST_REFRESH_CAPABILITIES_FAILED=Failed to refresh the capabilities of host ${VdsName}. Line 221: HOST_IFACE_DOWN=Interface '${InterfaceName}' on host ${VdsName}, changed state to down Line 222: HOST_IFACE_UP=Interface '${InterfaceName}' on host ${VdsName}, changed state to up Line 223: HOST_BOND_DOWN=Bond '${InterfaceName}' on host ${VdsName}, changed state to down > s/HOST_BOND_DOWN/HOST_BOND_STATE_DOWN Removed this message. Added separate message for bond slave. Line 224: HOST_BOND_UP=Bond '${InterfaceName}' on host ${VdsName}, changed state to up Line 225: VDS_DETECTED=Status of host ${VdsName} was set to ${HostStatus}. Line 226: VDS_FAILURE=Host ${VdsName} is non responsive. Line 227: VDS_MAINTENANCE=Host ${VdsName} was switched to Maintenance Mode. Line 220: HOST_REFRESH_CAPABILITIES_FAILED=Failed to refresh the capabilities of host ${VdsName}. Line 221: HOST_IFACE_DOWN=Interface '${InterfaceName}' on host ${VdsName}, changed state to down Line 222: HOST_IFACE_UP=Interface '${InterfaceName}' on host ${VdsName}, changed state to up Line 223: HOST_BOND_DOWN=Bond '${InterfaceName}' on host ${VdsName}, changed state to down Line 224: HOST_BOND_UP=Bond '${InterfaceName}' on host ${VdsName}, changed state to up > s/HOST_BOND_UP/HOST_BOND_STATE_UP Removed this message. Added separate message for bond slave. Line 225: VDS_DETECTED=Status of host ${VdsName} was set to ${HostStatus}. Line 226: VDS_FAILURE=Host ${VdsName} is non responsive. Line 227: VDS_MAINTENANCE=Host ${VdsName} was switched to Maintenance Mode. Line 228: VDS_MAINTENANCE_MANUAL_HA=Host ${VdsName} was switched to Maintenance mode, but Hosted Engine HA maintenance could not be enabled. Please enable it manually. http://gerrit.ovirt.org/#/c/26104/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java: Line 755: Set<VdsNetworkInterface> slaves = new HashSet<>(); Line 756: Map<String, InterfaceStatus> monitoredInterfaces = new HashMap<String, InterfaceStatus>(); Line 757: Line 758: for (VdsNetworkInterface iface : interfaces) { Line 759: if (iface.getBondName() != null) { > it would be nice to expose new method in VdsNetworkInterface named isSlave I will do it in a separate patch. Line 760: slaves.add(iface); Line 761: } Line 762: Line 763: String parentIfaceName = Line 760: slaves.add(iface); Line 761: } Line 762: Line 763: String parentIfaceName = Line 764: iface.getVlanId() == null ? iface.getName() : NetworkUtils.stripVlan(iface.getName()); > no need for null check - see the javadoc of NetworkUtils.stripVlan(String): Done Line 765: Line 766: // If the parent interface already marked as monitored- no need to check it again Line 767: if (monitoredInterfaces.containsKey(parentIfaceName)) { Line 768: continue; Line 769: } Line 770: Line 771: // The status of the interface should be monitored only if it has networks attached to it or has labels Line 772: if (StringUtils.isNotEmpty(iface.getNetworkName()) Line 773: || (iface.getLabels() != null && !iface.getLabels().isEmpty())) { > use NetworkUtils.isLabeled(iface) instead Done Line 774: VdsNetworkInterface parentIface = iface; Line 775: // If vlan find the parent interface Line 776: if (iface.getVlanId() != null) { Line 777: for (VdsNetworkInterface tmpIface : interfaces) { Line 773: || (iface.getLabels() != null && !iface.getLabels().isEmpty())) { Line 774: VdsNetworkInterface parentIface = iface; Line 775: // If vlan find the parent interface Line 776: if (iface.getVlanId() != null) { Line 777: for (VdsNetworkInterface tmpIface : interfaces) { > please replace the for loop with Entities.entitiesByName(interfaces) that w Done Line 778: if (parentIfaceName.equals(tmpIface.getName())) { Line 779: parentIface = tmpIface; Line 780: } Line 781: } Line 778: if (parentIfaceName.equals(tmpIface.getName())) { Line 779: parentIface = tmpIface; Line 780: } Line 781: } Line 782: } > please add a space line after the parenthesis Done Line 783: monitoredInterfaces.put(parentIface.getName(), parentIface.getStatistics().getStatus()); Line 784: } Line 785: } Line 786: Line 779: parentIface = tmpIface; Line 780: } Line 781: } Line 782: } Line 783: monitoredInterfaces.put(parentIface.getName(), parentIface.getStatistics().getStatus()); > why not to use parentIfaceName as the key ? Done Line 784: } Line 785: } Line 786: Line 787: // Slaves should be monitored if the bond is monitored Line 797: && oldIface.getStatistics().getStatus() != status) { Line 798: AuditLogableBase logable = new AuditLogableBase(_vds.getId()); Line 799: logable.setCustomId(oldIface.getName()); Line 800: logable.addCustomValue("InterfaceName", oldIface.getName()); Line 801: if (oldIface.getBonded() != null && oldIface.getBonded()) { > you could use: No need for this condition any more. Removed the separate message for bonds. Line 802: auditLog(logable, status == InterfaceStatus.UP ? AuditLogType.HOST_BOND_UP Line 803: : AuditLogType.HOST_BOND_DOWN); Line 804: } else { Line 805: auditLog(logable, status == InterfaceStatus.UP ? AuditLogType.HOST_IFACE_UP -- To view, visit http://gerrit.ovirt.org/26104 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I76953e4b52e35d2e18b6bc68051fa7dd6f3db129 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
