| Message ID | 1467808691-17280-13-git-send-email-pboca@cloudbasesolutions.com |
|---|---|
| State | Superseded |
| Delegated to: | Guru Shetty |
| Headers | show |
I am wondering if it is better if we could just import the shared library and call the detach function (https://github.com/openvswitch/ovs/blob/master/lib/daemon-windows.c#L342) instead of duplicating the effort. I wonder if the same could be applied un the *unix side as well. Thanks, Alin. > -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Paul Boca > Trimis: Wednesday, July 6, 2016 3:38 PM > Către: dev@openvswitch.org > Subiect: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon to > Windows > > Used subprocess.Popen instead os.fork (not implemented on windows) and > repaced of os.pipe with Windows pipes. > > To be able to identify the child process I added an extra parameter to > daemon process '--pipe-handle', this parameter also contains the parent > Windows pipe handle, used by the child to signal the start. > > The PID file is created directly on Windows, without using a temporary file > because the symbolic link doesn't inheriths the file lock set on temporary file. > > Signed-off-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com> > --- > V2: Fix lockf on Linux, small error on os.link and missing pipe_handle > parameter. > V3: Import modules at the start of the code > V4: Close file before trying to delete it in signal hooks. > On Windows the PID file cannot be deleted while it's handle > is opened for write. > V5: No changes > V6: Explicitly close the vlog file in detached daemon. On Windows, even if the > daemon is detached, the primary daemon is still holding a handle to the log > file, therefore the log cannot be moved/deleted even is the vlog/close > command > is sent to the detached daemon. > ---
Hi Alin, It was easier for me to call Windows APIs from python, that mimics detach_process, and to be in a better control on what's happening, than creating a *DLL and loading it in order to call this function. *On Windows the LIB file cannot be loaded as-is; a DLL with entry point needs to be created, allowing the ntdll loader to resolve the imported functions. I don't say it isn't doable, but this seemed to be the right solution for this. Also, creating a DLL loaded by python would add a dependency between python and C components. Regarding the *unix side, maybe a Linux guy can add his opinion about this. Thanks, Paul > -----Original Message----- > From: Alin Serdean > Sent: Tuesday, July 12, 2016 10:54 PM > To: Paul Boca; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon > to Windows > > I am wondering if it is better if we could just import the shared library and > call the detach function > (https://github.com/openvswitch/ovs/blob/master/lib/daemon- > windows.c#L342) instead of duplicating the effort. > > I wonder if the same could be applied un the *unix side as well. > > Thanks, > Alin. > > > -----Mesaj original----- > > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Paul Boca > > Trimis: Wednesday, July 6, 2016 3:38 PM > > Către: dev@openvswitch.org > > Subiect: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon > to > > Windows > > > > Used subprocess.Popen instead os.fork (not implemented on windows) > and > > repaced of os.pipe with Windows pipes. > > > > To be able to identify the child process I added an extra parameter to > > daemon process '--pipe-handle', this parameter also contains the parent > > Windows pipe handle, used by the child to signal the start. > > > > The PID file is created directly on Windows, without using a temporary file > > because the symbolic link doesn't inheriths the file lock set on temporary > file. > > > > Signed-off-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com> > > --- > > V2: Fix lockf on Linux, small error on os.link and missing pipe_handle > > parameter. > > V3: Import modules at the start of the code > > V4: Close file before trying to delete it in signal hooks. > > On Windows the PID file cannot be deleted while it's handle > > is opened for write. > > V5: No changes > > V6: Explicitly close the vlog file in detached daemon. On Windows, even if > the > > daemon is detached, the primary daemon is still holding a handle to the > log > > file, therefore the log cannot be moved/deleted even is the vlog/close > > command > > is sent to the detached daemon. > > ---
> -----Mesaj original----- > De la: Paul Boca > Trimis: Wednesday, July 13, 2016 11:24 AM > Către: Alin Serdean <aserdean@cloudbasesolutions.com>; > dev@openvswitch.org > Subiect: RE: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python > daemon to Windows > > Hi Alin, > > It was easier for me to call Windows APIs from python, that mimics > detach_process, and to be in a better control on what's happening, than > creating a *DLL and loading it in order to call this function. > *On Windows the LIB file cannot be loaded as-is; a DLL with entry point > needs to be created, allowing the ntdll loader to resolve the imported > functions. [Alin Gabriel Serdean: ] I understand the concern but we can deal with that pretty easily from the build system. I understand the reasoning behind it, but in my opinion it seems we are duplicating the effort and in case of problems it will mean another codebase to maintain. Adding Ben and Guru to the thread extra opinions are always welcomed :). > > I don't say it isn't doable, but this seemed to be the right solution for this. > Also, creating a DLL loaded by python would add a dependency between > python and C components. > > Regarding the *unix side, maybe a Linux guy can add his opinion about this. > > Thanks, > Paul > > > -----Original Message----- > > From: Alin Serdean > > Sent: Tuesday, July 12, 2016 10:54 PM > > To: Paul Boca; dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python > > daemon to Windows > > > > I am wondering if it is better if we could just import the shared > > library and call the detach function > > (https://github.com/openvswitch/ovs/blob/master/lib/daemon- > > windows.c#L342) instead of duplicating the effort. > > > > I wonder if the same could be applied un the *unix side as well. > > > > Thanks, > > Alin.
On 13 July 2016 at 07:49, Alin Serdean <aserdean@cloudbasesolutions.com> wrote: > > -----Mesaj original----- > > De la: Paul Boca > > Trimis: Wednesday, July 13, 2016 11:24 AM > > Către: Alin Serdean <aserdean@cloudbasesolutions.com>; > > dev@openvswitch.org > > Subiect: RE: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python > > daemon to Windows > > > > Hi Alin, > > > > It was easier for me to call Windows APIs from python, that mimics > > detach_process, and to be in a better control on what's happening, than > > creating a *DLL and loading it in order to call this function. > > *On Windows the LIB file cannot be loaded as-is; a DLL with entry point > > needs to be created, allowing the ntdll loader to resolve the imported > > functions. > [Alin Gabriel Serdean: ] I understand the concern but we can deal with > that pretty easily from the build system. > I understand the reasoning behind it, but in my opinion it seems we are > duplicating the effort and in case of problems it will mean another codebase > to maintain. > Adding Ben and Guru to the thread extra opinions are always welcomed :). > I think so far we have tried to keep Python code independent of OVS C libraries. My opinion is that we keep the problem space limited with this patch series ( and if there is time on anyone's hand, try and work on what Alin is suggesting to see whether it fits well with all the linux packaging and build system variations.) So far, python daemonization code hasn't seen any bugs for a few years on Linux, so I don't think there will be a lot of maintenance burden. The windows ones probably will start with some bugs.. > > > > I don't say it isn't doable, but this seemed to be the right solution > for this. > > Also, creating a DLL loaded by python would add a dependency between > > python and C components. > > > > Regarding the *unix side, maybe a Linux guy can add his opinion about > this. > > > > Thanks, > > Paul > > > > > -----Original Message----- > > > From: Alin Serdean > > > Sent: Tuesday, July 12, 2016 10:54 PM > > > To: Paul Boca; dev@openvswitch.org > > > Subject: RE: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python > > > daemon to Windows > > > > > > I am wondering if it is better if we could just import the shared > > > library and call the detach function > > > (https://github.com/openvswitch/ovs/blob/master/lib/daemon- > > > windows.c#L342) instead of duplicating the effort. > > > > > > I wonder if the same could be applied un the *unix side as well. > > > > > > Thanks, > > > Alin. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
> > Hi Alin, > > It was easier for me to call Windows APIs from python, that mimics > detach_process, and to be in a better control on what's happening, than > creating a *DLL and loading it in order to call this function. > *On Windows the LIB file cannot be loaded as-is; a DLL with entry point > needs to be created, allowing the ntdll loader to resolve the imported > functions. [Alin Gabriel Serdean: ] I understand the concern but we can deal with that pretty easily from the build system. I understand the reasoning behind it, but in my opinion it seems we are duplicating the effort and in case of problems it will mean another codebase to maintain. Adding Ben and Guru to the thread extra opinions are always welcomed :). I think so far we have tried to keep Python code independent of OVS C libraries. My opinion is that we keep the problem space limited with this patch series ( and if there is time on anyone's hand, try and work on what Alin is suggesting to see whether it fits well with all the linux packaging and build system variations.) So far, python daemonization code hasn't seen any bugs for a few years on Linux, so I don't think there will be a lot of maintenance burden. The windows ones probably will start with some bugs.. [Alin Gabriel Serdean: ] I’m ok with starting just with the python part and we can have another discussion about using the C libraries later on.
Thanks a lot for the patch. In my opinion I think this part should go in a different file. The patch looks good overall some comments questions/inlined. > -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Paul Boca > Trimis: Wednesday, July 6, 2016 3:38 PM > Către: dev@openvswitch.org > Subiect: [ovs-dev] [PATCH V6 12/17] python tests: Ported Python daemon to > Windows > > Used subprocess.Popen instead os.fork (not implemented on windows) and > repaced of os.pipe with Windows pipes. > > To be able to identify the child process I added an extra parameter to > daemon process '--pipe-handle', this parameter also contains the parent > Windows pipe handle, used by the child to signal the start. > > The PID file is created directly on Windows, without using a temporary file > because the symbolic link doesn't inheriths the file lock set on temporary file. > > Signed-off-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com> > import errno > -import fcntl > import os > -import resource > import signal > import sys > import time > +import threading [Alin Gabriel Serdean: ] is threading needed for both or just on Windows? > > import ovs.dirs > import ovs.fatal_signal > @@ -28,6 +27,15 @@ import ovs.timeval > import ovs.util > import ovs.vlog > > +if sys.platform == "win32": > + import ovs.fcntl_win as fcntl > + import win32file, win32pipe, win32api, win32security, win32con, > pywintypes [Alin Gabriel Serdean: ] this is an external library we need to add it to the docs as a prerequisite > + import msvcrt > + import subprocess > +else: > + import fcntl > + import resource > + > vlog = ovs.vlog.Vlog("daemon") > > # --detach: Should we run in the background? > @@ -53,6 +61,9 @@ _monitor = False > # File descriptor used by daemonize_start() and daemonize_complete(). > _daemonize_fd = None > > +# Running as the child process - Windows only. > +_detached = False > + [Alin Gabriel Serdean: ] Maybe add it just on windows with the sys check > RESTART_EXIT_CODE = 5 > > > @@ -109,13 +120,20 @@ def set_monitor(): > global _monitor > _monitor = True > > +def set_detached(wp): > + """Sets up a following call to daemonize() to fork a supervisory process to > + monitor the daemon and restart it if it dies due to an error signal.""" > + global _detached > + global _daemonize_fd > + _detached = True > + _daemonize_fd = int(wp) > + [Alin Gabriel Serdean: ] [Alin Gabriel Serdean: ] Maybe add it just on windows with the sys check or add a comment that it should be used on windows only > > def _fatal(msg): > vlog.err(msg) > sys.stderr.write("%s\n" % msg) > sys.exit(1) > > - > def _make_pidfile(): > """If a pidfile has been configured, creates it and stores the running > process's pid in it. Ensures that the pidfile will be deleted when the @@ - > 123,8 +141,12 @@ def _make_pidfile(): > pid = os.getpid() > > # Create a temporary pidfile. > - tmpfile = "%s.tmp%d" % (_pidfile, pid) > - ovs.fatal_signal.add_file_to_unlink(tmpfile) > + if sys.platform == "win32": > + tmpfile = _pidfile [Alin Gabriel Serdean: ] shouldn't we add "ovs.fatal_signal.add_file_to_unlink" here as well or we cannot use it? > + else: > + tmpfile = "%s.tmp%d" % (_pidfile, pid) > + ovs.fatal_signal.add_file_to_unlink(tmpfile) > + > try: > # This is global to keep Python from garbage-collecting and > # therefore closing our file after this function exits. That would @@ - > 147,40 +169,47 @@ def _make_pidfile(): > _fatal("%s: write failed: %s" % (tmpfile, e.strerror)) > > try: > - # Rename or link it to the correct name. > - if _overwrite_pidfile: > - try: > - os.rename(tmpfile, _pidfile) > - except OSError as e: > - _fatal("failed to rename \"%s\" to \"%s\" (%s)" > - % (tmpfile, _pidfile, e.strerror)) > - else: > - while True: > + if sys.platform != "win32": > + # Rename or link it to the correct name. > + if _overwrite_pidfile: > try: > - os.link(tmpfile, _pidfile) > - error = 0 > + os.rename(tmpfile, _pidfile) > except OSError as e: > - error = e.errno > - if error == errno.EEXIST: > - _check_already_running() > - elif error != errno.EINTR: > - break > - if error: > - _fatal("failed to link \"%s\" as \"%s\" (%s)" > - % (tmpfile, _pidfile, os.strerror(error))) > - > - # Ensure that the pidfile will get deleted on exit. > - ovs.fatal_signal.add_file_to_unlink(_pidfile) > - > - # Delete the temporary pidfile if it still exists. > - if not _overwrite_pidfile: > - error = ovs.fatal_signal.unlink_file_now(tmpfile) > - if error: > - _fatal("%s: unlink failed (%s)" % (tmpfile, os.strerror(error))) > + _fatal("failed to rename \"%s\" to \"%s\" (%s)" > + % (tmpfile, _pidfile, e.strerror)) > + else: > + while True: > + try: > + os.link(tmpfile, _pidfile) > + error = 0 > + except OSError as e: > + error = e.errno > + if error == errno.EEXIST: > + _check_already_running() > + elif error != errno.EINTR: > + break > + if error: > + _fatal("failed to link \"%s\" as \"%s\" (%s)" > + % (tmpfile, _pidfile, os.strerror(error))) > + > + # Ensure that the pidfile will get deleted on exit. > + ovs.fatal_signal.add_file_to_unlink(_pidfile) > + > + # Delete the temporary pidfile if it still exists. > + if not _overwrite_pidfile: > + error = ovs.fatal_signal.unlink_file_now(tmpfile) > + if error: > + _fatal("%s: unlink failed (%s)" % (tmpfile, os.strerror(error))) > + else: > + # Ensure that the pidfile will gets closed and deleted on exit. > + ovs.fatal_signal.add_file_to_close_and_unlink(_pidfile, > + file_handle) [Alin Gabriel Serdean: ] can't we use ovs.fatal_signal.add_file_to_unlink? Shouldn't we retry to rename the file? > > global _pidfile_dev > global _pidfile_ino > @@ -204,6 +233,97 @@ def _waitpid(pid, options): > pass > return -e.errno, 0 > > +def _windows_create_pipe(): > + sAttrs = win32security.SECURITY_ATTRIBUTES() > + sAttrs.bInheritHandle = 1 > + > + (read_pipe, write_pipe) = win32pipe.CreatePipe(sAttrs, 0) > + pid = win32api.GetCurrentProcess() > + write_pipe2 = win32api.DuplicateHandle(pid, write_pipe, pid, 0, 1, > win32con.DUPLICATE_SAME_ACCESS) [Alin Gabriel Serdean: ] maybe I am missing something but why do we need the duplicate? > + win32file.CloseHandle(write_pipe) > + > + return (read_pipe, write_pipe2) > + > +def __windows_fork_notify_startup(fd): > + if fd is not None: > + try: > + win32file.WriteFile(fd, "0", None) > + except: > + win32file.WriteFile(fd, bytes("0", 'UTF-8'), None) [Alin Gabriel Serdean: ] I don't really understand the utf-8 part and also that may throw an exception as well > + > +def _windows_read_pipe(fd): > + if fd is not None: > + sAttrs = win32security.SECURITY_ATTRIBUTES() > + sAttrs.bInheritHandle = 1 > + overlapped = pywintypes.OVERLAPPED() [Alin Gabriel Serdean: ] sAttrs and overlapped not used > + try: > + (ret, data) = win32file.ReadFile(fd, 1, None) > + return data > + except pywintypes.error as e: > + raise OSError(errno.EIO, "", "") > + > +def _windows_detach(proc, wfd): > + """ If the child process closes and it was detached > + then close the communication pipe so the parent process > + can terminate """ > + proc.wait() > + win32file.CloseHandle(wfd) > + > +def _windows_fork_and_wait_for_startup(): > + if _detached: > + return 0 > + > + ''' close the log file, on Windows cannot be moved while the parent has > + a reference on it.''' > + vlog.close_log_file() > + > + try: > + (rfd, wfd) = _windows_create_pipe() > + except OSError as e: [Alin Gabriel Serdean: ] I think it's a pywintypes.error not oserror > + sys.stderr.write("pipe failed: %s\n" % os.strerror(e.errno)) > + sys.exit(1) > + > + try: > + proc = subprocess.Popen("%s %s --pipe-handle=%ld" % > (sys.executable, " ".join(sys.argv), int(wfd)), close_fds=False, shell=False) > + pid = proc.pid > + #start a thread and wait the subprocess exit code > + thread = threading.Thread(target=_windows_detach, args=(proc, wfd)) > + thread.daemon = True > + thread.start() > + except OSError as e: [Alin Gabriel Serdean: ] pywintypes.error needs to be treated as well because closehandle can throw an exception > + sys.stderr.write("could not fork: %s\n" % os.strerror(e.errno)) > + sys.exit(1) > +
diff --git a/python/ovs/daemon.py b/python/ovs/daemon.py index bd06195..0b58458 100644 --- a/python/ovs/daemon.py +++ b/python/ovs/daemon.py @@ -13,12 +13,11 @@ # limitations under the License. import errno -import fcntl import os -import resource import signal import sys import time +import threading import ovs.dirs import ovs.fatal_signal @@ -28,6 +27,15 @@ import ovs.timeval import ovs.util import ovs.vlog +if sys.platform == "win32": + import ovs.fcntl_win as fcntl + import win32file, win32pipe, win32api, win32security, win32con, pywintypes + import msvcrt + import subprocess +else: + import fcntl + import resource + vlog = ovs.vlog.Vlog("daemon") # --detach: Should we run in the background? @@ -53,6 +61,9 @@ _monitor = False # File descriptor used by daemonize_start() and daemonize_complete(). _daemonize_fd = None +# Running as the child process - Windows only. +_detached = False + RESTART_EXIT_CODE = 5 @@ -109,13 +120,20 @@ def set_monitor(): global _monitor _monitor = True +def set_detached(wp): + """Sets up a following call to daemonize() to fork a supervisory process to + monitor the daemon and restart it if it dies due to an error signal.""" + global _detached + global _daemonize_fd + _detached = True + _daemonize_fd = int(wp) + def _fatal(msg): vlog.err(msg) sys.stderr.write("%s\n" % msg) sys.exit(1) - def _make_pidfile(): """If a pidfile has been configured, creates it and stores the running process's pid in it. Ensures that the pidfile will be deleted when the @@ -123,8 +141,12 @@ def _make_pidfile(): pid = os.getpid() # Create a temporary pidfile. - tmpfile = "%s.tmp%d" % (_pidfile, pid) - ovs.fatal_signal.add_file_to_unlink(tmpfile) + if sys.platform == "win32": + tmpfile = _pidfile + else: + tmpfile = "%s.tmp%d" % (_pidfile, pid) + ovs.fatal_signal.add_file_to_unlink(tmpfile) + try: # This is global to keep Python from garbage-collecting and # therefore closing our file after this function exits. That would @@ -147,40 +169,47 @@ def _make_pidfile(): _fatal("%s: write failed: %s" % (tmpfile, e.strerror)) try: - fcntl.lockf(file_handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + if sys.platform != "win32": + fcntl.lockf(file_handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + else: + fcntl.lockf(file_handle, fcntl.LOCK_SH | fcntl.LOCK_NB) except IOError as e: _fatal("%s: fcntl failed: %s" % (tmpfile, e.strerror)) - # Rename or link it to the correct name. - if _overwrite_pidfile: - try: - os.rename(tmpfile, _pidfile) - except OSError as e: - _fatal("failed to rename \"%s\" to \"%s\" (%s)" - % (tmpfile, _pidfile, e.strerror)) - else: - while True: + if sys.platform != "win32": + # Rename or link it to the correct name. + if _overwrite_pidfile: try: - os.link(tmpfile, _pidfile) - error = 0 + os.rename(tmpfile, _pidfile) except OSError as e: - error = e.errno - if error == errno.EEXIST: - _check_already_running() - elif error != errno.EINTR: - break - if error: - _fatal("failed to link \"%s\" as \"%s\" (%s)" - % (tmpfile, _pidfile, os.strerror(error))) - - # Ensure that the pidfile will get deleted on exit. - ovs.fatal_signal.add_file_to_unlink(_pidfile) - - # Delete the temporary pidfile if it still exists. - if not _overwrite_pidfile: - error = ovs.fatal_signal.unlink_file_now(tmpfile) - if error: - _fatal("%s: unlink failed (%s)" % (tmpfile, os.strerror(error))) + _fatal("failed to rename \"%s\" to \"%s\" (%s)" + % (tmpfile, _pidfile, e.strerror)) + else: + while True: + try: + os.link(tmpfile, _pidfile) + error = 0 + except OSError as e: + error = e.errno + if error == errno.EEXIST: + _check_already_running() + elif error != errno.EINTR: + break + if error: + _fatal("failed to link \"%s\" as \"%s\" (%s)" + % (tmpfile, _pidfile, os.strerror(error))) + + # Ensure that the pidfile will get deleted on exit. + ovs.fatal_signal.add_file_to_unlink(_pidfile) + + # Delete the temporary pidfile if it still exists. + if not _overwrite_pidfile: + error = ovs.fatal_signal.unlink_file_now(tmpfile) + if error: + _fatal("%s: unlink failed (%s)" % (tmpfile, os.strerror(error))) + else: + # Ensure that the pidfile will gets closed and deleted on exit. + ovs.fatal_signal.add_file_to_close_and_unlink(_pidfile, file_handle) global _pidfile_dev global _pidfile_ino @@ -204,6 +233,97 @@ def _waitpid(pid, options): pass return -e.errno, 0 +def _windows_create_pipe(): + sAttrs = win32security.SECURITY_ATTRIBUTES() + sAttrs.bInheritHandle = 1 + + (read_pipe, write_pipe) = win32pipe.CreatePipe(sAttrs, 0) + pid = win32api.GetCurrentProcess() + write_pipe2 = win32api.DuplicateHandle(pid, write_pipe, pid, 0, 1, win32con.DUPLICATE_SAME_ACCESS) + win32file.CloseHandle(write_pipe) + + return (read_pipe, write_pipe2) + +def __windows_fork_notify_startup(fd): + if fd is not None: + try: + win32file.WriteFile(fd, "0", None) + except: + win32file.WriteFile(fd, bytes("0", 'UTF-8'), None) + +def _windows_read_pipe(fd): + if fd is not None: + sAttrs = win32security.SECURITY_ATTRIBUTES() + sAttrs.bInheritHandle = 1 + overlapped = pywintypes.OVERLAPPED() + try: + (ret, data) = win32file.ReadFile(fd, 1, None) + return data + except pywintypes.error as e: + raise OSError(errno.EIO, "", "") + +def _windows_detach(proc, wfd): + """ If the child process closes and it was detached + then close the communication pipe so the parent process + can terminate """ + proc.wait() + win32file.CloseHandle(wfd) + +def _windows_fork_and_wait_for_startup(): + if _detached: + return 0 + + ''' close the log file, on Windows cannot be moved while the parent has + a reference on it.''' + vlog.close_log_file() + + try: + (rfd, wfd) = _windows_create_pipe() + except OSError as e: + sys.stderr.write("pipe failed: %s\n" % os.strerror(e.errno)) + sys.exit(1) + + try: + proc = subprocess.Popen("%s %s --pipe-handle=%ld" % (sys.executable, " ".join(sys.argv), int(wfd)), close_fds=False, shell=False) + pid = proc.pid + #start a thread and wait the subprocess exit code + thread = threading.Thread(target=_windows_detach, args=(proc, wfd)) + thread.daemon = True + thread.start() + except OSError as e: + sys.stderr.write("could not fork: %s\n" % os.strerror(e.errno)) + sys.exit(1) + + if pid > 0: + # Running in parent process. + ovs.fatal_signal.fork() + while True: + try: + s = _windows_read_pipe(rfd) + error = 0 + except OSError as e: + s = "" + error = e.errno + if error != errno.EINTR: + break + if len(s) != 1: + retval, status = _waitpid(pid, 0) + if retval == pid: + if os.WIFEXITED(status) and os.WEXITSTATUS(status): + # Child exited with an error. Convey the same error to + # our parent process as a courtesy. + sys.exit(os.WEXITSTATUS(status)) + else: + sys.stderr.write("fork child failed to signal " + "startup (%s)\n" + % ovs.process.status_msg(status)) + else: + assert retval < 0 + sys.stderr.write("waitpid failed (%s)\n" + % os.strerror(-retval)) + sys.exit(1) + + return pid def _fork_and_wait_for_startup(): try: @@ -250,7 +370,7 @@ def _fork_and_wait_for_startup(): os.close(rfd) else: - # Running in parent process. + # Running in child process. os.close(rfd) ovs.timeval.postfork() @@ -258,8 +378,13 @@ def _fork_and_wait_for_startup(): _daemonize_fd = wfd return pid +def fork_and_wait_for_startup(): + if sys.platform == "win32": + return _windows_fork_and_wait_for_startup() + else: + return _fork_and_wait_for_startup() -def _fork_notify_startup(fd): +def __fork_notify_startup(fd): if fd is not None: error, bytes_written = ovs.socket_util.write_fully(fd, "0") if error: @@ -267,6 +392,11 @@ def _fork_notify_startup(fd): sys.exit(1) os.close(fd) +def _fork_notify_startup(fd): + if sys.platform == "win32": + return __windows_fork_notify_startup(fd) + else: + return __fork_notify_startup(fd) def _should_restart(status): global RESTART_EXIT_CODE @@ -296,7 +426,7 @@ def _monitor_daemon(daemon_pid): % (daemon_pid, ovs.process.status_msg(status))) if _should_restart(status): - if os.WCOREDUMP(status): + if os.WCOREDUMP(status) and sys.platform != "win32": # Disable further core dumps to save disk space. try: resource.setrlimit(resource.RLIMIT_CORE, (0, 0)) @@ -319,7 +449,7 @@ def _monitor_daemon(daemon_pid): last_restart = ovs.timeval.msec() vlog.err("%s, restarting" % status_msg) - daemon_pid = _fork_and_wait_for_startup() + daemon_pid = fork_and_wait_for_startup() if not daemon_pid: break else: @@ -332,6 +462,9 @@ def _monitor_daemon(daemon_pid): def _close_standard_fds(): """Close stdin, stdout, stderr. If we're started from e.g. an SSH session, then this keeps us from holding that session open artificially.""" + if sys.platform == "win32": + return + null_fd = ovs.socket_util.get_null_fd() if null_fd >= 0: os.dup2(null_fd, 0) @@ -347,16 +480,17 @@ def daemonize_start(): nonzero exit code).""" if _detach: - if _fork_and_wait_for_startup() > 0: + if fork_and_wait_for_startup() > 0: # Running in parent process. sys.exit(0) # Running in daemon or monitor process. - os.setsid() + if sys.platform != "win32": + os.setsid() if _monitor: saved_daemonize_fd = _daemonize_fd - daemon_pid = _fork_and_wait_for_startup() + daemon_pid = fork_and_wait_for_startup() if daemon_pid > 0: # Running in monitor process. _fork_notify_startup(saved_daemonize_fd) @@ -502,6 +636,9 @@ def add_args(parser): help="Create pidfile (default %s)." % pidfile) group.add_argument("--overwrite-pidfile", action="store_true", help="With --pidfile, start even if already running.") + if sys.platform == "win32": + group.add_argument("--pipe-handle", + help="With --pidfile, start even if already running.") def handle_args(args): @@ -510,6 +647,9 @@ def handle_args(args): parent ArgumentParser should have been prepared by add_args() before calling parse_args().""" + if sys.platform == "win32" and args.pipe_handle: + set_detached(args.pipe_handle) + if args.detach: set_detach() diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py index 5b90559..c16d704 100644 --- a/python/ovs/fatal_signal.py +++ b/python/ovs/fatal_signal.py @@ -58,6 +58,17 @@ def add_file_to_unlink(file): _files[file] = None +def add_file_to_close_and_unlink(file, fd = None): + """Registers 'file' to be unlinked when the program terminates via + sys.exit() or a fatal signal and the 'fd' to be closed. On Windows a file + cannot be removed while it is open for writing.""" + global _added_hook + if not _added_hook: + _added_hook = True + add_hook(_unlink_files, _cancel_files, True) + _files[file] = fd + + def remove_file_to_unlink(file): """Unregisters 'file' from being unlinked when the program terminates via sys.exit() or a fatal signal.""" @@ -77,6 +88,8 @@ def unlink_file_now(file): def _unlink_files(): for file_ in _files: + if _files[file_]: + _files[file_].close() _unlink(file_) diff --git a/python/ovs/vlog.py b/python/ovs/vlog.py index 4996387..55edbc7 100644 --- a/python/ovs/vlog.py +++ b/python/ovs/vlog.py @@ -384,6 +384,17 @@ class Vlog(object): logger.addHandler(Vlog.__file_handler) @staticmethod + def close_log_file(): + """Closes the current log file. (This is useful on Windows, to ensure + that a reference to the file is not kept by the daemon in case of + detach.)""" + + if Vlog.__log_file: + logger = logging.getLogger("file") + logger.removeHandler(Vlog.__file_handler) + Vlog.__file_handler.close() + + @staticmethod def _unixctl_vlog_reopen(conn, unused_argv, unused_aux): if Vlog.__log_file: Vlog.reopen_log_file() @@ -396,6 +407,7 @@ class Vlog(object): if Vlog.__log_file: logger = logging.getLogger("file") logger.removeHandler(Vlog.__file_handler) + Vlog.__file_handler.close() conn.reply(None) @staticmethod diff --git a/tests/test-daemon.py b/tests/test-daemon.py index 63c1f70..a3b5751 100644 --- a/tests/test-daemon.py +++ b/tests/test-daemon.py @@ -26,8 +26,8 @@ def handler(signum, _): def main(): - - signal.signal(signal.SIGHUP, handler) + if sys.platform != "win32": + signal.signal(signal.SIGHUP, handler) parser = argparse.ArgumentParser( description="Open vSwitch daemonization test program for Python.")
Used subprocess.Popen instead os.fork (not implemented on windows) and repaced of os.pipe with Windows pipes. To be able to identify the child process I added an extra parameter to daemon process '--pipe-handle', this parameter also contains the parent Windows pipe handle, used by the child to signal the start. The PID file is created directly on Windows, without using a temporary file because the symbolic link doesn't inheriths the file lock set on temporary file. Signed-off-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com> --- V2: Fix lockf on Linux, small error on os.link and missing pipe_handle parameter. V3: Import modules at the start of the code V4: Close file before trying to delete it in signal hooks. On Windows the PID file cannot be deleted while it's handle is opened for write. V5: No changes V6: Explicitly close the vlog file in detached daemon. On Windows, even if the daemon is detached, the primary daemon is still holding a handle to the log file, therefore the log cannot be moved/deleted even is the vlog/close command is sent to the detached daemon. --- python/ovs/daemon.py | 222 ++++++++++++++++++++++++++++++++++++--------- python/ovs/fatal_signal.py | 13 +++ python/ovs/vlog.py | 12 +++ tests/test-daemon.py | 4 +- 4 files changed, 208 insertions(+), 43 deletions(-)