>>>>> 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,
>>>
>>> Hi Shy Liu,
>>>
>>>> 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.
>>>
>>> For e/w routed traffic from pod A to pod B, pod A will send an arp request
>>> for the router ip address. Who is going to reply to this request in this
>>> way?
>> Hi Lorenzo,
>> sorry that I had not describe it clearly.
>> We can disable arp response for the lsp which type is not router port nor
>> localport.
>> So the logical_switch still responses the arp request for lrp.
>
> IIUC what you mean here, this approach will be helpful just if pod A and pod B
> are on the same logical switch but they are running on two different
> hypervisors,
> right? What if pod A and pod B are on the same logical switch and they are
> running
> on the same HV?
>
> Regards,
> Lorenzo
Arp request always been broadcasted to the lsps those on the
same logical_switch.
I had hardcode it like what i said, and it worked fine for that vm A and vm B
are running on the same HV:
@@ -7529,6 +7541,13 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port
*op,
return;
}
+ if (!op->od->has_arp_proxy && !lsp_is_router(op->nbsp) &&
+ strcmp(op->nbsp->type, "localport")) {
+ /* When disable arp_proxy, not Add ARP/ND reply flows
+ * for a port unless router port or localport. */
+ return;
+ }
+
for (size_t i = 0; i < op->n_lsp_addrs; i++) {
for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) {
ds_clear(match);
But there maybe any other issues I have not consider :)
>>
>>> If ovn replies to the arp request from pod A but let the request go through
>>> the router pipeline I think even the other hypervisor will reply to the arp
>>> request (please remember the logical router is distributed in ovn) and so we
>>> will confuse the TOR switch since we will see the router mac address
>>> flapping
>>> between two ports. Do you agree or am I missing something?
>>
>> We can use 'ovn-chassis-mac-mappings’ option to avoid the problem
>> that the lrp's mac address flap. Right ?
>>
>>
>>>
>>> Regards,
>>> Lorenzo
>>
>>>
>>>>
>>>> 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
