| Message ID | 20230630142519.945822-1-fbl@sysclose.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] 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 | fail | github build: failed |
| 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 '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 <fbl@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 <aconole@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; > } > }
On 6/30/23 12:31, Aaron Conole wrote: > 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 '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 <fbl@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 <aconole@redhat.com> Thanks for reviewing and testing it. fbl > >> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 6/30/23 18:33, Flavio Leitner wrote: > > On 6/30/23 12:31, Aaron Conole wrote: >> 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 '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 <fbl@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 <aconole@redhat.com> > > Thanks for reviewing and testing it. > > fbl I didn't look very closely at a patch, but it does fail the Windows build unfortunately: utilities\ovs-vsctl.c(28): fatal error C1083: Cannot open include file: 'sysexits.h': No such file or directory Please, take a look. You should be able to setup appveyor account for yourself for testing. It has a free tier. Best regards, Ilya Maximets.
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; } }
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 <fbl@sysclose.org> --- 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(-)