[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