[ovs-dev] [PATCH 5/5] xenserver: Add monitor-external-ids daemon

[Ben Pfaff]

On Wed, Aug 25, 2010 at 04:10:23PM -0700, Justin Pettit wrote:
> The monitor-external-ids daemon monitors the external id columns of the Bridge
> and Interface OVSDB tables.  It's primary responsibility is to set the

"Its"

> "bridge-id" and "iface-id" keys in the Bridge and Interface tables,
> respectively.  It also looks for the use of "network-uuids" in the Bridge table
> and duplicates its value to the preferred "xs-network-uuids".

Since this modifies the xenserver directory it needs a Signed-off-by:
line.  (Maybe some of the other patches too?)

> diff --git a/xenserver/README b/xenserver/README
> index db2bd29..5380502 100644
> --- a/xenserver/README
> +++ b/xenserver/README
> @@ -49,6 +49,11 @@ files are:
>  
>          Open vSwitch-aware replacement for Citrix script of the same name.
>  
> +    usr_share_openvswitch_scripts_monitor-external-ids
> +
> +        Daemon to monitor the external id columns of the Bridge and

I would write external_ids here to make "grep"ping for it more
successful.

> +        Interface OVSDB tables.
> +
>      usr_share_openvswitch_scripts_refresh-xs-network-uuids
>  
>          Script to refresh Bridge table external-ids:xs-network-uuids

> diff --git a/xenserver/etc_init.d_openvswitch b/xenserver/etc_init.d_openvswitch
> index 850a7ed..69a71c4 100755
> --- a/xenserver/etc_init.d_openvswitch
> +++ b/xenserver/etc_init.d_openvswitch
> @@ -341,6 +341,12 @@ function start {
>      if [ "${ENABLE_BRCOMPAT}" = "y" ] ; then
>          start_brcompatd
>      fi
> +
> +    # Start daemon to monitor external ids
> +    export PYTHONPATH=/usr/share/openvswitch/python
> +    /usr/share/openvswitch/scripts/monitor-external-ids \
> +                 --pidfile --detach "$VSWITCHD_OVSDB_SERVER"

I would write
        PYTHONPATH=/usr/share/openvswitch/python \
            /usr/share/openvswitch/scripts/monitor-external-ids \
            --pidfile --detach "$VSWITCHD_OVSDB_SERVER"
instead, to ensure that PYTHONPATH doesn't "leak" into anything else
that runs after this in the init script.  Probably doesn't matter,
        but...

> +
>      touch /var/lock/subsys/openvswitch
>  }
>  
> @@ -348,6 +354,10 @@ function stop {
>      stop_daemon BRCOMPATD "$brcompatd"
>      stop_daemon VSWITCHD "$vswitchd"
>      stop_daemon OVSDB_SERVER "$ovsdb_server"
> +    if [ -e /var/run/openvswitch/monitor-external-ids.pid ]; then
> +        kill `cat /var/run/openvswitch/monitor-external-ids.pid`
> +        rm /var/run/openvswitch/monitor-external-ids.pid
> +    fi

Doesn't the stop_daemon function work here?  Oh, I see, we don't set a
_PIDFILE variable for the monitor daemon.  Maybe we should.  (And then
we'd need --pidfile-name=$whatever_PIDFILE when we start the daemon.)

> +def usage():
> +    print "usage: %s [OPTIONS] DATABASE" % sys.argv[0]
> +    print "where DATABASE is a socket on which ovsdb-server is listening."
> +    ovs.daemon.usage()
> +    print "Other options:"
> +    print "  -h, --help               display this help message"
> +    sys.exit(0)
> + 
> +def main(argv):
> +    try:
> +        options, args = getopt.gnu_getopt(
> +            argv[1:], 'h', ['help'] + ovs.daemon.LONG_OPTIONS)
> +    except getopt.GetoptError, geo:
> +        sys.stderr.write("%s: %s\n" % (ovs.util.PROGRAM_NAME, geo.msg))
> +        sys.exit(1)
> + 
> +    for key, value in options:
> +        if key in ['-h', '--help']:
> +            usage()
> +        elif not ovs.daemon.parse_opt(key, value):
> +            sys.stderr.write("%s: unhandled option %s\n"
> +                             % (ovs.util.PROGRAM_NAME, key))
> +            sys.exit(1)
> + 
> +    optKeys = [key for key, value in options]

I don't see anything that uses optKeys.

I kind of zoned out reading a lot of this Python code.  I've read and
written a lot of Python lately and can't quite maintain my attention.

I assume it works.