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

[Michal Privoznik]

On 01/24/2017 04:53 PM, 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.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> Reviewed-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
> ---
>  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)

Isn't 120 too much? It's 2 minutes. Such heavy loaded machine is not suitable for virtualization.

> +])
> +
> +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])

This doesn't need to be here.

> +  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);
>  
> 

There are other occurrences of --timeout=:

src/util/virnetdevopenvswitch.c:303:    cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5",
src/util/virnetdevopenvswitch.c:318:    cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5",
src/util/virnetdevopenvswitch.c:349:    cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5",
src/util/virnetdevopenvswitch.c:416:    cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get", "Interface",

No need to send another version. I can fix this before pushing. I just like to know your ideas to my points.

Michal