Milan Zamazal has posted comments on this change. Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 5: (4 comments) I think this change is tricky enough to deserve at least some comment in the source code. If I understand it right then: - If everything works well (op < op_timeout), no problem. - If period > sampling.TIMEOUT then this change is no-op. What happens in such a case on the next operation calls? Just harmless exceptions, or unnecessarily repeated operation attempts when the period is short, or even problem escalation, or something else? Perhaps consistent with the declared "no false negatives" strategy, but it should be appropriately explained. - The only case handled by the change is if op > period & period < sampling.TIMEOUT. False negatives possible, but at most for sampling.TIMEOUT minus op. Again, maybe worth a little note? If I don't understand it right then explanation is especially needed! Moreover, adding comment why we still insist on trying different periodic operations in case another kind of operation blocks us would be useful (as it seems to be new behavior introduced by this change). https://gerrit.ovirt.org/#/c/47246/5//COMMIT_MSG Commit Message: Line 27: We need to try another way to solve this problem, without Line 28: depend on libvirt so much. Line 29: Line 30: Now VDSM counts explicitely the uses of a given domain, using Line 31: a ExpiringCache per operation, in order to minimize the an/one ExpiringCache ..., in ... Line 32: inter-operation interference. We want to avoid false negative Line 33: as top-priority, but once this is granted, we also want to minimize Line 34: the false positive! Line 35: Line 30: Now VDSM counts explicitely the uses of a given domain, using Line 31: a ExpiringCache per operation, in order to minimize the Line 32: inter-operation interference. We want to avoid false negative Line 33: as top-priority, but once this is granted, we also want to minimize Line 34: the false positive! I'd suggest defining false negative (VM available, considered unavailable?) and false positive (VM unavailable, considered available?) here to avoid confusion. Line 35: Line 36: The rational for this change is Line 37: - isDomainReadyForCommands, which we used previously, needs Line 38: o access libvirt, thus can actually make things worse on extreme Line 34: the false positive! Line 35: Line 36: The rational for this change is Line 37: - isDomainReadyForCommands, which we used previously, needs Line 38: o access libvirt, thus can actually make things worse on extreme to access ... Line 39: load conditions. Line 40: - under normal load, reduce the interference and false positives Line 41: between operations Line 42: https://gerrit.ovirt.org/#/c/47246/5/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 229: continue Line 230: Line 231: op = self._create(vm_obj, self._skip_doms) Line 232: Line 233: if not op.required: Shouldn't vm_id be added to `skipped' here? Line 234: continue Line 235: # When dealing with blocked domains, we also want to avoid Line 236: # to pile up jobs that libvirt can't handle and eventually Line 237: # clog it. -- To view, visit https://gerrit.ovirt.org/47246 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd90750ab36cb90bad401fab0ff7ff98429e78a5 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
