Martin Peřina has posted comments on this change. Change subject: core: scanning of configured NICs for health ......................................................................
Patch Set 5: (8 comments) Personally I would introduce NicsHealthCheckManager in one patch and add usage of NicsHealthCheckManager in fencing flow in another patch http://gerrit.ovirt.org/#/c/34222/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsNotRespondingTreatmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsNotRespondingTreatmentCommand.java: Line 72: return new FenceExecutor(getVds(), getParameters().getAction()); Line 73: } Line 74: Line 75: private boolean shouldFencingBeSkipped(VDS vds) { Line 76: if (!nicsHealthCheckManager.isSystemHealthy()) { Please add an alert why fencing was skipped. And the same condition should be also added to SSHSoftFencingCommand.executeCommand() Line 77: return true; Line 78: } Line 79: Line 80: // check if fencing in cluster is enabled http://gerrit.ovirt.org/#/c/34222/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/enginehealth/NicsHealthCheckManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/enginehealth/NicsHealthCheckManager.java: Line 74: log.debug("Performing engine NICs health check on: " + nicsToString(nicsToCheck)); Line 75: Line 76: checkNicsAndUpdateEngineStatus(nicsToCheck); Line 77: Line 78: log.debug("Health check done, engine NICs are " + (engineNicHealthCache.isAllNicHealthy() ? "healthy" : "unhealthy") + "."); Please use slf4j way of formatting log messages: log.debug("Health check done, engine NICs are {}.", engineNicHealthCache.isAllNicHealthy() ? "healthy" : "unhealthy"); Line 79: } Line 80: Line 81: public boolean isSystemHealthy() { Line 82: return engineNicHealthCache.isAllNicHealthy(); Line 86: for (NetworkInterface nic : nicsToCheck) { Line 87: boolean nicIsHealthy = isNicHealthy(nic); Line 88: boolean updated = engineNicHealthCache.update(nic, nicIsHealthy); Line 89: if (updated && !nicIsHealthy) { Line 90: log.warn(String.format("Engine NIC '%1$s', configured for regular health check, is not UP!", Please use slf4j way of formatting log messages: log.warn("Engine NIC '{}', configured for regular health check, is not UP!", nic.getDisplayName()); Line 91: nic.getDisplayName())); Line 92: } Line 93: } Line 94: } Line 99: } catch (SocketException e) { Line 100: String message = String.format("Unable to get network interface '%1$s' status, pronouncing it unhealthy.", Line 101: networkInterface.getDisplayName()); Line 102: log.debug(message, e); Line 103: log.warn(message); Please use new suggested exception logging: log.warn("Unable to get network interface '{}' status, pronouncing it unhealthy: {}", networkInterface.getDisplayName(), e.getMessage()); log.debug("Exception", e); Line 104: return false; Line 105: } Line 106: } Line 107: Line 107: Line 108: /** Line 109: * @return string representation of NetworkInterface list for logging purposes. Line 110: */ Line 111: private String nicsToString(List<NetworkInterface> networkInterfaces) { Why not use: StringUtils.join(networkInterfaces, ", ") instead of this method? Line 112: int size = networkInterfaces.size(); Line 113: Line 114: if (size == 0) { Line 115: return ""; Line 141: Line 142: for (String nicName : nicNames) { Line 143: NetworkInterface networkInterface = networkInterfaceByName(nicName); Line 144: if (networkInterface == null) { Line 145: log.warn(String.format("NIC '%1$s' configured to be checked is not found in the system. This NIC will be ignored.", Please use slf4j way of formatting log messages: log.warn("NIC '{}' configured to be checked is not found in the system. This NIC will be ignored.", nicName); Line 146: nicName)); Line 147: } else { Line 148: result.add(networkInterface); Line 149: } Line 159: private NetworkInterface networkInterfaceByName(String nicName) { Line 160: try { Line 161: return NetworkInterface.getByName(nicName); Line 162: } catch (SocketException e) { Line 163: log.debug(String.format("Getting NetworkInterface by name '%1$s' failed.", nicName), e); Please use slf4j way of formatting log messages: log.debug(MessageFormatter.format("Getting NetworkInterface by name '{}' failed.", nicName), e); Line 164: return null; Line 165: } Line 166: } Line 167: Line 223: return result; Line 224: } Line 225: Line 226: private boolean delayOk(Date now, long lastFailure) { Line 227: return now.getTime() - lastFailure > (long) Config.getValue(ConfigValues.EngineNicsHealthCheckDelay); Why not use: Config.<Long> getValue(ConfigValues.EngineNicsHealthCheckDelay) instead of casting? Line 228: } Line 229: } -- To view, visit http://gerrit.ovirt.org/34222 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c092ab5d3cf33e1ef83cd0225d2803733fbde89 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
