Message ID | 20240509073543.563418-2-amusil@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,1/2,branch-24.03] tests: Add macro for checking flows after recompute. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Thu, May 9, 2024 at 3:36 AM Ales Musil <amusil@redhat.com> wrote: > > Currently the tunnel_key change for either LS/LR/LSP/LRP wasn't > consistent. That would lead to a situations when some old would still > be present, breaking the connection especially for already existing > FDBs and MAC bindings. > > Make sure the FDB entries are up to date by removing them from DB > when there is a tunnel_key change as those entries have only tunnel_key > refrences (dp_key, port_key). > > MAC bindings have references to the datapath and port name, instead of > removing those entries do recompute in the controller when we detect > tunnel_key change. This can be costly at scale, however the tunnel_key > is not expected to change constantly, in most cases it shouldn't change > at all. > > Fixes: b337750e45be ("northd: Incremental processing of VIF changes in 'northd' node.") > Fixes: 425f699e2b20 ("controller: fixed potential segfault when changing tunnel_key and deleting ls.") > Reported-at: https://issues.redhat.com/browse/FDP-393 > Acked-by: Mark Michelson <mmichels@redhat.com> > Signed-off-by: Ales Musil <amusil@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > (cherry picked from commit ddf051cbc6af24c303bf88970750e5c5fe285400) Thanks. Applied both the patches to branch-24.03. Numan > --- > controller/binding.c | 13 ++++++++-- > controller/ovn-controller.c | 27 +++++++------------ > northd/northd.c | 7 +++++ > tests/ovn.at | 52 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 79 insertions(+), 20 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 8ac2ce3e2..0712d7030 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -3126,8 +3126,17 @@ delete_done: > update_ld_peers(pb, b_ctx_out->local_datapaths); > } > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb); > - if (!handled) { > + if (!handle_updated_port(b_ctx_in, b_ctx_out, pb)) { > + handled = false; > + break; > + } > + > + if (!sbrec_port_binding_is_new(pb) && > + sbrec_port_binding_is_updated(pb, > + SBREC_PORT_BINDING_COL_TUNNEL_KEY) && > + get_local_datapath(b_ctx_out->local_datapaths, > + pb->datapath->tunnel_key)) { > + handled = false; > break; > } > } > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index a40712e53..113d3e05c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1893,7 +1893,6 @@ runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED, > engine_get_input("SB_datapath_binding", node)); > const struct sbrec_datapath_binding *dp; > struct ed_type_runtime_data *rt_data = data; > - struct local_datapath *ld; > > SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (dp, dp_table) { > if (sbrec_datapath_binding_is_deleted(dp)) { > @@ -1901,27 +1900,19 @@ runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED, > dp->tunnel_key)) { > return false; > } > + > + } > + > + if (sbrec_datapath_binding_is_updated( > + dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY) && > + !sbrec_datapath_binding_is_new(dp)) { > /* If the tunnel key got updated, get_local_datapath will not find > * the ld. Use get_local_datapath_no_hash which does not > * rely on the hash. > */ > - if (sbrec_datapath_binding_is_updated( > - dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY)) { > - if (get_local_datapath_no_hash(&rt_data->local_datapaths, > - dp->tunnel_key)) { > - return false; > - } > - } > - } else if (sbrec_datapath_binding_is_updated( > - dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY) > - && !sbrec_datapath_binding_is_new(dp)) { > - /* If the tunnel key is updated, remove the entry (with a wrong > - * hash) from the map. It will be (properly) added back later. > - */ > - if ((ld = get_local_datapath_no_hash(&rt_data->local_datapaths, > - dp->tunnel_key))) { > - hmap_remove(&rt_data->local_datapaths, &ld->hmap_node); > - local_datapath_destroy(ld); > + if (get_local_datapath_no_hash(&rt_data->local_datapaths, > + dp->tunnel_key)) { > + return false; > } > } > } > diff --git a/northd/northd.c b/northd/northd.c > index 8f20c4be3..a4bd3798b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4520,6 +4520,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > op->visited = true; > continue; > } > + > + uint32_t old_tunnel_key = op->tunnel_key; > if (!ls_port_reinit(op, ovnsb_idl_txn, &nd->ls_ports, > new_nbsp, NULL, > od, sb, ni->sbrec_mirror_table, > @@ -4529,6 +4531,11 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > goto fail; > } > add_op_to_northd_tracked_ports(&trk_lsps->updated, op); > + > + if (old_tunnel_key != op->tunnel_key) { > + delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, > + old_tunnel_key); > + } > } > op->visited = true; > } > diff --git a/tests/ovn.at b/tests/ovn.at > index c36fadb90..a64e09bb1 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -37383,6 +37383,8 @@ sim_add hv1 > as hv1 > check ovs-vsctl add-br br-phys > ovn_attach n1 br-phys 192.168.0.11 > +ovs-vsctl -- add-port br-int vif1 -- \ > + set interface vif1 external-ids:iface-id=lsp1 > > check ovn-nbctl --wait=hv ls-add ls \ > -- lsp-add ls lsp1 \ > @@ -37395,6 +37397,56 @@ check ovn-nbctl --wait=hv ls-add ls \ > addresses=router \ > -- lrp-set-gateway-chassis lr-ls hv1 > > +dp_uuid=$(fetch_column datapath _uuid external_ids:name=lr) > +ovn-sbctl create MAC_Binding ip=192.168.1.10 datapath=$dp_uuid logical_port=lr-ls mac='"00:00:00:00:00:01"' > + > +OVN_POPULATE_ARP > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +create_fdb() { > + ls_key=$(fetch_column datapath tunnel_key external_ids:name=ls) > + lsp_key=$(fetch_column port_binding tunnel_key logical_port=lsp1) > + > + ovn-sbctl create FDB mac='"00:00:00:00:00:01"' dp_key=$ls_key port_key=$lsp_key > +} > + > +AS_BOX([Logical switch tunnel_key change]) > +create_fdb > + > +check ovn-nbctl --wait=hv set Logical_Switch ls other_config:requested-tnl-key=10 > +ovn-sbctl list datapath > +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) > + > +check_row_count FDB 0 mac='"00:00:00:00:00:01"' > + > +AS_BOX([Logical switch port tunnel_key change]) > +create_fdb > + > +check ovn-nbctl --wait=hv set Logical_Switch_Port lsp1 options:requested-tnl-key=10 > +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) > + > +check_row_count FDB 0 mac='"00:00:00:00:00:01"' > + > +AS_BOX([Logical router tunnel_key change]) > +check ovn-nbctl --wait=hv set Logical_Router lr options:requested-tnl-key=20 > +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) > + > +check_row_count Mac_Binding 1 ip=192.168.1.10 > +AT_CHECK([ovs-ofctl dump-flows br-int table=67 | grep -c metadata=0x14], [0], [dnl > +1 > +]) > + > +AS_BOX([Logical router port tunnel_key change]) > +check ovn-nbctl --wait=hv set Logical_Router_Port lr-ls options:requested-tnl-key=20 > +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) > + > +check_row_count Mac_Binding 1 ip=192.168.1.10 > +AT_CHECK([ovs-ofctl dump-flows br-int table=67 | grep -c reg14=0x14], [0], [dnl > +1 > +]) > + > +AS_BOX([Logical switch tunnel_key change, potential segfault]) > sleep_controller hv1 > > check ovn-nbctl --wait=sb set Logical_Switch ls other_config:requested-tnl-key=1000 > -- > 2.44.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/binding.c b/controller/binding.c index 8ac2ce3e2..0712d7030 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -3126,8 +3126,17 @@ delete_done: update_ld_peers(pb, b_ctx_out->local_datapaths); } - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb); - if (!handled) { + if (!handle_updated_port(b_ctx_in, b_ctx_out, pb)) { + handled = false; + break; + } + + if (!sbrec_port_binding_is_new(pb) && + sbrec_port_binding_is_updated(pb, + SBREC_PORT_BINDING_COL_TUNNEL_KEY) && + get_local_datapath(b_ctx_out->local_datapaths, + pb->datapath->tunnel_key)) { + handled = false; break; } } diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index a40712e53..113d3e05c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1893,7 +1893,6 @@ runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED, engine_get_input("SB_datapath_binding", node)); const struct sbrec_datapath_binding *dp; struct ed_type_runtime_data *rt_data = data; - struct local_datapath *ld; SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (dp, dp_table) { if (sbrec_datapath_binding_is_deleted(dp)) { @@ -1901,27 +1900,19 @@ runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED, dp->tunnel_key)) { return false; } + + } + + if (sbrec_datapath_binding_is_updated( + dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY) && + !sbrec_datapath_binding_is_new(dp)) { /* If the tunnel key got updated, get_local_datapath will not find * the ld. Use get_local_datapath_no_hash which does not * rely on the hash. */ - if (sbrec_datapath_binding_is_updated( - dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY)) { - if (get_local_datapath_no_hash(&rt_data->local_datapaths, - dp->tunnel_key)) { - return false; - } - } - } else if (sbrec_datapath_binding_is_updated( - dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY) - && !sbrec_datapath_binding_is_new(dp)) { - /* If the tunnel key is updated, remove the entry (with a wrong - * hash) from the map. It will be (properly) added back later. - */ - if ((ld = get_local_datapath_no_hash(&rt_data->local_datapaths, - dp->tunnel_key))) { - hmap_remove(&rt_data->local_datapaths, &ld->hmap_node); - local_datapath_destroy(ld); + if (get_local_datapath_no_hash(&rt_data->local_datapaths, + dp->tunnel_key)) { + return false; } } } diff --git a/northd/northd.c b/northd/northd.c index 8f20c4be3..a4bd3798b 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4520,6 +4520,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, op->visited = true; continue; } + + uint32_t old_tunnel_key = op->tunnel_key; if (!ls_port_reinit(op, ovnsb_idl_txn, &nd->ls_ports, new_nbsp, NULL, od, sb, ni->sbrec_mirror_table, @@ -4529,6 +4531,11 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, goto fail; } add_op_to_northd_tracked_ports(&trk_lsps->updated, op); + + if (old_tunnel_key != op->tunnel_key) { + delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, + old_tunnel_key); + } } op->visited = true; } diff --git a/tests/ovn.at b/tests/ovn.at index c36fadb90..a64e09bb1 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -37383,6 +37383,8 @@ sim_add hv1 as hv1 check ovs-vsctl add-br br-phys ovn_attach n1 br-phys 192.168.0.11 +ovs-vsctl -- add-port br-int vif1 -- \ + set interface vif1 external-ids:iface-id=lsp1 check ovn-nbctl --wait=hv ls-add ls \ -- lsp-add ls lsp1 \ @@ -37395,6 +37397,56 @@ check ovn-nbctl --wait=hv ls-add ls \ addresses=router \ -- lrp-set-gateway-chassis lr-ls hv1 +dp_uuid=$(fetch_column datapath _uuid external_ids:name=lr) +ovn-sbctl create MAC_Binding ip=192.168.1.10 datapath=$dp_uuid logical_port=lr-ls mac='"00:00:00:00:00:01"' + +OVN_POPULATE_ARP +wait_for_ports_up +check ovn-nbctl --wait=hv sync + +create_fdb() { + ls_key=$(fetch_column datapath tunnel_key external_ids:name=ls) + lsp_key=$(fetch_column port_binding tunnel_key logical_port=lsp1) + + ovn-sbctl create FDB mac='"00:00:00:00:00:01"' dp_key=$ls_key port_key=$lsp_key +} + +AS_BOX([Logical switch tunnel_key change]) +create_fdb + +check ovn-nbctl --wait=hv set Logical_Switch ls other_config:requested-tnl-key=10 +ovn-sbctl list datapath +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) + +check_row_count FDB 0 mac='"00:00:00:00:00:01"' + +AS_BOX([Logical switch port tunnel_key change]) +create_fdb + +check ovn-nbctl --wait=hv set Logical_Switch_Port lsp1 options:requested-tnl-key=10 +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) + +check_row_count FDB 0 mac='"00:00:00:00:00:01"' + +AS_BOX([Logical router tunnel_key change]) +check ovn-nbctl --wait=hv set Logical_Router lr options:requested-tnl-key=20 +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) + +check_row_count Mac_Binding 1 ip=192.168.1.10 +AT_CHECK([ovs-ofctl dump-flows br-int table=67 | grep -c metadata=0x14], [0], [dnl +1 +]) + +AS_BOX([Logical router port tunnel_key change]) +check ovn-nbctl --wait=hv set Logical_Router_Port lr-ls options:requested-tnl-key=20 +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) + +check_row_count Mac_Binding 1 ip=192.168.1.10 +AT_CHECK([ovs-ofctl dump-flows br-int table=67 | grep -c reg14=0x14], [0], [dnl +1 +]) + +AS_BOX([Logical switch tunnel_key change, potential segfault]) sleep_controller hv1 check ovn-nbctl --wait=sb set Logical_Switch ls other_config:requested-tnl-key=1000