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_CLEANUP > AT_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? > 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 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; > @@ -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
