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