Piotr Kliczewski 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 In my opinion we should keep statusUpdate and status in the same place. It is more natural way to hold related data together. 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) We need to have it there to update vmdynamic with the latest data. 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) We need to have it there to update vmdynamic with the latest data. 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) We need to have it there to update vmdynamic with the latest data. 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 sinc We could move it to VmManager later after feature freeze. 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)) { > when the code is done here -> when the check is done here I think that it is not good idea. We just want to be sure that we do not update stale data. Everything else should be processed. 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
