[ovs-dev] [PATCH v2 12/15] ovn-nbctl: Initial support for daemon mode.

Mark Michelson mmichels at redhat.com
Thu Jul 12 21:10:04 UTC 2018


On 07/12/2018 09:40 AM, Jakub Sitnicki wrote:
> Make ovn-nbctl act as a unixctl server if we were asked to detach. This
> turns ovn-nbctl into a long-lived process that acts a proxy for
> interacting with NB DB. The main difference to regular mode of ovn-nbctl
> is that in the daemon mode, a local copy of database contents has to be
> obtained only once.
> 
> Just two unixctl commands are supported 'run' and 'exit'. The former can
> be used to run any ovn-nbctl command or a batch of them as so:
> 
>    ovs-appctl -t ovn-nbctl run [OPTIONS] COMMAND [-- [OPTIONS] COMMAND] ...
> 
> Running commands that have not yet been converted to not use ctl_fatal()
> will result in death of the daemon process. However, --monitor option
> can be used to keep the daemon running.
> 
> Signed-off-by: Jakub Sitnicki <jkbs at redhat.com>
> ---
>   ovn/utilities/ovn-nbctl.8.xml |  40 ++++++++
>   ovn/utilities/ovn-nbctl.c     | 213 ++++++++++++++++++++++++++++++++++++------
>   2 files changed, 227 insertions(+), 26 deletions(-)
> 
> diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
> index abba4ecdb..2cd2fab30 100644
> --- a/ovn/utilities/ovn-nbctl.8.xml
> +++ b/ovn/utilities/ovn-nbctl.8.xml
> @@ -913,6 +913,43 @@
>         </dd>
>       </dl>
>   
> +    <h1>Daemon Mode</h1>
> +
> +    <p>
> +      If <code>ovn-nbctl</code> is invoked with the <code>--detach</code>
> +      option (see <code>Daemon Options</code>, below), it runs in the
> +      background as a daemon and accepts commands from <code>ovs-appctl</code>
> +      (or another JSON-RPC client) indefinitely.  The currently supported
> +      commands are described below.
> +    </p>
> +
> +    <p>
> +
> +    </p>
> +
> +    <dl>
> +      <dt>
> +        <code>run</code> [<var>options</var>] <var>command</var>
> +        [<var>arg</var>...] [<code>--</code> [<var>options</var>]
> +        <var>command</var> [<var>arg</var>...] ...]
> +      </dt>
> +      <dd>
> +        Instructs the daemon process to run one or more <code>ovn-nbctl</code>
> +        commands described above and reply with the results of running these
> +        commands. Accepts the <code>--no-wait</code>, <code>--wait</code>,
> +        <code>--timeout</code>, <code>--dry-run</code>, <code>--oneline</code>,
> +        and the options described under <code>Table Formatting Options</code>
> +        in addition to the the command-specific options.
> +      </dd>
> +
> +      <dt><code>exit</code></dt>
> +      <dd>Causes <code>ovn-nbctl</code> to gracefully terminate.</dd>
> +    </dl>
> +
> +    <p>
> +      Daemon mode is considered experimental.
> +    </p>
> +
>       <h1>Options</h1>
>   
>       <dl>
> @@ -982,6 +1019,9 @@
>       </dd>
>       </dl>
>   
> +    <h2>Daemon Options</h2>
> +    <xi:include href="lib/daemon.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
> +
>       <h1>Logging options</h1>
>       <xi:include href="lib/vlog.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
>   
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 3dd24d193..ba9b7ca49 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -20,6 +20,7 @@
>   #include <stdio.h>
>   
>   #include "command-line.h"
> +#include "daemon.h"
>   #include "db-ctl-base.h"
>   #include "dirs.h"
>   #include "fatal-signal.h"
> @@ -38,6 +39,7 @@
>   #include "table.h"
>   #include "timeval.h"
>   #include "timer.h"
> +#include "unixctl.h"
>   #include "util.h"
>   #include "openvswitch/vlog.h"
>   
> @@ -80,6 +82,13 @@ OVS_NO_RETURN static void nbctl_exit(int status);
>   /* --leader-only, --no-leader-only: Only accept the leader in a cluster. */
>   static int leader_only = true;
>   
> +/* --unixctl-path: Path to use for unixctl server, for "monitor" and "snoop"
> +     commands. */
> +static char *unixctl_path;
> +
> +static unixctl_cb_func server_cmd_exit;
> +static unixctl_cb_func server_cmd_run;
> +
>   static void nbctl_cmd_init(void);
>   OVS_NO_RETURN static void usage(void);
>   static void parse_options(int argc, char *argv[], struct shash *local_options);
> @@ -98,15 +107,13 @@ static char * OVS_WARN_UNUSED_RESULT main_loop(const char *args,
>                                                  size_t n_commands,
>                                                  struct ovsdb_idl *idl,
>                                                  const struct timer *);
> +static void server_loop(struct ovsdb_idl *idl);
>   
>   int
>   main(int argc, char *argv[])
>   {
>       struct ovsdb_idl *idl;
> -    struct ctl_command *commands;
>       struct shash local_options;
> -    size_t n_commands;
> -    char *error;
>   
>       set_program_name(argv[0]);
>       fatal_ignore_sigpipe();
> @@ -119,38 +126,55 @@ main(int argc, char *argv[])
>       char *args = process_escape_args(argv);
>       shash_init(&local_options);
>       parse_options(argc, argv, &local_options);
> -    commands = ctl_parse_commands(argc - optind, argv + optind, &local_options,
> -                                  &n_commands);
> -    VLOG(ctl_might_write_to_db(commands, n_commands) ? VLL_INFO : VLL_DBG,
> -         "Called as %s", args);
> -
> -    if (timeout) {
> -        time_alarm(timeout);
> -    }
> +    argc -= optind;
> +    argv += optind;
>   
>       /* Initialize IDL. */
>       idl = the_idl = ovsdb_idl_create(db, &nbrec_idl_class, true, false);
>       ovsdb_idl_set_leader_only(idl, leader_only);
> -    error = run_prerequisites(commands, n_commands, idl);
> -    if (error) {
> -        ctl_fatal("%s", error);
> -    }
>   
> -    error = main_loop(args, commands, n_commands, idl, NULL);
> -    if (error) {
> -        ctl_fatal("%s", error);
> +    if (get_detach()) {
> +        if (argc != 0) {
> +            ctl_fatal("non-option arguments not supported with --detach "
> +                      "(use --help for help)");
> +        }
> +        server_loop(idl);
> +    } else {
> +        struct ctl_command *commands;
> +        size_t n_commands;
> +        char *error;
> +
> +        commands = ctl_parse_commands(argc, argv, &local_options, &n_commands);
> +        VLOG(ctl_might_write_to_db(commands, n_commands) ? VLL_INFO : VLL_DBG,
> +             "Called as %s", args);
> +
> +        if (timeout) {
> +            time_alarm(timeout);
> +        }
> +
> +        error = run_prerequisites(commands, n_commands, idl);
> +        if (error) {
> +            ctl_fatal("%s", error);
> +        }
> +
> +        error = main_loop(args, commands, n_commands, idl, NULL);
> +        if (error) {
> +            ctl_fatal("%s", error);
> +        }
> +
> +        struct ctl_command *c;
> +        for (c = commands; c < &commands[n_commands]; c++) {
> +            ds_destroy(&c->output);
> +            table_destroy(c->table);
> +            free(c->table);
> +            shash_destroy_free_data(&c->options);
> +        }
> +        free(commands);
>       }
>   
>       ovsdb_idl_destroy(idl);
>       idl = the_idl = NULL;
>   
> -    for (struct ctl_command *c = commands; c < &commands[n_commands]; c++) {
> -        ds_destroy(&c->output);
> -        table_destroy(c->table);
> -        free(c->table);
> -        shash_destroy_free_data(&c->options);
> -    }
> -    free(commands);
>       free(args);
>       exit(EXIT_SUCCESS);
>   }
> @@ -160,6 +184,7 @@ main_loop(const char *args, struct ctl_command *commands, size_t n_commands,
>             struct ovsdb_idl *idl, const struct timer *wait_timeout)
>   {
>       unsigned int seqno;
> +    bool idl_ready;
>   
>       /* Execute the commands.
>        *
> @@ -169,6 +194,11 @@ main_loop(const char *args, struct ctl_command *commands, size_t n_commands,
>        * it's because the database changed and we need to obtain an up-to-date
>        * view of the database before we try the transaction again. */
>       seqno = ovsdb_idl_get_seqno(idl);
> +
> +    /* IDL might have already obtained the database copy during previous
> +     * invocation. If so, we can't expect the sequence number to change before
> +     * we issue any new requests. */
> +    idl_ready = ovsdb_idl_has_ever_connected(idl);
>       for (;;) {
>           ovsdb_idl_run(idl);
>           if (!ovsdb_idl_is_alive(idl)) {
> @@ -177,7 +207,8 @@ main_loop(const char *args, struct ctl_command *commands, size_t n_commands,
>                         db, ovs_retval_to_string(retval));
>           }
>   
> -        if (seqno != ovsdb_idl_get_seqno(idl)) {
> +        if (idl_ready || seqno != ovsdb_idl_get_seqno(idl)) {
> +            idl_ready = false;
>               seqno = ovsdb_idl_get_seqno(idl);
>   
>               bool retry;
> @@ -214,6 +245,7 @@ parse_options(int argc, char *argv[], struct shash *local_options)
>           OPT_COMMANDS,
>           OPT_OPTIONS,
>           OPT_BOOTSTRAP_CA_CERT,
> +        DAEMON_OPTION_ENUMS,
>           VLOG_OPTION_ENUMS,
>           TABLE_OPTION_ENUMS,
>           SSL_OPTION_ENUMS,
> @@ -232,6 +264,7 @@ parse_options(int argc, char *argv[], struct shash *local_options)
>           {"leader-only", no_argument, &leader_only, true},
>           {"no-leader-only", no_argument, &leader_only, false},
>           {"version", no_argument, NULL, 'V'},
> +        DAEMON_LONG_OPTIONS,
>           VLOG_LONG_OPTIONS,
>           STREAM_SSL_LONG_OPTIONS,
>           {"bootstrap-ca-cert", required_argument, NULL, OPT_BOOTSTRAP_CA_CERT},
> @@ -336,6 +369,7 @@ parse_options(int argc, char *argv[], struct shash *local_options)
>               }
>               break;
>   
> +        DAEMON_OPTION_HANDLERS
>           VLOG_OPTION_HANDLERS
>           TABLE_OPTION_HANDLERS(&table_style)
>           STREAM_SSL_OPTION_HANDLERS
> @@ -529,6 +563,7 @@ Options:\n\
>              program_name, program_name, ctl_get_db_cmd_usage(),
>              ctl_list_db_tables_usage(), default_nb_db());
>       table_usage();
> +    daemon_usage();
>       vlog_usage();
>       printf("\
>     --no-syslog             equivalent to --verbose=nbctl:syslog:warn\n");
> @@ -4562,3 +4597,129 @@ nbctl_cmd_init(void)
>       ctl_init(&nbrec_idl_class, nbrec_table_classes, tables, NULL, nbctl_exit);
>       ctl_register_commands(nbctl_commands);
>   }
> +
> +static void
> +server_cmd_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                const char *argv[] OVS_UNUSED, void *exiting_)
> +{
> +    bool *exiting = exiting_;
> +    *exiting = true;
> +    unixctl_command_reply(conn, NULL);
> +}
> +
> +static void
> +server_cmd_run(struct unixctl_conn *conn, int argc, const char **argv_,
> +               void *idl_)
> +{
> +    struct ovsdb_idl *idl = idl_;
> +    struct ctl_command *commands = NULL;
> +    struct shash local_options;
> +    size_t n_commands = 0;
> +    char *error = NULL;
> +
> +    /* Copy args so that getopt() can permute them. Leave last entry NULL. */
> +    char **argv = xcalloc(argc + 1, sizeof *argv);
> +    for (int i = 0; i < argc; i++) {
> +        argv[i] = xstrdup(argv_[i]);
> +    }
> +
> +    /* Reset global state. */
> +    oneline = false;
> +    dry_run = false;
> +    wait_type = NBCTL_WAIT_NONE;
> +    force_wait = false;
> +    timeout = 0;
> +    table_style = table_style_default;

Not all global state is being reset here. The biggest thing I spotted 
was that the vlog level is not reset.

> +
> +    /* Parse commands & options. */
> +    char *args = process_escape_args(argv);
> +    shash_init(&local_options);
> +    optind = 0;
> +    parse_options(argc, argv, &local_options);

Calling parse_options() here is interesting. There are some options that 
are relevant, some options that are irrelevant and ignored, and there 
are some options that we really don't want to parse here.

For instance, "commands", "help", "version", and "options" will result 
in the server process exiting. I know in your cover letter, you 
mentioned that there were still some places where ctl_fatal() is called. 
I'm not sure if you had noticed these places where exit() is called as well.

> +    commands = ctl_parse_commands(argc - optind, argv + optind,
> +                                  &local_options, &n_commands);
> +    VLOG(ctl_might_write_to_db(commands, n_commands) ? VLL_INFO : VLL_DBG,
> +         "Running command %s", args);
> +
> +    struct timer *wait_timeout = NULL;
> +    struct timer wait_timeout_;
> +    if (timeout) {
> +        wait_timeout = &wait_timeout_;
> +        timer_set_duration(wait_timeout, timeout * 1000);
> +    }
> +
> +    error = run_prerequisites(commands, n_commands, idl);
> +    if (error) {
> +        unixctl_command_reply_error(conn, error);
> +        goto out;
> +    }
> +    error = main_loop(args, commands, n_commands, idl, wait_timeout);
> +    if (error) {
> +        unixctl_command_reply_error(conn, error);
> +        goto out;
> +    }
> +
> +    struct ds output = DS_EMPTY_INITIALIZER;
> +    for (struct ctl_command *c = commands; c < &commands[n_commands]; c++) {
> +        if (c->table) {
> +            table_format(c->table, &table_style, &output);
> +        } else if (oneline) {
> +            oneline_format(&c->output, &output);
> +        } else {
> +            ds_put_cstr(&output, ds_cstr_ro(&c->output));
> +        }
> +
> +        ds_destroy(&c->output);
> +        table_destroy(c->table);
> +        free(c->table);
> +    }
> +    unixctl_command_reply(conn, ds_cstr_ro(&output));
> +    ds_destroy(&output);
> +
> +out:
> +    free(error);
> +    for (struct ctl_command *c = commands; c < &commands[n_commands]; c++) {
> +        shash_destroy_free_data(&c->options);
> +    }
> +    free(commands);
> +    shash_destroy_free_data(&local_options);
> +    free(args);
> +    for (int i = 0; i < argc; i++) {
> +        free(argv[i]);
> +    }
> +    free(argv);
> +}
> +
> +static void
> +server_cmd_init(struct ovsdb_idl *idl, bool *exiting)
> +{
> +    unixctl_command_register("exit", "", 0, 0, server_cmd_exit, exiting);
> +    unixctl_command_register("run", "", 1, INT_MAX, server_cmd_run, idl);
> +}
> +
> +static void
> +server_loop(struct ovsdb_idl *idl)
> +{
> +    struct unixctl_server *server = NULL;
> +    bool exiting = false;
> +
> +    daemonize_start(false);
> +    int error = unixctl_server_create(unixctl_path, &server);
> +    if (error) {
> +        ctl_fatal("failed to create unixctl server (%s)",
> +                  ovs_retval_to_string(error));
> +    }
> +    server_cmd_init(idl, &exiting);
> +
> +    for (;;) {
> +        unixctl_server_run(server);
> +        daemonize_complete();

You should move the call to daemonize_complete() to outside the for 
loop. It is a no-op after the first time you call it.

> +        unixctl_server_wait(server);
> +        if (exiting) {
> +            break;
> +        }
> +        poll_block();
> +    }
> +
> +    unixctl_server_destroy(server);
> +}
> 



More information about the dev mailing list