Flavio Leitner <[email protected]> writes:
> On 6/26/23 16:48, Aaron Conole wrote:
>> Flavio Leitner <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
