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