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

Mark Michelson Wed, 19 Sep 2018 14:31:01 -0700

Hi Lorenzo,

I only had one finding on this, which I mentioned below. My only otherconcern is that this could use some documentation. If you can documentthis 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 <[email protected]>
---
  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 iswith values persisting between traces. I think it might be a good ideato 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:


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Reply via email to