Nir Soffer has uploaded a new change for review.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
concurrent: Use new concurrent.thread() utility
This patch remove eliminate some boilerplate code by using the new concurrent.thread() utility for creating threads.
Nice side effect of this refactoring is adding the missing @utils.traceback() preventing silent failures of this thread.
Change-Id: Id98bcdf7bb6c67583d9d994781e642441c9b8bdd Signed-off-by: Nir Soffer nsoffer@redhat.com --- M lib/vdsm/libvirtconnection.py M lib/vdsm/schedule.py M lib/vdsm/xmlrpc.py M vdsm/clientIF.py M vdsm/rpc/bindingxmlrpc.py 5 files changed, 16 insertions(+), 20 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/95/44895/1
diff --git a/lib/vdsm/libvirtconnection.py b/lib/vdsm/libvirtconnection.py index f7a3926..15ccf33 100644 --- a/lib/vdsm/libvirtconnection.py +++ b/lib/vdsm/libvirtconnection.py @@ -27,6 +27,7 @@ import signal
import libvirt +from . import concurrent from . import utils from .password import ProtectedPassword from .tool.configurators import passwd @@ -41,9 +42,8 @@
def start(self): assert not self.run - self.__thread = threading.Thread(target=self.__run, - name="libvirtEventLoop") - self.__thread.setDaemon(True) + self.__thread = concurrent.thread(self.__run, name="libvirtEventLoop", + logger=log.name) self.run = True self.__thread.start()
@@ -54,7 +54,6 @@ self.__thread.join() self.__thread = None
- @utils.traceback(on=log.name) def __run(self): try: libvirt.virEventRegisterDefaultImpl() diff --git a/lib/vdsm/schedule.py b/lib/vdsm/schedule.py index 05386c7..8407704 100644 --- a/lib/vdsm/schedule.py +++ b/lib/vdsm/schedule.py @@ -63,6 +63,7 @@ import threading import time
+from . import concurrent from . import utils
@@ -91,8 +92,8 @@ self._cond = threading.Condition(threading.Lock()) self._running = False self._calls = [] - self._thread = threading.Thread(target=self._run, name=self._name) - self._thread.daemon = True + self._thread = concurrent.thread(self._run, name=self._name, + logger=self._log.name)
def start(self): self._log.debug("Starting scheduler %s", self._name) @@ -137,7 +138,6 @@ self._cond.notify() return call
- @utils.traceback(on=_log.name) def _run(self): self._log.debug("started") try: diff --git a/lib/vdsm/xmlrpc.py b/lib/vdsm/xmlrpc.py index 66d9415..4327790 100644 --- a/lib/vdsm/xmlrpc.py +++ b/lib/vdsm/xmlrpc.py @@ -26,6 +26,7 @@ import sys import threading
+from . import concurrent from .config import config from .executor import TaskQueue from .utils import traceback @@ -71,15 +72,14 @@ if sock is self._STOP: return self.log.info("Starting request handler for %s:%d", addr[0], addr[1]) - t = threading.Thread(target=self._process_requests, args=(sock, addr)) - t.daemon = True + t = concurrent.thread(self._process_requests, args=(sock, addr), + logger=self.log.name) t.start()
def server_close(self): self.queue.clear() self.queue.put((self._STOP, self._STOP))
- @traceback(on=log.name) def _process_requests(self, sock, addr): self.log.info("Request handler for %s:%d started", addr[0], addr[1]) try: diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py index 7b9c490..3834bcb 100644 --- a/vdsm/clientIF.py +++ b/vdsm/clientIF.py @@ -39,6 +39,7 @@ import libvirt from vdsm import sslutils from vdsm import libvirtconnection +from vdsm import concurrent from vdsm import constants from vdsm import utils import caps @@ -108,8 +109,7 @@ self._netConfigDirty = False self._prepareMOM() secret.clear() - threading.Thread(target=self._recoverThread, - name='clientIFinit').start() + concurrent.thread(self._recoverThread, name='clientIFinit').start() self.channelListener.settimeout( config.getint('vars', 'guest_agent_timeout')) self.channelListener.start() @@ -290,9 +290,8 @@ def start(self): for binding in self.bindings.values(): binding.start() - self.thread = threading.Thread(target=self._reactor.process_requests, - name='Reactor thread') - self.thread.setDaemon(True) + self.thread = concurrent.thread(self._reactor.process_requests, + name='Reactor thread') self.thread.start()
def _getUUIDSpecPath(self, uuid): @@ -447,7 +446,6 @@ else: raise JsonRpcBindingsError()
- @utils.traceback() def _recoverThread(self): # Trying to run recover process until it works. During that time vdsm # stays in recovery mode (_recover=True), means all api requests diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py index 04cdf32..a85b7c4 100644 --- a/vdsm/rpc/bindingxmlrpc.py +++ b/vdsm/rpc/bindingxmlrpc.py @@ -30,6 +30,7 @@ from vdsm.password import (ProtectedPassword, protect_passwords, unprotect_passwords) +from vdsm import concurrent from vdsm import utils from vdsm import xmlrpc from vdsm.define import doneCode, errCode @@ -57,7 +58,6 @@ """ Register xml-rpc functions and serve clients until stopped """ - @utils.traceback(on=self.log.name) def threaded_start(): self.log.info("XMLRPC server running") self._registerFunctions() @@ -73,9 +73,8 @@ exc_info=True) self.log.info("XMLRPC server stopped")
- self._thread = threading.Thread(target=threaded_start, - name='BindingXMLRPC') - self._thread.daemon = True + self._thread = concurrent.thread(threaded_start, name='BindingXMLRPC', + logger=self.log.name) self._thread.start()
def add_socket(self, connected_socket, socket_address):
automation@ovirt.org has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
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'])
Francesco Romani has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 2: Code-Review+1
yep, code looks nicer.
Piotr Kliczewski has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 2: Code-Review+1
automation@ovirt.org has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
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'])
Nir Soffer has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 3:
I wonder if we should break this patch by sybsystem/group - lib, storage, virt, networking.
Ido Barkan has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 3: Code-Review+1
Nir Soffer has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 3: Verified+1
Did sanity test
- create and provision vm - create vm from template - create template from vm - create disks - hotplug/hotunplug disk - migrate vms - lsm between mixed domain types - remove template - remove vms
Everything seems to work normally.
Shahar Havivi has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 3: Code-Review-1 Verified-1
you need to verify v2v as well.
Francesco Romani has posted comments on this change.
Change subject: concurrent: Use new concurrent.thread() utility ......................................................................
Patch Set 3:
will fix migration.py and sampling.py once this patch is in.
automation@ovirt.org has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
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'])
Nir Soffer has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 4:
This version split this huge change by subsystem, to make it easier to review and verify.
Each behavior change is documented now per subsystem.
Francesco Romani has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 4: Code-Review+1
executor, libvirtconnection, profiling/memory and schedule: looks good to me.
Piotr Kliczewski has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 4: Code-Review+1
Nir Soffer has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 4:
Who wants to approve this?
automation@ovirt.org has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 5:
* 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'])
Ido Barkan has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 5: Code-Review+1
reapproving after rebases
gerrit-hooks has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 6:
* 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.6', 'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 6: Verified+1
Dan Kenigsberg has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 6: Code-Review+2
Dan Kenigsberg has submitted this change and it was merged.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
lib: Use new concurrent.thread() utility
This series eliminates some boilerplate code by using the new concurrent.thread() utility for creating threads.
Mostly this does change the behavior of the existing code, except:
- Thread that did not use @utils.traceback to prevent silent failures are protected now automatically.
- Non-daemon threads are now daemon threads that will never block the application during shutdown. I believe this behavior is always what we want, so I did not separated this change to another patch.
The series includes various patches by subsystem: lib, storage, networking, virt, and infra.
This patch updates the vdsm library to use the new utility.
Behavior changes:
- concurrent.tmap threads are protected from silent failures - netlink/monitor.Monitor threads are protected from silent failures - profiling/memory viewer thread is protected from silent failures
Change-Id: Id98bcdf7bb6c67583d9d994781e642441c9b8bdd Relates-To: https://bugzilla.redhat.com/1141422 Signed-off-by: Nir Soffer nsoffer@redhat.com Reviewed-on: https://gerrit.ovirt.org/44895 Reviewed-by: Francesco Romani fromani@redhat.com Reviewed-by: Piotr Kliczewski piotr.kliczewski@gmail.com Reviewed-by: Ido Barkan ibarkan@redhat.com Continuous-Integration: Jenkins CI Reviewed-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/concurrent.py M lib/vdsm/executor.py M lib/vdsm/libvirtconnection.py M lib/vdsm/netlink/monitor.py M lib/vdsm/profiling/memory.py M lib/vdsm/schedule.py M lib/vdsm/xmlrpc.py 7 files changed, 17 insertions(+), 24 deletions(-)
Approvals: Piotr Kliczewski: Looks good to me, but someone else must approve Nir Soffer: Verified Ido Barkan: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Francesco Romani: Looks good to me, but someone else must approve
gerrit-hooks has posted comments on this change.
Change subject: lib: Use new concurrent.thread() utility ......................................................................
Patch Set 7:
* Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
vdsm-patches@lists.fedorahosted.org