On Wed, Oct 31, 2018 at 11:18 PM Yifeng Sun <pkusunyif...@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 <nusid...@redhat.com> > wrote: > >> >> >> On Tue, Oct 30, 2018 at 11:50 PM Yifeng Sun <pkusunyif...@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 <nusid...@redhat.com> wrote: >>> >>>> From: Numan Siddique <nusid...@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 <tredae...@redhat.com> >>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1641672 >>>> Tested-by: Timothy Redaelli <tredae...@redhat.com> >>>> Signed-off-by: Numan Siddique <nusid...@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 >>>> d...@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev