Piotr Kliczewski has posted comments on this change. Change subject: core: VM Monitoring abstract fetching/analyzing/monitoring ......................................................................
Patch Set 9: (8 comments) http://gerrit.ovirt.org/#/c/28662/9/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmAnalyzer.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmAnalyzer.java: Line 258: // TODO switch command execution with state change and let a final execution point at #VmsMonitoring crate tasks out of the new state. this can be delegated to some task Q instead of running in-thread Line 259: private void proceedVmBeforeDeletion() { Line 260: AuditLogType type = AuditLogType.UNASSIGNED; Line 261: AuditLogableBase logable = new AuditLogableBase(getVdsManager().getVdsId(), dbVm.getId()); Line 262: switch (dbVm.getStatus()) { Why do we need to use switch statement here for single status? It would be simpler to use if. Line 263: case MigratingFrom: { Line 264: // if a VM that was a source host in migration process is now down with normal Line 265: // exit status that's OK, otherwise.. Line 266: if (vdsmVm.getVmDynamic() != null && vdsmVm.getVmDynamic().getExitStatus() != VmExitStatus.Normal) { Line 410: if (vmStatistics != null && vmStatistics.getVmBalloonInfo().getCurrentMemory() != null && Line 411: vmStatistics.getVmBalloonInfo().getCurrentMemory() > 0 && Line 412: dbVm.getMinAllocatedMem() > vmStatistics.getVmBalloonInfo().getCurrentMemory() / TO_MEGA_BYTES) { Line 413: AuditLogableBase auditLogable = new AuditLogableBase(); Line 414: auditLogable.addCustomValue("VmName", dbVm.getName()); Can me make those strings constants? Line 415: auditLogable.addCustomValue("VdsName", this.getVdsManager().getVdsName()); Line 416: auditLogable.addCustomValue("MemGuaranteed", String.valueOf(dbVm.getMinAllocatedMem())); Line 417: auditLogable.addCustomValue("MemActual", Line 418: Long.toString((vmStatistics.getVmBalloonInfo().getCurrentMemory() / TO_MEGA_BYTES))); Line 427: VmDynamic vdsmVmDynamic = vdsmVm.getVmDynamic(); Line 428: Line 429: // if not migrating here and not down Line 430: if (!inMigrationTo(vdsmVmDynamic, dbVm) && vdsmVmDynamic.getStatus() != VMStatus.Down) { Line 431: if (dbVm != null) { Is it possible to dbVm will become null in between if statements here? We have 4 if statements which check whether dbVm is null. Line 432: if (!StringUtils.equals(vdsmVmDynamic.getClientIp(), dbVm.getClientIp())) { Line 433: clientIpChanged = true; Line 434: } Line 435: } Line 798: */ Line 799: private void prepareGuestAgentNetworkDevicesForUpdate() { Line 800: if (vdsmVm != null) { Line 801: VmDynamic vmDynamicDynamic = vdsmVm.getVmDynamic(); Line 802: if (vmDynamicDynamic != null) { we can combine if condition here. Line 803: if (dbVm != null) { Line 804: List<VmGuestAgentInterface> vmGuestAgentInterfaces = vdsmVm.getVmGuestAgentInterfaces(); Line 805: int guestAgentNicHash = vmGuestAgentInterfaces == null ? 0 : vmGuestAgentInterfaces.hashCode(); Line 806: if (guestAgentNicHash != vmDynamicDynamic.getGuestAgentNicsHash()) { http://gerrit.ovirt.org/#/c/28662/9/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsListFetcher.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsListFetcher.java: Line 57: VDSCommandType.List, Line 58: new VdsIdAndVdsVDSCommandParametersBase(vdsManager.getVds())); Line 59: if (getList.getSucceeded()) { Line 60: vdsmVms = (Map<Guid, VmInternalData>) getList.getReturnValue(); Line 61: } Do we want to run onFetchVms if getList failed? It looks like we will but there won't be any delta. Line 62: onFetchVms(); Line 63: } Line 64: Line 65: protected void onFetchVms() { http://gerrit.ovirt.org/#/c/28662/9/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsMonitoring.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsMonitoring.java: Line 114: * analyze and react upon changes on the monitoredVms. relevant changes would Line 115: * be persisted and state transitions and internal commands would Line 116: * take place accordingly. Line 117: */ Line 118: public void begin() { This method perform refresh but begin suggests next steps. Line 119: try { Line 120: refreshExistingVmJobList(); Line 121: refreshVmStats(); Line 122: afterVMsRefreshTreatment(); Line 234: // If there are vms that require updating, Line 235: // get the new info from VDSM in one call, and then update them all Line 236: if (!vmsWithChangedDevices.isEmpty()) { Line 237: ArrayList<String> vmsToUpdate = new ArrayList<>(vmsWithChangedDevices.size()); Line 238: for (Pair<VM, VmInternalData> pair : vmsWithChangedDevices) { we can use getVmId method from this class here? Line 239: if (vmDynamicToSave.containsKey(pair.getFirst().getId())) { Line 240: vmDynamicToSave.get(pair.getFirst().getId()).setHash(pair.getSecond().getVmDynamic().getHash()); Line 241: } else { Line 242: addVmDynamicToList(pair.getSecond().getVmDynamic()); http://gerrit.ovirt.org/#/c/28662/9/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsStatisticsFetcher.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsStatisticsFetcher.java: Line 23: VDSCommandType.GetAllVmStats, Line 24: new VdsIdAndVdsVDSCommandParametersBase(vdsManager.getVds())); Line 25: if (getStats.getSucceeded()) { Line 26: vdsmVms = (Map<Guid, VmInternalData>) getStats.getReturnValue(); Line 27: } Do we want to run onFetchVms when getAllVmStats failed? Line 28: onFetchVms(); Line 29: } Line 30: Line 31: @Override -- To view, visit http://gerrit.ovirt.org/28662 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1adf0a95007140e89b080b5160ba93e340ee3ba6 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Roy Golan <rgo...@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
