Hi Russell,
On Mon, Jan 18, 2016 at 7:45 AM, Russell Bryant <russ...@ovn.org> wrote:
>
> Multiple logical ports on the same chassis that were connected to the
> same physical network via localnet ports were not able to send packets
> to each other. This was because ovn-controller created a single patch
> port between br-int and the physical network access bridge and used it
> for all localnet ports.
>
> The fix implemented here is to create a separate patch port for every
> logical port of type=localnet. An optimization is included where these
> ports are only created if the localnet port is on a logical switch with
> another logical port with an associated local VIF.
>
> A nice side effect of this fix is that the code in physical.c got a lot
> simpler, as localnet ports are now handled mostly like local VIFs.
>
> Fixes: c02819293d52 ("ovn: Add "localnet" logical port type.")
> Reported-by: Han Zhou <zhou...@gmail.com>
> Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064413.html
> Signed-off-by: Russell Bryant <russ...@ovn.org>
> ---
> ovn/controller/ovn-controller.c | 10 +-
> ovn/controller/patch.c | 80 +++++++++-----
> ovn/controller/patch.h | 4 +-
> ovn/controller/physical.c | 237
+++++++++-------------------------------
> ovn/controller/physical.h | 3 +-
> tests/ovn-controller.at | 39 ++++---
> tests/ovn.at | 10 +-
> 7 files changed, 140 insertions(+), 243 deletions(-)
>
This would require some document updatesthe reflect the change to localnet
port, at least ovn/controller/ovn-controller.8.xml and
ovn/ovn-architecture.7.xml.
>
>
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
>
> + struct ovsrec_bridge *br_ln = shash_find_data(&bridge_mappings,
network);
> + if (!br_ln) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
> + VLOG_ERR_RL(&rl, "localnet port '%s' has no bridge.",
> + binding->logical_port);
It might be more clear to log the network name, e.g.: bridge not found for
network name '%s' for localnet port '%s'
>
>
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
>
> - } else if (binding->parent_port) {
> + if (binding->parent_port && *binding->parent_port) {
Is the change of adding condition *binding->parent_port related to this
patch?
>
>
> + ofpbuf_clear(&ofpacts);
> + match_init_catchall(&match);
> + match_set_in_port(&match, ofport);
> + if (tag) {
> + match_set_dl_vlan(&match, htons(tag));
> + } else if (!strcmp(binding->type, "localnet")) {
> + /* Match priority-tagged frames, e.g. VLAN ID 0.
> + *
> + * We'll add a second flow for frames that lack any
802.1Q
> + * header later. */
> + match_set_dl_tci_masked(&match, htons(VLAN_CFI),
> + htons(VLAN_VID_MASK | VLAN_CFI));
> + }
>
> - if (zone_id) {
> - put_load(zone_id, MFF_LOG_CT_ZONE, 0, 32, &ofpacts);
> - }
> + /* Strip vlans. */
> + if (tag) {
> + ofpact_put_STRIP_VLAN(&ofpacts);
> + }
>
This if block can be combined with the above if (tag).
>
> @@ -530,22 +485,12 @@ physical_run(struct controller_ctx *ctx, enum
mf_field_id mff_ovn_geneve,
> - port->parent_port
> + port->parent_port && *port->parent_port
Same question as above.
>
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> +check_patches
> +
> +# Create a localnet port, but we should still have no patch ports, as
they
> +# won't be created until there's a localnet port on a logical switch with
> +# another logical port bound to this chassis.
> +ovn-sbctl \
> + -- --id=@dp101 create Datapath_Binding tunnel_key=101 \
> + -- create Port_Binding datapath=@dp101 logical_port=localnet1
tunnel_key=101 \
For Port_Binding, tunnel_key=101 better to be tunnel_key=1. This is not a
problem but it may cause confusion for reader :).
Thanks for the quick and nice work!!
--
Best regards,
Han
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
