> On 30 May 2023, at 13:10, Dumitru Ceara <dce...@redhat.com> wrote: > > On 5/30/23 11:59, Vladislav Odintsov wrote: >> Hi Dumitru, >> >> thanks for the review! >> My answers are inline. >> >>> On 30 May 2023, at 12:27, Dumitru Ceara <dce...@redhat.com> wrote: >>> >>> On 5/19/23 20:18, Vladislav Odintsov wrote: >>>> This patch is intended to make next two changes: >>>> >>>> 1. Support create/update of MAC_Binding for GARP/ND from HW VTEP. >>>> >>>> Prior to this patch MAC_Binding records were created only when LRP issued >>>> ARP request/Neighbor solicitation. If IP to MAC in network attached via >>>> vtep lport was changed the old MAC_Binding prevented normal >>>> communication. Now router port (chassisredirect) in logical switch with >>>> vtep lport catches GARP or Neighbor Solicitation and updates MAC_Binding. >>>> >>>> New flow with max priority was added in ls_in_arp_nd_resp and additional >>>> OF rule in table 37 (OFTABLE_REMOTE_OUTPUT) to process multicast packets >>>> received from RAMP_SWITCH. >>>> >>>> 2. Answer to ARP/ND on requests coming from HW VTEP in lrouter pipeline. >>>> >>>> This is needed to reduce duplicated arp-responses, which were generated >>>> by OVN arp-responder flows in node, where chassisredirect port located >>>> with responses coming directly from VM interfaces. >>>> >>>> As a result of this change, now vifs located on same chassis, where >>>> chassisredirect ports are located receive ARP/ND mcast packets, which >>>> previously were suppressed by arp-responder on the node. >>>> >>>> Tests and documentation were updated to conform to new behaviour. >>>> >>>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >>>> --- >>> >>> Hi Vladislav, >>> >>> Thanks for the patch. >>> >>>> controller/binding.c | 48 ++++++++++++++++++++ >>>> controller/local_data.h | 3 ++ >>>> controller/physical.c | 46 +++++++++++++++++-- >>>> northd/northd.c | 10 +++++ >>>> northd/ovn-northd.8.xml | 6 +++ >>>> tests/ovn.at <http://ovn.at/> | 99 >>>> ++++++++++++++++++++++++++++++++++++++++- >>> >>> Could you please add a NEWS entry for this behavior change? >> >> Sure, will do it in v2. >> >>> >>>> 6 files changed, 207 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/controller/binding.c b/controller/binding.c >>>> index c7a2b3f10..5932ad785 100644 >>>> --- a/controller/binding.c >>>> +++ b/controller/binding.c >>>> @@ -509,6 +509,30 @@ update_ld_multichassis_ports(const struct >>>> sbrec_port_binding *binding_rec, >>>> } >>>> } >>>> >>>> +static void >>>> +update_ld_vtep_port(const struct sbrec_port_binding *binding_rec, >>>> + struct hmap *local_datapaths) >>>> +{ >>>> + struct local_datapath *ld >>>> + = get_local_datapath(local_datapaths, >>>> + binding_rec->datapath->tunnel_key); >>>> + if (!ld) { >>>> + return; >>>> + } >>>> + >>>> + if (ld->vtep_port && strcmp(ld->vtep_port->logical_port, >>>> + binding_rec->logical_port)) { >>>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >>>> + VLOG_WARN_RL(&rl, "vtep port '%s' already set for datapath " >>>> + "'%"PRId64"', skipping the new port '%s'.", >>>> + ld->vtep_port->logical_port, >>>> + binding_rec->datapath->tunnel_key, >>>> + binding_rec->logical_port); >>>> + return; >>>> + } >>>> + ld->vtep_port = binding_rec; >>>> +} >>>> + >>>> static void >>>> update_ld_localnet_port(const struct sbrec_port_binding *binding_rec, >>>> struct shash *bridge_mappings, >>>> @@ -1987,6 +2011,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, >>>> struct binding_ctx_out *b_ctx_out) >>>> struct ovs_list external_lports = >>>> OVS_LIST_INITIALIZER(&external_lports); >>>> struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER( >>>> >>>> &multichassis_ports); >>>> + struct ovs_list vtep_lports = OVS_LIST_INITIALIZER(&vtep_lports); >>>> >>>> struct lport { >>>> struct ovs_list list_node; >>>> @@ -2010,8 +2035,13 @@ binding_run(struct binding_ctx_in *b_ctx_in, >>>> struct binding_ctx_out *b_ctx_out) >>>> >>>> switch (lport_type) { >>>> case LP_PATCH: >>>> + update_related_lport(pb, b_ctx_out); >>>> + break; >>>> case LP_VTEP: >>>> update_related_lport(pb, b_ctx_out); >>>> + struct lport *vtep_lport = xmalloc(sizeof *vtep_lport); >>>> + vtep_lport->pb = pb; >>>> + ovs_list_push_back(&vtep_lports, &vtep_lport->list_node); >>>> break; >>>> >>>> case LP_LOCALPORT: >>>> @@ -2111,6 +2141,16 @@ binding_run(struct binding_ctx_in *b_ctx_in, >>>> struct binding_ctx_out *b_ctx_out) >>>> free(multichassis_lport); >>>> } >>>> >>>> + /* Run through vtep lport list to see if there are vtep ports >>>> + * on local datapaths discovered from above loop, and update the >>>> + * corresponding local datapath accordingly. */ >>>> + struct lport *vtep_lport; >>>> + ovs_list_size(&vtep_lports); >>> >>> I guess this is a leftover. >> >> Oops, will fix it in v2. >> >>> >>>> + LIST_FOR_EACH_POP (vtep_lport, list_node, &vtep_lports) { >>>> + update_ld_vtep_port(vtep_lport->pb, b_ctx_out->local_datapaths); >>>> + free(vtep_lport); >>>> + } >>>> + >>>> shash_destroy(&bridge_mappings); >>>> /* Remove stale QoS entries. */ >>>> ovs_qos_entries_gc(b_ctx_in->ovs_idl_txn, >>>> b_ctx_in->ovsrec_port_by_qos, >>>> @@ -2175,6 +2215,11 @@ remove_pb_from_local_datapath(const struct >>>> sbrec_port_binding *pb, >>>> } >>>> } else if (!strcmp(pb->type, "external")) { >>>> remove_local_datapath_external_port(ld, pb->logical_port); >>>> + } else if (!strcmp(pb->type, "vtep")) { >>>> + if (ld->vtep_port && !strcmp(ld->vtep_port->logical_port, >>>> + pb->logical_port)) { >>>> + ld->vtep_port = NULL; >>>> + } >>>> } >>>> remove_local_datapath_multichassis_port(ld, pb->logical_port); >>>> } >>>> @@ -2836,6 +2881,7 @@ handle_updated_port(struct binding_ctx_in >>>> *b_ctx_in, >>>> >>>> case LP_VTEP: >>>> update_related_lport(pb, b_ctx_out); >>>> + update_ld_vtep_port(pb, b_ctx_out->local_datapaths); >>>> /* VTEP lports are claimed/released by ovn-controller-vteps. >>>> * We are not sure what changed. */ >>>> b_ctx_out->non_vif_ports_changed = true; >>>> @@ -3091,6 +3137,8 @@ delete_done: >>>> } else if (pb->n_additional_chassis) { >>>> update_ld_multichassis_ports(pb, >>>> >>>> b_ctx_out->local_datapaths); >>>> + } else if (lport_type == LP_VTEP) { >>>> + update_ld_vtep_port(pb, b_ctx_out->local_datapaths); >>>> } >>> >>> Nit: I think I'd move this "else if (lport_type == LP_VTEP) {" just >>> under the LP_EXTERNAL case. >> >> I’ll move it, but I guess it’s not possible to have additional chassis >> for vtep pb. >> >>> >>>> } >>>> sbrec_port_binding_index_destroy_row(target); >>>> diff --git a/controller/local_data.h b/controller/local_data.h >>>> index 748f009aa..19d13a558 100644 >>>> --- a/controller/local_data.h >>>> +++ b/controller/local_data.h >>>> @@ -50,6 +50,9 @@ struct local_datapath { >>>> /* The localnet port in this datapath, if any (at most one is >>>> allowed). */ >>>> const struct sbrec_port_binding *localnet_port; >>>> >>>> + /* The vtep port in this datapath, if any (at most one is >>>> allowed). */ >>>> + const struct sbrec_port_binding *vtep_port; >>>> + >>>> struct { >>>> const struct sbrec_port_binding *local; >>>> const struct sbrec_port_binding *remote; >>>> diff --git a/controller/physical.c b/controller/physical.c >>>> index 2925dcd1d..b75e120c7 100644 >>>> --- a/controller/physical.c >>>> +++ b/controller/physical.c >>>> @@ -194,6 +194,15 @@ get_localnet_port(const struct hmap >>>> *local_datapaths, int64_t tunnel_key) >>>> } >>>> >>>> >>>> +static const struct sbrec_port_binding * >>>> +get_vtep_port(const struct hmap *local_datapaths, int64_t tunnel_key) >>>> +{ >>>> + const struct local_datapath *ld = >>>> get_local_datapath(local_datapaths, >>>> + tunnel_key); >>>> + return ld ? ld->vtep_port : NULL; >>>> +} >>>> + >>>> + >>>> static struct zone_ids >>>> get_zone_ids(const struct sbrec_port_binding *binding, >>>> const struct simap *ct_zones) >>>> @@ -1732,7 +1741,7 @@ consider_mc_group(struct ovsdb_idl_index >>>> *sbrec_port_binding_by_name, >>>> struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis); >>>> struct sset vtep_chassis = SSET_INITIALIZER(&vtep_chassis); >>>> >>>> - struct match match; >>>> + struct match match, match_ramp; >>>> match_outport_dp_and_port_keys(&match, dp_key, mc->tunnel_key); >>>> >>>> /* Go through all of the ports in the multicast group: >>>> @@ -1755,10 +1764,10 @@ consider_mc_group(struct ovsdb_idl_index >>>> *sbrec_port_binding_by_name, >>>> * set the output port to be the router patch port for which >>>> * the redirect port was added. >>>> */ >>>> - struct ofpbuf ofpacts; >>>> + struct ofpbuf ofpacts, remote_ofpacts, remote_ofpacts_ramp; >>>> ofpbuf_init(&ofpacts, 0); >>>> - struct ofpbuf remote_ofpacts; >>>> ofpbuf_init(&remote_ofpacts, 0); >>>> + ofpbuf_init(&remote_ofpacts_ramp, 0); >>>> for (size_t i = 0; i < mc->n_ports; i++) { >>>> struct sbrec_port_binding *port = mc->ports[i]; >>>> >>>> @@ -1783,6 +1792,7 @@ consider_mc_group(struct ovsdb_idl_index >>>> *sbrec_port_binding_by_name, >>>> local_output_pb(port->tunnel_key, &ofpacts); >>>> } else { >>>> local_output_pb(port->tunnel_key, &remote_ofpacts); >>>> + local_output_pb(port->tunnel_key, &remote_ofpacts_ramp); >>>> } >>>> } if (!strcmp(port->type, "remote")) { >>>> if (port->chassis) { >>>> @@ -1866,6 +1876,35 @@ consider_mc_group(struct ovsdb_idl_index >>>> *sbrec_port_binding_by_name, >>>> * multicast group as the logical output port. */ >>>> put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, >>>> &remote_ofpacts); >>>> + >>>> + if (get_vtep_port(local_datapaths, >>>> mc->datapath->tunnel_key)) { >>> >>> I'd declare 'match_ramp' here as it's only used on this branch of the if >>> statement. >> >> Okay, will be fixed in v2. >> >>> >>>> + match_set_reg_masked(&match, MFF_LOG_FLAGS - >>>> MFF_REG0, 0, >>>> + MLF_RCV_FROM_RAMP); >>>> + >>> >>> Are we going to match less traffic now? We add this "flags & >>> MLF_RCV_FROM_RAMP" condition to the already existing priority-100 >>> OFTABLE_REMOTE_OUTPUT flow. Does this break multicast traffic not >>> received from RAMP (vtep) ports? >> >> Yes, this limits traffic matching on this flow. >> Such flow matches only traffic, which was received from non-RAMP lports. >> We need to flood traffic to all multicast group ports only if it was >> received from non-RAMP ports. >> This is due to fact that flood on the directon "from vtep" is done on >> the vtep (ramp) switch: it floods BUM to all hypervisors. >> If we don’t do it, we’ll duplicate BUM traffic (first packet is >> originally sent from VTEP switch to all hypervisors) and others are >> replicated on all hypervisors and re-sent to other hypervisors. >> >> Please, let me know if this makes sense. >> > > It does, sorry for the noise. I missed the value = 0 argument. > > Thanks for the clarification! > > I'm planning to apply the first 4 patches today and I'll look forward > for a v2 of this patch afterwards.
Okay, thanks. I’ll submit v2 right after #1-#4 merge. > > Regards, > Dumitru > Regards, Vladislav Odintsov _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
