3:02 p.m.
Nir Soffer has uploaded a new change for review.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
sourceRouteThread: Encapsulate inotify thread
sourceRouteThread defined a main function for the inotify thread, but
the thread was created and started from supervdsmServer main function.
This couple these modules, expose module implementation details, and
make both sourceRouteThread and supervdsmServer harder to understand and
maintain.
This patch move inotify thread creation boilerplate from supervdsmServer
main function to a start function in sourceRouteThread module. The
inotify thread main function is private now, unlikely to be used from
other modules by mistake.
A nice side effect is shortening the too-big-and-growing try-block in
supervdsmServer main.
Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
---
M vdsm/sourceRouteThread.py
M vdsm/supervdsmServer
2 files changed, 15 insertions(+), 7 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/22723/1
diff --git a/vdsm/sourceRouteThread.py b/vdsm/sourceRouteThread.py
index c913220..b7ba568 100644
--- a/vdsm/sourceRouteThread.py
+++ b/vdsm/sourceRouteThread.py
@@ -1,5 +1,6 @@
import logging
import os
+import threading
import pyinotify
@@ -10,6 +11,7 @@
SOURCE_ROUTES_FOLDER = P_VDSM_RUN + 'sourceRoutes'
configurator = Iproute2()
+_thread = None
class DHClientEventHandler(pyinotify.ProcessEvent):
@@ -42,7 +44,16 @@
self.process_IN_CLOSE_WRITE_filePath(event.pathname)
-def subscribeToInotifyLoop():
+def start():
+ global _thread
+ assert _thread is None
+ _thread = threading.Thread(target=_subscribeToInotifyLoop,
+ name='sourceRoute')
+ _thread.daemon = True
+ _thread.start()
+
+
+def _subscribeToInotifyLoop():
logging.debug("sourceRouteThread.subscribeToInotifyLoop started")
# Subscribe to pyinotify event
diff --git a/vdsm/supervdsmServer b/vdsm/supervdsmServer
index 408dc4e..b84db27 100755
--- a/vdsm/supervdsmServer
+++ b/vdsm/supervdsmServer
@@ -79,8 +79,8 @@
"%s-%s" + _UDEV_RULE_FILE_EXT
RUN_AS_TIMEOUT = config.getint("irs", "process_pool_timeout")
-
_running = True
+
class Timeout(RuntimeError):
pass
@@ -366,6 +366,7 @@
global _running
_running = False
+
def main(sockfile, pidfile=None):
log = logging.getLogger("SuperVdsm.Server")
@@ -414,11 +415,7 @@
log.debug("Started serving super vdsm object")
- _sourceRouteThread = threading.Thread(
- target=sourceRouteThread.subscribeToInotifyLoop,
- name='sourceRoute')
- _sourceRouteThread.daemon = True
- _sourceRouteThread.start()
+ sourceRouteThread.start()
while _running:
signal.pause()
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
6:10 p.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
sourceRouteThread: Encapsulate inotify thread
sourceRouteThread defined a main function for the inotify thread, but
the thread was created and started from supervdsmServer main function.
This couple these modules, expose module implementation details, and
make both sourceRouteThread and supervdsmServer harder to understand and
maintain.
This patch move inotify thread creation boilerplate from supervdsmServer
main function to a start function in sourceRouteThread module. The
inotify thread main function is private now, unlikely to be used from
other modules by mistake.
A nice side effect is shortening the too-big-and-growing try-block in
supervdsmServer main.
Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
Reviewed-on: http://gerrit.ovirt.org/22723
Reviewed-by: Assaf Muller <amuller(a)redhat.com>
Reviewed-by: Yaniv Bronhaim <ybronhei(a)redhat.com>
Tested-by: Yaniv Bronhaim <ybronhei(a)redhat.com>
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com>
---
M vdsm/sourceRouteThread.py
M vdsm/supervdsmServer
2 files changed, 10 insertions(+), 6 deletions(-)
Approvals:
Yaniv Bronhaim: Verified; Looks good to me, but someone else must approve
Assaf Muller: Looks good to me, but someone else must approve
Dan Kenigsberg: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
10:57 a.m.
Yaniv Bronhaim has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 4: Code-Review+1
I prefer that one. abandon http://gerrit.ovirt.org/#/c/22718/ and just be sure that the
thread run properly
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
3:39 a.m.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6395/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5502/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1101/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6298/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
3:28 a.m.
Nir Soffer has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 4:
Since the module does not care any more about starting multiple threads, the global
_thread is not needed.
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
2:58 a.m.
Assaf Muller has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3:
(1 comment)
....................................................
File vdsm/sourceRouteThread.py
Line 10: from vdsm import utils
Line 11:
Line 12:
Line 13: SOURCE_ROUTES_FOLDER = P_VDSM_RUN + 'sourceRoutes'
Line 14: configurator = Iproute2()
You misunderstood - I meant removing line 15, which is "_thread = None", and
line 49, which is "global _thread". They are redundant as far as I can see.
Line 15: _thread = None
Line 16:
Line 17:
Line 18: class DHClientEventHandler(pyinotify.ProcessEvent):
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
2:56 a.m.
Nir Soffer has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3:
(1 comment)
....................................................
File vdsm/sourceRouteThread.py
Line 10: from vdsm import utils
Line 11:
Line 12:
Line 13: SOURCE_ROUTES_FOLDER = P_VDSM_RUN + 'sourceRoutes'
Line 14: configurator = Iproute2()
- This line: Separating public and private variables blocks improve readability. If
configurator is actually a private variable, we can rename it to _configurator and remove
this line.
- Line 49: Separating "paragraphs" of code make it more readable. Removing it is
not related to this patch anyway.
Line 15: _thread = None
Line 16:
Line 17:
Line 18: class DHClientEventHandler(pyinotify.ProcessEvent):
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
2:27 p.m.
Antoni Segura Puimedon has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
11 a.m.
Assaf Muller has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3:
(1 comment)
....................................................
File vdsm/sourceRouteThread.py
Line 10: from vdsm import utils
Line 11:
Line 12:
Line 13: SOURCE_ROUTES_FOLDER = P_VDSM_RUN + 'sourceRoutes'
Line 14: configurator = Iproute2()
What do you think about removing this line and line 49?
Line 15: _thread = None
Line 16:
Line 17:
Line 18: class DHClientEventHandler(pyinotify.ProcessEvent):
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
10:58 a.m.
Nir Soffer has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3:
Removed the assert when starting the thread per Assaf request, and for separating
refactoring and behavior changes. I think this is good thing but Assaf is maintaining this
area.
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
10:51 a.m.
Nir Soffer has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3:
Rebase on http://gerrit.ovirt.org/22718 to remove the unneeded dependency on
http://gerrit.ovirt.org/22719, which is not related to these network patches.
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
4:24 a.m.
Nir Soffer has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 1:
Seeing as http://gerrit.ovirt.org/#/c/22718/ is a subset of this
patch, could you please abandon that one?
Do you mean squash them together?
http://gerrit.ovirt.org/22718 fixed the thread so it actually run in a different thread.
This patch is a simple refactoring that does not change any behavior (except added assert
when you try to start two threads).
It is bad practice to mix fixes with refactoring - these patches must be separate.
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
2:21 a.m.
Assaf Muller has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 1:
(3 comments)
....................................................
File vdsm/sourceRouteThread.py
Line 17: class DHClientEventHandler(pyinotify.ProcessEvent):
Line 18: def process_IN_CLOSE_WRITE_filePath(self, sourceRouteFilePath):
Line 19: logging.debug("Responding to DHCP response in %s" %
Line 20: sourceRouteFilePath)
Line 21: with open(sourceRouteFilePath, 'r') as sourceRouteFile:
If we fail to parse a file we should log the contents and name of the file, delete the
file and move on.
Line 22: sourceRouteContents = sourceRouteFile.read().split()
Line 23: action = sourceRouteContents[0]
Line 24: device = sourceRouteContents[-1]
Line 25: sourceRoute = DynamicSourceRoute(device, configurator)
Line 23: action = sourceRouteContents[0]
Line 24: device = sourceRouteContents[-1]
Line 25: sourceRoute = DynamicSourceRoute(device, configurator)
Line 26:
Line 27: if sourceRoute.isVDSMInterface():
Same comment as the one above.
Line 28: if action == 'configure':
Line 29: ip = sourceRouteContents[1]
Line 30: mask = sourceRouteContents[2]
Line 31: gateway = sourceRouteContents[3]
Line 26:
Line 27: if sourceRoute.isVDSMInterface():
Line 28: if action == 'configure':
Line 29: ip = sourceRouteContents[1]
Line 30: mask = sourceRouteContents[2]
If configure or remove fail we need to log the error, and consider if we wish to move on
and continue responding to further dhcp requests, or kill this thread and supervdsm. (Just
killing this thread alone makes no sense). This is coming back to many philosophical
debates we've had in the past. I vote to continue serving further DHCP requests and
not killing anything (Keep in mind that the error was logged).
Line 31: gateway = sourceRouteContents[3]
Line 32: sourceRoute.configure(ip, mask, gateway)
Line 33: else:
Line 34: sourceRoute.remove()
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
2:07 a.m.
Assaf Muller has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 1: Code-Review-1
(2 comments)
....................................................
File vdsm/sourceRouteThread.py
Line 14: _thread = None
Line 15:
Line 16:
Line 17: class DHClientEventHandler(pyinotify.ProcessEvent):
Line 18: def process_IN_CLOSE_WRITE_filePath(self, sourceRouteFilePath):
The same patch that causes _subscribeToInotifyLoop to start in a separate thread *has* to
catch exceptions and log them.
Line 19: logging.debug("Responding to DHCP response in %s" %
Line 20: sourceRouteFilePath)
Line 21: with open(sourceRouteFilePath, 'r') as sourceRouteFile:
Line 22: sourceRouteContents = sourceRouteFile.read().split()
Line 42:
Line 43: def process_IN_CLOSE_WRITE(self, event):
Line 44: self.process_IN_CLOSE_WRITE_filePath(event.pathname)
Line 45:
Line 46:
I'd rather get rid of this ugly check... I don't see what it adds to the table.
Yes, it is correct, as there's no reason to ever start two of these threads (And it
would be quite disastrous if that were to happen), but it's unpythonic and doesn't
guard against something that I actually worry happening.
Line 47: def start():
Line 48: global _thread
Line 49: assert _thread is None
Line 50: _thread = threading.Thread(target=_subscribeToInotifyLoop,
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:10 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
3:34 a.m.
Assaf Muller has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
3:15 p.m.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6259/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1096/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5472/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6365/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
12:08 p.m.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3: -Code-Review
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6384/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5491/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1100/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6287/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
1:57 a.m.
Assaf Muller has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 1:
Seeing as http://gerrit.ovirt.org/#/c/22718/ is a subset of this patch, could you please
abandon that one?
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
10:16 a.m.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 3: Code-Review-1
Build Unstable
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6384/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6283/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5491/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1100/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
10:05 a.m.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 2: Code-Review-1
Build Unstable
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6382/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6280/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5489/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_network_functional_tests/1099/ : SUCCESS
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
11:36 a.m.
Yaniv Bronhaim has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
4:39 a.m.
Nir Soffer has posted comments on this change.
Change subject: sourceRouteThread: Encapsulate inotify thread
......................................................................
Patch Set 1:
(5 comments)
Asaf - the comments regarding DHClientEventHandler are relevant because we run now in a
different thread, or they are issue that existed when we were running in the main thread?
If these are new issues caused by running in a separate thread, they should be handled in
http://gerrit.ovirt.org/22718.
If these issues were there when we were running in the main thread, they can be fixed
later in another patch.
I don't see how these issues are related to this refactoring.
....................................................
File vdsm/sourceRouteThread.py
Line 14: _thread = None
Line 15:
Line 16:
Line 17: class DHClientEventHandler(pyinotify.ProcessEvent):
Line 18: def process_IN_CLOSE_WRITE_filePath(self, sourceRouteFilePath):
I'll rebase over Antony patch that fixed that.
Line 19: logging.debug("Responding to DHCP response in %s" %
Line 20: sourceRouteFilePath)
Line 21: with open(sourceRouteFilePath, 'r') as sourceRouteFile:
Line 22: sourceRouteContents = sourceRouteFile.read().split()
Line 17: class DHClientEventHandler(pyinotify.ProcessEvent):
Line 18: def process_IN_CLOSE_WRITE_filePath(self, sourceRouteFilePath):
Line 19: logging.debug("Responding to DHCP response in %s" %
Line 20: sourceRouteFilePath)
Line 21: with open(sourceRouteFilePath, 'r') as sourceRouteFile:
This change the behavior, not in this patch.
Line 22: sourceRouteContents = sourceRouteFile.read().split()
Line 23: action = sourceRouteContents[0]
Line 24: device = sourceRouteContents[-1]
Line 25: sourceRoute = DynamicSourceRoute(device, configurator)
Line 23: action = sourceRouteContents[0]
Line 24: device = sourceRouteContents[-1]
Line 25: sourceRoute = DynamicSourceRoute(device, configurator)
Line 26:
Line 27: if sourceRoute.isVDSMInterface():
Same - not in this patch
Line 28: if action == 'configure':
Line 29: ip = sourceRouteContents[1]
Line 30: mask = sourceRouteContents[2]
Line 31: gateway = sourceRouteContents[3]
Line 26:
Line 27: if sourceRoute.isVDSMInterface():
Line 28: if action == 'configure':
Line 29: ip = sourceRouteContents[1]
Line 30: mask = sourceRouteContents[2]
If this class need changes, it should be done in another patch.
Line 31: gateway = sourceRouteContents[3]
Line 32: sourceRoute.configure(ip, mask, gateway)
Line 33: else:
Line 34: sourceRoute.remove()
Line 42:
Line 43: def process_IN_CLOSE_WRITE(self, event):
Line 44: self.process_IN_CLOSE_WRITE_filePath(event.pathname)
Line 45:
Line 46:
Why is this unpythonic? This is common Python idiom (See libvirtconnection.py)
I can split it to another patch since this adds new behavior. Other then that, I don't
see any issue here.
Line 47: def start():
Line 48: global _thread
Line 49: assert _thread is None
Line 50: _thread = threading.Thread(target=_subscribeToInotifyLoop,
--
To view, visit http://gerrit.ovirt.org/22723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e1d402ac1c1cd4de4c3e74910244b47e7977f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
3806
days inactive
3816
days old
vdsm-patches@lists.fedorahosted.org
22 comments
6 participants
participants (6)
-
amuller＠redhat.com -
asegurap＠redhat.com -
Dan Kenigsberg -
Jenkins CI RO -
Nir Soffer -
ybronhei＠redhat.com