ovs-vsctl: Exit with error if postdb checks report errors.

Today the exit code refers to the execution of the change
in the database. However, when not using parameter --no-wait
(default), the ovs-vsctl also checks if OVSDB transactions
are successfully recorded and reload by ovs-vswitchd. In this
case, an error message is printed if there is a problem during
the reload, like for example the one below:

 # ovs-vsctl add-port br0 gre0 -- \
    set interface gre0 type=ip6gre options:key=100 \
    options:remote_ip=2001::2
 ovs-vsctl: Error detected while setting up 'gre0': could not \
 add network device gre0 to ofproto (Address family not supported\
 by protocol).  See ovs-vswitchd log for details.
 ovs-vsctl: The default log directory is "/var/log/openvswitch".
 # echo $?
 0

This patch changes to exit with specific error code 160
(ERROR_BAD_ARGUMENTS) on Windows and 65 (EX_DATAERR) on
Linux or BSD if errors were reported during the reload.

This change may break existing scripts because ovs-vsctl will
start to fail when before it was succeeding. However, if an
error is printed, then it is likely that the change was not
functional anyway.

Reported-at: https://issues.redhat.com/browse/FDP-793
Signed-off-by: Flavio Leitner <fbl@sysclose.org>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

NEWS

@@ -20,6 +20,12 @@ Post-v3.5.0
        core file size.
    - ovs-appctl:
      * Added JSON output support to the 'ovs/route/show' command.
+   - ovs-vsctl:
+     * Now exits with error code 160 (ERROR_BAD_ARGUMENTS) on Windows and
+       65 (EX_DATAERR) on other platforms if it fails while waiting for
+       ovs-vswitchd to finish reconfiguring itself after a successful
+       database transaction.  E.g., when ovs-vswitchd fails to add a new
+       port or a bridge.
    - SSL/TLS:
      * Support for deprecated TLSv1 and TLSv1.1 protocols on OpenFlow and
        database connections is now removed.

tests/ovs-vsctl.at

@@ -1620,7 +1620,11 @@ cat >experr <<EOF
 ovs-vsctl: Error detected while setting up 'reserved_name'.  See ovs-vswitchd log for details.
 ovs-vsctl: The default log directory is "$OVS_RUNDIR".
 EOF
-AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr])
+if test "$IS_WIN32" = "yes"; then
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [160], [], [experr])
+else
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [65], [], [experr])
+fi
 # Prevent race.
 OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1])
 # Detect the warning log message
@@ -1655,7 +1659,11 @@ cat >experr <<EOF
 ovs-vsctl: Error detected while setting up 'reserved_name'.  See ovs-vswitchd log for details.
 ovs-vsctl: The default log directory is "$OVS_RUNDIR".
 EOF
-AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr])
+if test "$IS_WIN32" = "yes"; then
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [160], [], [experr])
+else
+AT_CHECK([ovs-vsctl add-port br0 reserved_name], [65], [], [experr])
+fi
 # Prevent race.
 OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1])
 # Detect the warning log message
@@ -1831,3 +1839,23 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0]
 
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
+
+AT_SETUP([ovs-vsctl -- return error if OVSDB reload issues are reported])
+OVS_VSWITCHD_START
+
+cat >experr << EOF
+ovs-vsctl: Error detected while setting up 'gre0': gre0: bad ip6gre 'remote_ip'
+gre0: ip6gre type requires valid 'remote_ip' argument.  See ovs-vswitchd log for details.
+ovs-vsctl: The default log directory is "$OVS_RUNDIR".
+EOF
+if test "$IS_WIN32" = "yes"; then
+AT_CHECK([ovs-vsctl add-port br0 gre0 -- set interface gre0 type=ip6gre \
+options:key=100 options:remote_ip=192.168.0.300], [160], [], [experr])
+else
+AT_CHECK([ovs-vsctl add-port br0 gre0 -- set interface gre0 type=ip6gre \
+options:key=100 options:remote_ip=192.168.0.300], [65], [], [experr])
+fi
+OVS_VSWITCHD_STOP(["/is not a valid IP address/d
+/netdev_vport|WARN|gre0: bad ip6gre 'remote_ip'/d
+/netdev|WARN|gre0: could not set configuration/d"])
+AT_CLEANUP

tests/ovs-vswitchd.at

@@ -222,7 +222,11 @@ cat >experr <<EOF
 ovs-vsctl: Error detected while setting up 'b/c'.  See ovs-vswitchd log for details.
 ovs-vsctl: The default log directory is "$OVS_RUNDIR".
 EOF
-AT_CHECK([ovs-vsctl add-br b/c -- set bridge b/c datapath-type=dummy], [0], [], [experr])
+if test "$IS_WIN32" = "yes"; then
+AT_CHECK([ovs-vsctl add-br b/c -- set bridge b/c datapath-type=dummy], [160], [], [experr])
+else
+AT_CHECK([ovs-vsctl add-br b/c -- set bridge b/c datapath-type=dummy], [65], [], [experr])
+fi
 AT_CHECK([test ! -e b/c.mgmt])
 
 OVS_VSWITCHD_STOP(['/ignoring bridge with invalid name/d'])

tests/system-afxdp.at

@@ -12,7 +12,7 @@ ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
 AT_CHECK([ovs-vsctl del-port ovs-p0])
 AT_CHECK([ovs-vsctl add-port br0 ovs-p0 -- \
                     set interface ovs-p0 type=afxdp options:n_rxq=42],
-         [0], [], [stderr])
+         [65], [], [stderr])
 OVS_WAIT_UNTIL([grep "ovs-p0: could not set configuration" ovs-vswitchd.log])
 sleep 5
 AT_CHECK([grep "ovs-p0: could not set configuration" ovs-vswitchd.log | wc -l],

tests/tunnel.at

@@ -1041,9 +1041,15 @@ AT_SETUP([tunnel - concomitant incompatible tunnels on the same port])
 OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \
                     options:remote_ip=flow ofport_request=1])
 
+if test "$IS_WIN32" = "yes"; then
 AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \
-                    options:remote_ip=flow options:exts=gbp options:key=1 ofport_request=2], [0],
+                    options:remote_ip=flow options:exts=gbp options:key=1 ofport_request=2], [160],
   [], [ignore])
+else
+AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \
+                    options:remote_ip=flow options:exts=gbp options:key=1 ofport_request=2], [65],
+  [], [ignore])
+fi
 
 AT_CHECK([grep 'p2: could not set configuration (File exists)' ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0],
   [p2: could not set configuration (File exists)

utilities/ovs-vsctl.8.in

@@ -909,6 +909,17 @@ Usage, syntax, or configuration file error.
 .IP "2"
 The \fIbridge\fR argument to \fBbr\-exists\fR specified the name of a
 bridge that does not exist.
+.IP "65 (Linux and BSD) or 160 (Windows)"
+The database transaction was committed successfully, but \fBovs\-vsctl\fR failed
+while waiting for \fBovs\-vswitchd\fR to finish reconfiguring itself to reflect
+the changes.  This typically happens when \fB--no\-wait\fR is not specified and
+the daemon does not acknowledge the update in time or encounters an error
+applying it.
+.br
+.sp
+Note that the change remains recorded in the database and is not rolled back.
+This exit code is intended to signal that while the database was updated, the
+system configuration may not yet reflect the intended changes.
 .SH "SEE ALSO"
 .
 .BR ovsdb\-server (1),

utilities/ovs-vsctl.c

@@ -56,6 +56,15 @@
 
 VLOG_DEFINE_THIS_MODULE(vsctl);
 
+/* Post OVSDB reload error reported. */
+#ifdef _WIN32
+#include <WinError.h>
+#define EXIT_POSTDB_ERROR ERROR_BAD_ARGUMENTS
+#else
+#include <sysexits.h>
+#define EXIT_POSTDB_ERROR EX_DATAERR
+#endif
+
 struct vsctl_context;
 
 /* --db: The database server to contact. */
@@ -115,7 +124,7 @@ static void parse_options(int argc, char *argv[], struct shash *local_options);
 static void run_prerequisites(struct ctl_command[], size_t n_commands,
                               struct ovsdb_idl *);
 static bool do_vsctl(const char *args, struct ctl_command *, size_t n,
-                     struct ovsdb_idl *);
+                     struct ovsdb_idl *, bool *postdb_err);
 
 /* post_db_reload_check frame work is to allow ovs-vsctl to do additional
  * checks after OVSDB transactions are successfully recorded and reload by
@@ -134,11 +143,13 @@ static bool do_vsctl(const char *args, struct ctl_command *, size_t n,
  * Current implementation only check for Post OVSDB reload failures on new
  * interface additions with 'add-br' and 'add-port' commands.
  *
+ * post_db_reload_do_checks() returns 'true' if a failure is reported.
+ *
  * post_db_reload_expect_iface()
  *
  * keep track of interfaces to be checked post OVSDB reload. */
 static void post_db_reload_check_init(void);
-static void post_db_reload_do_checks(const struct vsctl_context *);
+static bool post_db_reload_do_checks(const struct vsctl_context *);
 static void post_db_reload_expect_iface(const struct ovsrec_interface *);
 
 static struct uuid *neoteric_ifaces;
@@ -200,9 +211,15 @@ main(int argc, char *argv[])
         }
 
         if (seqno != ovsdb_idl_get_seqno(idl)) {
+            bool postdb_err;
+
             seqno = ovsdb_idl_get_seqno(idl);
-            if (do_vsctl(args, commands, n_commands, idl)) {
+            if (do_vsctl(args, commands, n_commands, idl, &postdb_err)) {
                 free(args);
+                if (postdb_err) {
+                    exit(EXIT_POSTDB_ERROR);
+                }
+
                 exit(EXIT_SUCCESS);
             }
         }
@@ -2824,10 +2841,10 @@ post_db_reload_expect_iface(const struct ovsrec_interface *iface)
     neoteric_ifaces[n_neoteric_ifaces++] = iface->header_.uuid;
 }
 
-static void
+static bool
 post_db_reload_do_checks(const struct vsctl_context *vsctl_ctx)
 {
-    bool print_error = false;
+    bool reconfig_failed = false;
     size_t i;
 
     for (i = 0; i < n_neoteric_ifaces; i++) {
@@ -2849,14 +2866,16 @@ post_db_reload_do_checks(const struct vsctl_context *vsctl_ctx)
                                  "See ovs-vswitchd log for details.",
                               iface->name);
                 }
-                print_error = true;
+                reconfig_failed = true;
             }
         }
     }
 
-    if (print_error) {
+    if (reconfig_failed) {
         ovs_error(0, "The default log directory is \"%s\".", ovs_logdir());
     }
+
+    return reconfig_failed;
 }
 
 
@@ -2965,7 +2984,7 @@ vsctl_parent_process_info(void)
 
 static bool
 do_vsctl(const char *args, struct ctl_command *commands, size_t n_commands,
-         struct ovsdb_idl *idl)
+         struct ovsdb_idl *idl, bool *postdb_err)
 {
     struct ovsdb_idl_txn *txn;
     const struct ovsrec_open_vswitch *ovs;
@@ -2977,6 +2996,8 @@ do_vsctl(const char *args, struct ctl_command *commands, size_t n_commands,
     int64_t next_cfg = 0;
     char *ppid_info = NULL;
 
+    ovs_assert(postdb_err);
+    *postdb_err = false;
     txn = the_idl_txn = ovsdb_idl_txn_create(idl);
     if (dry_run) {
         ovsdb_idl_txn_set_dry_run(txn);
@@ -3139,7 +3160,9 @@ do_vsctl(const char *args, struct ctl_command *commands, size_t n_commands,
             ovsdb_idl_run(idl);
             OVSREC_OPEN_VSWITCH_FOR_EACH (ovs, idl) {
                 if (ovs->cur_cfg >= next_cfg) {
-                    post_db_reload_do_checks(&vsctl_ctx);
+                    if (post_db_reload_do_checks(&vsctl_ctx)) {
+                        *postdb_err = true;
+                    }
                     goto done;
                 }
             }