> > On 7/13/22 18:07, Lorenzo Bianconi wrote: >>> On 6/17/22 00:31, Lorenzo Bianconi wrote: >>>> When using VLAN backed networks and OVN routers leveraging the >>>> 'ovn-chassis-mac-mappings' option, the eth.src field is replaced by the >>>> chassis mac address in order to not expose the router mac address from >>>> different nodes and confuse the TOR switch. However doing so the TOR >>>> switch is not able to learn the port/mac bindings for routed E/W traffic >>>> and it is force to always flood it. Fix this issue adding the capability >>>> to send GARP traffic for logical switch ports if the corresponding logical >>>> switch has the ovn-lsp-garp parameter set to true in the option column. >>>> More into about the issue can be found here [0]. >>>> >>>> [0] >>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050678.html >>>> >>>> <https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050678.html> >>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2087779 >>>> <https://bugzilla.redhat.com/show_bug.cgi?id=2087779> >>>> >>>> Signed-off-by: Lorenzo Bianconi <[email protected] >>>> <mailto:[email protected]>> >>>> --- >>> >>> Hi Lorenzo, >> >> Hi Dumitru, >> >> Thanks for reviewing it :) >> >>> >>> I have a few concerns with this approach: >>> >>> a. The CMS will have to set this option for all VMs on all logical >>> switches which will enable periodic GARPs for all of them all the time. >>> That seems like quite a lot of broadcast traffic in the fabric. >>> >>> b. There's no guarantee that the GARPs are sent in time to prevent the >>> FDB timeouts on the ToR switches. At best we could make the interval >>> configurable but I don't think this is way better either. >>> >>> c. This is not really introduced by your patch but we will be causing >>> this more often now. With the topology: >>> >>> (HV1) VM1 -- LS1 --- LR -- LS2 -- VM2 (HV2) >>> (VLAN-backed network) >>> >>> HV1 configured with chassis mac mapping HV1-MAC >>> HV2 configured with chassis mac mapping HV2-MAC >>> >>> We're leaking MAC addresses from LS1's broadcast domain (VM1-MAC) and >>> from LS2's broadcast domain (VM2-MAC) into the fabric. I'm not sure >>> that's OK. >>> >>> I think a proper solution is to change how we run the logical pipelines >>> in case of vlan-backed networks. We currently have an assymetry: >>> >>> For packets flowing from VM1 to VM2 we execute: >>> - on HV1: LS1-ingress, LS1-egress, LR-ingress, LR-egress, LS2-ingress >>> - on HV2: LS2-egress >>> >>> For packets flowing from VM2 to VM1 we execute: >>> - on HV2: LS2-ingress, LS2-egress, LR-ingress, LR-egress, LS1-ingress >>> - on HV1: LS1-egress >>> >>> What if we change this to: >>> VM1 -> VM2: >>> - on HV1: LS1-ingress, LS1-egress, LR-ingress >>> - on HV2: LR-egress, LS2-ingress, LS2-egress >>> >>> VM2 -> VM1: >>> - on HV2: LS2-ingress, LS2-egress, LR-ingress >>> - on HV2: LR-egress, LS1-ingress, LS1-egress >> >> I do not know why the current architecture is done this way (any >> suggestions??). >> I guess the approach you suggested should work. Are we introducing any >> backward >> compatibility issue? > > We would probably create a compatibility issue. :) > > I don't know if this approach would even work, I was just trying to > imagine how traffic from within OVN should be seen on the fabric in this > case. > > Maybe we need to think some more about other options. > > Regards, > Dumitru Hi Dumitru and Lorenzo, in my opinion, the flood issue maybe caused by the implemention that logical_switch always be an arp-proxy for lsp which has known address, but this implemention is not applicable to vlan backed network. Right ?
I have an solution: Maybe we can add a option named arp-proxy for logical_switch that always be false if network type is vlan, then disable the logical_switch response the arp for the lsp which belongs to the vlan backed network. So that this solution can let the destination MAC address always learnt by TOR for vlan backed network. But the lrp might learn the destination MAC address and put_arp, so this operation also should been disabled. This is not useful for logical_switch which is external net. Reference: [0] https://mail.openvswitch.org/pipermail/ovs-discuss/2022-May/051837.html <https://mail.openvswitch.org/pipermail/ovs-discuss/2022-May/051837.html>. > >> >> Regards, >> Lorenzo >> >>> >>> Would this we ensure that we always only use ovn-chassis-mac-mappings on >>> the VLAN network and avoid flooding on the ToR? >>> >>> Regards, >>> Dumitru >>> >>>> controller/pinctrl.c | 85 +++++++++++++++++++++++++++++--------------- >>>> northd/northd.c | 29 +++++++++++++++ >>>> 2 files changed, 85 insertions(+), 29 deletions(-) >>>> >>>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>>> index 9a1a0faa1..eb5739bfc 100644 >>>> --- a/controller/pinctrl.c >>>> +++ b/controller/pinctrl.c >>>> @@ -4533,7 +4533,8 @@ send_garp_rarp(struct rconn *swconn, struct >>>> garp_rarp_data *garp_rarp, >>>> garp_rarp->backoff *= 2; >>>> garp_rarp->announce_time = current_time + garp_rarp->backoff * >>>> 1000; >>>> } else { >>>> - garp_rarp->announce_time = LLONG_MAX; >>>> + /* Default timeout is 180s. */ >>>> + garp_rarp->announce_time = current_time + 180 * 1000; >>>> } >>>> return garp_rarp->announce_time; >>>> } >>>> @@ -5510,14 +5511,15 @@ ip_mcast_querier_wait(long long int query_time) >>>> >>>> /* Get localnet vifs, local l3gw ports and ofport for localnet patch >>>> ports. */ >>>> static void >>>> -get_localnet_vifs_l3gwports( >>>> +get_local_vifs_l3gwports( >>>> struct ovsdb_idl_index *sbrec_port_binding_by_datapath, >>>> struct ovsdb_idl_index *sbrec_port_binding_by_name, >>>> const struct ovsrec_bridge *br_int, >>>> const struct sbrec_chassis *chassis, >>>> const struct hmap *local_datapaths, >>>> struct sset *localnet_vifs, >>>> - struct sset *local_l3gw_ports) >>>> + struct sset *local_l3gw_ports, >>>> + struct sset *local_vifs) >>>> { >>>> for (int i = 0; i < br_int->n_ports; i++) { >>>> const struct ovsrec_port *port_rec = br_int->ports[i]; >>>> @@ -5574,7 +5576,8 @@ get_localnet_vifs_l3gwports( >>>> /* Get l3gw ports. Consider port bindings with type "l3gateway" >>>> * that connect to gateway routers (if local), and consider port >>>> * bindings of type "patch" since they might connect to >>>> - * distributed gateway ports with NAT addresses. */ >>>> + * distributed gateway ports with NAT addresses. >>>> + * Get LSP ports if requested by CMS. */ >>>> >>>> sbrec_port_binding_index_set_datapath(target, ld->datapath); >>>> SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target, >>>> @@ -5583,6 +5586,11 @@ get_localnet_vifs_l3gwports( >>>> || !strcmp(pb->type, "patch")) { >>>> sset_add(local_l3gw_ports, pb->logical_port); >>>> } >>>> + /* GARP packets for lsp ports. */ >>>> + if (pb->chassis == chassis && >>>> + smap_get_bool(&pb->options, "ovn-lsp-garp", false)) { >>>> + sset_add(local_vifs, pb->logical_port); >>>> + } >>>> } >>>> } >>>> sbrec_port_binding_index_destroy_row(target); >>>> @@ -5761,6 +5769,26 @@ send_garp_rarp_run(struct rconn *swconn, long long >>>> int *send_garp_rarp_time) >>>> } >>>> } >>>> >>>> +static void >>>> +send_garp_rarp_update_for_pb_set( >>>> + struct ovsdb_idl_txn *ovnsb_idl_txn, >>>> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, >>>> + struct ovsdb_idl_index *sbrec_port_binding_by_name, >>>> + struct sset *vif_set, const struct hmap *local_datapaths, >>>> + struct shash *nat_addresses) >>>> +{ >>>> + const char *iface_id; >>>> + SSET_FOR_EACH (iface_id, vif_set) { >>>> + const struct sbrec_port_binding *pb = lport_lookup_by_name( >>>> + sbrec_port_binding_by_name, iface_id); >>>> + if (pb) { >>>> + send_garp_rarp_update(ovnsb_idl_txn, >>>> + sbrec_mac_binding_by_lport_ip, >>>> + local_datapaths, pb, nat_addresses); >>>> + } >>>> + } >>>> +} >>>> + >>>> /* Called by pinctrl_run(). Runs with in the main ovn-controller >>>> * thread context. */ >>>> static void >>>> @@ -5776,15 +5804,17 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn >>>> *ovnsb_idl_txn, >>>> { >>>> struct sset localnet_vifs = SSET_INITIALIZER(&localnet_vifs); >>>> struct sset local_l3gw_ports = SSET_INITIALIZER(&local_l3gw_ports); >>>> + struct sset local_vifs = SSET_INITIALIZER(&local_vifs); >>>> struct sset nat_ip_keys = SSET_INITIALIZER(&nat_ip_keys); >>>> struct shash nat_addresses; >>>> >>>> shash_init(&nat_addresses); >>>> >>>> - get_localnet_vifs_l3gwports(sbrec_port_binding_by_datapath, >>>> - sbrec_port_binding_by_name, >>>> - br_int, chassis, local_datapaths, >>>> - &localnet_vifs, &local_l3gw_ports); >>>> + get_local_vifs_l3gwports(sbrec_port_binding_by_datapath, >>>> + sbrec_port_binding_by_name, >>>> + br_int, chassis, local_datapaths, >>>> + &localnet_vifs, &local_l3gw_ports, >>>> + &local_vifs); >>>> >>>> get_nat_addresses_and_keys(sbrec_port_binding_by_name, >>>> &nat_ip_keys, &local_l3gw_ports, >>>> @@ -5795,36 +5825,33 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn >>>> *ovnsb_idl_txn, >>>> struct shash_node *iter; >>>> SHASH_FOR_EACH_SAFE (iter, &send_garp_rarp_data) { >>>> if (!sset_contains(&localnet_vifs, iter->name) && >>>> - !sset_contains(&nat_ip_keys, iter->name)) { >>>> + !sset_contains(&nat_ip_keys, iter->name) && >>>> + !sset_contains(&local_vifs, iter->name)) { >>>> send_garp_rarp_delete(iter->name); >>>> } >>>> } >>>> >>>> /* Update send_garp_rarp_data. */ >>>> - const char *iface_id; >>>> - SSET_FOR_EACH (iface_id, &localnet_vifs) { >>>> - const struct sbrec_port_binding *pb = lport_lookup_by_name( >>>> - sbrec_port_binding_by_name, iface_id); >>>> - if (pb) { >>>> - send_garp_rarp_update(ovnsb_idl_txn, >>>> - sbrec_mac_binding_by_lport_ip, >>>> - local_datapaths, pb, &nat_addresses); >>>> - } >>>> - } >>>> - >>>> + send_garp_rarp_update_for_pb_set(ovnsb_idl_txn, >>>> + sbrec_mac_binding_by_lport_ip, >>>> + sbrec_port_binding_by_name, >>>> + &localnet_vifs, local_datapaths, >>>> + &nat_addresses); >>>> + send_garp_rarp_update_for_pb_set(ovnsb_idl_txn, >>>> + sbrec_mac_binding_by_lport_ip, >>>> + sbrec_port_binding_by_name, >>>> + &local_vifs, local_datapaths, >>>> + &nat_addresses); >>>> /* Update send_garp_rarp_data for nat-addresses. */ >>>> - const char *gw_port; >>>> - SSET_FOR_EACH (gw_port, &local_l3gw_ports) { >>>> - const struct sbrec_port_binding *pb >>>> - = lport_lookup_by_name(sbrec_port_binding_by_name, gw_port); >>>> - if (pb) { >>>> - send_garp_rarp_update(ovnsb_idl_txn, >>>> sbrec_mac_binding_by_lport_ip, >>>> - local_datapaths, pb, &nat_addresses); >>>> - } >>>> - } >>>> + send_garp_rarp_update_for_pb_set(ovnsb_idl_txn, >>>> + sbrec_mac_binding_by_lport_ip, >>>> + sbrec_port_binding_by_name, >>>> + &local_l3gw_ports, local_datapaths, >>>> + &nat_addresses); >>>> >>>> /* pinctrl_handler thread will send the GARPs. */ >>>> >>>> + sset_destroy(&local_vifs); >>>> sset_destroy(&localnet_vifs); >>>> sset_destroy(&local_l3gw_ports); >>>> >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index 0d6ebccde..9a4a880a7 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -6446,6 +6446,34 @@ ovn_update_ipv6_options(struct hmap *ports) >>>> } >>>> } >>>> >>>> +static void >>>> +ovn_update_lsp_garp_options(struct hmap *ports) >>>> +{ >>>> + struct ovn_port *op; >>>> + HMAP_FOR_EACH (op, key_node, ports) { >>>> + if (!op->nbsp) { >>>> + continue; >>>> + } >>>> + if (op->nbsp->type[0] || op->nbsp->parent_name) { >>>> + continue; >>>> + } >>>> + >>>> + struct ovn_datapath *od = op->od; >>>> + if (!od || !od->nbs) { >>>> + continue; >>>> + } >>>> + >>>> + struct smap options; >>>> + smap_clone(&options, &op->sb->options); >>>> + >>>> + bool ovn_lsp_garp = smap_get_bool(&od->nbs->other_config, >>>> + "ovn-lsp-garp", false); >>>> + smap_add(&options, "ovn-lsp-garp", ovn_lsp_garp ? "true" : >>>> "false"); >>>> + sbrec_port_binding_set_options(op->sb, &options); >>>> + smap_destroy(&options); >>>> + } >>>> +} >>>> + >>>> static void >>>> build_port_group_lswitches(struct northd_input *input_data, >>>> struct hmap *pgs, >>>> @@ -15381,6 +15409,7 @@ ovnnb_db_run(struct northd_input *input_data, >>>> stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); >>>> ovn_update_ipv6_options(&data->ports); >>>> ovn_update_ipv6_prefix(&data->ports); >>>> + ovn_update_lsp_garp_options(&data->ports); >>>> >>>> sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); >>>> sync_address_sets(input_data, ovnsb_txn, &data->datapaths); >>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
