[ovs-dev] [PATCH v3] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build
Yifeng Sun
pkusunyifeng at gmail.com
Wed Oct 31 17:48:37 UTC 2018
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 <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