[ovs-dev] [PATCH v4] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build

nusiddiq at redhat.com nusiddiq at redhat.com
Fri Nov 2 11:41:01 UTC 2018


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 bool argument 'resolve_host' to
the function inet_parse_active() to resolve the host only if it is 'true'.

ovn-nbctl/ovn-northd will pass 'false' when it calls this function to parse
the load balancer values.

Reported-by: Timothy Redaelli <tredaelli at redhat.com>
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1641672
Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
---

v3 -> v4
------
   * Instead of adding a new function, added new arguement
     'resolve_dns' to the existing function inet_parse_active() as
     suggested by Ben

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            | 7 ++++---
 lib/socket-util.h            | 2 +-
 lib/stream.c                 | 2 +-
 ofproto/ofproto-dpif-sflow.c | 2 +-
 ovn/northd/ovn-northd.c      | 2 +-
 ovn/utilities/ovn-nbctl.c    | 6 +++---
 ovn/utilities/ovn-trace.c    | 2 +-
 ovsdb/raft-private.c         | 2 +-
 8 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/lib/socket-util.c b/lib/socket-util.c
index df9b01a9e..8d18e043d 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -518,12 +518,13 @@ exit:
  * 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.
+ * It resolves the host if 'resolve_host' is true.
  *
  * 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)
+                  struct sockaddr_storage *ss, bool resolve_host)
 {
     char *target = xstrdup(target_);
     char *port, *host;
@@ -538,7 +539,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);
@@ -575,7 +576,7 @@ inet_open_active(int style, const char *target, int default_port,
     int error;
 
     /* Parse. */
-    if (!inet_parse_active(target, default_port, &ss)) {
+    if (!inet_parse_active(target, default_port, &ss, true)) {
         error = EAFNOSUPPORT;
         goto exit;
     }
diff --git a/lib/socket-util.h b/lib/socket-util.h
index 6d386304d..a65433d90 100644
--- a/lib/socket-util.h
+++ b/lib/socket-util.h
@@ -49,7 +49,7 @@ ovs_be32 guess_netmask(ovs_be32 ip);
 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);
+                       struct sockaddr_storage *ssp, bool resolve_host);
 int inet_open_active(int style, const char *target, int default_port,
                      struct sockaddr_storage *ssp, int *fdp, uint8_t dscp);
 
diff --git a/lib/stream.c b/lib/stream.c
index 4e15fe0c8..c4dabda39 100644
--- a/lib/stream.c
+++ b/lib/stream.c
@@ -751,7 +751,7 @@ stream_parse_target_with_default_port(const char *target, int default_port,
                                       struct sockaddr_storage *ss)
 {
     return ((!strncmp(target, "tcp:", 4) || !strncmp(target, "ssl:", 4))
-            && inet_parse_active(target + 4, default_port, ss));
+            && inet_parse_active(target + 4, default_port, ss, true));
 }
 
 /* Attempts to guess the content type of a stream whose first few bytes were
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index 62a09b5d1..7da31753c 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -468,7 +468,7 @@ sflow_choose_agent_address(const char *agent_device,
     const char *target;
     SSET_FOR_EACH (target, targets) {
         struct sockaddr_storage ss;
-        if (inet_parse_active(target, SFL_DEFAULT_COLLECTOR_PORT, &ss)) {
+        if (inet_parse_active(target, SFL_DEFAULT_COLLECTOR_PORT, &ss, true)) {
             /* sFlow only supports target in default routing table with
              * packet mark zero.
              */
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index e42ceea99..5c6c80e03 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -3218,7 +3218,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_active(key, 0, &ss, false)) {
         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..9d1b22089 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_active(lb_vip, 0, &ss_vip, false)) {
         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_active(token, -1, &ss_dst, false)) {
                 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_active(node->key, 0, &ss, false)) {
                 continue;
             }
 
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 40a79ceea..cbf3e54d2 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_active(s, 0, &ss, false)) {
         ovs_fatal(0, "%s: bad address", s);
     }
 
diff --git a/ovsdb/raft-private.c b/ovsdb/raft-private.c
index 07996e35b..e5e2c29cf 100644
--- a/ovsdb/raft-private.c
+++ b/ovsdb/raft-private.c
@@ -33,7 +33,7 @@ raft_address_validate(const char *address)
         return NULL;
     } else if (!strncmp(address, "ssl:", 4) || !strncmp(address, "tcp:", 4)) {
         struct sockaddr_storage ss;
-        if (!inet_parse_active(address + 4, -1, &ss)) {
+        if (!inet_parse_active(address + 4, -1, &ss, true)) {
             return ovsdb_error(NULL, "%s: syntax error in address", address);
         }
         return NULL;
-- 
2.19.1



More information about the dev mailing list