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.
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