[ovs-dev,v2] OVN: add CT_LB action to ovn-trace
diff mbox series

Message ID ec53b6aaa87ab0a3945ce4c90061cc0b01427195.1537454474.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev,v2] OVN: add CT_LB action to ovn-trace
Related show

Commit Message

Lorenzo Bianconi Sept. 20, 2018, 2:46 p.m. UTC
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 VIP pool. If --lb_dst is not provided the destination ip will be
randomly choosen

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- update ovn-trace manual
- reset to 0 lb_dst when it is consumed by ct_lb action
---
 ovn/utilities/ovn-trace.8.xml | 12 ++++++-
 ovn/utilities/ovn-trace.c     | 64 +++++++++++++++++++++++++++++++++--
 2 files changed, 72 insertions(+), 4 deletions(-)

Comments

Mark Michelson Sept. 20, 2018, 8:46 p.m. UTC | #1
LGTM

Acked-by: Mark Michelson <mmichels@redhat.com>

On 09/20/2018 10:46 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 VIP pool. If --lb_dst is not provided the destination ip will be
> randomly choosen
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - update ovn-trace manual
> - reset to 0 lb_dst when it is consumed by ct_lb action
> ---
>   ovn/utilities/ovn-trace.8.xml | 12 ++++++-
>   ovn/utilities/ovn-trace.c     | 64 +++++++++++++++++++++++++++++++++--
>   2 files changed, 72 insertions(+), 4 deletions(-)
> 
> diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml
> index 83b397f3a..3d75c7b86 100644
> --- a/ovn/utilities/ovn-trace.8.xml
> +++ b/ovn/utilities/ovn-trace.8.xml
> @@ -255,7 +255,11 @@
>   
>       <dt><code>ct_lb</code></dt>
>       <dd>
> -      Not yet implemented; currently implemented as a no-op.
> +      Forks the pipeline. In one fork, sets <code>ip4.dst</code> (or
> +      <code>ip6.dst</code>) to the ip provided with <code>--lb_dst</code>
> +      option (or to an ip randomly chosen from VIP pool). In the other fork,
> +      the pipeline continues without change after the <code>ct_lb</code>
> +      action
>       </dd>
>   
>       <dt><code>ct_commit</code></dt>
> @@ -424,6 +428,12 @@
>         </p>
>       </dd>
>   
> +    <dt><code>--lb_dst</code>[<code>=</code><var>ip</var>]</dt>
> +    <dd>
> +      Selects an ip from VIP pool to use as destination of the packet.
> +      lb_dst is not available in daemon mode
> +    </dd>
> +
>       <dt><code>--friendly-names</code></dt>
>       <dt><code>--no-friendly-names</code></dt>
>       <dd>
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 7ca3d97aa..ee4583deb 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;
> +
>   /* --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,45 @@ 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 (i < ct_lb->n_dsts) {
> +            memset(&lb_dst, 0, sizeof(struct ovnact_ct_lb_dst));
> +        }
> +
> +        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 +1969,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:
>
Ben Pfaff Sept. 21, 2018, 7:36 p.m. UTC | #2
On Fri, Sep 21, 2018 at 03:45:00PM +0200, Lorenzo Bianconi wrote:
> > Here's a version that improves on these dimensions.  I have not tested
> > it.  What do you think?
> 
> thx for fixing these bits. The patch works properly, just one comment inline

...

> > +        } else {
> > +            ovntrace_node_append(super, OVNTRACE_NODE_ERROR,
> > +                                 "*** no load balancing destination "
> > +                                 "(use --lb-dst)");
> 
> This codepath is hit when you provide --ct option set to 'new' since in the
> switch egress pipeline ct_lb->n_dsts is 0 and lb_dst.family is set to
> AF_UNSPEC (but the processing continues properly). Is it ok to remove it or
> maybe check if pipeline is set to OVNACT_P_INGRESS?

OK, I moved this so that it only triggers if ct_lb->n_dsts != 0, which I
think should avoid the problem.  With that change, I applied it to
master.

Thanks again!

Patch
diff mbox series

diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml
index 83b397f3a..3d75c7b86 100644
--- a/ovn/utilities/ovn-trace.8.xml
+++ b/ovn/utilities/ovn-trace.8.xml
@@ -255,7 +255,11 @@ 
 
     <dt><code>ct_lb</code></dt>
     <dd>
-      Not yet implemented; currently implemented as a no-op.
+      Forks the pipeline. In one fork, sets <code>ip4.dst</code> (or
+      <code>ip6.dst</code>) to the ip provided with <code>--lb_dst</code>
+      option (or to an ip randomly chosen from VIP pool). In the other fork,
+      the pipeline continues without change after the <code>ct_lb</code>
+      action
     </dd>
 
     <dt><code>ct_commit</code></dt>
@@ -424,6 +428,12 @@ 
       </p>
     </dd>
 
+    <dt><code>--lb_dst</code>[<code>=</code><var>ip</var>]</dt>
+    <dd>
+      Selects an ip from VIP pool to use as destination of the packet.
+      lb_dst is not available in daemon mode
+    </dd>
+
     <dt><code>--friendly-names</code></dt>
     <dt><code>--no-friendly-names</code></dt>
     <dd>
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 7ca3d97aa..ee4583deb 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;
+
 /* --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,45 @@  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 (i < ct_lb->n_dsts) {
+            memset(&lb_dst, 0, sizeof(struct ovnact_ct_lb_dst));
+        }
+
+        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 +1969,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: