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 '65'
> (EX_DATAERR) 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>
> ---
LGTM. I did a quick double check for FreeBSD and Mac OS X, and the
error code is the same value as on linux systems.
I don't have a windows machine to test with and unfortunately the robot
doesn't build series_* branches on appveyor (maybe something to look
at). We may need a workaround for windows - but I'll let Alin take a
look.
Acked-by: Aaron Conole <acon...@redhat.com>
> v2:
> Followed Aaron's suggestion to return EX_DATAERR.
>
> NEWS | 4 ++++
> tests/ovs-vsctl.at | 19 +++++++++++++++++--
> tests/ovs-vswitchd.at | 2 +-
> tests/tunnel.at | 2 +-
> utilities/ovs-vsctl.8.in | 2 ++
> utilities/ovs-vsctl.c | 30 ++++++++++++++++++++++++------
> 6 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 66d5a4ea3..cb148a09f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -28,6 +28,10 @@ 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 65 (EX_DATAERR) 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..b282798cc 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], [65], [], [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], [65], [], [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 65 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], [65], [], [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..80a748355 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], [65],
> [], [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..d281c9e6c 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], [65],
> [], [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..7c7e7bc29 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 "65"
> +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..d7338080b 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -25,6 +25,7 @@
> #include <stdarg.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <sysexits.h>
> #include <unistd.h>
>
> #include "db-ctl-base.h"
> @@ -56,6 +57,9 @@
>
> VLOG_DEFINE_THIS_MODULE(vsctl);
>
> +/* Post OVSDB reload error reported. */
> +#define EXIT_POSTDB_ERROR EX_DATAERR
> +
> struct vsctl_context;
>
> /* --db: The database server to contact. */
> @@ -115,7 +119,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 +138,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 +206,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 +2686,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 +2719,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 +2829,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 +2841,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 +3005,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
