Re: [PATCH v2] network: make openvswitch call timeout compile time configurable

[Laine Stump <laine@xxxxxxxxx>]

On 01/24/2017 10:53 AM, Boris Fiuczynski wrote:
> Since a successful completion of the calls to openvswitch is expected
> a long timeout should be chosen to account for heavily loaded systems.
> Therefore this patch increases the timeout value from 5 to 120 seconds
> as default value and also allows to set the openvswitch timeout value
> by specifying with-ovs-timeout when running configure.

Why make it configurable during build? I don't think we do this with any 
other type of timeout value or limit. If you think it may need to change 
based on circumstances, why not just put it in libvirtd.conf and be done 
with it?

In the meantime, I agree with Michal that any machine that takes 120 
seconds to get a response from any ovs command is beyond the limits of 
usable; we certainly shouldn't cater our defaults to that.


> Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxxxxxxx>
> ---
>   configure.ac                    |  3 +++
>   m4/virt-ovs-timeout.m4          | 34 ++++++++++++++++++++++++++++++++++
>   src/util/virnetdevopenvswitch.c |  9 +++++----
>   3 files changed, 42 insertions(+), 4 deletions(-)
>   create mode 100644 m4/virt-ovs-timeout.m4
>
> diff --git a/configure.ac b/configure.ac
> index 7efaddb..e70dfdc 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -463,6 +463,7 @@ LIBVIRT_ARG_LOADER_NVRAM
>   LIBVIRT_ARG_LOGIN_SHELL
>   LIBVIRT_ARG_HOST_VALIDATE
>   LIBVIRT_ARG_TLS_PRIORITY
> +LIBVIRT_ARG_OVS_TIMEOUT
>   LIBVIRT_ARG_SYSCTL_CONFIG
>   
>   
> @@ -477,6 +478,7 @@ LIBVIRT_CHECK_LOADER_NVRAM
>   LIBVIRT_CHECK_LOGIN_SHELL
>   LIBVIRT_CHECK_HOST_VALIDATE
>   LIBVIRT_CHECK_TLS_PRIORITY
> +LIBVIRT_CHECK_OVS_TIMEOUT
>   LIBVIRT_CHECK_SYSCTL_CONFIG
>   LIBVIRT_CHECK_NSS
>   
> @@ -1012,6 +1014,7 @@ LIBVIRT_RESULT_LOADER_NVRAM
>   LIBVIRT_RESULT_LOGIN_SHELL
>   LIBVIRT_RESULT_HOST_VALIDATE
>   LIBVIRT_RESULT_TLS_PRIORITY
> +LIBVIRT_RESULT_OVS_TIMEOUT
>   AC_MSG_NOTICE([])
>   AC_MSG_NOTICE([Developer Tools])
>   AC_MSG_NOTICE([])
> diff --git a/m4/virt-ovs-timeout.m4 b/m4/virt-ovs-timeout.m4
> new file mode 100644
> index 0000000..a5174d5
> --- /dev/null
> +++ b/m4/virt-ovs-timeout.m4
> @@ -0,0 +1,34 @@
> +dnl The OVS timeout check
> +dnl
> +dnl Copyright (C) 2017 IBM Corporation
> +dnl
> +dnl This library is free software; you can redistribute it and/or
> +dnl modify it under the terms of the GNU Lesser General Public
> +dnl License as published by the Free Software Foundation; either
> +dnl version 2.1 of the License, or (at your option) any later version.
> +dnl
> +dnl This library is distributed in the hope that it will be useful,
> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +dnl Lesser General Public License for more details.
> +dnl
> +dnl You should have received a copy of the GNU Lesser General Public
> +dnl License along with this library.  If not, see
> +dnl <http://www.gnu.org/licenses/>.
> +dnl
> +
> +AC_DEFUN([LIBVIRT_ARG_OVS_TIMEOUT], [
> +  LIBVIRT_ARG_WITH([OVS_TIMEOUT],
> +                   [set the default OVS timeout],
> +                   120)
> +])
> +
> +AC_DEFUN([LIBVIRT_CHECK_OVS_TIMEOUT], [
> +  AC_DEFINE_UNQUOTED([OVS_TIMEOUT], ["$with_ovs_timeout"],
> +                     [default OVS timeout])
> +])
> +
> +AC_DEFUN([LIBVIRT_RESULT_OVS_TIMEOUT], [
> +dnl  AC_MSG_NOTICE([OVS timeout: $with_ovs_timeout])
> +  LIBVIRT_RESULT([       OVS timeout], [$with_ovs_timeout])
> +])
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index e6cb096..ba44b3b 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -35,6 +35,7 @@
>   #include "virlog.h"
>   
>   #define VIR_FROM_THIS VIR_FROM_NONE
> +#define OVS_TIMEOUT_ARG "--timeout=" OVS_TIMEOUT
>   
>   VIR_LOG_INIT("util.netdevopenvswitch");
>   
> @@ -89,7 +90,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>   
>       cmd = virCommandNew(OVSVSCTL);
>   
> -    virCommandAddArgList(cmd, "--timeout=5", "--", "--if-exists", "del-port",
> +    virCommandAddArgList(cmd, OVS_TIMEOUT_ARG, "--", "--if-exists", "del-port",
>                            ifname, "--", "add-port", brname, ifname, NULL);
>   
>       if (virtVlan && virtVlan->nTags > 0) {
> @@ -183,7 +184,7 @@ int virNetDevOpenvswitchRemovePort(const char *brname ATTRIBUTE_UNUSED, const ch
>       virCommandPtr cmd = NULL;
>   
>       cmd = virCommandNew(OVSVSCTL);
> -    virCommandAddArgList(cmd, "--timeout=5", "--", "--if-exists", "del-port", ifname, NULL);
> +    virCommandAddArgList(cmd, OVS_TIMEOUT_ARG, "--", "--if-exists", "del-port", ifname, NULL);
>   
>       if (virCommandRun(cmd, NULL) < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -212,7 +213,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
>       size_t len;
>       int ret = -1;
>   
> -    cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "--if-exists", "get", "Interface",
> +    cmd = virCommandNewArgList(OVSVSCTL, OVS_TIMEOUT_ARG, "--if-exists", "get", "Interface",
>                                  ifname, "external_ids:PortData", NULL);
>   
>       virCommandSetOutputBuffer(cmd, migrate);
> @@ -255,7 +256,7 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname)
>           return 0;
>       }
>   
> -    cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
> +    cmd = virCommandNewArgList(OVSVSCTL, OVS_TIMEOUT_ARG, "set",
>                                  "Interface", ifname, NULL);
>       virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
>   


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list