[ovs-dev] [PATCH] ovn: Fix flows between 2 local ports.
Justin Pettit
jpettit at nicira.com
Fri Aug 7 22:11:11 UTC 2015
Thanks for tracking down the issue and supplying a patch, Russell. However, I think the core problem was that there was a missing flow. Specifically, the one described in the last sentence from this part of the ovn-architecture man page:
Each flow in table 32 matches on a logical output port for
unicast or multicast logical ports that include a logical port
on a remote hypervisor. Each flow’s actions implement sending
a packet to the port it matches. For unicast logical output
ports on remote hypervisors, the actions set the tunnel key to
the correct value, then send the packet on the tunnel port to
the correct hypervisor. (When the remote hypervisor receives
the packet, table 0 there will recognize it as a tunneled
packet and pass it along to table 33.) For multicast logical
output ports, the actions send one copy of the packet to each
remote hypervisor, in the same way as for unicast destina‐
tions. If a multicast group includes a logical port or ports
on the local hypervisor, then its actions also resubmit to ta‐
ble 33. Table 32 also includes a fallback flow that resubmits
to table 33 if there is no other match.
I've sent out a patch, which I think addresses the issue. Would you be able to review the patch and make sure it solves the problem you were seeing?
I think we should work on some of the naming and descriptions of these flows, since it wasn't obvious to me that there should be a fallback flow for local traffic in the remote flow table! I imagine this will improve over time.
Thanks!
--Justin
> On Aug 6, 2015, at 12:23 PM, Russell Bryant <rbryant at redhat.com> wrote:
>
> Packets were always resubmmited to the remote output table, which
> handles output to a remote chassis. Broadcast packets were only sent
> remotely and not to local ports on the same logical switch. Unicast
> packets between 2 local ports also didn't work.
>
> This patch makes it so packets are sent to both remote and local
> output tables. It would be a bit cleaner to only submit to the
> necessary table(s), but always resubmitting to both and letting the
> matching work it out seems to be fine and gets things working again.
>
> Signed-off-by: Russell Bryant <rbryant at redhat.com>
> ---
> ovn/controller/lflow.c | 14 ++++++++++----
> ovn/lib/actions.c | 20 +++++++++++++-------
> ovn/lib/actions.h | 9 +++++----
> tests/test-ovn.c | 5 +++--
> 4 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 9246e61..efa0253 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -271,9 +271,14 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table)
> : OFTABLE_LOG_EGRESS_PIPELINE);
> uint8_t next_phys_table
> = lflow->table_id + 1 < LOG_PIPELINE_LEN ? phys_table + 1 : 0;
> - uint8_t output_phys_table = (ingress
> - ? OFTABLE_REMOTE_OUTPUT
> - : OFTABLE_LOG_TO_PHY);
> + uint8_t oftable_output[] = { OFTABLE_LOCAL_OUTPUT, OFTABLE_REMOTE_OUTPUT };
> + uint8_t oftable_log_to_phy[] = { OFTABLE_LOG_TO_PHY };
> + uint8_t *output_phys_table = (ingress
> + ? oftable_output
> + : oftable_log_to_phy);
> + size_t n_output_tables = (ingress
> + ? ARRAY_SIZE(oftable_output)
> + : ARRAY_SIZE(oftable_log_to_phy));
> /* Translate OVN actions into OpenFlow actions.
> *
> * XXX Deny changes to 'outport' in egress pipeline. */
> @@ -284,7 +289,8 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table)
>
> ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
> error = actions_parse_string(lflow->actions, &symtab, &ldp->ports,
> - next_phys_table, output_phys_table,
> + next_phys_table,
> + output_phys_table, n_output_tables,
> &ofpacts, &prereqs);
> if (error) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 0a0158a..51b04ae 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -31,7 +31,8 @@ struct action_context {
> struct lexer *lexer; /* Lexer for pulling more tokens. */
> const struct shash *symtab; /* Symbol table. */
> uint8_t next_table_id; /* OpenFlow table for 'next' to resubmit. */
> - uint8_t output_table_id; /* OpenFlow table for 'output' to resubmit. */
> + uint8_t *output_table_id; /* OpenFlow tables for 'output' to resubmit. */
> + size_t n_output_table_id;
> const struct simap *ports; /* Map from port name to number. */
>
> /* State. */
> @@ -162,7 +163,10 @@ parse_actions(struct action_context *ctx)
> action_error(ctx, "\"next\" action not allowed here.");
> }
> } else if (lexer_match_id(ctx->lexer, "output")) {
> - emit_resubmit(ctx, ctx->output_table_id);
> + size_t i;
> + for (i = 0; i < ctx->n_output_table_id; i++) {
> + emit_resubmit(ctx, ctx->output_table_id[i]);
> + }
> } else {
> action_syntax_error(ctx, "expecting action");
> }
> @@ -205,8 +209,8 @@ parse_actions(struct action_context *ctx)
> char * OVS_WARN_UNUSED_RESULT
> actions_parse(struct lexer *lexer, const struct shash *symtab,
> const struct simap *ports, uint8_t next_table_id,
> - uint8_t output_table_id, struct ofpbuf *ofpacts,
> - struct expr **prereqsp)
> + uint8_t *output_table_id, size_t n_output_table_id,
> + struct ofpbuf *ofpacts, struct expr **prereqsp)
> {
> size_t ofpacts_start = ofpacts->size;
>
> @@ -216,6 +220,7 @@ actions_parse(struct lexer *lexer, const struct shash *symtab,
> ctx.ports = ports;
> ctx.next_table_id = next_table_id;
> ctx.output_table_id = output_table_id;
> + ctx.n_output_table_id = n_output_table_id;
> ctx.error = NULL;
> ctx.ofpacts = ofpacts;
> ctx.prereqs = NULL;
> @@ -237,8 +242,8 @@ actions_parse(struct lexer *lexer, const struct shash *symtab,
> char * OVS_WARN_UNUSED_RESULT
> actions_parse_string(const char *s, const struct shash *symtab,
> const struct simap *ports, uint8_t next_table_id,
> - uint8_t output_table_id, struct ofpbuf *ofpacts,
> - struct expr **prereqsp)
> + uint8_t *output_table_id, size_t n_output_table_id,
> + struct ofpbuf *ofpacts, struct expr **prereqsp)
> {
> struct lexer lexer;
> char *error;
> @@ -246,7 +251,8 @@ actions_parse_string(const char *s, const struct shash *symtab,
> lexer_init(&lexer, s);
> lexer_get(&lexer);
> error = actions_parse(&lexer, symtab, ports, next_table_id,
> - output_table_id, ofpacts, prereqsp);
> + output_table_id, n_output_table_id,
> + ofpacts, prereqsp);
> lexer_destroy(&lexer);
>
> return error;
> diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
> index 74cd185..dec2ba1 100644
> --- a/ovn/lib/actions.h
> +++ b/ovn/lib/actions.h
> @@ -17,6 +17,7 @@
> #ifndef OVN_ACTIONS_H
> #define OVN_ACTIONS_H 1
>
> +#include <unistd.h>
> #include <stdint.h>
> #include "compiler.h"
>
> @@ -28,13 +29,13 @@ struct simap;
>
> char *actions_parse(struct lexer *, const struct shash *symtab,
> const struct simap *ports, uint8_t next_table_id,
> - uint8_t output_table_id, struct ofpbuf *ofpacts,
> - struct expr **prereqsp)
> + uint8_t *output_table_id, size_t n_output_table_id,
> + struct ofpbuf *ofpacts, struct expr **prereqsp)
> OVS_WARN_UNUSED_RESULT;
> char *actions_parse_string(const char *s, const struct shash *symtab,
> const struct simap *ports, uint8_t next_table_id,
> - uint8_t output_table_id, struct ofpbuf *ofpacts,
> - struct expr **prereqsp)
> + uint8_t *output_table_id, size_t n_output_table_id,
> + struct ofpbuf *ofpacts, struct expr **prereqsp)
> OVS_WARN_UNUSED_RESULT;
>
> #endif /* ovn/actions.h */
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 60b87de..60b280d 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1138,8 +1138,9 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
> char *error;
>
> ofpbuf_init(&ofpacts, 0);
> - error = actions_parse_string(ds_cstr(&input), &symtab, &ports, 11, 64,
> - &ofpacts, &prereqs);
> + uint8_t output_table_id = 64;
> + error = actions_parse_string(ds_cstr(&input), &symtab, &ports, 11,
> + &output_table_id, 1, &ofpacts, &prereqs);
> if (!error) {
> struct ds output;
>
> --
> 2.4.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list