| Message ID | 20230617173426.1270451-1-fbl@sysclose.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] 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 '3' > 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> > --- > NEWS | 3 +++ > tests/ovs-vsctl.at | 19 +++++++++++++++++-- > tests/ovs-vswitchd.at | 2 +- > tests/tunnel.at | 2 +- > utilities/ovs-vsctl.8.in | 2 ++ > utilities/ovs-vsctl.c | 29 +++++++++++++++++++++++------ > 6 files changed, 47 insertions(+), 10 deletions(-) > > diff --git a/NEWS b/NEWS > index cfd466663..f8f7b7655 100644 > --- a/NEWS > +++ b/NEWS > @@ -28,6 +28,9 @@ 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 3 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..2554152df 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], [3], [], [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], [3], [], [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 3 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], [3], [], [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..81604111b 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], [3], [], [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..cf66cc085 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], [3], > [], [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..929285e66 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 "3" > +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..8daa1d409 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -56,6 +56,9 @@ > > VLOG_DEFINE_THIS_MODULE(vsctl); > > +/* Post OVSDB reload error reported. */ > +#define EXIT_POSTDB_ERROR 3 > + Maybe we can use a definition from sysexits.h, like: #define EX_SOFTWARE 70 /* internal software error */ or #define EX_PROTOCOL 76 /* remote error in protocol */ WDYT? There's an effort to try and standardize the exit codes (according to https://tldp.org/LDP/abs/html/exitcodes.html and https://man.freebsd.org/cgi/man.cgi?query=sysexits&sektion=3) Maybe we should try and adopt it? > struct vsctl_context; > > /* --db: The database server to contact. */ > @@ -115,7 +118,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 +137,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 +205,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 +2685,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 +2718,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 +2828,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 +2840,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 +3004,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/26/23 16:48, 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 '3' >> 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> >> --- >> NEWS | 3 +++ >> tests/ovs-vsctl.at | 19 +++++++++++++++++-- >> tests/ovs-vswitchd.at | 2 +- >> tests/tunnel.at | 2 +- >> utilities/ovs-vsctl.8.in | 2 ++ >> utilities/ovs-vsctl.c | 29 +++++++++++++++++++++++------ >> 6 files changed, 47 insertions(+), 10 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index cfd466663..f8f7b7655 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -28,6 +28,9 @@ 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 3 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..2554152df 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], [3], [], [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], [3], [], [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 3 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], [3], [], [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..81604111b 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], [3], [], [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..cf66cc085 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], [3], >> [], [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..929285e66 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 "3" >> +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..8daa1d409 100644 >> --- a/utilities/ovs-vsctl.c >> +++ b/utilities/ovs-vsctl.c >> @@ -56,6 +56,9 @@ >> >> VLOG_DEFINE_THIS_MODULE(vsctl); >> >> +/* Post OVSDB reload error reported. */ >> +#define EXIT_POSTDB_ERROR 3 >> + > Maybe we can use a definition from sysexits.h, like: > > #define EX_SOFTWARE 70 /* internal software error */ > or > #define EX_PROTOCOL 76 /* remote error in protocol */ > > WDYT? There's an effort to try and standardize the exit codes > (according to https://tldp.org/LDP/abs/html/exitcodes.html and > https://man.freebsd.org/cgi/man.cgi?query=sysexits&sektion=3) > > Maybe we should try and adopt it? Ok, following that standard seems like a good idea to me. I am not sure about the error code though. It seems like the error below would be better: EX_DATAERR (65) The input data was incorrect in some way. This should only be used for user's data and not system files. fbl > struct vsctl_context; >> >> /* --db: The database server to contact. */ >> @@ -115,7 +118,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 +137,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 +205,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 +2685,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 +2718,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 +2828,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 +2840,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 +3004,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
Flavio Leitner <fbl@sysclose.org> writes: > On 6/26/23 16:48, 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 '3' >>> 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> >>> --- >>> NEWS | 3 +++ >>> tests/ovs-vsctl.at | 19 +++++++++++++++++-- >>> tests/ovs-vswitchd.at | 2 +- >>> tests/tunnel.at | 2 +- >>> utilities/ovs-vsctl.8.in | 2 ++ >>> utilities/ovs-vsctl.c | 29 +++++++++++++++++++++++------ >>> 6 files changed, 47 insertions(+), 10 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index cfd466663..f8f7b7655 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -28,6 +28,9 @@ 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 3 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..2554152df 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], [3], [], [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], [3], [], [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 3 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], [3], [], [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..81604111b 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], [3], [], [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..cf66cc085 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], [3], >>> [], [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..929285e66 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 "3" >>> +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..8daa1d409 100644 >>> --- a/utilities/ovs-vsctl.c >>> +++ b/utilities/ovs-vsctl.c >>> @@ -56,6 +56,9 @@ >>> VLOG_DEFINE_THIS_MODULE(vsctl); >>> +/* Post OVSDB reload error reported. */ >>> +#define EXIT_POSTDB_ERROR 3 >>> + >> Maybe we can use a definition from sysexits.h, like: >> >> #define EX_SOFTWARE 70 /* internal software error */ >> or >> #define EX_PROTOCOL 76 /* remote error in protocol */ >> >> WDYT? There's an effort to try and standardize the exit codes >> (according to https://tldp.org/LDP/abs/html/exitcodes.html and >> https://man.freebsd.org/cgi/man.cgi?query=sysexits&sektion=3) >> >> Maybe we should try and adopt it? > > Ok, following that standard seems like a good idea to me. > > I am not sure about the error code though. It seems like > > the error below would be better: > > EX_DATAERR (65) The input data was incorrect in some > way. This > should only be used for user's data and not system > files. Okay - makes sense to me. > fbl > > > >> struct vsctl_context; >>> /* --db: The database server to contact. */ >>> @@ -115,7 +118,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 +137,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 +205,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 +2685,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 +2718,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 +2828,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 +2840,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 +3004,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
diff --git a/NEWS b/NEWS index cfd466663..f8f7b7655 100644 --- a/NEWS +++ b/NEWS @@ -28,6 +28,9 @@ 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 3 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..2554152df 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], [3], [], [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], [3], [], [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 3 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], [3], [], [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..81604111b 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], [3], [], [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..cf66cc085 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], [3], [], [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..929285e66 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 "3" +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..8daa1d409 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -56,6 +56,9 @@ VLOG_DEFINE_THIS_MODULE(vsctl); +/* Post OVSDB reload error reported. */ +#define EXIT_POSTDB_ERROR 3 + struct vsctl_context; /* --db: The database server to contact. */ @@ -115,7 +118,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 +137,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 +205,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 +2685,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 +2718,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 +2828,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 +2840,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 +3004,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 '3' 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> --- NEWS | 3 +++ tests/ovs-vsctl.at | 19 +++++++++++++++++-- tests/ovs-vswitchd.at | 2 +- tests/tunnel.at | 2 +- utilities/ovs-vsctl.8.in | 2 ++ utilities/ovs-vsctl.c | 29 +++++++++++++++++++++++------ 6 files changed, 47 insertions(+), 10 deletions(-)