[ovs-dev] [PATCH] ovn-nbctl: Fix the ovn-nbctl test "LBs - daemon" which fails during rpm build
Numan Siddique
nusiddiq at redhat.com
Tue Oct 23 06:20:59 UTC 2018
On Mon, Oct 22, 2018 at 11:21 PM Mark Michelson <mmichels at redhat.com> wrote:
> On 10/22/2018 11:49 AM, nusiddiq at redhat.com wrote:
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > 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 it calls dns_resolve(). It blocks because
> > networking is disabled with mock rpm build.
> >
> > To address this issue, this patch adds a new function -
> > inet_parse_active_address_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 and ovn-nbctl. It is fine to
> > use this function as load balancer VIP cannot be a hostname.
>
> I think the change should also be made in ovn-trace.c, parse_lb_option().
>
Thanks Timothy for the comments and testing it out and Mark for the
comments. I have addressed
the review comments and submitted the v2 -
https://patchwork.ozlabs.org/patch/988035/
Thanks
Numan
> >
> > 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>
> > ---
> > lib/socket-util.c | 49 ++++++++++++++++++++++++++++-----------
> > lib/socket-util.h | 2 ++
> > ovn/northd/ovn-northd.c | 2 +-
> > ovn/utilities/ovn-nbctl.c | 6 ++---
> > 4 files changed, 42 insertions(+), 17 deletions(-)
> >
> > diff --git a/lib/socket-util.c b/lib/socket-util.c
> > index df9b01a9e..fcd1e5dc5 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,38 @@ 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, may be an IPv4 address or an IPv6 address
>
> I see that the difference between this and
> inet_parse_active_address_and_port() is that you use the term <host>
> instead of <IP>. However, the text still only mentions that <host> may
> be an IPv4 or IPv6 address. I think also specifying that a hostname is
> accepted would make the description more clear.
>
> > + * 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 remote address into
> '*ss'.
> > + * On failure, logs an error, stores zeros into '*ss', and returns
> false. */
> > +bool
> > +inet_parse_active_address_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..447a12cfc 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_active_address_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..ddc696499 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_active_address_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 798c97276..a2ab646fe 100644
> > --- a/ovn/utilities/ovn-nbctl.c
> > +++ b/ovn/utilities/ovn-nbctl.c
> > @@ -2637,7 +2637,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_address_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;
> > @@ -2667,7 +2667,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_address_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;
> > @@ -2786,7 +2786,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_address_and_port(node->key, 0, &ss))
> {
> > continue;
> > }
> >
> >
>
>
More information about the dev
mailing list