On 01/19/2016 10:32 PM, Han Zhou wrote:
> Hi Russell,
>
> On Mon, Jan 18, 2016 at 7:45 AM, Russell Bryant <russ...@ovn.org
> <mailto: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 <mailto:zhou...@gmail.com>>
>> Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064413.html
>> Signed-off-by: Russell Bryant <russ...@ovn.org <mailto: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 <http://ovn-controller.at> | 39 ++++---
>> tests/ovn.at <http://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.
Oh, right. Thanks for pointing that out.
>
>>
>>
>> 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'
Good idea, done.
>
>>
>>
>> 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?
It's only related because I hit it while testing this patch. I will
split it out in v2.
>>
>>
>> + 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).
Done.
>>
>> @@ -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.
I have it split out in v2.
>>
>>
>> diff --git a/tests/ovn-controller.at <http://ovn-controller.at>
> b/tests/ovn-controller.at <http://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 :).
Good point. I changed it.
> Thanks for the quick and nice work!!
Thanks for the detailed review! I'll post v2 in a few minutes.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
