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 | expand |
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: >
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!
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:
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(-)