On 6/26/23 16:48, Aaron Conole wrote:
Flavio Leitner <f...@sysclose.org> writes: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 '3' 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://bugzilla.redhat.com/1731553 Signed-off-by: Flavio Leitner <f...@sysclose.org> --- NEWS | 3 +++ tests/ovs-vsctl.at | 19 +++++++++++++++++-- tests/ovs-vswitchd.at | 2 +- tests/tunnel.at | 2 +- utilities/ovs-vsctl.8.in | 2 ++ utilities/ovs-vsctl.c | 29 +++++++++++++++++++++++------ 6 files changed, 47 insertions(+), 10 deletions(-) diff --git a/NEWS b/NEWS index cfd466663..f8f7b7655 100644 --- a/NEWS +++ b/NEWS @@ -28,6 +28,9 @@ Post-v3.1.0 * Added new options --[ovsdb-server|ovs-vswitchd]-umask=MODE to set umask value when starting OVS daemons. E.g., use --ovsdb-server-umask=0002 in order to create OVSDB sockets with access mode of 0770. + - ovs-vsctl: + * Exit with error code 3 if errors were reported while checking if OVSDB + transactions are successfully recorded and reload by ovs-vswitchd. - QoS: * Added new configuration option 'jitter' for a linux-netem QoS type. - DPDK: diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index a368bff6e..2554152df 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -1522,7 +1522,7 @@ 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]) +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [3], [], [experr]) # Prevent race. OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1]) # Detect the warning log message @@ -1560,7 +1560,7 @@ 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]) +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [3], [], [experr]) # Prevent race. OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1]) # Detect the warning log message @@ -1737,3 +1737,18 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0]OVS_VSCTL_CLEANUPAT_CLEANUP + +AT_SETUP([ovs-vsctl -- return error if OVSDB reload issues are reported]) +OVS_VSWITCHD_START +dnl check if ovs-vsctl returns error 3 if ovs-vswitchd fails to reload. + +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 +AT_CHECK([ovs-vsctl add-port br0 gre0 -- set interface gre0 type=ip6gre options:key=100 options:remote_ip=192.168.0.300], [3], [], [experr]) +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 diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at index 977b2eba1..81604111b 100644 --- a/tests/ovs-vswitchd.at +++ b/tests/ovs-vswitchd.at @@ -222,7 +222,7 @@ 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]) +AT_CHECK([ovs-vsctl add-br b/c -- set bridge b/c datapath-type=dummy], [3], [], [experr]) AT_CHECK([test ! -e b/c.mgmt])OVS_VSWITCHD_STOP(['/ignoring bridge with invalid name/d'])diff --git a/tests/tunnel.at b/tests/tunnel.at index ddeb66bc9..cf66cc085 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -1009,7 +1009,7 @@ OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ options:remote_ip=flow ofport_request=1])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], [3], [], [ignore])AT_CHECK([grep 'p2: could not set configuration (File exists)' ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0],diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in index 9e319aa1c..929285e66 100644 --- a/utilities/ovs-vsctl.8.in +++ b/utilities/ovs-vsctl.8.in @@ -892,6 +892,8 @@ 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 "3" +An error has been reported post OVSDB reload. .SH "SEE ALSO" . .BR ovsdb\-server (1), diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index 2f5ac1a26..8daa1d409 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -56,6 +56,9 @@VLOG_DEFINE_THIS_MODULE(vsctl); +/* Post OVSDB reload error reported. */+#define EXIT_POSTDB_ERROR 3 +Maybe we can use a definition from sysexits.h, like: #define EX_SOFTWARE 70 /* internal software error */ or #define EX_PROTOCOL 76 /* remote error in protocol */ WDYT? There's an effort to try and standardize the exit codes (according to https://tldp.org/LDP/abs/html/exitcodes.html and https://man.freebsd.org/cgi/man.cgi?query=sysexits&sektion=3) Maybe we should try and adopt it?
Ok, following that standard seems like a good idea to me. I am not sure about the error code though. It seems like the error below would be better:EX_DATAERR (65) The input data was incorrect in some way. This
should only be used for user's data and not system files. fbl
struct vsctl_context;/* --db: The database server to contact. */@@ -115,7 +118,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 *);/* 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 +137,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_check 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 +205,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); } } @@ -2674,7 +2685,7 @@ 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; @@ -2707,6 +2718,8 @@ post_db_reload_do_checks(const struct vsctl_context *vsctl_ctx) if (print_error) { ovs_error(0, "The default log directory is \"%s\".", ovs_logdir()); } + + return print_error; }@@ -2815,7 +2828,7 @@ vsctl_parent_process_info(void)static booldo_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; @@ -2827,6 +2840,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); @@ -2989,7 +3004,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; } }_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev