Yevgeny Zaspitsky has posted comments on this change. Change subject: core: Audit log warning about display network connectivity change. (CollectVdsNetworkDataVDSCommand) ......................................................................
Patch Set 2: (11 comments) http://gerrit.ovirt.org/#/c/27168/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcEventNotificationUtils.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/VdcEventNotificationUtils.java: Line 37 Line 38 Line 39 Line 40 Line 41 > was it removed unintentionally ? Done Line 38: AddEventNotificationEntry(EventNotificationEntity.Host, AuditLogType.VDS_HIGH_SWAP_USE); Line 39: AddEventNotificationEntry(EventNotificationEntity.Host, AuditLogType.VDS_LOW_SWAP); Line 40: AddEventNotificationEntry(EventNotificationEntity.Host, AuditLogType.HOST_INTERFACE_STATE_DOWN); Line 41: AddEventNotificationEntry(EventNotificationEntity.Host, Line 42: AuditLogType.NETWORK_UPDATE_DISPLAY_FOR_HOST_WITH_ACTIVE_VM); > perhaps DISPLAY_NETWORK_UPDATE_FOR_HOST_WITH_ACTIVE_VM ? that's derived from the pattern in AuditLogType Line 43: AddEventNotificationEntry(EventNotificationEntity.VirtHost, AuditLogType.VDS_SET_NONOPERATIONAL_DOMAIN); Line 44: AddEventNotificationEntry(EventNotificationEntity.VirtHost, AuditLogType.SYSTEM_CHANGE_STORAGE_POOL_STATUS_NO_HOST_FOR_SPM); Line 45: AddEventNotificationEntry(EventNotificationEntity.VirtHost, AuditLogType.SYSTEM_DEACTIVATED_STORAGE_DOMAIN); Line 46: // VM http://gerrit.ovirt.org/#/c/27168/2/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 514: NETWORK_REMOVE_TEMPLATE_INTERFACE_FAILED=Failed to remove Interface ${InterfaceName} (${InterfaceType}) from Template ${VmTemplateName}. (User: ${UserName}) Line 515: NETWORK_REMOVE_VM_INTERFACE=Interface ${InterfaceName} (${InterfaceType}) was removed from VM ${VmName}. (User: ${UserName}) Line 516: NETWORK_REMOVE_VM_INTERFACE_FAILED=Failed to remove Interface ${InterfaceName} (${InterfaceType}) from VM ${VmName}. (User: ${UserName}) Line 517: NETWORK_UPDATE_DISPLAY_FOR_CLUSTER_WITH_ACTIVE_VM=Update Display Network (${NetworkName}) for Cluster ${VdsGroupName} with active VMs. The change will be applied to those VMs after the next reboot. Line 518: NETWORK_UPDATE_DISPLAY_FOR_HOST_WITH_ACTIVE_VM=Update Display Network Host ${VdsName} with active VMs. The change will be applied to those VMs after the next reboot. Running VMs might loose display connectivity until then. > s/Update Display Network Host/Update Display Network in Host ? shouldn't that be same as in AuditLogType? Line 519: NETWORK_UPDATE_DISPLAY_TO_VDS_GROUP=Update Display Network (${NetworkName}) for Cluster ${VdsGroupName}. (User: ${UserName}) Line 520: NETWORK_UPDATE_DISPLAY_TO_VDS_GROUP_FAILED=Failed to update Display Network (${NetworkName}) for Cluster ${VdsGroupName}. (User: ${UserName}) Line 521: NETWORK_UPDATE_NETWORK_TO_VDS_INTERFACE=Update Network ${NetworkName} in Host ${VdsName}. (User: ${UserName}) Line 522: NETWORK_UPDATE_NETWORK_TO_VDS_INTERFACE_FAILED=Failed to update Network ${NetworkName} in Host ${VdsName}. (User: ${UserName}) http://gerrit.ovirt.org/#/c/27168/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/CollectVdsNetworkDataVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/CollectVdsNetworkDataVDSCommand.java: Line 159: } Line 160: Line 161: private static void logChangedDisplayNetwork( Line 162: VDS vds, Line 163: Collection<Network> engineHostNetworks, > engine prefix for variable name is redundant the "engine" prefix here is as opposite to the vdsm set of data Line 164: Collection<VdsNetworkInterface> engineInterfaces) { Line 165: Line 166: if (isVmRunningOnHost(vds.getId())) { Line 167: final Network engineDisplayNetwork = findDisplayNetwork(engineHostNetworks); Line 177: final DisplayInterfaceEqualityPredicate displayIneterfaceEqualityPredicate = Line 178: new DisplayInterfaceEqualityPredicate(engineDisplayInterface); Line 179: if (vdsmDisplayInterface == null // the display interface is't on host anymore Line 180: || !displayIneterfaceEqualityPredicate.eval(vdsmDisplayInterface)) { Line 181: final AuditLogableBase loggable = new AuditLogableBase(); > can be instantiate using: Done Line 182: loggable.setVdsId(vds.getId()); Line 183: AuditLogDirector.log(loggable, AuditLogType.NETWORK_UPDATE_DISPLAY_FOR_HOST_WITH_ACTIVE_VM); Line 184: } Line 185: } Line 185: } Line 186: } Line 187: Line 188: private static boolean isVmRunningOnHost(Guid hostId) { Line 189: return !DbFacade.getInstance().getVmDao().getAllRunningForVds(hostId).isEmpty(); > replacing with VmDynamicDAO.getAllRunningForVds(id) will save couple of bit Done Line 190: } Line 191: Line 192: private static Collection<Network> findNetworksOnInterfaces( Line 193: Collection<VdsNetworkInterface> ifaces, Line 198: if (clusterNetworksByName.containsKey(interfaceNetworkName)) { Line 199: final Network network = clusterNetworksByName.get(interfaceNetworkName); Line 200: networks.add(network); Line 201: } Line 202: } > please add a space line after closing bracket. Done Line 203: return networks; Line 204: } Line 205: Line 206: private static Network findDisplayNetwork(Collection<Network> networks) { Line 207: Network managementNetwork = null; Line 208: for (Network network : networks) { Line 209: if (network.getCluster().isDisplay()) { Line 210: return network; Line 211: } > please add a space line after closing bracket. Done Line 212: if (NetworkUtils.isManagementNetwork(network)) { Line 213: managementNetwork = network; Line 214: } Line 215: } Line 211: } Line 212: if (NetworkUtils.isManagementNetwork(network)) { Line 213: managementNetwork = network; Line 214: } Line 215: } > same Done Line 216: return managementNetwork; Line 217: } Line 218: Line 219: private static void logUnsynchronizedNetworks(VDS vds, Map<String, Network> networks) { http://gerrit.ovirt.org/#/c/27168/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/predicates/DisplayInterfaceEqualityPredicate.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/predicates/DisplayInterfaceEqualityPredicate.java: Line 22: // at this stage both of the objects are not null Line 23: EqualsBuilder eb = new EqualsBuilder(); Line 24: eb.append(iface.getName(), otherIface.getName()); Line 25: eb.append(iface.getAddress(), otherIface.getAddress()); Line 26: return eb.isEquals(); > why not just: Basically I'd say why not this way. :-) But EqualsBuilder javadoc explains better than me. That's why you should review the code in an IDE... ;-) Line 27: } http://gerrit.ovirt.org/#/c/27168/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/predicates/IsNetworkOnInterfacePredicate.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/predicates/IsNetworkOnInterfacePredicate.java: Line 6: public final class IsNetworkOnInterfacePredicate implements Predicate<VdsNetworkInterface> { Line 7: private final String networkName; Line 8: Line 9: public IsNetworkOnInterfacePredicate(String networkName) { Line 10: this.networkName = networkName; > how about adding constraint for assuring networkName isn't null ? Done Line 11: } Line 12: Line 13: @Override Line 14: public boolean eval(VdsNetworkInterface vdsNetworkInterface) { -- To view, visit http://gerrit.ovirt.org/27168 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9949fc6ed52871ccfb1a15397e2cc722ad231b8f Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
