Re: [ovs-dev] [PATCH v3] ovs-vsctl: Exit with error if postdb checks report errors.
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 > --- > > 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 < 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 < 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
Re: [ovs-dev] [PATCH v3] ovs-vsctl: Exit with error if postdb checks report errors.
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 Thanks Flavio for the patch. I verified the Windows part trough AppVeyor. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] 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 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 > --- Still looks good to me. Acked-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] 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 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 --- 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. - 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 >experrexperr < +#define EXIT_POSTDB_ERROR ERROR_BAD_ARGUMENTS +#else +#include +#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 !=
