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

Jakub Sitnicki jkbs at redhat.com
Fri Jul 13 08:51:52 UTC 2018


On Thu, 12 Jul 2018 17:10:04 -0400
Mark Michelson <mmichels at redhat.com> wrote:

> 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(-)
> > 

(...)

> > +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.

Let me fix that.

> 
> > +
> > +    /* 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.

Perhaps I need to revisit the idea of having an options parses tailored
to server mode needs. Other option would be to ignore options that don't
make sense for the server.

> 
> > +    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.

You're right. The server loop is modeled after ovn-trace main loop.
So now I'm also wondering if I should check the IDL connection state
before calling daemonize_complete(). Let me do some testing to figure
it out.

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



More information about the dev mailing list