Arik Hadas has posted comments on this change. Change subject: events: removing vm.getStats from event processing ......................................................................
Patch Set 17: (6 comments) https://gerrit.ovirt.org/#/c/40205/17/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java: Line 75: private Map<GraphicsType, GraphicsInfo> graphicsInfos; Line 76: private Long guestMemoryCached; Line 77: private Long guestMemoryBuffered; Line 78: private Long guestMemoryFree; Line 79: private Double statusUpdatedTime; this field should not be in VmDynamic/VM, you need to get the time reported by VDSM and report it back to the monitoring. this field is not persisted (like devices-hash) and not related to VM (like the other field in VmDynamic). so imo, this field should reside in VmInternalData instead Line 80: private String guestOsVersion; Line 81: private String guestOsDistribution; Line 82: private String guestOsCodename; Line 83: private ArchitectureType guestOsArch; https://gerrit.ovirt.org/#/c/40205/17/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/DestroyVmVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/DestroyVmVDSCommand.java: Line 91: private void changeStatus(DestroyVmVDSCommandParameters parameters, VM curVm) { Line 92: // do the state transition only if that VM is really running on SRC Line 93: if (getParameters().getVdsId().equals(curVm.getRunOnVds())) { Line 94: ResourceManager.getInstance().InternalSetVmStatus(curVm, Line 95: parameters.getGracefully() ? VMStatus.PoweringDown : VMStatus.Down, curVm.getStatusUpdatedTime()); not needed (the value is null and won't be used anyway) Line 96: } Line 97: } https://gerrit.ovirt.org/#/c/40205/17/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/MigrateVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/MigrateVDSCommand.java: Line 28: VM vm = getVmDao().get(getParameters().getVmId()); Line 29: Line 30: if (vdsReturnValue.getSucceeded()) { Line 31: ResourceManager.getInstance().AddAsyncRunningVm(getParameters().getVmId()); Line 32: ResourceManager.getInstance().InternalSetVmStatus(vm, VMStatus.MigratingFrom, vm.getStatusUpdatedTime()); not needed (the value is null and won't be used anyway) Line 33: vm.setMigratingToVds(getParameters().getDstVdsId()); Line 34: vmManager.update(vm.getDynamicData()); Line 35: getVDSReturnValue().setReturnValue(VMStatus.MigratingFrom); Line 36: } else { https://gerrit.ovirt.org/#/c/40205/17/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java: Line 316: * @param vm Line 317: */ Line 318: public void SetVmUnknown(VM vm) { Line 319: RemoveAsyncRunningVm(vm.getId()); Line 320: InternalSetVmStatus(vm, VMStatus.Unknown, vm.getStatusUpdatedTime()); not needed (the value is null and won't be used anyway) Line 321: // log VM transition to unknown status Line 322: AuditLogableBase logable = new AuditLogableBase(); Line 323: logable.setVmId(vm.getId()); Line 324: auditLogDirector.log(logable, AuditLogType.VM_SET_TO_UNKNOWN_STATUS); https://gerrit.ovirt.org/#/c/40205/17/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 69: Line 70: public class VdsManager { Line 71: private static Logger log = LoggerFactory.getLogger(VdsManager.class); Line 72: private static Map<Guid, String> recoveringJobIdMap = new ConcurrentHashMap<Guid, String>(); Line 73: private final ConcurrentMap<Guid, Double> vmStatusUpdated = new ConcurrentHashMap<Guid, Double>(); this is not a good place to store it. it should be placed in VmManager since it is a value per VM Line 74: private final Object lockObj = new Object(); Line 75: private final AtomicInteger mFailedToRunVmAttempts; Line 76: private final AtomicInteger mUnrespondedAttempts; Line 77: private final Guid vdsId; https://gerrit.ovirt.org/#/c/40205/17/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 616: * Line 617: * @param vmDynamic Line 618: */ Line 619: protected void addVmDynamicToList(VmDynamic vmDynamic) { Line 620: if (vdsManager.shouldUpdateVmStatus(vmDynamic)) { it looks like far more deeper in the stack from where it should be - let's say that we got an event with a latter time than another one we already processed, when the code is done here it means we'll process it and will just not save it to the DB. I think it should be much higher in the stack - maybe even not to call the perform method in this class if the fetched stats are the latest Line 621: vmDynamicToSave.put(vmDynamic.getId(), vmDynamic); Line 622: } Line 623: } Line 624: -- To view, visit https://gerrit.ovirt.org/40205 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib15df9ce78bbb86c9284bdd6b772d0e6801db765 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
