| Message ID | 20240823170215.86490-1-shibir.basak@nutanix.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,v2] controller: Cleanup FDB entries when a VIF goes down. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi folks, Can you please review the v2 patch? Thanks, Shibir On 23-Aug-2024, at 10:32 PM, Shibir Basak <shibir.basak@nutanix.com> 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 <naveen.yerramneni@nutanix.com> Signed-off-by: Shibir Basak <shibir.basak@nutanix.com> --- v2: Rebase on top of latest main Addressed review comments --- controller/ovn-controller.c | 5 ++++ controller/pinctrl.c | 39 +++++++++++++++++++++----- controller/pinctrl.h | 1 + lib/automake.mk | 4 ++- lib/fdb.c | 33 ++++++++++++++++++++++ lib/fdb.h | 26 +++++++++++++++++ northd/northd.c | 17 +---------- tests/ovn.at | 56 +++++++++++++++++++++++++++++++++++++ 8 files changed, 157 insertions(+), 24 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 854d80bdf..12e71aa2d 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 @@ -5642,6 +5646,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..ff4d1e262 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, @@ -5034,6 +5038,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 +5059,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 +5068,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 +5087,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 +5112,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 +5123,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 +5144,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 +5154,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 +5171,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 +5197,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 +6561,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 +6580,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 +6616,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); } } @@ -6612,7 +6637,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 +6650,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..e31a6c304 --- /dev/null +++ b/lib/fdb.c @@ -0,0 +1,33 @@ +/* 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: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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..7bef72f91 --- /dev/null +++ b/lib/fdb.h @@ -0,0 +1,26 @@ +/* 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: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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/northd/northd.c b/northd/northd.c index c4824aae3..47ca7975e 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 a1d689e84..0209bff59 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -31794,6 +31794,62 @@ 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 +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 +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 + +# 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 +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 -- 2.22.3
On 8/23/24 19:02, 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 <naveen.yerramneni@nutanix.com> > Signed-off-by: Shibir Basak <shibir.basak@nutanix.com> > --- > v2: Rebase on top of latest main > Addressed review comments > --- Hi Shibir, Thanks for the additional comments in https://mail.openvswitch.org/pipermail/ovs-dev/2024-August/416672.html, that helps! Overall the patch looks OK to me. I left a few (minor) comments though. > controller/ovn-controller.c | 5 ++++ > controller/pinctrl.c | 39 +++++++++++++++++++++----- > controller/pinctrl.h | 1 + > lib/automake.mk | 4 ++- > lib/fdb.c | 33 ++++++++++++++++++++++ > lib/fdb.h | 26 +++++++++++++++++ > northd/northd.c | 17 +---------- > tests/ovn.at | 56 +++++++++++++++++++++++++++++++++++++ > 8 files changed, 157 insertions(+), 24 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 854d80bdf..12e71aa2d 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 > @@ -5642,6 +5646,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..ff4d1e262 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, > @@ -5034,6 +5038,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 +5059,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 +5068,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 +5087,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 +5112,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 +5123,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 +5144,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 +5154,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 +5171,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 +5197,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 +6561,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 +6580,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 +6616,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); > } > } > @@ -6612,7 +6637,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 +6650,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..e31a6c304 > --- /dev/null > +++ b/lib/fdb.c > @@ -0,0 +1,33 @@ > +/* 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: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * 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); > +} Shouldn't we move this one here too? 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; } > diff --git a/lib/fdb.h b/lib/fdb.h > new file mode 100644 > index 000000000..7bef72f91 > --- /dev/null > +++ b/lib/fdb.h > @@ -0,0 +1,26 @@ > +/* 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: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * 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/northd/northd.c b/northd/northd.c > index c4824aae3..47ca7975e 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 a1d689e84..0209bff59 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -31794,6 +31794,62 @@ 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 > +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 We don't need to set tx/rxq_pcap or ofport-request. > +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 Same here. > +ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys We should prefix the ovs-vsctl commands with "check " too just to be sure things don't fail silently. > + > + One new line is probably enough. > +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 > + We should wait for the two FDB entries to be created before we continue and we should also make sure ovn-controller processed the update. I think we need: 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 > +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]) This can be rewritten/simplified as: 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 Regards, Dumitru
Thanks Dumitru for the comments. I will address these add share v3 soon. On 16-Sep-2024, at 4:27 PM, Dumitru Ceara <dceara@redhat.com> wrote: !-------------------------------------------------------------------| CAUTION: External Email |-------------------------------------------------------------------! On 8/23/24 19:02, 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 <naveen.yerramneni@nutanix.com> Signed-off-by: Shibir Basak <shibir.basak@nutanix.com> --- v2: Rebase on top of latest main Addressed review comments --- Hi Shibir, Thanks for the additional comments in https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2024-2DAugust_416672.html&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=J_K0MJPpCtNDlwjhN2QhekTY9vu81w25yXZQGXHjTL1P7ZlaefhwMSpqPZiDFnNC&s=AQ_72TbtMtMLVERHTDcDKbOiKZE9ZPaQiMZHjsgllds&e= , that helps! Overall the patch looks OK to me. I left a few (minor) comments though. controller/ovn-controller.c | 5 ++++ controller/pinctrl.c | 39 +++++++++++++++++++++----- controller/pinctrl.h | 1 + lib/automake.mk | 4 ++- lib/fdb.c | 33 ++++++++++++++++++++++ lib/fdb.h | 26 +++++++++++++++++ northd/northd.c | 17 +---------- tests/ovn.at | 56 +++++++++++++++++++++++++++++++++++++ 8 files changed, 157 insertions(+), 24 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 854d80bdf..12e71aa2d 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 @@ -5642,6 +5646,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..ff4d1e262 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, @@ -5034,6 +5038,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 +5059,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 +5068,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 +5087,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 +5112,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 +5123,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 +5144,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 +5154,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 +5171,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 +5197,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 +6561,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 +6580,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 +6616,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); } } @@ -6612,7 +6637,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 +6650,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..e31a6c304 --- /dev/null +++ b/lib/fdb.c @@ -0,0 +1,33 @@ +/* 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=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=J_K0MJPpCtNDlwjhN2QhekTY9vu81w25yXZQGXHjTL1P7ZlaefhwMSpqPZiDFnNC&s=CxYrZvKXTIvnVdkWNd4ZDkQBoV6uUEzC5pPwMgD7Q9I&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); +} Shouldn't we move this one here too? 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; } diff --git a/lib/fdb.h b/lib/fdb.h new file mode 100644 index 000000000..7bef72f91 --- /dev/null +++ b/lib/fdb.h @@ -0,0 +1,26 @@ +/* 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=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=awcqDnvm2rXKiid0_aQK6D2Z4I6GarSRmeHuqVFlgU4&m=J_K0MJPpCtNDlwjhN2QhekTY9vu81w25yXZQGXHjTL1P7ZlaefhwMSpqPZiDFnNC&s=CxYrZvKXTIvnVdkWNd4ZDkQBoV6uUEzC5pPwMgD7Q9I&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/northd/northd.c b/northd/northd.c index c4824aae3..47ca7975e 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<http://ovn.at/> b/tests/ovn.at<http://ovn.at/> index a1d689e84..0209bff59 100644 --- a/tests/ovn.at<http://ovn.at/> +++ b/tests/ovn.at<http://ovn.at/> @@ -31794,6 +31794,62 @@ 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 +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 We don't need to set tx/rxq_pcap or ofport-request. +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 Same here. +ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys We should prefix the ovs-vsctl commands with "check " too just to be sure things don't fail silently. + + One new line is probably enough. +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 + We should wait for the two FDB entries to be created before we continue and we should also make sure ovn-controller processed the update. I think we need: 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 +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]) This can be rewritten/simplified as: 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 Regards, Dumitru
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 854d80bdf..12e71aa2d 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 @@ -5642,6 +5646,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..ff4d1e262 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, @@ -5034,6 +5038,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 +5059,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 +5068,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 +5087,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 +5112,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 +5123,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 +5144,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 +5154,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 +5171,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 +5197,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 +6561,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 +6580,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 +6616,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); } } @@ -6612,7 +6637,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 +6650,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..e31a6c304 --- /dev/null +++ b/lib/fdb.c @@ -0,0 +1,33 @@ +/* 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: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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..7bef72f91 --- /dev/null +++ b/lib/fdb.h @@ -0,0 +1,26 @@ +/* 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: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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/northd/northd.c b/northd/northd.c index c4824aae3..47ca7975e 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 a1d689e84..0209bff59 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -31794,6 +31794,62 @@ 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 +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 +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 + +# 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 +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