ovn-nbctl: Make daemon mode more transparent.

This makes ovn-nbctl transparently use daemon mode if an appropriate
environment variable is set.

It also transforms ovn-nbctl.at so that it runs each ovn-nbctl test in
"direct" mode and in daemon mode.  It uses a combination of m4 macros and
shell functions to keep from expanding the generated testsuite more than
necessary.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Mark Michelson <mmichels@redhat.com>

NEWS

@@ -35,10 +35,8 @@ v2.10.0 - xx xxx xxxx
      * ACL match conditions can now match on Port_Groups as well as address
        sets that are automatically generated by Port_Groups.  ACLs can be
        applied directly to Port_Groups as well.
-     * ovn-nbctl can now run as a daemon (long-lived, background process)
+     * ovn-nbctl can now run as a daemon (long-lived, background process).
-       running commands in response to JSON-RPC requests received over a UNIX
+       See ovn-nbctl(8) for details.
-       socket. Requests to run commands can be sent using ovs-appctl tool, same
-       as for any other OVS/OVN daemon. See ovn-nbctl(8) for details.
    - DPDK:
      * New 'check-dpdk' Makefile target to run a new system testsuite.
        See Testing topic for the details.

lib/daemon.h

@@ -84,6 +84,15 @@
             daemon_set_new_user(optarg);        \
             break;
 
+#define DAEMON_OPTION_CASES                     \
+        case OPT_DETACH:                        \
+        case OPT_NO_SELF_CONFINEMENT:           \
+        case OPT_NO_CHDIR:                      \
+        case OPT_PIDFILE:                       \
+        case OPT_OVERWRITE_PIDFILE:             \
+        case OPT_MONITOR:                       \
+        case OPT_USER_GROUP:
+
 void set_detach(void);
 void daemon_set_monitor(void);
 void set_no_chdir(void);
@@ -138,6 +147,16 @@ pid_t read_pidfile(const char *name);
         case OPT_USER_GROUP:                    \
             daemon_set_new_user(optarg);
 
+#define DAEMON_OPTION_CASES                     \
+        case OPT_DETACH:                        \
+        case OPT_NO_SELF_CONFINEMENT:           \
+        case OPT_NO_CHDIR:                      \
+        case OPT_PIDFILE:                       \
+        case OPT_PIPE_HANDLE:                   \
+        case OPT_SERVICE:                       \
+        case OPT_SERVICE_MONITOR:               \
+        case OPT_USER_GROUP:
+
 void control_handler(DWORD request);
 void set_pipe_handle(const char *pipe_handle);
 #endif /* _WIN32 */

lib/stream-ssl.h

@@ -58,6 +58,9 @@ void stream_ssl_set_ciphers(const char *arg);
                                                         \
         case OPT_SSL_CIPHERS:                           \
             stream_ssl_set_ciphers(optarg);             \
-            break;                                      
+            break;
+
+#define STREAM_SSL_CASES \
+    case 'p': case 'c': case 'C': case OPT_SSL_PROTOCOLS: case OPT_SSL_CIPHERS:
 
 #endif /* stream-ssl.h */

ovn/utilities/ovn-nbctl.8.xml

@@ -971,15 +971,62 @@
     <h1>Daemon Mode</h1>
 
     <p>
-      If <code>ovn-nbctl</code> is invoked with the <code>--detach</code>
+      When it is invoked in the most ordinary way, <code>ovn-nbctl</code>
-      option (see <code>Daemon Options</code>, below), it runs in the
+      connects to an OVSDB server that hosts the northbound database, retrieves
-      background as a daemon and accepts commands from <code>ovs-appctl</code>
+      a partial copy of the database that is complete enough to do its work,
-      (or another JSON-RPC client) indefinitely.  The currently supported
+      sends a transaction request to the server, and receives and processes the
-      commands are described below.
+      server's reply.  In common interactive use, this is fine, but if the
+      database is large, the step in which <code>ovn-nbctl</code> retrieves a
+      partial copy of the database can take a long time, which yields poor
+      performance overall.
     </p>
 
     <p>
+      To improve performance in such a case, <code>ovn-nbctl</code> offers a
+      "daemon mode," in which the user first starts <code>ovn-nbctl</code>
+      running in the background and afterward uses the daemon to execute
+      operations.  Over several <code>ovn-nbctl</code> command invocations,
+      this performs better overall because it retrieves a copy of the database
+      only once at the beginning, not once per program run.
+    </p>
 
+    <p>
+      Use the <code>--detach</code> option to start an <code>ovn-nbctl</code>
+      daemon.  With this option, <code>ovn-nbctl</code> prints the name of a
+      control socket to stdout.  The client should save this name in
+      environment variable <env>OVN_NB_DAEMON</env>.  Under the Bourne shell
+      this might be done like this:
+    </p>
+
+    <pre fixed="yes">
+      export OVN_NB_DAEMON=$(ovn-nbctl --pidfile --detach)
+    </pre>
+
+    <p>
+      When <env>OVN_NB_DAEMON</env> is set, <code>ovn-nbctl</code>
+      automatically and transparently uses the daemon to execute its commands.
+    </p>
+
+    <p>
+      When the daemon is no longer needed, kill it and unset the environment
+      variable, e.g.:
+    </p>
+
+    <pre fixed="yes">
+      kill $(cat /var/run/ovn-nbctl.pid)
+      unset OVN_NB_DAEMON
+    </pre>
+
+    <p>
+      Daemon mode is experimental.
+    </p>
+
+    <h2>Daemon Commands</h2>
+
+    <p>
+      Daemon mode is internally implemented using the same mechanism used by
+      <code>ovs-appctl</code>.  One may also use <code>ovs-appctl</code>
+      directly with the following commands:
     </p>
 
     <dl>
@@ -1001,10 +1048,6 @@
       <dd>Causes <code>ovn-nbctl</code> to gracefully terminate.</dd>
     </dl>
 
-    <p>
-      Daemon mode is considered experimental.
-    </p>
-
     <h1>Options</h1>
 
     <dl>

ovn/utilities/ovn-nbctl.c

@@ -24,6 +24,7 @@
 #include "db-ctl-base.h"
 #include "dirs.h"
 #include "fatal-signal.h"
+#include "jsonrpc.h"
 #include "openvswitch/json.h"
 #include "ovn/lib/acl-log.h"
 #include "ovn/lib/ovn-nb-idl.h"
@@ -91,7 +92,15 @@ 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);
+static struct option *get_all_options(void);
+static bool has_option(const struct ovs_cmdl_parsed_option *, size_t n,
+                       int option);
+static void nbctl_client(const char *socket_name,
+                         const struct ovs_cmdl_parsed_option *, size_t n,
+                         int argc, char *argv[]);
+static bool will_detach(const struct ovs_cmdl_parsed_option *, size_t n);
+static void apply_options_direct(const struct ovs_cmdl_parsed_option *,
+                                 size_t n, struct shash *local_options);
 static char * OVS_WARN_UNUSED_RESULT run_prerequisites(struct ctl_command[],
                                                        size_t n_commands,
                                                        struct ovsdb_idl *);
@@ -123,10 +132,47 @@ main(int argc, char *argv[])
 
     nbctl_cmd_init();
 
-    /* Parse command line. */
+    /* ovn-nbctl has three operation modes:
+     *
+     *    - Direct: Executes commands by contacting ovsdb-server directly.
+     *
+     *    - Server: Runs in the background as a daemon waiting for requests
+     *      from ovn-nbctl running in client mode.
+     *
+     *    - Client: Executes commands by passing them to an ovn-nbctl running
+     *      in the server mode.
+     *
+     * At this point we don't know what mode we're running in.  The mode partly
+     * depends on the command line.  So, for now we transform the command line
+     * into a parsed form, and figure out what to do with it later.
+     */
     char *args = process_escape_args(argv);
+    struct ovs_cmdl_parsed_option *parsed_options;
+    size_t n_parsed_options;
+    char *error_s = ovs_cmdl_parse_all(argc, argv, get_all_options(),
+                                       &parsed_options, &n_parsed_options);
+    if (error_s) {
+        ctl_fatal("%s", error_s);
+    }
+
+    /* Now figure out the operation mode:
+     *
+     *    - A --detach option implies server mode.
+     *
+     *    - An OVN_NB_DAEMON environment variable implies client mode.
+     *
+     *    - Otherwise, we're in direct mode. */
+    char *socket_name = getenv("OVN_NB_DAEMON");
+    if (socket_name && socket_name[0]
+        && !will_detach(parsed_options, n_parsed_options)) {
+        nbctl_client(socket_name, parsed_options, n_parsed_options,
+                     argc, argv);
+    }
+
+    /* Parse command line. */
     shash_init(&local_options);
-    parse_options(argc, argv, &local_options);
+    apply_options_direct(parsed_options, n_parsed_options, &local_options);
+    free(parsed_options);
     argc -= optind;
     argv += optind;
 
@@ -256,6 +302,8 @@ enum {
     OPT_LOCAL,
     OPT_COMMANDS,
     OPT_OPTIONS,
+    OPT_LEADER_ONLY,
+    OPT_NO_LEADER_ONLY,
     OPT_BOOTSTRAP_CA_CERT,
     MAIN_LOOP_OPTION_ENUMS,
     DAEMON_OPTION_ENUMS,
@@ -347,8 +395,8 @@ append_command_options(const struct option *options, int opt_val)
     return o;
 }
 
-static void
+static struct option *
-parse_options(int argc, char *argv[], struct shash *local_options)
+get_all_options(void)
 {
     static const struct option global_long_options[] = {
         {"db", required_argument, NULL, OPT_DB},
@@ -356,8 +404,8 @@ parse_options(int argc, char *argv[], struct shash *local_options)
         {"help", no_argument, NULL, 'h'},
         {"commands", no_argument, NULL, OPT_COMMANDS},
         {"options", no_argument, NULL, OPT_OPTIONS},
-        {"leader-only", no_argument, &leader_only, true},
+        {"leader-only", no_argument, NULL, OPT_LEADER_ONLY},
-        {"no-leader-only", no_argument, &leader_only, false},
+        {"no-leader-only", no_argument, NULL, OPT_NO_LEADER_ONLY},
         {"version", no_argument, NULL, 'V'},
         MAIN_LOOP_LONG_OPTIONS,
         DAEMON_LONG_OPTIONS,
@@ -367,24 +415,57 @@ parse_options(int argc, char *argv[], struct shash *local_options)
         TABLE_LONG_OPTIONS,
         {NULL, 0, NULL, 0},
     };
-    const int n_global_long_options = ARRAY_SIZE(global_long_options) - 1;
-    struct option *options;
-    size_t i;
 
-    options = append_command_options(global_long_options, OPT_LOCAL);
+    static struct option *options;
-
+    if (!options) {
-    struct ovs_cmdl_parsed_option *parsed_options;
+        options = append_command_options(global_long_options, OPT_LOCAL);
-    size_t n_po;
-    char *error = ovs_cmdl_parse_all(argc, argv, options,
-                                     &parsed_options, &n_po);
-    if (error) {
-        ctl_fatal("%s", error);
     }
 
+    return options;
+}
+
+static bool
+has_option(const struct ovs_cmdl_parsed_option *parsed_options, size_t n,
+           int option)
+{
     for (const struct ovs_cmdl_parsed_option *po = parsed_options;
-         po < &parsed_options[n_po]; po++) {
+         po < &parsed_options[n]; po++) {
+        if (po->o->val == option) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static bool
+will_detach(const struct ovs_cmdl_parsed_option *parsed_options, size_t n)
+{
+    return has_option(parsed_options, n, OPT_DETACH);
+}
+
+static char * OVS_WARN_UNUSED_RESULT
+add_local_option(const char *name, const char *arg,
+                 struct shash *local_options)
+{
+    char *full_name = xasprintf("--%s", name);
+    if (shash_find(local_options, full_name)) {
+        char *error = xasprintf("'%s' option specified multiple times",
+                                full_name);
+        free(full_name);
+        return error;
+    }
+    shash_add_nocopy(local_options, full_name, nullable_xstrdup(arg));
+    return NULL;
+}
+
+static void
+apply_options_direct(const struct ovs_cmdl_parsed_option *parsed_options,
+                     size_t n, struct shash *local_options)
+{
+    for (const struct ovs_cmdl_parsed_option *po = parsed_options;
+         po < &parsed_options[n]; po++) {
         bool handled;
-        error = handle_main_loop_option(po->o->val, po->arg, &handled);
+        char *error = handle_main_loop_option(po->o->val, po->arg, &handled);
         if (error) {
             ctl_fatal("%s", error);
         }
@@ -403,13 +484,10 @@ parse_options(int argc, char *argv[], struct shash *local_options)
             break;
 
         case OPT_LOCAL:
-            if (shash_find(local_options, po->o->name)) {
+            error = add_local_option(po->o->name, po->arg, local_options);
-                ctl_fatal("'%s' option specified multiple times",
+            if (error) {
-                            po->o->name);
+                ctl_fatal("%s", error);
             }
-            shash_add_nocopy(local_options,
-                             xasprintf("--%s", po->o->name),
-                             nullable_xstrdup(po->arg));
             break;
 
         case 'h':
@@ -421,9 +499,17 @@ parse_options(int argc, char *argv[], struct shash *local_options)
             /* fall through */
 
         case OPT_OPTIONS:
-            ctl_print_options(global_long_options);
+            ctl_print_options(get_all_options());
             /* fall through */
 
+        case OPT_LEADER_ONLY:
+            leader_only = true;
+            break;
+
+        case OPT_NO_LEADER_ONLY:
+            leader_only = false;
+            break;
+
         case 'V':
             ovs_print_version(0, 0);
             printf("DB Schema %s\n", nbrec_get_db_version());
@@ -448,16 +534,10 @@ parse_options(int argc, char *argv[], struct shash *local_options)
             break;
         }
     }
-    free(parsed_options);
 
     if (!db) {
         db = default_nb_db();
     }
-
-    for (i = n_global_long_options; options[i].name; i++) {
-        free(CONST_CAST(char *, options[i].name));
-    }
-    free(options);
 }
 
 static void
@@ -641,6 +721,10 @@ Other options:\n\
     exit(EXIT_SUCCESS);
 }
 
+/* One should not use ctl_fatal() within commands because it will kill the
+ * daemon if we're in daemon mode.  Use ctl_error() instead and return
+ * gracefully.  */
+#define ctl_fatal dont_use_ctl_fatal_use_ctl_error_and_return
 
 /* Find a logical router given its id. */
 static char * OVS_WARN_UNUSED_RESULT
@@ -2368,35 +2452,41 @@ nbctl_meter_add(struct ctl_context *ctx)
     const char *name = ctx->argv[1];
     NBREC_METER_FOR_EACH (meter, ctx->idl) {
         if (!strcmp(meter->name, name)) {
-            ctl_fatal("meter with name \"%s\" already exists", name);
+            ctl_error(ctx, "meter with name \"%s\" already exists", name);
+            return;
         }
     }
 
     if (!strncmp(name, "__", 2)) {
-        ctl_fatal("meter names that begin with \"__\" are reserved");
+        ctl_error(ctx, "meter names that begin with \"__\" are reserved");
+        return;
     }
 
     const char *action = ctx->argv[2];
     if (strcmp(action, "drop")) {
-        ctl_fatal("action must be \"drop\"");
+        ctl_error(ctx, "action must be \"drop\"");
+        return;
     }
 
     int64_t rate;
     if (!ovs_scan(ctx->argv[3], "%"SCNd64, &rate)
         || rate < 1 || rate > UINT32_MAX) {
-        ctl_fatal("rate must be in the range 1...4294967295");
+        ctl_error(ctx, "rate must be in the range 1...4294967295");
+        return;
     }
 
     const char *unit = ctx->argv[4];
     if (strcmp(unit, "kbps") && strcmp(unit, "pktps")) {
-        ctl_fatal("unit must be \"kbps\" or \"pktps\"");
+        ctl_error(ctx, "unit must be \"kbps\" or \"pktps\"");
+        return;
     }
 
     int64_t burst = 0;
     if (ctx->argc > 5) {
         if (!ovs_scan(ctx->argv[5], "%"SCNd64, &burst)
             || burst < 0 || burst > UINT32_MAX) {
-            ctl_fatal("burst must be in the range 0...4294967295");
+            ctl_error(ctx, "burst must be in the range 0...4294967295");
+            return;
         }
     }
 
@@ -4965,6 +5055,10 @@ nbctl_cmd_init(void)
     ctl_init(&nbrec_idl_class, nbrec_table_classes, tables, NULL, nbctl_exit);
     ctl_register_commands(nbctl_commands);
 }
+
+/* Server implementation. */
+
+#undef ctl_fatal
 
 static const struct option *
 find_option_by_value(const struct option *options, int value)
@@ -5021,14 +5115,10 @@ server_parse_options(int argc, char *argv[], struct shash *local_options,
 
         switch (c) {
         case OPT_LOCAL:
-            if (shash_find(local_options, options[idx].name)) {
+            error = add_local_option(options[idx].name, optarg, local_options);
-                error = xasprintf("'%s' option specified multiple times",
+            if (error) {
-                                  options[idx].name);
                 goto out;
             }
-            shash_add_nocopy(local_options,
-                             xasprintf("--%s", options[idx].name),
-                             nullable_xstrdup(optarg));
             break;
 
         VLOG_OPTION_HANDLERS
@@ -5177,7 +5267,7 @@ 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);
+    unixctl_command_register("run", "", 0, INT_MAX, server_cmd_run, idl);
 }
 
 static void
@@ -5192,6 +5282,8 @@ server_loop(struct ovsdb_idl *idl)
         ctl_fatal("failed to create unixctl server (%s)",
                   ovs_retval_to_string(error));
     }
+    puts(unixctl_server_get_path(server));
+    fflush(stdout);
     server_cmd_init(idl, &exiting);
 
     for (;;) {
@@ -5217,3 +5309,113 @@ server_loop(struct ovsdb_idl *idl)
 
     unixctl_server_destroy(server);
 }
+
+static void
+nbctl_client(const char *socket_name,
+             const struct ovs_cmdl_parsed_option *parsed_options, size_t n,
+             int argc, char *argv[])
+{
+    struct svec args = SVEC_EMPTY_INITIALIZER;
+
+    for (const struct ovs_cmdl_parsed_option *po = parsed_options;
+         po < &parsed_options[n]; po++) {
+        optarg = po->arg;
+        switch (po->o->val) {
+        case OPT_DB:
+            VLOG_WARN("not using ovn-nbctl daemon because of %s option",
+                      po->o->name);
+            svec_destroy(&args);
+            return;
+
+        case OPT_NO_SYSLOG:
+            vlog_set_levels(&this_module, VLF_SYSLOG, VLL_WARN);
+            break;
+
+        case 'h':
+            usage();
+            exit(EXIT_SUCCESS);
+
+        case OPT_COMMANDS:
+            ctl_print_commands();
+            /* fall through */
+
+        case OPT_OPTIONS:
+            ctl_print_options(get_all_options());
+            /* fall through */
+
+        case OPT_LEADER_ONLY:
+        case OPT_NO_LEADER_ONLY:
+        case OPT_BOOTSTRAP_CA_CERT:
+        STREAM_SSL_CASES
+        DAEMON_OPTION_CASES
+            VLOG_INFO("using ovn-nbctl daemon, ignoring %s option",
+                      po->o->name);
+            break;
+
+        case 'V':
+            ovs_print_version(0, 0);
+            printf("DB Schema %s\n", nbrec_get_db_version());
+            exit(EXIT_SUCCESS);
+
+        case 't':
+            timeout = strtoul(po->arg, NULL, 10);
+            if (timeout < 0) {
+                ctl_fatal("value %s on -t or --timeout is invalid", po->arg);
+            }
+            break;
+
+        VLOG_OPTION_HANDLERS
+        TABLE_OPTION_HANDLERS(&table_style)
+
+        case OPT_LOCAL:
+        default:
+            if (po->arg) {
+                svec_add_nocopy(&args,
+                                xasprintf("--%s=%s", po->o->name, po->arg));
+            } else {
+                svec_add_nocopy(&args, xasprintf("--%s", po->o->name));
+            }
+            break;
+        }
+    }
+    svec_add(&args, "--");
+    for (int i = optind; i < argc; i++) {
+        svec_add(&args, argv[i]);
+    }
+
+    if (timeout) {
+        time_alarm(timeout);
+    }
+
+    struct jsonrpc *client;
+    int error = unixctl_client_create(socket_name, &client);
+    if (error) {
+        ctl_fatal("%s: could not connect to ovn-nb daemon (%s); "
+                  "unset OVN_NB_DAEMON to avoid using daemon",
+                  socket_name, ovs_strerror(error));
+    }
+
+    char *cmd_result;
+    char *cmd_error;
+    error = unixctl_client_transact(client, "run",
+                                    args.n, args.names,
+                                    &cmd_result, &cmd_error);
+    if (error) {
+        ctl_fatal("%s: transaction error (%s)",
+                  socket_name, ovs_strerror(error));
+    }
+    svec_destroy(&args);
+
+    int exit_status;
+    if (cmd_error) {
+        exit_status = EXIT_FAILURE;
+        fprintf(stderr, "%s: %s", program_name, cmd_error);
+    } else {
+        exit_status = EXIT_SUCCESS;
+        fputs(cmd_result, stdout);
+    }
+    free(cmd_result);
+    free(cmd_error);
+    jsonrpc_close(client);
+    exit(exit_status);
+}

tests/ovn-nbctl.at

@@ -1,30 +1,59 @@
 AT_BANNER([ovn-nbctl])
 
+OVS_START_SHELL_HELPERS
 # OVN_NBCTL_TEST_START
 m4_define([OVN_NBCTL_TEST_START],
-  [dnl Create ovn-nb database.
+  [AT_KEYWORDS([ovn])
-   AT_KEYWORDS([ovn])
+   AT_CAPTURE_FILE([ovsdb-server.log])
+   ovn_nbctl_test_start $1])
+ovn_nbctl_test_start() {
+   dnl Create ovn-nb database.
    AT_CHECK([ovsdb-tool create ovn-nb.db $abs_top_srcdir/ovn/ovn-nb.ovsschema])
 
    dnl Start ovsdb-server.
    AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file --remote=punix:$OVS_RUNDIR/ovnnb_db.sock ovn-nb.db], [0], [], [stderr])
    on_exit "kill `cat ovsdb-server.pid`"
+   AS_CASE([$1],
+     [daemon],
+       [export OVN_NB_DAEMON=$(ovn-nbctl --pidfile --detach --log-file -vsocket_util:off)
+        on_exit "kill `cat ovn-nbctl.pid`"],
+     [direct], [],
+     [*], [AT_FAIL_IF(:)])
    AT_CHECK([ovn-nbctl init])
    AT_CHECK([[sed < stderr '
 /vlog|INFO|opened log file/d
 /ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d']])
-   AT_CAPTURE_FILE([ovsdb-server.log])
+}
-])
 
 # OVN_NBCTL_TEST_STOP
-m4_define([OVN_NBCTL_TEST_STOP],
+m4_define([OVN_NBCTL_TEST_STOP], [ovn_nbctl_test_stop])
-  [AT_CHECK([check_logs "$1"])
+ovn_nbctl_test_stop() {
-   OVS_APP_EXIT_AND_WAIT([ovsdb-server])])
+   AT_CHECK([check_logs "$1"])
+   OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+}
+OVS_END_SHELL_HELPERS
 
+# OVN_NBCTL_TEST(NAME, TITLE, COMMANDS)
+m4_define([OVN_NBCTL_TEST],
+   [OVS_START_SHELL_HELPERS
+    $1() {
+      $3
+    }
+    OVS_END_SHELL_HELPERS
 
-AT_SETUP([ovn-nbctl - basic switch commands])
+    AT_SETUP([ovn-nbctl - $2 - direct])
-OVN_NBCTL_TEST_START
+    OVN_NBCTL_TEST_START direct
+    $1
+    OVN_NBCTL_TEST_STOP
+    AT_CLEANUP
 
+    AT_SETUP([ovn-nbctl - $2 - daemon])
+    OVN_NBCTL_TEST_START daemon
+    $1
+    OVN_NBCTL_TEST_STOP
+    AT_CLEANUP])
+
+OVN_NBCTL_TEST([ovn_nbctl_basic_switch], [basic switch commands], [
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl ls-list | uuidfilt], [0], [dnl
 <0> (ls0)
@@ -73,16 +102,11 @@ AT_CHECK([ovn-nbctl --add-duplicate ls-add], [1], [],
 ])
 AT_CHECK([ovn-nbctl --may-exist ls-add], [1], [],
   [ovn-nbctl: --may-exist requires specifying a name
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - basic logical switch port commands])
+OVN_NBCTL_TEST([ovn_nbctl_basic_lsp], [basic logical switch port commands], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl lsp-add ls0 lp0])
 AT_CHECK([ovn-nbctl lsp-add ls0 lp0], [1], [],
@@ -125,16 +149,11 @@ AT_CHECK([ovn-nbctl --may-exist lsp-add ls0 lp2 lp3 10], [1], [],
 AT_CHECK([ovn-nbctl clear Logical_Switch_Port lp2 tag_request])
 AT_CHECK([ovn-nbctl --may-exist lsp-add ls0 lp2 lp3 5], [1], [],
   [ovn-nbctl: lp2: port already exists but has no tag_request
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - lport addresses])
+OVN_NBCTL_TEST([ovn_nbctl_lport_addresses], [lport addresses], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl lsp-add ls0 lp0])
 AT_CHECK([ovn-nbctl lsp-get-addresses lp0], [0], [dnl
@@ -148,16 +167,11 @@ unknown
 
 AT_CHECK([ovn-nbctl lsp-set-addresses lp0])
 AT_CHECK([ovn-nbctl lsp-get-addresses lp0], [0], [dnl
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - port security])
+OVN_NBCTL_TEST([ovn_nbctl_port_security], [port security], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl lsp-add ls0 lp0])
 AT_CHECK([ovn-nbctl lsp-get-addresses lp0], [0], [dnl
@@ -171,15 +185,13 @@ aa:bb:cc:dd:ee:ff
 
 AT_CHECK([ovn-nbctl lsp-set-port-security lp0])
 AT_CHECK([ovn-nbctl lsp-get-port-security lp0], [0], [dnl
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-m4_define([OVN_NBCTL_TEST_ACL],
+OVN_NBCTL_TEST([ovn_nbctl_acls], [ACLs], [
-  [AT_CHECK([ovn-nbctl $2 --log acl-add $1 from-lport 600 udp drop])
+ovn_nbctl_test_acl() {
+   AT_CHECK([ovn-nbctl $2 --log acl-add $1 from-lport 600 udp drop])
    AT_CHECK([ovn-nbctl $2 --log --name=test --severity=info acl-add $1 to-lport 500 udp drop])
    AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 400 tcp drop])
    AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 300 tcp drop])
@@ -222,17 +234,14 @@ from-lport   200 (ip) drop
 from-lport   600 (udp) drop
 from-lport   200 (ip) drop
 ])
-])
+}
-
-AT_SETUP([ovn-nbctl - ACLs])
-OVN_NBCTL_TEST_START
 
 AT_CHECK([ovn-nbctl ls-add ls0])
-OVN_NBCTL_TEST_ACL([ls0])
+ovn_nbctl_test_acl ls0
 AT_CHECK([ovn-nbctl ls-add ls1])
-OVN_NBCTL_TEST_ACL([ls1], [--type=switch])
+ovn_nbctl_test_acl ls1 --type=switch
 AT_CHECK([ovn-nbctl create port_group name=pg0], [0], [ignore])
-OVN_NBCTL_TEST_ACL([pg0], [--type=port-group])
+ovn_nbctl_test_acl pg0 --type=port-group
 
 dnl Test when port group doesn't exist
 AT_CHECK([ovn-nbctl --type=port-group acl-add pg1 to-lport 100 ip drop], [1], [], [dnl
@@ -243,16 +252,11 @@ dnl Test when same name exists in logical switches and portgroups
 AT_CHECK([ovn-nbctl create port_group name=ls0], [0], [ignore])
 AT_CHECK([ovn-nbctl acl-add ls0 to-lport 100 ip drop], [1], [], [stderr])
 AT_CHECK([grep 'exists in both' stderr], [0], [ignore])
-AT_CHECK([ovn-nbctl --type=port-group acl-add ls0 to-lport 100 ip drop], [0], [ignore])
+AT_CHECK([ovn-nbctl --type=port-group acl-add ls0 to-lport 100 ip drop], [0], [ignore])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - QoS])
+OVN_NBCTL_TEST([ovn_nbctl_qos], [QoS], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 tcp dscp=63])
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 500 udp rate=100 burst=1000])
@@ -317,16 +321,11 @@ AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip dscpa=-1], [1], [],
 
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip burst=123], [1], [],
 [ovn-nbctl: Either "rate" and/or "dscp" must be specified
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - Meters])
+OVN_NBCTL_TEST([ovn_nbctl_meters], [meters], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl meter-add meter1 drop 10 kbps])
 AT_CHECK([ovn-nbctl meter-add meter2 drop 3 kbps 2])
 AT_CHECK([ovn-nbctl meter-add meter3 drop 100 kbps 200])
@@ -384,15 +383,11 @@ meter4: bands:
 dnl Delete all meters.
 AT_CHECK([ovn-nbctl meter-del])
 AT_CHECK([ovn-nbctl meter-list], [0], [dnl
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - NATs])
+OVN_NBCTL_TEST([ovn_nbctl_nats], [NATs], [
-OVN_NBCTL_TEST_START
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl lr-nat-add lr0 snatt 30.0.0.2 192.168.1.2], [1], [],
 [ovn-nbctl: snatt: type must be one of "dnat", "snat" and "dnat_and_snat".
@@ -537,15 +532,11 @@ snat             30.0.0.1           192.168.1.0/24
 AT_CHECK([ovn-nbctl lr-nat-del lr0])
 AT_CHECK([ovn-nbctl lr-nat-list lr0], [0], [])
 AT_CHECK([ovn-nbctl lr-nat-del lr0])
-AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat])
+AT_CHECK([ovn-nbctl lr-nat-del lr0 dnat])])
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - LBs])
+OVN_NBCTL_TEST([ovn_nbctl_lbs], [LBs], [
-OVN_NBCTL_TEST_START
-
 dnl Add two LBs.
 AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
 [ovn-nbctl: 30.0.0.10:80a: should be an IP address (or an IP address and a port number with : as a separator).
@@ -761,16 +752,11 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
 AT_CHECK([ovn-nbctl lr-lb-del lr0])
-AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
 
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
-dnl
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - LBs IPv6])
+OVN_NBCTL_TEST([ovn_nbctl_lbs_ipv6], [LBs IPv6], [
-OVN_NBCTL_TEST_START
-
 dnl A bunch of commands that should fail
 AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
 [ovn-nbctl: [[ae0f::10]]:80a: should be an IP address (or an IP address and a port number with : as a separator).
@@ -1025,15 +1011,11 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
 AT_CHECK([ovn-nbctl lr-lb-del lr0])
-AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
 
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - basic logical router commands])
+OVN_NBCTL_TEST([ovn_nbctl_basic_lr], [basic logical router commands], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl lr-list | uuidfilt], [0], [dnl
 <0> (lr0)
@@ -1082,16 +1064,11 @@ AT_CHECK([ovn-nbctl --add-duplicate lr-add], [1], [],
 ])
 AT_CHECK([ovn-nbctl --may-exist lr-add], [1], [],
   [ovn-nbctl: --may-exist requires specifying a name
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - basic logical router port commands])
+OVN_NBCTL_TEST([ovn_nbctl_basic_lrp], [basic logical router port commands], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02 192.168.1.1/24], [1], [],
   [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02
@@ -1157,15 +1134,11 @@ AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/2
   [ovn-nbctl: lrp1: port already exists with different network
 ])
 
-AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 10.0.0.1/24 192.168.1.1/24 peer=lrp1-peer])
+AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 10.0.0.1/24 192.168.1.1/24 peer=lrp1-peer])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - logical router port gateway chassis])
+OVN_NBCTL_TEST([ovn_nbctl_lrp_gw_chassi], [logical router port gateway chassis], [
-OVN_NBCTL_TEST_START
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
 AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [])
@@ -1221,16 +1194,11 @@ AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl
 lrp0-chassis2    10
 lrp0-chassis3     5
 lrp0-chassis1     1
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - logical router port enable and disable])
+OVN_NBCTL_TEST([ovn_nbctl_lrp_enable], [logical router port enable and disable], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
 AT_CHECK([ovn-nbctl lrp-get-enabled lrp0], [0], [enabled
@@ -1246,16 +1214,11 @@ AT_CHECK([ovn-nbctl lrp-get-enabled lrp0], [0], [enabled
 
 AT_CHECK([ovn-nbctl lrp-set-enabled lrp0 xyzzy], [1], [],
   [ovn-nbctl: xyzzy: state must be "enabled" or "disabled"
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - routes])
+OVN_NBCTL_TEST([ovn_nbctl_routes], [routes], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl lr-add lr0])
 
 dnl Check IPv4 routes
@@ -1364,16 +1327,11 @@ IPv6 Routes
             2001:db8::/64        2001:db8:0:f102::1 dst-ip lp0
           2001:db8:1::/64        2001:db8:0:f103::1 dst-ip
                      ::/0        2001:db8:0:f101::1 dst-ip
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - lsp types])
+OVN_NBCTL_TEST([ovn_nbctl_lsp_types], [lsp types], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl lsp-add ls0 lp0])
 
@@ -1437,30 +1395,20 @@ dnl Empty string should work too
 AT_CHECK([ovn-nbctl lsp-set-type lp0 ""])
 AT_CHECK([ovn-nbctl lsp-get-type lp0], [0], [dnl
 
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - connection])
+OVN_NBCTL_TEST([ovn_nbctl_connection], [connection], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl --inactivity-probe=30000 set-connection ptcp:6641:127.0.0.1 punix:$OVS_RUNDIR/ovnnb_db.sock])
 AT_CHECK([ovn-nbctl list connection | grep inactivity_probe], [0], [dnl
 inactivity_probe    : 30000
 inactivity_probe    : 30000
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - dry run mode])
+OVN_NBCTL_TEST([ovn_nbctl_dry_run_mode], [dry run mode], [
-OVN_NBCTL_TEST_START
-
 dnl Check that dry run has no permanent effect.
 AT_CHECK([ovn-nbctl --dry-run ls-add ls0 -- ls-list | uuidfilt], [0], [dnl
 <0> (ls0)
@@ -1472,16 +1420,11 @@ dnl Check that dry-run mode is not sticky.
 AT_CHECK([ovn-nbctl ls-add ls0])
 AT_CHECK([ovn-nbctl ls-list | uuidfilt], [0], [dnl
 <0> (ls0)
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - oneline output])
+OVN_NBCTL_TEST([ovn_nbctl_oneline_output], [oneline output], [
-OVN_NBCTL_TEST_START
-
 AT_CHECK([ovn-nbctl ls-add ls0 -- ls-add ls1])
 
 dnl Expect one line for one command.
@@ -1493,16 +1436,11 @@ dnl Expect lines for two commands.
 AT_CHECK([ovn-nbctl --oneline ls-list -- ls-list | uuidfilt], [0], [dnl
 <0> (ls0)\n<1> (ls1)
 <0> (ls0)\n<1> (ls1)
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - commands parser error paths])
+OVN_NBCTL_TEST([ovn_nbctl_error_paths], [commands parser error paths], [
-OVN_NBCTL_TEST_START
-
 dnl FIXME: Duplicate options are allowed when passed with global options.
 dnl        For example: ovn-nbctl --if-exists --if-exists list Logical_Switch
 
@@ -1569,21 +1507,13 @@ AT_CHECK([ovn-nbctl show foo --bar], [1], [], [stderr])
 AT_CHECK([grep 'command takes at most .* arguments' stderr], [0], [ignore])
 
 AT_CHECK([ovn-nbctl -- show foo --bar], [1], [], [stderr])
-AT_CHECK([grep 'command takes at most .* arguments' stderr], [0], [ignore])
+AT_CHECK([grep 'command takes at most .* arguments' stderr], [0], [ignore])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - Port Groups])
+OVN_NBCTL_TEST([ovn_nbctl_port_groups], [port groups], [
-OVN_NBCTL_TEST_START
-
 dnl Check that port group can be looked up by name
 AT_CHECK([ovn-nbctl create Port_Group name=pg0], [0], [ignore])
 AT_CHECK([ovn-nbctl get Port_Group pg0 name], [0], [dnl
 "pg0"
-])
+])])
-
-OVN_NBCTL_TEST_STOP
-AT_CLEANUP