Re: [ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.
On 6/30/23 18:33, Flavio Leitner wrote: > > On 6/30/23 12:31, Aaron Conole wrote: >> Flavio Leitner 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 >>> --- >> 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 > > 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.
On 6/30/23 12:31, Aaron Conole wrote: Flavio Leitner 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 --- 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 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 >experrexperr << 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 < 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
Re: [ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.
Flavio Leitner 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 > --- 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 > 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 < 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 < 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 < 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
[ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.
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 --- 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 >experrexperr < #include #include +#include #include #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, _err)) { free(args); +if (postdb_err) { +
