[ovs-dev] OVN: add CT_LB action to ovn-trace

Add CT_LB action to ovn-trace utility in order to fix the
following ovn-trace error if a load balancer rule is added to
OVN configuration

ct_next(ct_state=est|trk /* default (use --ct to customize) */) {
	    *** ct_lb action not implemented;
};

Add '--lb_dst' option in order to specify the ip address to use
in the VIP pool. If --lb_dst is not provided the destination ip will be
randomly choosen

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 ovn/utilities/ovn-trace.c | 61 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 3 deletions(-)

Hi Lorenzo,

I only had one finding on this, which I mentioned below. My only other 
concern is that this could use some documentation. If you can document 
this in the ovn-trace manpage, I think we'll be good.

On 09/18/2018 11:27 AM, Lorenzo Bianconi wrote:
> Add CT_LB action to ovn-trace utility in order to fix the
> following ovn-trace error if a load balancer rule is added to
> OVN configuration
> 
> ct_next(ct_state=est|trk /* default (use --ct to customize) */) {
> 	    *** ct_lb action not implemented;
> };
> 
> Add '--lb_dst' option in order to specify the ip address to use
> in the VIP pool. If --lb_dst is not provided the destination ip will be
> randomly choosen
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   ovn/utilities/ovn-trace.c | 61 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 7ca3d97aa..5755ea0bc 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -46,6 +46,7 @@
>   #include "stream.h"
>   #include "unixctl.h"
>   #include "util.h"
> +#include "random.h"
>   
>   VLOG_DEFINE_THIS_MODULE(ovntrace);
>   
> @@ -77,6 +78,9 @@ static uint32_t *ct_states;
>   static size_t n_ct_states;
>   static size_t ct_state_idx;
>   
> +/* --lb_dst: load balancer destination info */
> +static struct ovnact_ct_lb_dst lb_dst;
> +

ovn-trace has a daemon mode. In that mode, you run `ovn-trace --detach 
--pidfile` and then you can run `ovs-appctl -t ovn-trace trace 
<datapath> <flow>`

In daemon mode, will the use of a global cause an issue? My concern is 
with values persisting between traces. I think it might be a good idea 
to zero out lb_dst on each iteration of the main loop.

>   /* --friendly-names, --no-friendly-names: Whether to substitute human-friendly
>    * port and datapath names for the awkward UUIDs typically used in the actual
>    * logical flows. */
> @@ -186,6 +190,16 @@ parse_ct_option(const char *state_s_)
>       ct_states[n_ct_states++] = state;
>   }
>   
> +static void
> +parse_lb_option(const char *s)
> +{
> +    if (ip_parse(s, &lb_dst.ipv4)) {
> +        lb_dst.family = AF_INET;
> +    } else if (ipv6_parse(s, &lb_dst.ipv6)) {
> +        lb_dst.family = AF_INET6;
> +    }
> +}
> +
>   static void
>   parse_options(int argc, char *argv[])
>   {
> @@ -202,7 +216,8 @@ parse_options(int argc, char *argv[])
>           OPT_NO_FRIENDLY_NAMES,
>           DAEMON_OPTION_ENUMS,
>           SSL_OPTION_ENUMS,
> -        VLOG_OPTION_ENUMS
> +        VLOG_OPTION_ENUMS,
> +        OPT_LB_DST
>       };
>       static const struct option long_options[] = {
>           {"db", required_argument, NULL, OPT_DB},
> @@ -217,6 +232,7 @@ parse_options(int argc, char *argv[])
>           {"no-friendly-names", no_argument, NULL, OPT_NO_FRIENDLY_NAMES},
>           {"help", no_argument, NULL, 'h'},
>           {"version", no_argument, NULL, 'V'},
> +        {"lb_dst", required_argument, NULL, OPT_LB_DST},
>           DAEMON_LONG_OPTIONS,
>           VLOG_LONG_OPTIONS,
>           STREAM_SSL_LONG_OPTIONS,
> @@ -274,6 +290,10 @@ parse_options(int argc, char *argv[])
>               use_friendly_names = false;
>               break;
>   
> +        case OPT_LB_DST:
> +            parse_lb_option(optarg);
> +            break;
> +
>           case 'h':
>               usage();
>   
> @@ -1822,6 +1842,42 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
>        * flow, not ct_flow. */
>   }
>   
> +static void
> +execute_ct_lb(const struct ovnact_ct_lb *ct_lb,
> +              const struct ovntrace_datapath *dp, struct flow *uflow,
> +              enum ovnact_pipeline pipeline, struct ovs_list *super)
> +{
> +    struct flow ct_lb_flow = *uflow;
> +
> +    if (ct_lb->n_dsts) {
> +        int i, dest = random_range(ct_lb->n_dsts);
> +
> +        for (i = 0; i < ct_lb->n_dsts; i++) {
> +            if ((lb_dst.family == AF_INET &&
> +                 ct_lb->dsts[i].ipv4 == lb_dst.ipv4) ||
> +                (lb_dst.family == AF_INET6 &&
> +                 ipv6_addr_equals(&ct_lb->dsts[i].ipv6,
> +                                  &lb_dst.ipv6))) {
> +                dest = i;
> +                break;
> +            }
> +        }
> +
> +        if (ct_lb->dsts->family == AF_INET6) {
> +            ct_lb_flow.ipv6_dst = ct_lb->dsts[dest].ipv6;
> +        } else {
> +            ct_lb_flow.nw_dst = ct_lb->dsts[dest].ipv4;
> +        }
> +        if (ct_lb->dsts->port > 0) {
> +            ct_lb_flow.tp_dst = ct_lb->dsts->port;
> +        }
> +    }
> +
> +    struct ovntrace_node *node = ovntrace_node_append(
> +        super, OVNTRACE_NODE_TRANSFORMATION, "ct_lb");
> +    trace__(dp, &ct_lb_flow, ct_lb->ltable, pipeline, &node->subs);
> +}
> +
>   static void
>   execute_log(const struct ovnact_log *log, struct flow *uflow,
>               struct ovs_list *super)
> @@ -1910,8 +1966,7 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>               break;
>   
>           case OVNACT_CT_LB:
> -            ovntrace_node_append(super, OVNTRACE_NODE_ERROR,
> -                                 "*** ct_lb action not implemented");
> +            execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline, super);
>               break;
>   
>           case OVNACT_CT_CLEAR:
>

> Hi Lorenzo,
>

Hi Mark,

thx for the review

> I only had one finding on this, which I mentioned below. My only other
> concern is that this could use some documentation. If you can document this
> in the ovn-trace manpage, I think we'll be good.
> 

ack, will do in v2

> On 09/18/2018 11:27 AM, Lorenzo Bianconi wrote:
> > Add CT_LB action to ovn-trace utility in order to fix the
> > following ovn-trace error if a load balancer rule is added to
> > OVN configuration
> > 
> > ct_next(ct_state=est|trk /* default (use --ct to customize) */) {
> > 	    *** ct_lb action not implemented;
> > };
> > 
> > Add '--lb_dst' option in order to specify the ip address to use
> > in the VIP pool. If --lb_dst is not provided the destination ip will be
> > randomly choosen
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> >   ovn/utilities/ovn-trace.c | 61 +++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 58 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> > index 7ca3d97aa..5755ea0bc 100644
> > --- a/ovn/utilities/ovn-trace.c
> > +++ b/ovn/utilities/ovn-trace.c
> > @@ -46,6 +46,7 @@
> >   #include "stream.h"
> >   #include "unixctl.h"
> >   #include "util.h"
> > +#include "random.h"
> >   VLOG_DEFINE_THIS_MODULE(ovntrace);
> > @@ -77,6 +78,9 @@ static uint32_t *ct_states;
> >   static size_t n_ct_states;
> >   static size_t ct_state_idx;
> > +/* --lb_dst: load balancer destination info */
> > +static struct ovnact_ct_lb_dst lb_dst;
> > +
> 
> ovn-trace has a daemon mode. In that mode, you run `ovn-trace --detach
> --pidfile` and then you can run `ovs-appctl -t ovn-trace trace <datapath>
> <flow>`
> 
> In daemon mode, will the use of a global cause an issue? My concern is with
> values persisting between traces. I think it might be a good idea to zero
> out lb_dst on each iteration of the main loop.

At the moment if you run ovn-trace in daemon mode you will be able to specify
just summary options (e.g. --minimal or --detailed). If you agree I can
respin v2 fixing ovn-trace manual and I will fix daemon mode in a separate
patchset.  Do you agree?

Regards,
Lorenzo

> 
> >   /* --friendly-names, --no-friendly-names: Whether to substitute human-friendly
> >    * port and datapath names for the awkward UUIDs typically used in the actual
> >    * logical flows. */
> > @@ -186,6 +190,16 @@ parse_ct_option(const char *state_s_)
> >       ct_states[n_ct_states++] = state;
> >   }
> > +static void
> > +parse_lb_option(const char *s)
> > +{
> > +    if (ip_parse(s, &lb_dst.ipv4)) {
> > +        lb_dst.family = AF_INET;
> > +    } else if (ipv6_parse(s, &lb_dst.ipv6)) {
> > +        lb_dst.family = AF_INET6;
> > +    }
> > +}
> > +
> >   static void
> >   parse_options(int argc, char *argv[])
> >   {
> > @@ -202,7 +216,8 @@ parse_options(int argc, char *argv[])
> >           OPT_NO_FRIENDLY_NAMES,
> >           DAEMON_OPTION_ENUMS,
> >           SSL_OPTION_ENUMS,
> > -        VLOG_OPTION_ENUMS
> > +        VLOG_OPTION_ENUMS,
> > +        OPT_LB_DST
> >       };
> >       static const struct option long_options[] = {
> >           {"db", required_argument, NULL, OPT_DB},
> > @@ -217,6 +232,7 @@ parse_options(int argc, char *argv[])
> >           {"no-friendly-names", no_argument, NULL, OPT_NO_FRIENDLY_NAMES},
> >           {"help", no_argument, NULL, 'h'},
> >           {"version", no_argument, NULL, 'V'},
> > +        {"lb_dst", required_argument, NULL, OPT_LB_DST},
> >           DAEMON_LONG_OPTIONS,
> >           VLOG_LONG_OPTIONS,
> >           STREAM_SSL_LONG_OPTIONS,
> > @@ -274,6 +290,10 @@ parse_options(int argc, char *argv[])
> >               use_friendly_names = false;
> >               break;
> > +        case OPT_LB_DST:
> > +            parse_lb_option(optarg);
> > +            break;
> > +
> >           case 'h':
> >               usage();
> > @@ -1822,6 +1842,42 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
> >        * flow, not ct_flow. */
> >   }
> > +static void
> > +execute_ct_lb(const struct ovnact_ct_lb *ct_lb,
> > +              const struct ovntrace_datapath *dp, struct flow *uflow,
> > +              enum ovnact_pipeline pipeline, struct ovs_list *super)
> > +{
> > +    struct flow ct_lb_flow = *uflow;
> > +
> > +    if (ct_lb->n_dsts) {
> > +        int i, dest = random_range(ct_lb->n_dsts);
> > +
> > +        for (i = 0; i < ct_lb->n_dsts; i++) {
> > +            if ((lb_dst.family == AF_INET &&
> > +                 ct_lb->dsts[i].ipv4 == lb_dst.ipv4) ||
> > +                (lb_dst.family == AF_INET6 &&
> > +                 ipv6_addr_equals(&ct_lb->dsts[i].ipv6,
> > +                                  &lb_dst.ipv6))) {
> > +                dest = i;
> > +                break;
> > +            }
> > +        }
> > +
> > +        if (ct_lb->dsts->family == AF_INET6) {
> > +            ct_lb_flow.ipv6_dst = ct_lb->dsts[dest].ipv6;
> > +        } else {
> > +            ct_lb_flow.nw_dst = ct_lb->dsts[dest].ipv4;
> > +        }
> > +        if (ct_lb->dsts->port > 0) {
> > +            ct_lb_flow.tp_dst = ct_lb->dsts->port;
> > +        }
> > +    }
> > +
> > +    struct ovntrace_node *node = ovntrace_node_append(
> > +        super, OVNTRACE_NODE_TRANSFORMATION, "ct_lb");
> > +    trace__(dp, &ct_lb_flow, ct_lb->ltable, pipeline, &node->subs);
> > +}
> > +
> >   static void
> >   execute_log(const struct ovnact_log *log, struct flow *uflow,
> >               struct ovs_list *super)
> > @@ -1910,8 +1966,7 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> >               break;
> >           case OVNACT_CT_LB:
> > -            ovntrace_node_append(super, OVNTRACE_NODE_ERROR,
> > -                                 "*** ct_lb action not implemented");
> > +            execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline, super);
> >               break;
> >           case OVNACT_CT_CLEAR:
> > 
>