Change in vdsm[master]: periodic: use name for the work sent to executor

Francesco Romani has uploaded a new change for review.

Change subject: periodic: use name for the work sent to executor
......................................................................

periodic: use name for the work sent to executor

WIP
to improve debuggability

Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79
Signed-off-by: Francesco Romani 
---
M tests/periodicTests.py
M vdsm/virt/periodic.py
2 files changed, 6 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/48193/1

diff --git a/tests/periodicTests.py b/tests/periodicTests.py
index 5320223..1fee0b3 100644
--- a/tests/periodicTests.py
+++ b/tests/periodicTests.py
@@ -266,7 +266,7 @@
     def __init__(self, fail=False):
         self._fail = fail

-    def dispatch(self, func, timeout):
+    def dispatch(self, func, timeout, name):
         if self._fail:
             raise executor.TooManyTasks()
         else:
diff --git a/vdsm/virt/periodic.py b/vdsm/virt/periodic.py
index 8e4012a..ead51d8 100644
--- a/vdsm/virt/periodic.py
+++ b/vdsm/virt/periodic.py
@@ -167,6 +167,7 @@
         self._lock = threading.Lock()
         self._running = False
         self._call = None
+        self._name = str(self._func)

def start(self):
         with self._lock:
@@ -213,7 +214,7 @@
         Send `func' to Executor to be run as soon as possible.
         """
         self._call = None
-        self._executor.dispatch(self, self._timeout)
+        self._executor.dispatch(self, self._timeout, self._name)
         self._step()

@@ -250,6 +251,7 @@
         skipped = []

for vm_id, vm_obj in vms.iteritems():
+            name = '%s on %s' % (self._create, vm_id)
             try:
                 if self._busy_doms.get(vm_id, False):
                     skipped.append(vm_id)
@@ -270,11 +272,10 @@

except Exception:
                 # we want to make sure to have VM UUID logged
-                self._log.exception("while dispatching %s to VM '%s'",
-                                    self._create, vm_id)
+                self._log.exception("while dispatching %s", name)
             else:
                 try:
-                    self._executor.dispatch(op, self._timeout)
+                    self._executor.dispatch(op, self._timeout, name)
                 except executor.TooManyTasks:
                     skipped.append(vm_id)

[e-mail] has posted comments on this change.

Change subject: periodic: use name for the work sent to executor
......................................................................

Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: periodic: use name for the work sent to executor
......................................................................

Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: periodic: use name for the work sent to executor
......................................................................

Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: periodic: use name for the work sent to executor
......................................................................

Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: periodic: use name for the work sent to executor
......................................................................

Patch Set 5:

* #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: use name for the work sent to executor
......................................................................

Patch Set 5: Verified+1

verified together: 48191, 48193, 48192, 48333
verified with 48332 using steps provided in https://bugzilla.redhat.com/show_bug.cgi?id=1250839#c7

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: periodic: use name for the work sent to executor
......................................................................

Patch Set 5: Code-Review+1

Change in vdsm[master]: periodic: add executor-compatible naming

[e-mail] has posted comments on this change.

Change subject: periodic: add executor-compatible naming
......................................................................

Patch Set 6:

* #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'])

Change in vdsm[master]: periodic: add executor-compatible naming

Francesco Romani has posted comments on this change.

Change subject: periodic: add executor-compatible naming
......................................................................

Patch Set 6: Verified+1

verified - actually on branch 3.6, while checking https://gerrit.ovirt.org/#/c/48348/

logs as expected.

Change in vdsm[master]: periodic: add executor-compatible naming

Milan Zamazal has posted comments on this change.

Change subject: periodic: add executor-compatible naming
......................................................................

Patch Set 6:

(2 comments)

https://gerrit.ovirt.org/#/c/48193/6//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: We want to make as easy as possible to debug
Line 10: thread leaks from executor, so we make the periodic
Line 11: framework support names for Operations, using the
Line 12: way Executor expects them.
What's the executor-compatible naming? Is NAME utilized anywhere outside periodic.py, perhaps in another patch (I can't see it in the current source code)?
Line 13: 
Line 14: Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79
Line 15: Bug-Url: https://bugzilla.redhat.com/1250839
Line 16: Backport-To: 3.6

https://gerrit.ovirt.org/#/c/48193/6/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 262:         return '<VmDispatcher operation=%s at 0x%x>' % (
Line 263:             self._create, id(self)
Line 264:         )
Line 265: 
Line 266:     __repr__ = __str__
Why renaming __repr__ to __str__ and then defining this alias here? Wasn't having just __repr__ method enough?
Line 267: 
Line 268: 
Line 269: class _RunnableOnVm(object):
Line 270:

Change in vdsm[master]: periodic: add executor-compatible naming

Francesco Romani has posted comments on this change.

Change subject: periodic: add executor-compatible naming
......................................................................

Patch Set 6:

(2 comments)

https://gerrit.ovirt.org/#/c/48193/6//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: We want to make as easy as possible to debug
Line 10: thread leaks from executor, so we make the periodic
Line 11: framework support names for Operations, using the
Line 12: way Executor expects them.
> What's the executor-compatible naming? Is NAME utilized anywhere outside pe

It is a relic of versions past. Now we just use the standard python tools (__str__ magic method), so I can rephrase this commit message.
Line 13: 
Line 14: Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79
Line 15: Bug-Url: https://bugzilla.redhat.com/1250839
Line 16: Backport-To: 3.6

https://gerrit.ovirt.org/#/c/48193/6/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 262:         return '<VmDispatcher operation=%s at 0x%x>' % (
Line 263:             self._create, id(self)
Line 264:         )
Line 265: 
Line 266:     __repr__ = __str__
> Why renaming __repr__ to __str__ and then defining this alias here? Wasn't

Back in time I added __repr__ to have something nicer output in the logs, but without much thought. executor now uses str(), hence the newly added __str__.

I decided to keep __repr__ here, hence the alias.
Line 267: 
Line 268: 
Line 269: class _RunnableOnVm(object):
Line 270:

Change in vdsm[master]: periodic: add executor-compatible naming

Milan Zamazal has posted comments on this change.

Change subject: periodic: add executor-compatible naming
......................................................................

Patch Set 6:

(2 comments)

https://gerrit.ovirt.org/#/c/48193/6/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 262:         return '<VmDispatcher operation=%s at 0x%x>' % (
Line 263:             self._create, id(self)
Line 264:         )
Line 265: 
Line 266:     __repr__ = __str__
> Back in time I added __repr__ to have something nicer output in the logs, b

Just thought about retaining __repr__ here (as `str' falls back to it), but if you think using __str__ here (and in other classes here) is more appropriate then it's fine for me.
Line 267: 
Line 268: 
Line 269: class _RunnableOnVm(object):
Line 270:

Line 292: 
Line 293:     def __str__(self):
Line 294:         return '<%s vm=%s at 0x%x>' % (
Line 295:             self.NAME, self._vm.id, id(self)
Line 296:         )
Why not simply using self.__class__.__name__ here and introducing NAME?
Line 297: 
Line 298: 
Line 299: class UpdateVolumes(_RunnableOnVm):
Line 300:

Change in vdsm[master]: periodic: add executor-compatible naming

Francesco Romani has posted comments on this change.

Change subject: periodic: add executor-compatible naming
......................................................................

Patch Set 6:

(2 comments)

https://gerrit.ovirt.org/#/c/48193/6/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 262:         return '<VmDispatcher operation=%s at 0x%x>' % (
Line 263:             self._create, id(self)
Line 264:         )
Line 265: 
Line 266:     __repr__ = __str__
> Just thought about retaining __repr__ here (as `str' falls back to it), but

You are right, as the doc -why I should have refreshed earlier- reminds us:
https://docs.python.org/2/reference/datamodel.html#object.__repr__

However, here I abused __repr__ in the old code, because this representation definitely leans toward "informal", and not even attempts to be a valid python expression. So it always was more akin to __str__ than __repr__ in the intentions.

For this reason I'll keep the rename and drop the alias, and maybe readd back __repr__ in a later patch, with better context and rationale.
Line 267: 
Line 268: 
Line 269: class _RunnableOnVm(object):
Line 270:

Line 292: 
Line 293:     def __str__(self):
Line 294:         return '<%s vm=%s at 0x%x>' % (
Line 295:             self.NAME, self._vm.id, id(self)
Line 296:         )
> Why not simply using self.__class__.__name__ here and introducing NAME?

no good reason. Changing - thanks!
Line 297: 
Line 298: 
Line 299: class UpdateVolumes(_RunnableOnVm):
Line 300:

Change in vdsm[master]: periodic: add __str__ methods

gerrit-hooks has posted comments on this change.

Change subject: periodic: add __str__ methods
......................................................................

Patch Set 7:

* #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.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: periodic: add __str__ methods

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: periodic: add __str__ methods
......................................................................

Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/48193/7/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 314: 
Line 315: 
Line 316: class NumaInfoMonitor(_RunnableOnVm):
Line 317: 
Line 318:     NAME = "NumaInfoMonitor"
why are these any better than self.__class__.__name__ ?
Line 319: 
Line 320:     @property
Line 321:     def required(self):
Line 322:         return self._vm.hasGuestNumaNode

Change in vdsm[master]: periodic: add __str__ methods

Francesco Romani has posted comments on this change.

Change subject: periodic: add __str__ methods
......................................................................

Patch Set 7: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/48193/7/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 314: 
Line 315: 
Line 316: class NumaInfoMonitor(_RunnableOnVm):
Line 317: 
Line 318:     NAME = "NumaInfoMonitor"
> why are these any better than self.__class__.__name__ ?

They are not at all, Milan already pointed out and I just missed to update the patch.
Line 319: 
Line 320:     @property
Line 321:     def required(self):
Line 322:         return self._vm.hasGuestNumaNode

Change in vdsm[master]: periodic: add __str__ methods

gerrit-hooks has posted comments on this change.

Change subject: periodic: add __str__ methods
......................................................................

Patch Set 8:

* #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.6 ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: periodic: add __str__ methods

Milan Zamazal has posted comments on this change.

Change subject: periodic: add __str__ methods
......................................................................

Patch Set 8: Code-Review+1