On 9/13/24 18:20, Shibir Basak wrote: > Hi Dumitru/team, > Hi Shibir,
> Please feel free to take a look and share your comments (if any) on this > v2 patch. > Sorry for the delay in reviewing the v2 patch. I'm starting now. Regards, Dumitru > Thanks, > Shibir > >> On 26-Aug-2024, at 12:56 PM, Shibir Basak <[email protected]> >> wrote: >> >> Hi Dumitru, >> >> I have sent a v2 patch on top of this with some changes along with rebase. >> Please feel free add review comments or suggestions. >> >> Thanks, >> Shibir >> >>> On 16-Aug-2024, at 7:35 PM, Shibir Basak <[email protected]> >>> wrote: >>> >>> >>> >>>> On 15-Aug-2024, at 1:20 AM, Dumitru Ceara <[email protected]> wrote: >>>> >>>> !-------------------------------------------------------------------| >>>> CAUTION: External Email >>>> >>>> |-------------------------------------------------------------------! >>>> >>>> On 8/14/24 13:24, Shibir Basak wrote: >>>>> Hi Dumitru, >>>>> >>>>> Thanks for your comments. >>>>> I have responded inline. >>>>> >>>>>> On 07-Aug-2024, at 4:58 PM, Dumitru Ceara <[email protected]> wrote: >>>>>> >>>>>> !-------------------------------------------------------------------| >>>>>> CAUTION: External Email >>>>>> >>>>>> |-------------------------------------------------------------------! >>>>>> >>>>>> On 8/2/24 17:07, Shibir Basak wrote: >>>>>>> When a VIF port goes down, all FDB entries associated >>>>>>> with it are deleted. >>>>>>> This helps is reducing traffic outage due to stale FDB >>>>>>> entries present until the lsp is removed. >>>>>>> >>>>>>> Acked-by: Naveen Yerramneni <[email protected]> >>>>>>> Signed-off-by: Shibir Basak <[email protected]> >>>>>>> --- >>>>>> >>>>>> Hi Shibir, >>>>>> >>>>>> Thanks for the patch! >>>>>> >>>>>> As this is a bug fix it probably needs a: >>>>>> Fixes: dd94f1266ca4 ("northd: MAC learning: Add logical flows for >>>>>> fdb.”) >>>>> Sure >>>>>> >>>>>> I have a few more comments below. >>>>>> >>>>>>> controller/ovn-controller.c | 5 ++++ >>>>>>> controller/pinctrl.c | 40 ++++++++++++++++++++------ >>>>>>> controller/pinctrl.h | 1 + >>>>>>> lib/automake.mk | 4 ++- >>>>>>> lib/fdb.c | 33 +++++++++++++++++++++ >>>>>>> lib/fdb.h | 26 +++++++++++++++++ >>>>>>> tests/ovn.at | 57 >>>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>>> 7 files changed, 157 insertions(+), 9 deletions(-) >>>>>>> create mode 100644 lib/fdb.c >>>>>>> create mode 100644 lib/fdb.h >>>>>>> >>>>>>> diff --git a/controller/ovn-controller.c >>>>>>> b/controller/ovn-controller.c >>>>>>> index 6a7cca673..2acdddd09 100644 >>>>>>> --- a/controller/ovn-controller.c >>>>>>> +++ b/controller/ovn-controller.c >>>>>>> @@ -4936,6 +4936,10 @@ main(int argc, char *argv[]) >>>>>>> = ovsdb_idl_index_create2(ovnsb_idl_loop.idl, >>>>>>> &sbrec_fdb_col_mac, >>>>>>> &sbrec_fdb_col_dp_key); >>>>>>> + struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port >>>>>>> + = ovsdb_idl_index_create2(ovnsb_idl_loop.idl, >>>>>>> + &sbrec_fdb_col_port_key, >>>>>>> + &sbrec_fdb_col_dp_key); >>>>>>> struct ovsdb_idl_index *sbrec_mac_binding_by_datapath >>>>>>> = mac_binding_by_datapath_index_create(ovnsb_idl_loop.idl); >>>>>>> struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath >>>>>>> @@ -5645,6 +5649,7 @@ main(int argc, char *argv[]) >>>>>>> sbrec_igmp_group, >>>>>>> sbrec_ip_multicast, >>>>>>> sbrec_fdb_by_dp_key_mac, >>>>>>> + sbrec_fdb_by_dp_and_port, >>>>>>> >>>>>>> sbrec_dns_table_get(ovnsb_idl_loop.idl), >>>>>>> sbrec_controller_event_table_get( >>>>>>> ovnsb_idl_loop.idl), >>>>>>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>>>>>> index 7cbb0cf81..9e37e1693 100644 >>>>>>> --- a/controller/pinctrl.c >>>>>>> +++ b/controller/pinctrl.c >>>>>>> @@ -62,6 +62,7 @@ >>>>>>> #include "lflow.h" >>>>>>> #include "ip-mcast.h" >>>>>>> #include "ovn-sb-idl.h" >>>>>>> +#include "lib/fdb.h" >>>>>>> >>>>>>> VLOG_DEFINE_THIS_MODULE(pinctrl); >>>>>>> >>>>>>> @@ -236,6 +237,7 @@ static void send_garp_rarp_prepare( >>>>>>> struct ovsdb_idl_index *sbrec_port_binding_by_datapath, >>>>>>> struct ovsdb_idl_index *sbrec_port_binding_by_name, >>>>>>> struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip, >>>>>>> + struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port, >>>>>>> const struct ovsrec_bridge *, >>>>>>> const struct sbrec_chassis *, >>>>>>> const struct hmap *local_datapaths, >>>>>>> @@ -4145,6 +4147,7 @@ pinctrl_run(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> struct ovsdb_idl_index *sbrec_igmp_groups, >>>>>>> struct ovsdb_idl_index *sbrec_ip_multicast_opts, >>>>>>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, >>>>>>> + struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port, >>>>>>> const struct sbrec_dns_table *dns_table, >>>>>>> const struct sbrec_controller_event_table *ce_table, >>>>>>> const struct sbrec_service_monitor_table *svc_mon_table, >>>>>>> @@ -4166,7 +4169,8 @@ pinctrl_run(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> sbrec_port_binding_by_key, chassis); >>>>>>> send_garp_rarp_prepare(ovnsb_idl_txn, >>>>>>> sbrec_port_binding_by_datapath, >>>>>>> sbrec_port_binding_by_name, >>>>>>> - sbrec_mac_binding_by_lport_ip, br_int, >>>>>>> chassis, >>>>>>> + sbrec_mac_binding_by_lport_ip, >>>>>>> + sbrec_fdb_by_dp_and_port, br_int, >>>>>>> chassis, >>>>>>> local_datapaths, active_tunnels, >>>>>>> ovs_table); >>>>>>> prepare_ipv6_ras(local_active_ports_ras, >>>>>>> sbrec_port_binding_by_name); >>>>>>> prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, >>>>>>> @@ -5017,7 +5021,6 @@ wait_put_mac_bindings(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn) >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> - >>>>>> >>>>>> Unrelated change. This line feed should not be removed. >>>>> Sure, will revert in v2. >>>>>> >>>>>>> /* >>>>>>> * Send gratuitous/reverse ARP for vif on localnet. >>>>>>> * >>>>>>> @@ -5034,6 +5037,7 @@ struct garp_rarp_data { >>>>>>> * announcement (in msecs). */ >>>>>>> uint32_t dp_key; /* Datapath used to output this >>>>>>> GARP. */ >>>>>>> uint32_t port_key; /* Port to inject the GARP into. */ >>>>>>> + bool enabled; /* is garp/rarp announcement >>>>>>> enabled */ >>>>>>> }; >>>>>>> >>>>>>> /* Contains GARPs/RARPs to be sent. Protected by pinctrl_mutex*/ >>>>>>> @@ -5054,7 +5058,7 @@ destroy_send_garps_rarps(void) >>>>>>> /* Runs with in the main ovn-controller thread context. */ >>>>>>> static void >>>>>>> add_garp_rarp(const char *name, const struct eth_addr ea, >>>>>>> ovs_be32 ip, >>>>>>> - uint32_t dp_key, uint32_t port_key) >>>>>>> + uint32_t dp_key, uint32_t port_key, bool enabled) >>>>>>> { >>>>>>> struct garp_rarp_data *garp_rarp = xmalloc(sizeof *garp_rarp); >>>>>>> garp_rarp->ea = ea; >>>>>>> @@ -5063,6 +5067,7 @@ add_garp_rarp(const char *name, const struct >>>>>>> eth_addr ea, ovs_be32 ip, >>>>>>> garp_rarp->backoff = 1000; /* msec. */ >>>>>>> garp_rarp->dp_key = dp_key; >>>>>>> garp_rarp->port_key = port_key; >>>>>>> + garp_rarp->enabled = enabled; >>>>>>> shash_add(&send_garp_rarp_data, name, garp_rarp); >>>>>>> >>>>>>> /* Notify pinctrl_handler so that it can wakeup and process >>>>>>> @@ -5081,6 +5086,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> bool garp_continuous) >>>>>>> { >>>>>>> volatile struct garp_rarp_data *garp_rarp = NULL; >>>>>>> + bool is_garp_rarp_enabled = >>>>>>> !smap_get_bool(&binding_rec->options, >>>>>>> + "disable_garp_rarp", >>>>>>> false); >>>>>>> >>>>>>> /* Skip localports as they don't need to be announced */ >>>>>>> if (!strcmp(binding_rec->type, "localport")) { >>>>>>> @@ -5104,6 +5111,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> if (garp_rarp) { >>>>>>> garp_rarp->dp_key = >>>>>>> binding_rec->datapath->tunnel_key; >>>>>>> garp_rarp->port_key = binding_rec->tunnel_key; >>>>>>> + garp_rarp->enabled = is_garp_rarp_enabled; >>>>>>> if (garp_max_timeout != garp_rarp_max_timeout || >>>>>>> garp_continuous != garp_rarp_continuous) { >>>>>>> /* reset backoff */ >>>>>>> @@ -5114,7 +5122,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> add_garp_rarp(name, laddrs->ea, >>>>>>> laddrs->ipv4_addrs[i].addr, >>>>>>> binding_rec->datapath->tunnel_key, >>>>>>> - binding_rec->tunnel_key); >>>>>>> + binding_rec->tunnel_key, >>>>>>> + is_garp_rarp_enabled); >>>>>>> send_garp_locally(ovnsb_idl_txn, >>>>>>> sbrec_mac_binding_by_lport_ip, >>>>>>> local_datapaths, binding_rec, >>>>>>> laddrs->ea, >>>>>>> @@ -5134,6 +5143,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> if (garp_rarp) { >>>>>>> garp_rarp->dp_key = >>>>>>> binding_rec->datapath->tunnel_key; >>>>>>> garp_rarp->port_key = >>>>>>> binding_rec->tunnel_key; >>>>>>> + garp_rarp->enabled = is_garp_rarp_enabled; >>>>>>> if (garp_max_timeout != >>>>>>> garp_rarp_max_timeout || >>>>>>> garp_continuous != >>>>>>> garp_rarp_continuous) { >>>>>>> /* reset backoff */ >>>>>>> @@ -5143,7 +5153,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> } else { >>>>>>> add_garp_rarp(name, laddrs->ea, >>>>>>> 0, >>>>>>> binding_rec->datapath->tunnel_key, >>>>>>> - binding_rec->tunnel_key); >>>>>>> + binding_rec->tunnel_key, >>>>>>> + is_garp_rarp_enabled); >>>>>>> } >>>>>>> free(name); >>>>>>> } >>>>>>> @@ -5159,6 +5170,7 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> if (garp_rarp) { >>>>>>> garp_rarp->dp_key = binding_rec->datapath->tunnel_key; >>>>>>> garp_rarp->port_key = binding_rec->tunnel_key; >>>>>>> + garp_rarp->enabled = is_garp_rarp_enabled; >>>>>>> if (garp_max_timeout != garp_rarp_max_timeout || >>>>>>> garp_continuous != garp_rarp_continuous) { >>>>>>> /* reset backoff */ >>>>>>> @@ -5184,7 +5196,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> add_garp_rarp(binding_rec->logical_port, >>>>>>> laddrs.ea, ip, >>>>>>> binding_rec->datapath->tunnel_key, >>>>>>> - binding_rec->tunnel_key); >>>>>>> + binding_rec->tunnel_key, >>>>>>> + is_garp_rarp_enabled); >>>>>>> if (ip) { >>>>>>> send_garp_locally(ovnsb_idl_txn, >>>>>>> sbrec_mac_binding_by_lport_ip, >>>>>>> local_datapaths, binding_rec, >>>>>>> laddrs.ea, ip); >>>>>>> @@ -6547,6 +6560,10 @@ send_garp_rarp_run(struct rconn *swconn, long >>>>>>> long int *send_garp_rarp_time) >>>>>>> long long int current_time = time_msec(); >>>>>>> *send_garp_rarp_time = LLONG_MAX; >>>>>>> SHASH_FOR_EACH (iter, &send_garp_rarp_data) { >>>>>>> + struct garp_rarp_data *garp_rarp = iter->data; >>>>>>> + if (!garp_rarp->enabled) { >>>>>>> + continue; >>>>>>> + } >>>>>>> long long int next_announce = send_garp_rarp(swconn, >>>>>>> iter->data, >>>>>>> current_time); >>>>>>> if (*send_garp_rarp_time > next_announce) { >>>>>>> @@ -6562,6 +6579,7 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> struct ovsdb_idl_index >>>>>>> *sbrec_port_binding_by_datapath, >>>>>>> struct ovsdb_idl_index >>>>>>> *sbrec_port_binding_by_name, >>>>>>> struct ovsdb_idl_index >>>>>>> *sbrec_mac_binding_by_lport_ip, >>>>>>> + struct ovsdb_idl_index >>>>>>> *sbrec_fdb_by_dp_and_port, >>>>>>> const struct ovsrec_bridge *br_int, >>>>>>> const struct sbrec_chassis *chassis, >>>>>>> const struct hmap *local_datapaths, >>>>>>> @@ -6597,12 +6615,18 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> &nat_ip_keys, &local_l3gw_ports, >>>>>>> chassis, active_tunnels, >>>>>>> &nat_addresses); >>>>>>> + >>>>>>> /* For deleted ports and deleted nat ips, remove from >>>>>>> * send_garp_rarp_data. */ >>>>>>> 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)) { >>>>>>> + struct garp_rarp_data *data = iter->data; >>>>>>> + /* Cleanup FDB entries for inactive ports */ >>>>>>> + delete_fdb_entries(sbrec_fdb_by_dp_and_port, >>>>>>> + data->dp_key, >>>>>>> + data->port_key); >>>>>> >>>>>> It feels wrong to use the garp/rarp mechanism to cleanup FDB on port >>>>>> deletion. It also forces the addition of the 'is_garp_rarp_enabled' >>>>>> flag and creating 'garp_rarp_data' structures for cases in which we >>>>>> don't really need them (we send no garps/rarps). >>>>>> >>>>>> What if we do this in a different way? >>>>>> >>>>>> In binding.c we set port_binding.up to "false" whenever ports are >>>>>> removed. Should we cleanup stale FDB entries there instead? >>>>>> >>>>>> There are two places where we do that: >>>>>> binding_lport_set_down() >>>>>> port_binding_set_down() >>>>>> >>>>>> What do you think? >>>>> >>>>> Here, we are also trying to address the scenario where the vif port >>>>> link >>>>> state goes down >>>>> and not only handle scenarios such as port removal or iface-id >>>>> detachment. >>>>> >>>>> For example, when a VM migration is completed and the hypervisor is in >>>>> process of removing >>>>> the VM on the source side, the VIF port state goes down. We need to >>>>> detect the port link state >>>>> down event and delete the FDB entries as soon as possible to reduce >>>>> traffic loss. >>>>> >>>>> We clubbed it with garp_rarp code because this code is already tracking >>>>> the VLAN bridged ports. >>>>> >>>> >>>> Sorry but I still don't really understand how you're addressing the VIF >>>> port link going down. Do you mind detailing that part, please? >>>> >>>> Would it make sense to add a system test (system-ovn.at >>>> <http://system-ovn.at/>) for this case >>>> too then? >>>> >>>> I still think we should try to not overload the semantics of the >>>> garp/rarp code in pinctrl.c but maybe I'm misunderstanding the >>>> situation. >>>> >>>> Thanks, >>>> Dumitru >>> >>> Sometime back as part of the commit >>> d34509941ea6c9b5e5847a9f96ea5f235f56cccd we >>> had registered for interface link-state events in ovn-controller. >>> Hence, in case of a link state change, send_garp_rarp_prepare() is >>> triggered which >>> gets the latest active ports via get_localnet_vifs_l3gwports(), and >>> compares with the stored >>> set of ports in the send_garp_rarp_data and takes action >>> (send/delete) accordingly. >>> >>> We planned to reuse these garp_rarp data structures to avoid a lot of >>> code duplication. >>> >>> Presently we have added a test case which is closely simulating a >>> link down event, if not exactly. >>> "test x`ovn-nbctl lsp-get-up vif0` = xdown” >>> Perhaps, I will check in system-on.at <http://system-on.at/> if there >>> is a better way. >>> >>> Do let me know your views or if you have any further queries. >>> >>> Thanks, >>> Shibir >>>> >>>>>> >>>>>>> send_garp_rarp_delete(iter->name); >>>>>>> } >>>>>>> } >>>>>>> @@ -6612,7 +6636,7 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> 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 && !smap_get_bool(&pb->options, "disable_garp_rarp", >>>>>>> false)) { >>>>>>> + if (pb) { >>>>>>> send_garp_rarp_update(ovnsb_idl_txn, >>>>>>> sbrec_mac_binding_by_lport_ip, >>>>>>> local_datapaths, pb, >>>>>>> &nat_addresses, >>>>>>> @@ -6625,7 +6649,7 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> 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 && !smap_get_bool(&pb->options, "disable_garp_rarp", >>>>>>> false)) { >>>>>>> + if (pb) { >>>>>>> send_garp_rarp_update(ovnsb_idl_txn, >>>>>>> sbrec_mac_binding_by_lport_ip, >>>>>>> local_datapaths, pb, >>>>>>> &nat_addresses, >>>>>>> garp_max_timeout, garp_continuous); >>>>>>> diff --git a/controller/pinctrl.h b/controller/pinctrl.h >>>>>>> index 3462b670c..6613c87e4 100644 >>>>>>> --- a/controller/pinctrl.h >>>>>>> +++ b/controller/pinctrl.h >>>>>>> @@ -49,6 +49,7 @@ void pinctrl_run(struct ovsdb_idl_txn >>>>>>> *ovnsb_idl_txn, >>>>>>> struct ovsdb_idl_index *sbrec_igmp_groups, >>>>>>> struct ovsdb_idl_index *sbrec_ip_multicast_opts, >>>>>>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, >>>>>>> + struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port, >>>>>>> const struct sbrec_dns_table *, >>>>>>> const struct sbrec_controller_event_table *, >>>>>>> const struct sbrec_service_monitor_table *, >>>>>>> diff --git a/lib/automake.mk b/lib/automake.mk >>>>>>> index b69e854b0..3ff098331 100644 >>>>>>> --- a/lib/automake.mk >>>>>>> +++ b/lib/automake.mk >>>>>>> @@ -49,7 +49,9 @@ lib_libovn_la_SOURCES = \ >>>>>>> lib/stopwatch-names.h \ >>>>>>> lib/vif-plug-provider.h \ >>>>>>> lib/vif-plug-provider.c \ >>>>>>> -lib/vif-plug-providers/dummy/vif-plug-dummy.c >>>>>>> +lib/vif-plug-providers/dummy/vif-plug-dummy.c \ >>>>>>> +lib/fdb.c \ >>>>>>> +lib/fdb.h >>>>>>> nodist_lib_libovn_la_SOURCES = \ >>>>>>> lib/ovn-dirs.c \ >>>>>>> lib/ovn-nb-idl.c \ >>>>>>> diff --git a/lib/fdb.c b/lib/fdb.c >>>>>>> new file mode 100644 >>>>>>> index 000000000..00e3ac902 >>>>>>> --- /dev/null >>>>>>> +++ b/lib/fdb.c >>>>>>> @@ -0,0 +1,33 @@ >>>>>>> +/* Copyright (c) 2024, Red Hat, Inc. >>>>>> >>>>>> This is probably a copy/paste error. Should it say Nutanix? >>>>> Sure, we will update appropriately in v2. >>>>>> >>>>>>> + * >>>>>>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>>>>>> + * you may not use this file except in compliance with the License. >>>>>>> + * You may obtain a copy of the License at: >>>>>>> + * >>>>>>> + * >>>>>>> >>>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e=> >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e=>> >>>>>>> >>>>>>> + * >>>>>>> + * Unless required by applicable law or agreed to in writing, >>>>>>> software >>>>>>> + * distributed under the License is distributed on an "AS IS" BASIS, >>>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>>>>>> implied. >>>>>>> + * See the License for the specific language governing >>>>>>> permissions and >>>>>>> + * limitations under the License. >>>>>>> + */ >>>>>>> + >>>>>>> +#include <config.h> >>>>>>> +#include "lib/fdb.h" >>>>>>> + >>>>>>> +void >>>>>>> +delete_fdb_entries(struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port, >>>>>>> + uint32_t dp_key, uint32_t port_key) >>>>>>> +{ >>>>>>> + struct sbrec_fdb *target = >>>>>>> + sbrec_fdb_index_init_row(sbrec_fdb_by_dp_and_port); >>>>>>> + sbrec_fdb_index_set_dp_key(target, dp_key); >>>>>>> + sbrec_fdb_index_set_port_key(target, port_key); >>>>>>> + >>>>>>> + struct sbrec_fdb *fdb_e; >>>>>>> + SBREC_FDB_FOR_EACH_EQUAL (fdb_e, target, >>>>>>> sbrec_fdb_by_dp_and_port) { >>>>>>> + sbrec_fdb_delete(fdb_e); >>>>>>> + } >>>>>>> + sbrec_fdb_index_destroy_row(target); >>>>>>> +} >>>>>>> diff --git a/lib/fdb.h b/lib/fdb.h >>>>>>> new file mode 100644 >>>>>>> index 000000000..7b64bc7f2 >>>>>>> --- /dev/null >>>>>>> +++ b/lib/fdb.h >>>>>>> @@ -0,0 +1,26 @@ >>>>>>> +/* Copyright (c) 2024, Red Hat, Inc. >>>>>> >>>>>> This is probably a copy/paste error. Should it say Nutanix? >>>>> Sure, we will update appropriately in v2. >>>>>> >>>>>>> + * >>>>>>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>>>>>> + * you may not use this file except in compliance with the License. >>>>>>> + * You may obtain a copy of the License at: >>>>>>> + * >>>>>>> + * >>>>>>> >>>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e=> >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=56wz88_J295ONGQivF_7tnPT-sl6OYGFn6SgI0SyDqzhlvk9I43PsRbUI7CzhkEd&s=tQmYz8z4JIoAztE8J_AhDteZBW4vo467WTpXMviFiPs&e=>> >>>>>>> >>>>>>> + * >>>>>>> + * Unless required by applicable law or agreed to in writing, >>>>>>> software >>>>>>> + * distributed under the License is distributed on an "AS IS" BASIS, >>>>>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>>>>>> implied. >>>>>>> + * See the License for the specific language governing >>>>>>> permissions and >>>>>>> + * limitations under the License. >>>>>>> + */ >>>>>>> + >>>>>>> +#ifndef OVN_FDB_H >>>>>>> +#define OVN_FDB_H 1 >>>>>>> + >>>>>>> +#include "ovsdb-idl.h" >>>>>>> +#include "ovn-sb-idl.h" >>>>>>> + >>>>>>> +void >>>>>>> +delete_fdb_entries(struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port, >>>>>>> + uint32_t dp_key, uint32_t port_key); >>>>>>> + >>>>>>> +#endif >>>>>>> diff --git a/tests/ovn.at >>>>>>> <http://ovn.at/> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e=> >>>>>>> > b/tests/ovn.at <http://ovn.at/> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e=> >>>>>>> > >>>>>>> index b31afbfb3..a0df87196 100644 >>>>>>> --- a/tests/ovn.at >>>>>>> <http://ovn.at/> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e=> >>>>>>> > >>>>>>> +++ b/tests/ovn.at >>>>>>> <http://ovn.at/> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e= >>>>>>> >>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at_&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=AEijf5o6HDlnTtOFAM8OgRGx_e563BXUntvK-hUYmYoFNIn5UOad2n04VyTX1Rke&s=nQqUZU3nSpgtVwdzhMon7Lx94Au4jRVtMa38r78TnlE&e=> >>>>>>> > >>>>>>> @@ -31706,6 +31706,63 @@ OVN_CLEANUP([hv1], [hv2], [hv3]) >>>>>>> AT_CLEANUP >>>>>>> ]) >>>>>>> >>>>>>> +OVN_FOR_EACH_NORTHD([ >>>>>>> +AT_SETUP([OVN FDB cleanup when vif goes down]) >>>>>> >>>>>> No real need for "OVN" in the test case name. When running >>>>>> it looks like this: >>>>>> >>>>>> OVN end-to-end tests >>>>>> >>>>>> 405: OVN FDB cleanup when vif goes down -- parallelization=yes -- >>>>>> ovn_monitor_all=yes ok >>>>> Sure, we will update appropriately in v2. >>>>>> >>>>>>> +ovn_start >>>>>>> + >>>>>>> +net_add n1 >>>>>>> + >>>>>>> +AT_CHECK([ovn-nbctl ls-add ls0]) >>>>>>> + >>>>>>> +AT_CHECK([ovn-nbctl lsp-add ls0 ln_port]) >>>>>>> +AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown]) >>>>>>> +AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet]) >>>>>>> +AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1]) >>>>>>> +AT_CHECK([ovn-nbctl set logical_switch_port ln_port >>>>>>> options:localnet_learn_fdb=true]) >>>>>>> + >>>>>>> +AT_CHECK([ovn-nbctl lsp-add ls0 vif0]) >>>>>>> +AT_CHECK([ovn-nbctl lsp-set-addresses vif0 "00:00:00:00:10:10 >>>>>>> 192.168.10.10" unknown]) >>>>>> >>>>>> All these "AT_CHECK" can be replaced with "check". >>>>> Sure, we will address test related comments appropriately in v2. >>>>>> >>>>>>> + >>>>>>> +sim_add hv1 >>>>>>> +as hv1 >>>>>>> +ovs-vsctl add-br br-phys >>>>>>> +ovn_attach n1 br-phys 192.168.0.1 >>>>>>> +ovs-vsctl -- add-port br-int vif0 -- \ >>>>>>> + set interface vif0 external-ids:iface-id=vif0 \ >>>>>>> + options:tx_pcap=hv1/vif0-tx.pcap \ >>>>>>> + options:rxq_pcap=hv1/vif0-rx.pcap \ >>>>>>> + ofport-request=1 >>>>>>> +ovs-vsctl -- add-port br-phys ext0 -- \ >>>>>>> + set interface ext0 \ >>>>>>> + options:tx_pcap=hv1/ext0-tx.pcap \ >>>>>>> + options:rxq_pcap=hv1/ext0-rx.pcap \ >>>>>>> + ofport-request=2 >>>>>>> +ovs-vsctl set open . >>>>>>> external_ids:ovn-bridge-mappings=physnet1:br-phys >>>>>>> + >>>>>>> + >>>>>>> +wait_for_ports_up >>>>>> >>>>>> You should probably add a sync here: >>>>>> >>>>>> check ovn-nbctl --wait=hv sync >>>>>> >>>>>>> +ls0_key=$(fetch_column datapath_binding tunnel_key >>>>>>> external_ids:name=ls0) >>>>>>> +vif0_key=$(fetch_column port_binding tunnel_key logical_port=vif0) >>>>>>> + >>>>>>> +ovn-sbctl create FDB mac="00\:00\:00\:00\:10\:10" dp_key=$ls0_key >>>>>>> port_key=$vif0_key >>>>>>> +ovn-sbctl create FDB mac="00\:00\:00\:00\:20\:20" dp_key=$ls0_key >>>>>>> port_key=$vif0_key >>>>>>> + >>>>>>> +ovn-sbctl list fdb >>>>>> >>>>>> This is not really needed for the test, it can probably be removed. >>>>>> >>>>>>> +# vif going down should purge all relevant FDB entries >>>>>>> +ovs-vsctl del-port br-int vif0 >>>>>>> +OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up vif0` = xdown]) >>>>>>> +AT_CHECK([ovn-nbctl --wait=hv sync]) >>>>>> >>>>>> Please replace AT_CHECK with "check". >>>>>> >>>>>>> +ovn-sbctl list fdb >>>>>> >>>>>> This one can be removed. >>>>>> >>>>>>> +OVS_WAIT_UNTIL([test $(ovn-sbctl list fdb | grep -c >>>>>>> "00:00:00:00:10:10") = 0]) >>>>>>> +OVS_WAIT_UNTIL([test $(ovn-sbctl list fdb | grep -c >>>>>>> "00:00:00:00:20:20") = 0])> + >>>>>>> +# vif0 should not be a local datapath in hv1 as vif0 is down >>>>>>> +# Hence ln_port should not be a related port. >>>>>>> +OVN_CLEANUP([hv1 >>>>>>> +ln_port])> +AT_CLEANUP >>>>>>> +]) >>>>>>> + >>>>>>> OVN_FOR_EACH_NORTHD([ >>>>>>> AT_SETUP([container port changed to normal port and then deleted]) >>>>>>> ovn_start >>>>>> >>>>>> Regards, >>>>>> Dumitru >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
