| Message ID | 20230704232928.2209346-1-fbl@sysclose.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3] ovs-vsctl: Exit with error if postdb checks report errors. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Flavio Leitner <fbl@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 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 <fbl@sysclose.org> > --- Still looks good to me. Acked-by: Aaron Conole <aconole@redhat.com>
On 5 Jul 2023, at 1: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 <fbl@sysclose.org> Thanks Flavio for the patch. I verified the Windows part trough AppVeyor. Acked-by: Eelco Chaudron <echaudro@redhat.com>
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 <fbl@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; > } > }
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. - 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]) +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). .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 *); /* 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. + * * 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; @@ -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; } }
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 <fbl@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(-)