Milan Zamazal has posted comments on this change. Change subject: periodic: more cautious return to fast path ......................................................................
Patch Set 2: Code-Review-1 (6 comments) Looks like a good idea to me. -1 just due to the typos. https://gerrit.ovirt.org/#/c/48190/2//COMMIT_MSG Commit Message: Line 24: However, we go back in the fast path as soon as we can, when we Line 25: don't see any more timeouts (good) and when the blacklist is expired. Line 26: Line 27: And eventually here comes the trouble. When the blacklist expires, Line 28: we assume eveything is fine again, but the actual check is done by ... everything ... Line 29: the next fast path cycle. Line 30: This works, but can leak a thread. Line 31: Line 32: There are circumnstances on which we must take the risk of leaking Line 26: Line 27: And eventually here comes the trouble. When the blacklist expires, Line 28: we assume eveything is fine again, but the actual check is done by Line 29: the next fast path cycle. Line 30: This works, but can leak a thread. Maybe it's worth to define what kind of leak is experienced here. Is it a blocked libvirt call, until it finishes (i.e. unblocks or fails with an exception)? Line 31: Line 32: There are circumnstances on which we must take the risk of leaking Line 33: one thread, but in this case we can do another cautionary slow path Line 34: cycle sampling all domains to see if they seems actually right, and Line 28: we assume eveything is fine again, but the actual check is done by Line 29: the next fast path cycle. Line 30: This works, but can leak a thread. Line 31: Line 32: There are circumnstances on which we must take the risk of leaking ... circumstances ... Line 33: one thread, but in this case we can do another cautionary slow path Line 34: cycle sampling all domains to see if they seems actually right, and Line 35: only after that jump back in the fast path. Line 36: Line 33: one thread, but in this case we can do another cautionary slow path Line 34: cycle sampling all domains to see if they seems actually right, and Line 35: only after that jump back in the fast path. Line 36: Line 37: Please note that this is approach has still some amount of risk: ... this approach ... Line 38: maybe a domain gets stuck between the last succesfull slow cycle Line 39: and the new fast cycle. Line 40: Line 41: But we don't have any protection for this cases. Adding a cautionary Line 37: Please note that this is approach has still some amount of risk: Line 38: maybe a domain gets stuck between the last succesfull slow cycle Line 39: and the new fast cycle. Line 40: Line 41: But we don't have any protection for this cases. Adding a cautionary ... for these cases. ... Line 42: extra slow path cycles gives us all the protection we can possibly Line 43: get. Line 44: Line 45: The drawback of this patch is that the slow path is more consuming, https://gerrit.ovirt.org/#/c/48190/2/vdsm/virt/sampling.py File vdsm/virt/sampling.py: Line 493: Line 494: def __call__(self): Line 495: timestamp = self._stats_cache.clock() Line 496: # we need to update our state variable here, not after the call, Line 497: # because if the call get stuck, the update may be stale and harmful ... gets ... Line 498: on_fast_path = self._on_fast_path Line 499: # we are deep in the hot path. bool(ExpiringCache) Line 500: # *is* costly so we should avoid it if we can. Line 501: fast_path = ( -- To view, visit https://gerrit.ovirt.org/48190 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40dda0a308d6be6b2c8b8217a26ca5e7e9717546 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: 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
