| Message ID | 1470646161-142366-1-git-send-email-bluecmd@google.com |
|---|---|
| State | RFC |
| Headers | show |
On Mon, Aug 08, 2016 at 10:49:21AM +0200, Christian Svensson wrote: > As handling software via cgroups is more and more common, PID files are > not always needed. This change adds --disable-pid-socket-path to > ./configure to build OpenvSwitch without this PID section of the socket > path. This offers a way for software to easier know the socket path for > daemons independent on which platform it is running on. > > Today the socket path is by default one of: > Linux: RUNDIR/daemon.$PID.ctl > Windows: RUNDIR/daemon.ctl > > For Linux it is thus commonplace to also run the daemon with --pidfile > to be able to communicate with the daemon. For Windows there is no such > need (or possibility). > > In order to preserve default behavior the new flag ships as enabled. > Documentation currently describes the default behavior only. > > ovsdb-client is updated to support setting explicit socket path when > running in detached mode. > > This closes https://github.com/openvswitch/ovs-issues/issues/114. > > Signed-off-by: Christian Svensson <bluecmd@google.com> I don't think it's reasonable to make this a configuration-time option. I want Open vSwitch behavior to be fairly uniform at runtime, not unpredictable based on whatever flags were supplied at build time. I suggest that you just use "--pidfile=<program>.pid" on the command line, instead.
Thanks for the comments. On Mon, Aug 8, 2016 at 6:29 PM, Ben Pfaff <blp@ovn.org> wrote: > > I don't think it's reasonable to make this a configuration-time option. > I want Open vSwitch behavior to be fairly uniform at runtime, not > unpredictable based on whatever flags were supplied at build time. > Isn't it already as rundir is configured exactly like this and behavior (what to name the socket) is affected by it? I agree it's not perfect, but I couldn't think of a better way of solving this while still providing backwards compatibility. Besides, it's not like it's dead code, it's just making the Linux platform use the Window platform's logic for calculating the socket path. In my opinion the side effect that the PID logic is configurable to be OS agnostic is reason alone that this is a good change, but I'm obviously biased. > I suggest that you just use "--pidfile=<program>.pid" on the command > line, instead. Sorry, but how does that solve the problem? Maybe I'm missing something, but that only activates the behavior to make socket paths resolvable without globing. The alternative I'm looking for would be something like --unixctl=/run/openvswitch/my-daemon.ctl but that does not exist. This particular use-case is for OpenSwitch. Having OpenvSwitch using PID files would be the only service to do so, and we're faced with either having --pidfile on every single daemon that uses OVSDB (a lot) or change the compile time default. Hopefully it makes sense why we want to avoid this complexity in our case.
On Mon, Aug 08, 2016 at 07:32:12PM +0200, Christian Svensson wrote: > The alternative I'm looking for would be something like > --unixctl=/run/openvswitch/my-daemon.ctl but that does not exist. OK, let's add that then.
On Mon, Aug 8, 2016 at 7:59 PM, Ben Pfaff <blp@ovn.org> wrote: > OK, let's add that then. > While it's an alternative, I feel like going down that path is hasty and for my use case I will probably just patch our local fork of OpenvSwitch. Why so hostile towards this change? Is there a better forum I can explain why I believe this problem should be reconsidered? Is there really anyone that can justify the way socket paths are created should contain the PID? You clearly saw the problem with it in the Windows code base, but yet pushed on to keep it in the Linux one. I simply do not see why you're not taking this patch as a way to encourage people to migrate towards a saner default, and somewhere down the line maybe even switch the default behavior.
On Mon, Aug 8, 2016 at 7:59 PM, Ben Pfaff <blp@ovn.org> wrote: > OK, let's add that then. > I forgot to add a critical point why I'm not ready to consider the alternative yet: One of the optional goals I'm trying to achieve is that ovs-appctl -t my-daemon should still work (keeping all documentation etc.), which obviously wouldn't work with just --unixctl.
On Mon, Aug 08, 2016 at 10:05:03PM +0200, Christian Svensson wrote: > On Mon, Aug 8, 2016 at 7:59 PM, Ben Pfaff <blp@ovn.org> wrote: > > > OK, let's add that then. > > While it's an alternative, I feel like going down that path is hasty and > for my use case I will probably just patch our local fork of OpenvSwitch. > > Why so hostile towards this change? Is there a better forum I can explain > why I believe this problem should be reconsidered? Including the PID allows multiple daemons of a single type to run. I also don't think you're solving a real problem, because reading a pidfile isn't difficult. > Is there really anyone that can justify the way socket paths are created > should contain the PID? You clearly saw the problem with it in the Windows > code base, but yet pushed on to keep it in the Linux one. I simply do not > see why you're not taking this patch as a way to encourage people to > migrate towards a saner default, and somewhere down the line maybe even > switch the default behavior. I think that Windows doesn't support getpid(). MSDN documents it that way, at least. That is probably why we omit it on Windows, though I do not know for sure. Guru, is that the reason? I don't understand why you want to change this so badly. It's not hard to read a pidfile. I definitely don't want to fork OVS behavior here based on a configuration flag, as I already explained. Basically: I see little benefit to your change, and some drawbacks.
> > Is there really anyone that can justify the way socket paths are > > created should contain the PID? You clearly saw the problem with it in > > the Windows code base, but yet pushed on to keep it in the Linux one. > > I simply do not see why you're not taking this patch as a way to > > encourage people to migrate towards a saner default, and somewhere > > down the line maybe even switch the default behavior. > > I think that Windows doesn't support getpid(). MSDN documents it that way, > at least. That is probably why we omit it on Windows, though I do not know > for sure. Guru, is that the reason? > Sorry to step in... Windows supports a form of getpid() (https://msdn.microsoft.com/en-us/library/t2y34y40.aspx). On Windows it does not make sense to make our own daemon(service) behavior, because we can simply rely on a common on the service manager (just be subscribing to it), which brings more to the table and users are already used to it. Most of the daemon code under windows is used just to keep compatibility for unit tests and the ease to maintain them. I hope it brings more light on the subject. Thanks, Alin.
> > While it's an alternative, I feel like going down that path is hasty > > and for my use case I will probably just patch our local fork of OpenvSwitch. > > > > Why so hostile towards this change? Is there a better forum I can > > explain why I believe this problem should be reconsidered? > > Including the PID allows multiple daemons of a single type to run. I also don't > think you're solving a real problem, because reading a pidfile isn't difficult. [Alin Gabriel Serdean: ] I think this patch will tamper with multiple datapath setups (AFAIK OVS on Linux still supports it). > > > Is there really anyone that can justify the way socket paths are > > created should contain the PID? You clearly saw the problem with it in > > the Windows code base, but yet pushed on to keep it in the Linux one. > > I simply do not see why you're not taking this patch as a way to > > encourage people to migrate towards a saner default, and somewhere > > down the line maybe even switch the default behavior. > > I think that Windows doesn't support getpid(). MSDN documents it that way, > at least. That is probably why we omit it on Windows, though I do not know > for sure. Guru, is that the reason? > > I don't understand why you want to change this so badly. It's not hard to > read a pidfile. > > I definitely don't want to fork OVS behavior here based on a configuration > flag, as I already explained. > > Basically: I see little benefit to your change, and some drawbacks.
On Mon, Aug 8, 2016 at 11:59 PM, Ben Pfaff <blp@ovn.org> wrote: > > Including the PID allows multiple daemons of a single type to run. While technically true, in practice I would argue this doesn't hold and is indeed one of the reason I made this change. The documented way to connect to a daemon is "ovs-appctl -t my-daemon". That reads the PID file "my-daemin.pid" and uses "my-daemon.$PID.ctl". That requires my PID file given to the daemon to match the internal "program_name" for the socket resolution code to work. I.e., I cannot do --pidfile=my-daemon2.pid and then ovs-appctl -t my-daemon2 will work. In order to call the second one you would have to do: ovs-appctl -t $PWD/my-daemon.$(<my-daemon2.pid).ctl I do hope we can agree on that requiring the caller to know both the PID name and the internal program name is far from perfect. With this change daemons that do support setting unixctl would be really able to run multiple instances. my-daemon --unixctl=$RUNDIR/my-daemon.ctl my-daemon --unixctl=$RUNDIR/my-daemon2.ctl ovs-appctl -t my-daemon ovs-appctl -t my-daemon2 would both work. I also don't think you're solving a real problem, because reading a > pidfile isn't difficult. > I don't understand why you want to change this so badly. It's not hard > to read a pidfile. Just because it isn't difficult doesn't mean it's the right way to do things. I definitely don't want to fork OVS behavior here based on a > configuration flag, as I already explained. > But you already do! The configuration flag today is called WIN32. With this patch it's called WITH_PID_SOCKET_PATH and defaults to YES/NO depending on the platform. The code that is behind #ifdef $OS is much smaller, and the one behind #ifdef $FEATURE is larger. That should make more code testable in both platforms, making the total code aliveness higher.
On Tue, Aug 9, 2016 at 9:10 AM, Christian Svensson <christian@cmd.nu> wrote: > > ovs-appctl -t $PWD/my-daemon.$(<my-daemon2.pid).ctl > To compromise on the client side: How would you feel making ovs-appctl and socket_name_from_target look for both naming schemes? Psuedo code: if isfile(target + '.ctl'): return target + '.ctl' elif isfile(target + '.pid'): [.. do classical lookup ..] return target + pid + '.ctl' This would have the same benefit of getting rid of the platform dependent conditionals, but it would obviously open up a small window for confusion. With this reducing the scope of --disable-pid-socket-path to only affecting the server socket naming and not client behavior, would that change how you feel about the complexity of the feature flag? Thanks,
diff --git a/AUTHORS b/AUTHORS index b598f4b..1226958 100644 --- a/AUTHORS +++ b/AUTHORS @@ -44,6 +44,7 @@ Casey Barker crbarker@google.com Chandra Sekhar Vejendla csvejend@us.ibm.com Christoph Jaeger cj@linux.com Chris Wright chrisw@sous-sol.org +Christian Svensson bluecmd@google.com Chuck Short zulcss@ubuntu.com Ciara Loftus ciara.loftus@intel.com Cong Wang amwang@redhat.com diff --git a/Makefile.am b/Makefile.am index 49010b3..77d2417 100644 --- a/Makefile.am +++ b/Makefile.am @@ -9,6 +9,7 @@ AUTOMAKE_OPTIONS = foreign subdir-objects ACLOCAL_AMFLAGS = -I m4 SUBDIRS = datapath +AM_DISTCHECK_CONFIGURE_FLAGS = AM_CPPFLAGS = $(SSL_CFLAGS) AM_LDFLAGS = $(SSL_LDFLAGS) AM_LDFLAGS += $(OVS_LDFLAGS) @@ -42,6 +43,13 @@ AM_CPPFLAGS += -DNDEBUG AM_CFLAGS += -fomit-frame-pointer endif +if WITH_PID_SOCKET_PATH +AM_DISTCHECK_CONFIGURE_FLAGS += '--enable-pid-socket-path' +AM_CPPFLAGS += -DWITH_PID_SOCKET_PATH +else +AM_DISTCHECK_CONFIGURE_FLAGS += '--disable-pid-socket-path' +endif + if WIN32 psep=";" else diff --git a/configure.ac b/configure.ac index 05d80d5..399f5c3 100644 --- a/configure.ac +++ b/configure.ac @@ -101,6 +101,7 @@ OVS_CHECK_DOT OVS_CHECK_IF_PACKET OVS_CHECK_IF_DL OVS_CHECK_STRTOK_R +OVS_CHECK_PID_SOCKET_PATH AC_CHECK_DECLS([sys_siglist], [], [], [[#include <signal.h>]]) AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct stat.st_mtimensec], [], [], [[#include <sys/stat.h>]]) diff --git a/lib/unixctl.c b/lib/unixctl.c index 57d6577..7ba65c0 100644 --- a/lib/unixctl.c +++ b/lib/unixctl.c @@ -188,6 +188,8 @@ unixctl_command_reply_error(struct unixctl_conn *conn, const char *error) /* Creates a unixctl server listening on 'path', which for POSIX may be: * * - NULL, in which case <rundir>/<program>.<pid>.ctl is used. + * If built with --disable-pid-socket-path it is instead + * <rundir>/<program>.ctl. * * - A name that does not start with '/', in which case it is put in * <rundir>. @@ -236,7 +238,7 @@ unixctl_server_create(const char *path, struct unixctl_server **serverp) punix_path = xasprintf("punix:%s", abs_path); free(abs_path); } else { -#ifndef _WIN32 +#ifdef WITH_PID_SOCKET_PATH punix_path = xasprintf("punix:%s/%s.%ld.ctl", ovs_rundir(), program_name, (long int) getpid()); #else diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 index a448223..28315f7 100644 --- a/m4/openvswitch.m4 +++ b/m4/openvswitch.m4 @@ -589,3 +589,29 @@ AC_DEFUN([OVS_CHECK_PRAGMA_MESSAGE], [AC_DEFINE(HAVE_PRAGMA_MESSAGE,1,[Define if compiler supports #pragma message directive])]) ]) + +dnl Checks if user wants to include PID in default socket path. +AC_DEFUN([OVS_CHECK_PID_SOCKET_PATH], + [AC_ARG_ENABLE( + [pid-socket-path], + [AC_HELP_STRING([--disable-pid-socket-path], + [Do not include PID in socket path])], + [case "${enableval}" in + (yes) with_pid_socket_path=yes ;; + (no) with_pid_socket_path=no ;; + (*) AC_MSG_ERROR([bad value ${enableval} for +--enable-pid-socket-path]) ;; + esac], + [if test "$WIN32" = yes; then + with_pid_socket_path=no + else + with_pid_socket_path=yes + fi]) + AC_SUBST([WITH_PID_SOCKET_PATH]) + if test $with_pid_socket_path != no; then + WITH_PID_SOCKET_PATH=yes + else + WITH_PID_SOCKET_PATH=no + fi + AM_CONDITIONAL([WITH_PID_SOCKET_PATH], + [test x$with_pid_socket_path = xyes])]) diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index 1f83f3b..b12650c 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -71,6 +71,8 @@ struct ovsdb_client_command { /* --timestamp: Print a timestamp before each update on "monitor" command? */ static bool timestamp; +static char *unixctl_path = NULL; + /* Format for table output. */ static struct table_style table_style = TABLE_STYLE_DEFAULT; @@ -171,6 +173,7 @@ parse_options(int argc, char *argv[]) { enum { OPT_BOOTSTRAP_CA_CERT = UCHAR_MAX + 1, + OPT_UNIXCTL, OPT_TIMESTAMP, VLOG_OPTION_ENUMS, DAEMON_OPTION_ENUMS, @@ -180,6 +183,7 @@ parse_options(int argc, char *argv[]) {"help", no_argument, NULL, 'h'}, {"version", no_argument, NULL, 'V'}, {"timestamp", no_argument, NULL, OPT_TIMESTAMP}, + {"unixctl", required_argument, NULL, OPT_UNIXCTL}, VLOG_LONG_OPTIONS, DAEMON_LONG_OPTIONS, #ifdef HAVE_OPENSSL @@ -220,6 +224,10 @@ parse_options(int argc, char *argv[]) timestamp = true; break; + case OPT_UNIXCTL: + unixctl_path = optarg; + break; + case '?': exit(EXIT_FAILURE); @@ -288,6 +296,7 @@ usage(void) daemon_usage(); vlog_usage(); printf("\nOther options:\n" + " --unixctl=SOCKET override default control socket name\n" " -h, --help display this help message\n" " -V, --version display version information\n"); exit(EXIT_SUCCESS); @@ -956,7 +965,7 @@ do_monitor__(struct jsonrpc *rpc, const char *database, if (get_detach()) { int error; - error = unixctl_server_create(NULL, &unixctl); + error = unixctl_server_create(unixctl_path, &unixctl); if (error) { ovs_fatal(error, "failed to create unixctl server"); } @@ -1470,7 +1479,7 @@ do_lock(struct jsonrpc *rpc, const char *method, const char *lock) if (get_detach()) { int error; - error = unixctl_server_create(NULL, &unixctl); + error = unixctl_server_create(unixctl_path, &unixctl); if (error) { ovs_fatal(error, "failed to create unixctl server"); } diff --git a/python/automake.mk b/python/automake.mk index 1c8fa38..eb9a120 100644 --- a/python/automake.mk +++ b/python/automake.mk @@ -73,6 +73,7 @@ ovs-install-data-local: -e 's,[@]bindir[@],$(bindir),g' \ -e 's,[@]sysconfdir[@],$(sysconfdir),g' \ -e 's,[@]DBDIR[@],$(DBDIR),g' \ + -e 's,[@]WITH_PID_SOCKET_PATH[@],$(WITH_PID_SOCKET_PATH),g' \ < $(srcdir)/python/ovs/dirs.py.template \ > python/ovs/dirs.py.tmp $(MKDIR_P) $(DESTDIR)$(pkgdatadir)/python/ovs @@ -101,7 +102,7 @@ $(srcdir)/python/ovs/version.py: config.status if cmp -s $(@F).tmp $@; then touch $@; rm $(@F).tmp; else mv $(@F).tmp $@; fi ALL_LOCAL += $(srcdir)/python/ovs/dirs.py -$(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template +$(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template config.status $(AM_V_GEN)sed \ -e '/^##/d' \ -e 's,[@]pkgdatadir[@],/usr/local/share/openvswitch,g' \ @@ -110,6 +111,7 @@ $(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template -e 's,[@]bindir[@],/usr/local/bin,g' \ -e 's,[@]sysconfdir[@],/usr/local/etc,g' \ -e 's,[@]DBDIR[@],/usr/local/etc/openvswitch,g' \ - < $? > $@.tmp && \ - mv $@.tmp $@ + -e 's,[@]WITH_PID_SOCKET_PATH[@],$(WITH_PID_SOCKET_PATH),g' \ + < $< > $(@F).tmp && \ + if cmp -s $(@F).tmp $@; then touch $@; rm $(@F).tmp; else mv $(@F).tmp $@; fi EXTRA_DIST += python/ovs/dirs.py.template diff --git a/python/ovs/dirs.py b/python/ovs/dirs.py index c67aecb..5de5c4a 100644 --- a/python/ovs/dirs.py +++ b/python/ovs/dirs.py @@ -21,6 +21,8 @@ PKGDATADIR = os.environ.get("OVS_PKGDATADIR", """/usr/local/share/openvswitch""" RUNDIR = os.environ.get("OVS_RUNDIR", """/var/run""") LOGDIR = os.environ.get("OVS_LOGDIR", """/usr/local/var/log""") BINDIR = os.environ.get("OVS_BINDIR", """/usr/local/bin""") +WITH_PID_SOCKET_PATH = ("y" in + os.environ.get("OVS_WITH_PID_SOCKET_PATH", "yes")) DBDIR = os.environ.get("OVS_DBDIR") if not DBDIR: diff --git a/python/ovs/dirs.py.template b/python/ovs/dirs.py.template index fb31b74..0bfbf9b 100644 --- a/python/ovs/dirs.py.template +++ b/python/ovs/dirs.py.template @@ -21,6 +21,8 @@ PKGDATADIR = os.environ.get("OVS_PKGDATADIR", """@pkgdatadir@""") RUNDIR = os.environ.get("OVS_RUNDIR", """@RUNDIR@""") LOGDIR = os.environ.get("OVS_LOGDIR", """@LOGDIR@""") BINDIR = os.environ.get("OVS_BINDIR", """@bindir@""") +WITH_PID_SOCKET_PATH = ("y" in + os.environ.get("OVS_WITH_PID_SOCKET_PATH", "@WITH_PID_SOCKET_PATH@")) DBDIR = os.environ.get("OVS_DBDIR") if not DBDIR: diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py index 48b56d4..5477c6e 100644 --- a/python/ovs/unixctl/__init__.py +++ b/python/ovs/unixctl/__init__.py @@ -76,6 +76,9 @@ def socket_name_from_target(target): if target.startswith('/') or target.find(':') > -1: return 0, target + if not ovs.dirs.WITH_PID_SOCKET_PATH: + return 0, "%s/%s.ctl" % (ovs.dirs.RUNDIR, target) + pidfile_name = "%s/%s.pid" % (ovs.dirs.RUNDIR, target) pid = ovs.daemon.read_pidfile(pidfile_name) if pid < 0: diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py index 8595ed8..1283131 100644 --- a/python/ovs/unixctl/server.py +++ b/python/ovs/unixctl/server.py @@ -15,7 +15,6 @@ import copy import errno import os -import sys import six from six.moves import range @@ -188,14 +187,11 @@ class UnixctlServer(object): if path is not None: path = "punix:%s" % ovs.util.abs_file_name(ovs.dirs.RUNDIR, path) + elif ovs.dirs.WITH_PID_SOCKET_PATH: + path = "punix:%s/%s.%d.ctl" % (ovs.dirs.RUNDIR, + ovs.util.PROGRAM_NAME, os.getpid()) else: - if sys.platform == "win32": - path = "punix:%s/%s.ctl" % (ovs.dirs.RUNDIR, - ovs.util.PROGRAM_NAME) - else: - path = "punix:%s/%s.%d.ctl" % (ovs.dirs.RUNDIR, - ovs.util.PROGRAM_NAME, - os.getpid()) + path = "punix:%s/%s.ctl" % (ovs.dirs.RUNDIR, ovs.util.PROGRAM_NAME) if version is None: version = ovs.version.VERSION diff --git a/tests/atlocal.in b/tests/atlocal.in index 55070d8..e1741cf 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -2,6 +2,7 @@ HAVE_OPENSSL='@HAVE_OPENSSL@' HAVE_PYTHON='@HAVE_PYTHON@' HAVE_PYTHON3='@HAVE_PYTHON3@' +WITH_PID_SOCKET_PATH='@WITH_PID_SOCKET_PATH@' EGREP='@EGREP@' PERL='@PERL@' diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at index 8eca16c..5b5f09b 100644 --- a/tests/ovn-controller-vtep.at +++ b/tests/ovn-controller-vtep.at @@ -25,8 +25,8 @@ m4_define([OVN_CONTROLLER_VTEP_START], dnl Start ovsdb-server. AT_CHECK([ovsdb-server --detach --no-chdir --pidfile=$OVS_RUNDIR/ovsdb-server.pid --log-file --remote=punix:$OVS_RUNDIR/db.sock vswitchd.db vtep.db], [0], [], [stderr]) - AT_CHECK([ovsdb-server --detach --no-chdir --pidfile=$OVS_RUNDIR/ovsdb-nb-server.pid --log-file=ovsdb-nb-server.log --remote=punix:$OVS_RUNDIR/ovnnb_db.sock ovn-nb.db], [0], [], [stderr]) - AT_CHECK([ovsdb-server --detach --no-chdir --pidfile=$OVS_RUNDIR/ovsdb-sb-server.pid --log-file=ovsdb-sb-server.log --remote=punix:$OVS_RUNDIR/ovnsb_db.sock ovn-sb.db ovn-sb.db], [0], [], [stderr]) + AT_CHECK([ovsdb-server --detach --no-chdir --unixctl=$OVS_RUNDIR/ovsdb-nb-server.ctl --pidfile=$OVS_RUNDIR/ovsdb-nb-server.pid --log-file=ovsdb-nb-server.log --remote=punix:$OVS_RUNDIR/ovnnb_db.sock ovn-nb.db], [0], [], [stderr]) + AT_CHECK([ovsdb-server --detach --no-chdir --unixctl=$OVS_RUNDIR/ovsdb-sb-server.ctl --pidfile=$OVS_RUNDIR/ovsdb-sb-server.pid --log-file=ovsdb-sb-server.log --remote=punix:$OVS_RUNDIR/ovnsb_db.sock ovn-sb.db ovn-sb.db], [0], [], [stderr]) on_exit "kill `cat ovsdb-server.pid` `cat ovsdb-nb-server.pid` `cat ovsdb-sb-server.pid`" AT_CHECK([[sed < stderr ' /vlog|INFO|opened log file/d diff --git a/tests/ovsdb-lock.at b/tests/ovsdb-lock.at index 7152c5d..af5aa27 100644 --- a/tests/ovsdb-lock.at +++ b/tests/ovsdb-lock.at @@ -37,7 +37,7 @@ AT_CLEANUP OVSDB_CHECK_LOCK_SETUP([unlock], [positive]) AT_CHECK([ovsdb-client --detach --pidfile lock unix:socket lock0 >c1-output 2>&1], [0], [], []) -AT_CHECK([ovsdb-client --detach lock unix:socket lock0 >c2-output 2>&1], +AT_CHECK([ovsdb-client --detach --unixctl=dummy.ctl lock unix:socket lock0 >c2-output 2>&1], [0], [], []) AT_CHECK([ovs-appctl -t ovsdb-client unlock lock0], [0], [], []) OVS_APP_EXIT_AND_WAIT([ovsdb-server]) diff --git a/tests/ovsdb-monitor.at b/tests/ovsdb-monitor.at index 6d51a1a..0f9fdea 100644 --- a/tests/ovsdb-monitor.at +++ b/tests/ovsdb-monitor.at @@ -73,19 +73,18 @@ m4_define([OVSDB_CHECK_MONITOR_COND], AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/server-pid --remote=punix:socket --unixctl="`pwd`"/unixctl --log-file="`pwd`"/ovsdb-server-log db >/dev/null 2>&1], [0], [], []) if test "$IS_WIN32" = "yes"; then - AT_CHECK([ovsdb-client -vjsonrpc --pidfile="`pwd`"/client-pid -d json monitor-cond --format=csv unix:socket $4 '[$8]' $5 $9 > output &], + AT_CHECK([ovsdb-client -vjsonrpc --unixctl="`pwd`"/client-ctl --pidfile="`pwd`"/client-pid -d json monitor-cond --format=csv unix:socket $4 '[$8]' $5 $9 > output &], [0], [ignore], [ignore], [kill `cat server-pid`]) sleep 1 else - AT_CHECK([ ovsdb-client -vjsonrpc --detach --no-chdir --pidfile="`pwd`"/client-pid -d json monitor-cond --format=csv unix:socket $4 '[$8]' $5 $9 > output], + AT_CHECK([ ovsdb-client -vjsonrpc --detach --no-chdir --unixctl="`pwd`"/client-ctl --pidfile="`pwd`"/client-pid -d json monitor-cond --format=csv unix:socket $4 '[$8]' $5 $9 > output], [0], [ignore], [ignore], [kill `cat server-pid`]) fi m4_foreach([txn], [$6], [AT_CHECK([ovsdb-client transact unix:socket 'txn'], [0], [ignore], [ignore], [kill `cat server-pid client-pid`])]) - CLIENT_PID=`cat "$OVS_RUNDIR"/client-pid 2>/dev/null` m4_foreach([cond], [$10], - [AT_CHECK([ovs-appctl -t "`pwd`"/ovsdb-client.$CLIENT_PID.ctl ovsdb-client/cond_change $5 'cond'], [0], [ignore], [ignore])]) + [AT_CHECK([ovs-appctl -t "`pwd`"/client-ctl ovsdb-client/cond_change $5 'cond'], [0], [ignore], [ignore])]) AT_CHECK([ovsdb-client transact unix:socket '[["$4"]]'], [0], [ignore], [ignore], [kill `cat server-pid client-pid`]) AT_CHECK([ovs-appctl -t "`pwd`"/unixctl -e exit], [0], [ignore], [ignore]) diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at index 2031897..f75230b 100644 --- a/tests/unixctl-py.at +++ b/tests/unixctl-py.at @@ -88,20 +88,22 @@ m4_define([UNIXCTL_BAD_TARGET_PYN], [AT_SETUP([unixctl bad target - $1]) AT_SKIP_IF([test $2 = no]) - AT_CHECK([PYAPPCTL_PYN([$3]) -t bogus doit], [1], [], [stderr]) - AT_CHECK_UNQUOTED([tail -1 stderr], [0], [dnl + if test "$WITH_PID_SOCKET_PATH" = "yes"; then + AT_CHECK([PYAPPCTL_PYN([$3]) -t bogus doit], [1], [], [stderr]) + AT_CHECK_UNQUOTED([tail -1 stderr], [0], [dnl appctl.py: cannot read pidfile "`pwd`/bogus.pid" (No such file or directory) ]) - if test "$IS_WIN32" = "no"; then - AT_CHECK([PYAPPCTL_PYN([$3]) -t /bogus/path.pid doit], [1], [], [stderr]) - AT_CHECK([tail -1 stderr], [0], [dnl + if test "$IS_WIN32" = "no"; then + AT_CHECK([PYAPPCTL_PYN([$3]) -t /bogus/path.pid doit], [1], [], [stderr]) + AT_CHECK([tail -1 stderr], [0], [dnl appctl.py: cannot connect to "/bogus/path.pid" (No such file or directory) ]) - else - AT_CHECK([PYAPPCTL_PYN([$3]) -t c:/bogus/path.pid doit], [1], [], [stderr]) - AT_CHECK([tail -1 stderr], [0], [dnl + else + AT_CHECK([PYAPPCTL_PYN([$3]) -t c:/bogus/path.pid doit], [1], [], [stderr]) + AT_CHECK([tail -1 stderr], [0], [dnl appctl.py: cannot connect to "c:/bogus/path.pid" (No such file or directory) ]) + fi fi AT_CLEANUP]) diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c index 8f87cc4..62172b8 100644 --- a/utilities/ovs-appctl.c +++ b/utilities/ovs-appctl.c @@ -201,6 +201,12 @@ connect_to_target(const char *target) #ifndef _WIN32 if (target[0] != '/') { +#else + /* On windows, if the 'target' contains ':', we make an assumption that + * it is an absolute path. */ + if (!strchr(target, ':')) { +#endif +#ifdef WITH_PID_SOCKET_PATH char *pidfile_name; pid_t pid; @@ -213,9 +219,6 @@ connect_to_target(const char *target) socket_name = xasprintf("%s/%s.%ld.ctl", ovs_rundir(), target, (long int) pid); #else - /* On windows, if the 'target' contains ':', we make an assumption that - * it is an absolute path. */ - if (!strchr(target, ':')) { socket_name = xasprintf("%s/%s.ctl", ovs_rundir(), target); #endif } else {
As handling software via cgroups is more and more common, PID files are not always needed. This change adds --disable-pid-socket-path to ./configure to build OpenvSwitch without this PID section of the socket path. This offers a way for software to easier know the socket path for daemons independent on which platform it is running on. Today the socket path is by default one of: Linux: RUNDIR/daemon.$PID.ctl Windows: RUNDIR/daemon.ctl For Linux it is thus commonplace to also run the daemon with --pidfile to be able to communicate with the daemon. For Windows there is no such need (or possibility). In order to preserve default behavior the new flag ships as enabled. Documentation currently describes the default behavior only. ovsdb-client is updated to support setting explicit socket path when running in detached mode. This closes https://github.com/openvswitch/ovs-issues/issues/114. Signed-off-by: Christian Svensson <bluecmd@google.com> --- AUTHORS | 1 + Makefile.am | 8 ++++++++ configure.ac | 1 + lib/unixctl.c | 4 +++- m4/openvswitch.m4 | 26 ++++++++++++++++++++++++++ ovsdb/ovsdb-client.c | 13 +++++++++++-- python/automake.mk | 8 +++++--- python/ovs/dirs.py | 2 ++ python/ovs/dirs.py.template | 2 ++ python/ovs/unixctl/__init__.py | 3 +++ python/ovs/unixctl/server.py | 12 ++++-------- tests/atlocal.in | 1 + tests/ovn-controller-vtep.at | 4 ++-- tests/ovsdb-lock.at | 2 +- tests/ovsdb-monitor.at | 7 +++---- tests/unixctl-py.at | 18 ++++++++++-------- utilities/ovs-appctl.c | 9 ++++++--- 17 files changed, 89 insertions(+), 32 deletions(-)