Moti Asayag has posted comments on this change. Change subject: engine: Audit log warning about display network connectivity change. (CollectVdsNetworkDataVDSCommand) ......................................................................
Patch Set 2: (12 comments) http://gerrit.ovirt.org/#/c/27168/2//COMMIT_MSG Commit Message: Line 7: CollectVdsNetworkDataVDSCommand the class name can be omitted from the subject. 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 ? 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 ? 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 518: Update Display Network Host s/Update Display Network Host/Update Display Network in Host ? see NETWORK_UPDATE_NETWORK_TO_VDS_INTERFACE s/those VMs after the next reboot/those VMs after their next reboot 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 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: public AuditLogableBase(final Guid vdsId) instead of the next assignment. 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 bits and db cpu. 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. 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. 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 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: return iface.getName().equals(otherIface.getName()) && StringUtiles.equals(iface.getAddress(), otherIface.getAddress()); ? 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 ? 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 <yzasp...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
