On 7/5/23 01:29, Flavio Leitner 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://bugzilla.redhat.com/1731553
> Signed-off-by: Flavio Leitner <f...@sysclose.org>
> ---
>
> 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 | 30 ++++++++++++++++++++++++++++--
> tests/ovs-vswitchd.at | 6 +++++-
> tests/tunnel.at | 8 +++++++-
> utilities/ovs-vsctl.8.in | 4 ++++
> utilities/ovs-vsctl.c | 35 +++++++++++++++++++++++++++++------
> 6 files changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 6a990c921..8c733e417 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -30,6 +30,11 @@ 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 160 (ERROR_BAD_ARGUMENTS) on Windows or
> + 65 (EX_DATAERR) on other platforms if errors were reported while
> + checking if OVSDB transactions are successfully recorded and reload
> + by ovs-vswitchd.
This description is confusing to me and will likely be confusing
for OVS users. 'if errors were reported while checking if OVSDB
transactions are successfully recorded' is not correct, because the
database transaction already succeeded (i.e. recorded) at the moment
the check is happening, so the data is already successfully written
into the database. The 'reload by ovs-vswitchd' is not defined,
i.e. it's not clear what it means.
The man page is using the 'waiting for ovs-vswitchd to reconfigure
itself' or 'waits until ovs-vswitchd has finished reconfiguring
itself' wording. We should probably base the description on similar
terms.
> - 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..a8274734f 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1522,7 +1522,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
> @@ -1560,7 +1564,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
> @@ -1737,3 +1745,21 @@ 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])
Would be better to wrap these long lines.
> +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 977b2eba1..8fcfc6ec1 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 ddeb66bc9..5550d298d 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -1008,9 +1008,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 9e319aa1c..4e2c96710 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -892,6 +892,10 @@ 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 (Linux and BSD).
> +.IP "160"
> +An error has been reported post OVSDB reload (Windows).
Same comment here. The 'OVSDB reload' in an undefined term, it's
unclear what it means. We should use words similar to how the
--no-wait option is described. We should also mention that the
change actually stays applied in the database and not rolled back.
And we should describe the relationship between this error and
the use of --no-wait option.
Since there will be more text here, we may, I guess, combine the
.IP blocks into one in order to avoid duplication. E.g.,
.IP "65 (Linux and BSD) or 160 (Windows)"
> .SH "SEE ALSO"
> .
> .BR ovsdb\-server (1),
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 2f5ac1a26..fcc01e8ad 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 *);
Please add a name for the boolean argument. Name is necessary for
generic type arguments in order to understand the meaning.
>
> /* 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_check returns 'true' if a failure is reported.
s/post_db_reload_check/post_db_reload_do_checks/ ?
> + *
> * 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);
> }
> }
> @@ -2674,7 +2691,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;
Should this variable be renamed?
> @@ -2707,6 +2724,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 +2834,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 +2846,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 +3010,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
