Francesco Romani has uploaded a new change for review.
Change subject: periodic: explicitely track domain availability ......................................................................
periodic: explicitely track domain availability
The periodic sampling code knows that libvirt can become clogged (most common case: unresponsive storage) and tries to not make things worse, avoiding to schedule work on unresponding domain.
To do so, the code needs a way to know or at least guesstimate the libvirt responsiveness.
The initial implementation relied on virDomainGetControlInfo, which reports if there is any monitor job currently running, or not.
This approach has the plus to have one single source of information, which is also the most reliable - libvirt. But it also creates a a dangerous short circuit, because the controller relies on the controlled itself to assess its health.
Furthermore, we experienced firsthand a bad failure of the aforementioned control code under high scale, and this was the final blow to the initial approach. We need to try another way to solve this problem, without depend on libvirt so much.
Now VDSM counts explicitely the uses of a given domain, using a ExpiringCache per operation, in order to minimize the inter-operation interference. We want to avoid false negative as top-priority, but once this is granted, we also want to minimize the false positive!
The rational for this change is - isDomainReadyForCommands, which we used previously, needs o access libvirt, thus can actually make things worse on extreme load conditions. - under normal load, reduce the interference and false positives between operations
Change-Id: Idd90750ab36cb90bad401fab0ff7ff98429e78a5 Bug-Url: https://bugzilla.redhat.com/1250839 Signed-off-by: Francesco Romani fromani@redhat.com --- M tests/periodicTests.py M vdsm/virt/periodic.py M vdsm/virt/sampling.py 3 files changed, 23 insertions(+), 31 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/46/47246/1
diff --git a/tests/periodicTests.py b/tests/periodicTests.py index ce4d592..1d730b7 100644 --- a/tests/periodicTests.py +++ b/tests/periodicTests.py @@ -241,8 +241,9 @@
VMS = defaultdict(int)
- def __init__(self, vm): + def __init__(self, vm, skip_doms): self._vm = vm + self._skip_doms = skip_doms
@property def required(self): @@ -250,26 +251,19 @@ raise ValueError('required failed') return True
- @property - def runnable(self): - if getattr(self._vm, 'fail_runnable', False): - raise ValueError('runnable failed') - return True - def __call__(self): - _Visitor.VMS[self._vm.id] += 1 + vm_id = self._vm.id + self._skip_doms[vm_id] = True + _Visitor.VMS[vm_id] += 1 + del self._skip_doms[vm_id]
class _Nop(object): - def __init__(self, _): + def __init__(self, *args): pass
@property def required(self): - return True - - @property - def runnable(self): return True
def __call__(self): diff --git a/vdsm/virt/periodic.py b/vdsm/virt/periodic.py index 13d7632..9c05f30 100644 --- a/vdsm/virt/periodic.py +++ b/vdsm/virt/periodic.py @@ -31,6 +31,7 @@
from . import sampling from . import virdomain +from .utils import ExpiringCache
# just a made up number. Maybe should be equal to number of cores? @@ -215,6 +216,7 @@ self._executor = executor self._create = create self._timeout = timeout + self._skip_doms = ExpiringCache(ttl=sampling.TIMEOUT)
def __call__(self): vms = self._get_vms() @@ -222,7 +224,11 @@
for vm_id, vm_obj in vms.iteritems(): try: - op = self._create(vm_obj) + to_skip = self._skip_doms.get(vm_id, False) + if to_skip: + continue + + op = self._create(vm_obj, self._skip_doms)
if not op.required: continue @@ -233,9 +239,6 @@ # still OK if occasional misdetection occours, but we # definitely want to avoid known-bad situation and to # needlessly overload libvirt. - if not op.runnable: - skipped.append(vm_id) - continue
except Exception: # we want to make sure to have VM UUID logged @@ -257,14 +260,13 @@
class _RunnableOnVm(object): - def __init__(self, vm): + def __init__(self, vm, skip_doms): self._vm = vm - - @property - def runnable(self): - return self._vm.isDomainReadyForCommands() + self._skip_doms = skip_doms
def __call__(self): + vm_id = self._vm.id # keep reference for safety + self._skip_doms[vm_id] = True try: self._execute() except virdomain.NotConnectedError: @@ -272,7 +274,9 @@ # race on shutdown: next cycle won't pick up this VM. # both cases: let's reduce the log spam. self._vm.log.warning('could not run on %s: domain not connected', - self._vm.id) + vm_id) + finally: + del self._skip_doms[vm_id]
def _execute(self): raise NotImplementedError @@ -298,12 +302,6 @@ @property def required(self): return self._vm.hasGuestNumaNode - - @property - def runnable(self): - # NUMA operations don't require QEMU monitor access - # (inspected libvirt sources v1.2.17) - return True
def _execute(self): self._vm.updateNumaInfo() diff --git a/vdsm/virt/sampling.py b/vdsm/virt/sampling.py index 1ad9d46..6f8ecce 100644 --- a/vdsm/virt/sampling.py +++ b/vdsm/virt/sampling.py @@ -501,12 +501,12 @@ # a multiple of known timeout and period: # - vm sampling period is 15s (we control that) # - libvirt (default) qemu monitor timeout is 30s (we DON'T contol this) -_TIMEOUT = 40.0 +TIMEOUT = 40.0
class VMBulkSampler(object): def __init__(self, conn, get_vms, stats_cache, - stats_flags=0, timeout=_TIMEOUT): + stats_flags=0, timeout=TIMEOUT): self._conn = conn self._get_vms = get_vms self._stats_cache = stats_cache
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 1:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 2:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 3:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 4:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 5:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 6:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 7:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
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.
Francesco Romani has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 5:
(4 comments)
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 ...
Done 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?)
Good point, will do. 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 ...
Done 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?
No, because "required" means: "should this operation be done at all on this vm?" if an operation is "required", then we must check if an operation is runnable, so if the current state of the VM allows so.
"required" it is supposed to change much less frequently than "runnable", and it is not affected by libvirt congestion.
"skipped", lastly, means "this operation is required, but it is not runnable on this vm now, so let's track it". Hence, it is correct NOT to add a not required operation. 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.
Francesco Romani has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 5:
- If period > sampling.TIMEOUT then this change is no-op.
What happens in such a case on the next operation calls?
It is implicitely enforced by the choice of default timeouts. I had a patch to make the choice of timeout more robust, but got lost, I need to restore it and add it in queue for 3.6.1.
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.
If timeout is too short (less than one period), there is no clogging protection, so all the good intentions go down the sink :\
- 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?
Will add
If I don't understand it right then explanation is especially needed!
I agree with this sentiment, not sure how to convey all the context needed to properly understand this change.
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).
Correct, in the sense that we have interference between operations. The periodic operations are not independent from each other, really. They seems so in VDSM, but we have one shared resource (domain) on which each one insists.
So it is always possible that one operation on one domain stalls the others acting on the same domain (this was true even before when we used one thread per VM). The problem here is that operations should not stall unrelated domains.
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 8:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 9:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Milan Zamazal has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 9:
(3 comments)
https://gerrit.ovirt.org/#/c/47246/9/vdsm/virt/periodic.py File vdsm/virt/periodic.py:
Line 211: """ Line 212: Line 213: _log = logging.getLogger("virt.periodic.VmDispatcher") Line 214: Line 215: def __init__(self, get_vms, executor, create, timeout, ttl): Please document ttl argument. Line 216: """ Line 217: get_vms: callable which will return a dict which maps Line 218: vm_ids to vm_instances Line 219: executor: executor.Executor instance
Line 217: get_vms: callable which will return a dict which maps Line 218: vm_ids to vm_instances Line 219: executor: executor.Executor instance Line 220: create: callable to obtain the real callable to Line 221: dispatch, with its timeout I wouldn't mind if `timeout' was properly documented as part of this change as well. (It's not clear from the docstring itself what `timeout' refers to here.) Line 222: """ Line 223: self._get_vms = get_vms Line 224: self._executor = executor Line 225: self._create = create
Line 232: Line 233: for vm_id, vm_obj in vms.iteritems(): Line 234: try: Line 235: to_skip = self._skip_doms.get(vm_id, False) Line 236: if to_skip: Sorry about my previously misplaced note regarding `skipped'. What I actually meant was: Originally, if we skipped on `not op.runnable' and op was required, vm_id was added to `skipped'. It's not so now under similar circumstances. Is it intentional? Line 237: continue Line 238: Line 239: op = self._create(vm_obj, self._skip_doms) Line 240:
Francesco Romani has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 9:
(3 comments)
https://gerrit.ovirt.org/#/c/47246/9/vdsm/virt/periodic.py File vdsm/virt/periodic.py:
Line 211: """ Line 212: Line 213: _log = logging.getLogger("virt.periodic.VmDispatcher") Line 214: Line 215: def __init__(self, get_vms, executor, create, timeout, ttl):
Please document ttl argument.
Done Line 216: """ Line 217: get_vms: callable which will return a dict which maps Line 218: vm_ids to vm_instances Line 219: executor: executor.Executor instance
Line 217: get_vms: callable which will return a dict which maps Line 218: vm_ids to vm_instances Line 219: executor: executor.Executor instance Line 220: create: callable to obtain the real callable to Line 221: dispatch, with its timeout
I wouldn't mind if `timeout' was properly documented as part of this change
Done (in trivial separate patch). Line 222: """ Line 223: self._get_vms = get_vms Line 224: self._executor = executor Line 225: self._create = create
Line 232: Line 233: for vm_id, vm_obj in vms.iteritems(): Line 234: try: Line 235: to_skip = self._skip_doms.get(vm_id, False) Line 236: if to_skip:
Sorry about my previously misplaced note regarding `skipped'. What I actual
Good catch. The "skipped" list is used for debugging, and could be very useful to have. Let me reintroduce the tracking, adapted to the new code environment. Line 237: continue Line 238: Line 239: op = self._create(vm_obj, self._skip_doms) Line 240:
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 10:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 11:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 12:
* Update tracker::#1250839::OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Milan Zamazal has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 12: Code-Review+1
automation@ovirt.org has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 13:
* #1250839::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1250839::OK, public bug * Check Product::#1250839::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Francesco Romani has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 13: Verified+1
verified running patched vdsm and SIGSTOP'ing QEMU. More verification will follow, but this patch seems good enough for master already. Port to stable branch will require additional verification as well, as usual.
Dan Kenigsberg has posted comments on this change.
Change subject: periodic: explicitely track domain availability ......................................................................
Patch Set 13: Code-Review-1
(4 comments)
https://gerrit.ovirt.org/#/c/47246/13//COMMIT_MSG Commit Message:
Line 25: high scale this is a bit vague. was it the consumption of libvirt worker thread by virDomainGetControlInfo?
https://gerrit.ovirt.org/#/c/47246/13/vdsm/virt/periodic.py File vdsm/virt/periodic.py:
Line 229: self._get_vms = get_vms Line 230: self._executor = executor Line 231: self._create = create Line 232: self._timeout = timeout Line 233: self._busy_doms = ExpiringCache(ttl) I'm a bit confused - why do we need this one *on top* of the existing VMBulkSampler._skip_doms ?
(I'm sure this only proves that I don't understand your suggested implementation) Line 234: Line 235: def __call__(self): Line 236: vms = self._get_vms() Line 237: skipped = []
Line 241: False why bother changing the default=None ?
Line 245: op = self._create(vm_obj, self._busy_doms) Line 246: Line 247: if not op.required: Line 248: continue Line 249: # When dealing with blocked domains, we also want to avoid this comment should be kept with its code, and probably rephrased. Line 250: # to pile up jobs that libvirt can't handle and eventually Line 251: # clog it. Line 252: # We don't care too much about precise tracking, so it is Line 253: # still OK if occasional misdetection occours, but we
vdsm-patches@lists.fedorahosted.org