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

Numan Siddique nusiddiq at redhat.com
Wed Oct 31 18:10:00 UTC 2018


On Wed, Oct 31, 2018 at 11:18 PM Yifeng Sun <pkusunyifeng at gmail.com> wrote:

> What I actually meant to say is to add a default timeout to present
> dns_resolve__, say 15 seconds.
> Then other parts of code won't need be changed. If you think this is good,
> I will create a patch.
>

Sounds good to me. Please note that the test case [1] is failing because of
check

******
AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a 192.168.10.10:80,
192.168.10.20:80], [1], [],
[ovn-nbctl: 30.0.0.1a: should be an IP address (or an IP address and a port
number with : as a separator).
])
******

I think AT_CHECK times out after 10 seconds. So I am not sure what should
be the default timeout in dns_resolve__().

[1] - https://github.com/openvswitch/ovs/blob/master/tests/ovn-nbctl.at#L553

Thanks
Numan


> Thanks,
> Yifeng
>
> On Tue, Oct 30, 2018 at 9:34 PM Numan Siddique <nusiddiq at redhat.com>
> wrote:
>
>>
>>
>> On Tue, Oct 30, 2018 at 11:50 PM Yifeng Sun <pkusunyifeng at gmail.com>
>> wrote:
>>
>>> I feel another option to fix this issue is to add a new function like
>>> dns_resolve_timeout__, what do you think?
>>>
>>
>> I think that can fix the problem. I am not too familiar with libunbound.
>> Does it support specifying a timeout or
>> aync approch needs to be taken with a timeout ?
>> ovs/ovn utilities use dns_resolve_sync__(). Does it makes sense instead
>> to use dns_resolve_timeout__()
>> so that the fix would be transparent to the caller of dns_resolve().
>>
>> If you would like to send a patch with the new function, please do so. We
>> can drop this patch.
>>
>> Thanks
>> Numan
>>
>>
>>> 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