On Wed, Nov 6, 2024 at 5:08 AM Shibir Basak <[email protected]> wrote: > > Thanks for your feedback Dumitru and Numan. > I assume you are referring to the “link_state” column in the interface table?
That is correct. > > The proposed solution looks cleaner to me as well. > We will revisit and come back with a patch. Looking forward to it. Numan > > Thanks, > Shibir > From: Dumitru Ceara <[email protected]> > Date: Wednesday, 6 November 2024 at 2:46 PM > To: Numan Siddique <[email protected]>, Shibir Basak <[email protected]> > Cc: [email protected] <[email protected]> > Subject: Re: [ovs-dev] [PATCH ovn v3] controller: Cleanup FDB entries when a > VIF goes down. > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > Hi Shibir, Numan, > > Sorry for the delay in replying. > > On 10/23/24 16:32, Numan Siddique wrote: > > On Mon, Oct 21, 2024 at 11:40 AM Shibir Basak <[email protected]> > > wrote: > >> > >> Hi Team/Dumitru, > >> > >> Can you review the updated v3 patch, > >> > >> Thanks, > >> Shibir > > > > > > Hi Shibir, > > > > Thanks for the patch. > > > > I've a few comments about this patch. I did see the discussions and > > your replies in v1 and v2. > > So I'm aware of the context why you've chosen the garp/rarp mechanism > > to clean up FDB entries. > > > > I've a few questions and comments. > > > > 1. Your patch doesn't clean up FDP entries of logical ports if the > > logical switch doesn't have a localnet port. > > Is that going to be an issue? > > > > 2. If ovn-controller is down when the VM's link goes down, then the > > FDB Entry is not cleaned up until > > it's done by north aging (if aging threshold is configured) > > > > Looks to me this solution is incomplete and doesn't seem like the > > correct way to do in pinctrl.c. > > > > I've below suggestions to address this issue > > > > 1. Presently we bind a logical port in ovn-controller if the > > - OVS interface record has external_ids:iface-id set and > > - ofport column of OVS interface record is set > > > > We can add another condition to binding > > - "link" column of the OVS interface record is set to > > "up" by ovs-vswitchd. > > > > If the link column is set to "down" ovn-controller should unbind > > the logical port. > > This change can be handled in > > binding_handle_ovs_interface_changes() in binding.c. > > > > This requires changes in binding.c > > (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_blob_main_controller_binding.c-23L2037&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=F4flTSTYKNPVq5Eau3mplM6mfgyThPJ5ZT0ICWUBdhb_pL5ghHlPkhnMuKNoosl2&s=C1X9ToE5X5rINrJ5O-cBreo7nUgaTm6xdn_PphezGEk&e= > > ) > > and changes can be a bit tricky and it needs thorough testing. > > > > 2. Add the FDB flows in table OFTABLE_GET_FDB for each FDB record > > (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_blob_main_controller_lflow.c-23L2054&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=F4flTSTYKNPVq5Eau3mplM6mfgyThPJ5ZT0ICWUBdhb_pL5ghHlPkhnMuKNoosl2&s=RSHIQyyAGdDcwvr9kIVMmDBeIAsO_UM7g4Z_WJVumZA&e= > > ) > > only > > if the port binding is "up". > > > > I think this is a good idea, thanks for bringing this up Numan! > > > 3. ovn-northd can clean up the stale fdb records (for deleted ports). > > > > ovn-northd already does that (for deleted ports). We do have a corner > case when two port_bindings change tunnel_keys (e.g., swap values due to > requested_tnl_id changes from the user): we might end up with incorrect > FDB records. However, that's a different issue, not really relevant to > this patch. > > > With this approach, all the ovn-controllers in the deployment will > > remove the FDB flows from table OFTABLE_GET_FDB as soon as the port > > binding status goes down. > > > > If the above approach seems too complicated I'm fine with this patch > > as an interim solution. > > > > I vote for avoiding an interim solution. If what you propose Numan > works for Shibir too, I'd prefer that. > > > @Dumitru Ceara wdyt ? > > > > Thanks > > Numan > > > > > > Thanks, > Dumitru > > > > > > > > >> > >>> On 19 Sep 2024, at 4:12 PM, Shibir Basak <[email protected]> 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]> > >>> --- > >>> v1: initial version > >>> v2: Rebase on top of latest main > >>> Addressed review comments from Dumitru > >>> v3: Addressed review comments from Dumitru > >>> --- > >>> controller/ovn-controller.c | 5 ++++ > >>> controller/pinctrl.c | 58 ++++++++++++++++++++----------------- > >>> controller/pinctrl.h | 1 + > >>> lib/automake.mk | 4 ++- > >>> lib/fdb.c | 49 +++++++++++++++++++++++++++++++ > >>> lib/fdb.h | 29 +++++++++++++++++++ > >>> northd/northd.c | 17 +---------- > >>> tests/ovn.at | 50 ++++++++++++++++++++++++++++++++ > >>> 8 files changed, 170 insertions(+), 43 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 64be08ce3..1dca2d579 100644 > >>> --- a/controller/ovn-controller.c > >>> +++ b/controller/ovn-controller.c > >>> @@ -4939,6 +4939,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 c86b4f940..44ad72987 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, > >>> @@ -376,9 +378,6 @@ static void bfd_monitor_run(struct ovsdb_idl_txn > >>> *ovnsb_idl_txn, > >>> OVS_REQUIRES(pinctrl_mutex); > >>> static void init_fdb_entries(void); > >>> static void destroy_fdb_entries(void); > >>> -static const struct sbrec_fdb *fdb_lookup( > >>> - struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, > >>> - uint32_t dp_key, const char *mac); > >>> static void run_put_fdb(struct ovsdb_idl_txn *ovnsb_idl_txn, > >>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, > >>> struct ovsdb_idl_index *sbrec_port_binding_by_key, > >>> @@ -4146,6 +4145,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, > >>> @@ -4167,7 +4167,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, > >>> @@ -5035,6 +5036,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*/ > >>> @@ -5055,7 +5057,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; > >>> @@ -5064,6 +5066,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 > >>> @@ -5082,6 +5085,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")) { > >>> @@ -5105,6 +5110,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 */ > >>> @@ -5115,7 +5121,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, > >>> @@ -5135,6 +5142,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 */ > >>> @@ -5144,7 +5152,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); > >>> } > >>> @@ -5160,6 +5169,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 */ > >>> @@ -5185,7 +5195,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); > >>> @@ -6548,6 +6559,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) { > >>> @@ -6563,6 +6578,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, > >>> @@ -6598,12 +6614,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); > >>> send_garp_rarp_delete(iter->name); > >>> } > >>> } > >>> @@ -6613,7 +6635,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, > >>> @@ -6626,7 +6648,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); > >>> @@ -8884,22 +8906,6 @@ destroy_fdb_entries(void) > >>> hmap_destroy(&put_fdbs); > >>> } > >>> > >>> -static const struct sbrec_fdb * > >>> -fdb_lookup(struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, uint32_t > >>> dp_key, > >>> - const char *mac) > >>> -{ > >>> - struct sbrec_fdb *fdb = > >>> sbrec_fdb_index_init_row(sbrec_fdb_by_dp_key_mac); > >>> - sbrec_fdb_index_set_dp_key(fdb, dp_key); > >>> - sbrec_fdb_index_set_mac(fdb, mac); > >>> - > >>> - const struct sbrec_fdb *retval > >>> - = sbrec_fdb_index_find(sbrec_fdb_by_dp_key_mac, fdb); > >>> - > >>> - sbrec_fdb_index_destroy_row(fdb); > >>> - > >>> - return retval; > >>> -} > >>> - > >>> static void > >>> run_put_fdb(struct ovsdb_idl_txn *ovnsb_idl_txn, > >>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, > >>> 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..80e20ab30 > >>> --- /dev/null > >>> +++ b/lib/fdb.c > >>> @@ -0,0 +1,49 @@ > >>> +/* Copyright (c) 2024, Nutanix, Inc. > >>> + * > >>> + * 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=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=F4flTSTYKNPVq5Eau3mplM6mfgyThPJ5ZT0ICWUBdhb_pL5ghHlPkhnMuKNoosl2&s=srv3gwmIA7FL8JMhHQnpZ4ZLgScyOLrRArOA498kSZo&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); > >>> +} > >>> + > >>> +const struct sbrec_fdb * > >>> +fdb_lookup(struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, uint32_t > >>> dp_key, > >>> + const char *mac) > >>> +{ > >>> + struct sbrec_fdb *fdb = > >>> sbrec_fdb_index_init_row(sbrec_fdb_by_dp_key_mac); > >>> + sbrec_fdb_index_set_dp_key(fdb, dp_key); > >>> + sbrec_fdb_index_set_mac(fdb, mac); > >>> + > >>> + const struct sbrec_fdb *retval > >>> + = sbrec_fdb_index_find(sbrec_fdb_by_dp_key_mac, fdb); > >>> + > >>> + sbrec_fdb_index_destroy_row(fdb); > >>> + > >>> + return retval; > >>> +} > >>> diff --git a/lib/fdb.h b/lib/fdb.h > >>> new file mode 100644 > >>> index 000000000..b214ce626 > >>> --- /dev/null > >>> +++ b/lib/fdb.h > >>> @@ -0,0 +1,29 @@ > >>> +/* Copyright (c) 2024, Nutanix, Inc. > >>> + * > >>> + * 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=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=F4flTSTYKNPVq5Eau3mplM6mfgyThPJ5ZT0ICWUBdhb_pL5ghHlPkhnMuKNoosl2&s=srv3gwmIA7FL8JMhHQnpZ4ZLgScyOLrRArOA498kSZo&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); > >>> +const struct sbrec_fdb * > >>> +fdb_lookup(struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, > >>> + uint32_t dp_key, const char *mac); > >>> + > >>> +#endif > >>> diff --git a/northd/northd.c b/northd/northd.c > >>> index a267cd5f8..793641662 100644 > >>> --- a/northd/northd.c > >>> +++ b/northd/northd.c > >>> @@ -42,6 +42,7 @@ > >>> #include "lib/ovn-sb-idl.h" > >>> #include "lib/ovn-util.h" > >>> #include "lib/lb.h" > >>> +#include "lib/fdb.h" > >>> #include "lflow-mgr.h" > >>> #include "memory.h" > >>> #include "northd.h" > >>> @@ -3398,22 +3399,6 @@ cleanup_stale_fdb_entries(const struct > >>> sbrec_fdb_table *sbrec_fdb_table, > >>> } > >>> } > >>> > >>> -static 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); > >>> -} > >>> - > >>> struct service_monitor_info { > >>> struct hmap_node hmap_node; > >>> const struct sbrec_service_monitor *sbrec_mon; > >>> diff --git a/tests/ovn.at b/tests/ovn.at > >>> index 4b6e8132f..2b4346814 100644 > >>> --- a/tests/ovn.at > >>> +++ b/tests/ovn.at > >>> @@ -31794,6 +31794,56 @@ OVN_CLEANUP([hv1], [hv2], [hv3]) > >>> AT_CLEANUP > >>> ]) > >>> > >>> +OVN_FOR_EACH_NORTHD([ > >>> +AT_SETUP([FDB cleanup when vif goes down]) > >>> +ovn_start > >>> + > >>> +net_add n1 > >>> + > >>> +check ovn-nbctl ls-add ls0 > >>> + > >>> +check ovn-nbctl lsp-add ls0 ln_port > >>> +check ovn-nbctl lsp-set-addresses ln_port unknown > >>> +check ovn-nbctl lsp-set-type ln_port localnet > >>> +check ovn-nbctl lsp-set-options ln_port network_name=physnet1 > >>> +check ovn-nbctl set logical_switch_port ln_port > >>> options:localnet_learn_fdb=true > >>> + > >>> +check ovn-nbctl lsp-add ls0 vif0 > >>> +check ovn-nbctl lsp-set-addresses vif0 "00:00:00:00:10:10 192.168.10.10" > >>> unknown > >>> + > >>> +sim_add hv1 > >>> +as hv1 > >>> +check ovs-vsctl add-br br-phys > >>> +ovn_attach n1 br-phys 192.168.0.1 > >>> +check ovs-vsctl -- add-port br-int vif0 -- \ > >>> + set interface vif0 external-ids:iface-id=vif0 > >>> +check ovs-vsctl -- add-port br-phys ext0 > >>> +check ovs-vsctl set open . > >>> external_ids:ovn-bridge-mappings=physnet1:br-phys > >>> + > >>> +wait_for_ports_up > >>> +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 > >>> +wait_row_count sb:FDB 2 > >>> +check ovn-nbctl --wait=hv sync > >>> + > >>> +# 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]) > >>> +check ovn-nbctl --wait=hv sync > >>> +wait_row_count sb:FDB 0 mac="00:00:00:00:10:10" > >>> +wait_row_count sb:FDB 0 mac="00:00:00:00:20:20" > >>> + > >>> +# 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 > >>> -- > >>> 2.22.3 > >>> > >> > >> _______________________________________________ > >> dev mailing list > >> [email protected] > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=F4flTSTYKNPVq5Eau3mplM6mfgyThPJ5ZT0ICWUBdhb_pL5ghHlPkhnMuKNoosl2&s=potl8z8fBmANOyb8ejtba6BdaCTCBaTDOQ4_8mSEZxs&e= > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
