[ovs-dev] [PATCH v3] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

Yifeng Sun pkusunyifeng at gmail.com
Tue Oct 30 18:19:59 UTC 2018


I feel another option to fix this issue is to add a new function like
dns_resolve_timeout__, what do you think?

Thanks,
Yifeng

On Thu, Oct 25, 2018 at 2:58 AM <nusiddiq at redhat.com> wrote:

> From: Numan Siddique <nusiddiq at redhat.com>
>
> When 'make check' is called by the mock rpm build (which disables
> networking),
> the test "ovn-nbctl: LBs - daemon" fails when it runs the command
> "ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80".
> ovn-nbctl
> extracts the vip by calling the socket util function 'inet_parse_active()',
> and this function blocks when libunbound function ub_resolve() is called
> further down. ub_resolve() is a blocking function without timeout and all
> the
> ovs/ovn utilities use this function.
>
> As reported by Timothy Redaelli, the issue can also be reproduced by
> running
> the below commands
>
> $ sudo unshare -mn -- sh -c 'ip addr add dev lo 127.0.0.1 && \
>   mount --bind /dev/null /etc/resolv.conf && runuser $SUDO_USER'
> $ make sandbox SANDBOXFLAGS="--ovn"
> $ ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a \
>   192.168.10.10:80,192.168.10.20:80
>
> To address this issue, this patch adds a new function -
> inet_parse_ip_addr_and_port()
> which expects IP:[port] address in the 'target_' argument and disables
> resolving
> the host. This new function is now used in ovn-northd, ovn-nbctl and
> ovn-trace.
> It is fine to use this function as load balancer VIP cannot be a hostname.
>
> Reported-by: Timothy Redaelli <tredaelli at redhat.com>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1641672
> Tested-by: Timothy Redaelli <tredaelli at redhat.com>
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> ---
>
> v2 -> v3
> ------
>    * Renamed the function inet_parse_active_address_and_port()
>      to inet_parse_ip_addr_and_port()
>
> v1 -> v2
> -------
>   * Addressed review comments from Mark
>      - Updated the documentation of the inet_parse_active()
>      - Used the new function inet_parse_active_address_and_port()
>        in ovn-trace
>
>  lib/socket-util.c         | 50 +++++++++++++++++++++++++++++----------
>  lib/socket-util.h         |  2 ++
>  ovn/northd/ovn-northd.c   |  2 +-
>  ovn/utilities/ovn-nbctl.c |  6 ++---
>  ovn/utilities/ovn-trace.c |  2 +-
>  5 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index df9b01a9e..5bcffe744 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -512,18 +512,9 @@ exit:
>      return false;
>  }
>
> -/* Parses 'target', which should be a string in the format
> "<host>[:<port>]".
> - * <host>, which is required, may be an IPv4 address or an IPv6 address
> - * enclosed in square brackets.  If 'default_port' is nonnegative then
> <port>
> - * is optional and defaults to 'default_port' (use 0 to make the kernel
> choose
> - * an available port, although this isn't usually appropriate for active
> - * connections).  If 'default_port' is negative, then <port> is required.
> - *
> - * On success, returns true and stores the parsed remote address into
> '*ss'.
> - * On failure, logs an error, stores zeros into '*ss', and returns false.
> */
> -bool
> -inet_parse_active(const char *target_, int default_port,
> -                  struct sockaddr_storage *ss)
> +static bool
> +inet_parse_active__(const char *target_, int default_port,
> +                  struct sockaddr_storage *ss, bool resolve_host)
>  {
>      char *target = xstrdup(target_);
>      char *port, *host;
> @@ -538,7 +529,7 @@ inet_parse_active(const char *target_, int
> default_port,
>          ok = false;
>      } else {
>          ok = parse_sockaddr_components(ss, host, port, default_port,
> -                                       target_, true);
> +                                       target_, resolve_host);
>      }
>      if (!ok) {
>          memset(ss, 0, sizeof *ss);
> @@ -547,6 +538,39 @@ inet_parse_active(const char *target_, int
> default_port,
>      return ok;
>  }
>
> +/* Parses 'target', which should be a string in the format
> "<host>[:<port>]".
> + * <host>, which is required, can be a host name, IPv4 address or an IPv6
> + * address and may be enclosed in square brackets.  If 'default_port' is
> + * nonnegative then <port> is optional and defaults to 'default_port'
> (use 0
> + * to make the kernel choose an available port, although this isn't
> usually
> + * appropriate for active connections).  If 'default_port' is negative,
> + * then <port> is required.
> + *
> + * On success, returns true and stores the parsed remote address into
> '*ss'.
> + * On failure, logs an error, stores zeros into '*ss', and returns false.
> */
> +bool
> +inet_parse_active(const char *target_, int default_port,
> +                  struct sockaddr_storage *ss)
> +{
> +    return inet_parse_active__(target_, default_port, ss, true);
> +}
> +
> +/* Parses 'target', which should be a string in the format
> "<IP>[:<port>]".
> + * <IP>, which is required, should be an IPv4 address or an IPv6 address
> + * and may be enclosed in square brackets. If 'default_port' is
> + * nonnegative then <port> is optional and defaults to 'default_port'
> (use 0
> + * to make the kernel choose an available port, although this isn't
> usually
> + * appropriate for active connections).  If 'default_port' is negative,
> + * then <port> is required.
> + *
> + * On success, returns true and stores the parsed address into '*ss'.
> + * On failure, logs an error, stores zeros into '*ss', and returns false.
> */
> +bool
> +inet_parse_ip_addr_and_port(const char *target_, int default_port,
> +                            struct sockaddr_storage *ss)
> +{
> +    return inet_parse_active__(target_, default_port, ss, false);
> +}
>
>  /* Opens a non-blocking IPv4 or IPv6 socket of the specified 'style' and
>   * connects to 'target', which should be a string in the format
> diff --git a/lib/socket-util.h b/lib/socket-util.h
> index 6d386304d..34463a877 100644
> --- a/lib/socket-util.h
> +++ b/lib/socket-util.h
> @@ -50,6 +50,8 @@ void inet_parse_host_port_tokens(char *s, char **hostp,
> char **portp);
>  void inet_parse_port_host_tokens(char *s, char **portp, char **hostp);
>  bool inet_parse_active(const char *target, int default_port,
>                         struct sockaddr_storage *ssp);
> +bool inet_parse_ip_addr_and_port(const char *target, int default_port,
> +                                 struct sockaddr_storage *ssp);
>  int inet_open_active(int style, const char *target, int default_port,
>                       struct sockaddr_storage *ssp, int *fdp, uint8_t
> dscp);
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 439651f80..96386965c 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -3209,7 +3209,7 @@ ip_address_and_port_from_lb_key(const char *key,
> char **ip_address,
>                                  uint16_t *port, int *addr_family)
>  {
>      struct sockaddr_storage ss;
> -    if (!inet_parse_active(key, 0, &ss)) {
> +    if (!inet_parse_ip_addr_and_port(key, 0, &ss)) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>          VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key
> %s",
>                       key);
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 75dcb0752..134ffac18 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -2635,7 +2635,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>      }
>
>      struct sockaddr_storage ss_vip;
> -    if (!inet_parse_active(lb_vip, 0, &ss_vip)) {
> +    if (!inet_parse_ip_addr_and_port(lb_vip, 0, &ss_vip)) {
>          ctl_error(ctx, "%s: should be an IP address (or an IP address "
>                    "and a port number with : as a separator).", lb_vip);
>          return;
> @@ -2665,7 +2665,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>          struct sockaddr_storage ss_dst;
>
>          if (lb_vip_port) {
> -            if (!inet_parse_active(token, -1, &ss_dst)) {
> +            if (!inet_parse_ip_addr_and_port(token, -1, &ss_dst)) {
>                  ctl_error(ctx, "%s: should be an IP address and a port "
>                            "number with : as a separator.", token);
>                  goto out;
> @@ -2784,7 +2784,7 @@ lb_info_add_smap(const struct nbrec_load_balancer
> *lb,
>              const struct smap_node *node = nodes[i];
>
>              struct sockaddr_storage ss;
> -            if (!inet_parse_active(node->key, 0, &ss)) {
> +            if (!inet_parse_ip_addr_and_port(node->key, 0, &ss)) {
>                  continue;
>              }
>
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 40a79ceea..251af9dae 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -194,7 +194,7 @@ static void
>  parse_lb_option(const char *s)
>  {
>      struct sockaddr_storage ss;
> -    if (!inet_parse_active(s, 0, &ss)) {
> +    if (!inet_parse_ip_addr_and_port(s, 0, &ss)) {
>          ovs_fatal(0, "%s: bad address", s);
>      }
>
> --
> 2.17.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list