This patch breaks afxdp testsuite.
I am working on a fix.
Flavio
On Sat, 12 Jul 2025 19:25:57 -0300
Flavio Leitner <f...@sysclose.org> wrote:
> 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 <f...@sysclose.org>
> ---
>
> v4:
> Reworded the NEWS entry.
> Reworded the man-page change.
> Wrapped the 2 long AT_CHECK() lines.
> Added the name for the bool argument.
> Updated the comment to use post_db_reload_do_checks.
> Renamed print_error to reconfig_failed.
>
> v3: Fixed the Windows build issue reported by Ilya.
> Return ERROR_BAD_ARGUMENTS on Windows.
> v2:
> Followed Aaron's suggestion to return EX_DATAERR.
>
> NEWS | 5 +++++
> tests/ovs-vsctl.at | 32 +++++++++++++++++++++++++++++--
> tests/ovs-vswitchd.at | 6 +++++-
> tests/tunnel.at | 8 +++++++-
> utilities/ovs-vsctl.8.in | 11 +++++++++++
> utilities/ovs-vsctl.c | 41 +++++++++++++++++++++++++++++++---------
> 6 files changed, 90 insertions(+), 13 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c7bb53f2d..b5af71492 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -37,6 +37,11 @@ Post-v3.5.0
> the OVS distribution in the 3.0 release and is no longer present in
> any supported versions of OVS. The remaining documentation of this
> kernel module relates to topics for older releases of OVS.
> + - 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.
>
>
> v3.5.0 - 17 Feb 2025
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index e488e292d..ba00293db 100644
> --- a/tests/ovs-vsctl.at
> +++ b/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
> diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
> index 730363e83..173e62367 100644
> --- a/tests/ovs-vswitchd.at
> +++ b/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'])
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index 60ed8ba6a..09e3c94c7 100644
> --- a/tests/tunnel.at
> +++ b/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) diff --git a/utilities/ovs-vsctl.8.in
> b/utilities/ovs-vsctl.8.in index 575b7c0bf..e41e6364b 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/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),
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 691b974e3..3ccbcd294 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/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;
> }
> }
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
