This patch fails to compile, I am not commenting on the correctness of the patch at this point
On Mon, Aug 17, 2026 at 6:49 AM Alexandra Rukomoinikova via dev < [email protected]> wrote: > Commit [1] propagated other_config:is-vtep from the SB Chassis to the > local tunnel Interface and read it back from there. That doesn't > survive an ovn-controller upgrade: tunnel_add() sets other_config only > when it creates a new interface, while the path that reuses an existing > tunnel compares just the interface type and options. After an upgrade > the tunnel ports already exist with unchanged options, so the flag is > never added to them and chassis_tunnel->is_ramp_tunnel stays false > until the tunnel happens to be recreated for some other reason. As a > result, ICMP "fragmentation needed" packets arriving from RAMP tunnels > kept being dropped on upgraded chassis. > Instead, look the chassis up in the SB database by the name encoded in > the tunnel id and read other_config:is-vtep directly from there. This > doesn't depend on any locally stored state, so it works for tunnels > created by older versions as well. > > [1] > https://github.com/ovn-org/ovn/commit/3391e61cdcd55ba4d11b30a096ba5aee3e435994 > Fixes: 3391e61cdcd5 ("controller: Skip frag-needed handling for VTEP ICMP > packets.") > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > --- > controller/encaps.c | 9 --------- > controller/local_data.c | 8 +++++++- > controller/ovn-controller.c | 2 +- > 3 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/controller/encaps.c b/controller/encaps.c > index 048e85c38..61ae55965 100644 > --- a/controller/encaps.c > +++ b/controller/encaps.c > @@ -43,7 +43,6 @@ encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type); > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options); > - ovsdb_idl_track_add_column(ovs_idl, > &ovsrec_interface_col_other_config); > } > > /* Enough context to create a new tunnel, using tunnel_add(). */ > @@ -208,7 +207,6 @@ tunnel_add(struct tunnel_ctx *tc, > const struct ovsrec_open_vswitch_table *ovs_table) > { > struct smap options = SMAP_INITIALIZER(&options); > - struct smap other_config = SMAP_INITIALIZER(&other_config); > smap_add(&options, "remote_ip", encap->ip); > smap_add(&options, "local_ip", local_ip); > smap_add(&options, "key", "flow"); > @@ -286,11 +284,6 @@ tunnel_add(struct tunnel_ctx *tc, > } > } > > - if (is_ramp_tunnel(&chassis_rec->other_config)) { > - /* Propagate ramp switch flag from chassis to interface. */ > - smap_add(&other_config, "is-vtep", "true"); > - } > - > /* If there's an existing tunnel record that does not need any change, > * keep it. Otherwise, create a new record (if there was an existing > * record, the new record will supplant it and encaps_run() will > delete > @@ -338,7 +331,6 @@ tunnel_add(struct tunnel_ctx *tc, > ovsrec_interface_set_name(iface, port_name); > ovsrec_interface_set_type(iface, encap->type); > ovsrec_interface_set_options(iface, &options); > - ovsrec_interface_set_other_config(iface, &other_config); > > struct ovsrec_port *port = ovsrec_port_insert(tc->ovs_txn); > ovsrec_port_set_name(port, port_name); > @@ -354,7 +346,6 @@ exit: > free(tunnel_entry_id); > free(tunnel_entry_id_old); > smap_destroy(&options); > - smap_destroy(&other_config); > } > > static bool > diff --git a/controller/local_data.c b/controller/local_data.c > index af6c75b40..02c8563fe 100644 > --- a/controller/local_data.c > +++ b/controller/local_data.c > @@ -30,6 +30,7 @@ > #include "lport.h" > #include "lib/ovn-util.h" > #include "lib/ovn-sb-idl.h" > +#include "lib/chassis-index.h" > #include "local_data.h" > #include "lport.h" > > @@ -457,6 +458,7 @@ tracked_datapaths_destroy(struct hmap > *tracked_datapaths) > void > local_nonvif_data_run(const struct ovsrec_bridge *br_int, > const struct sbrec_chassis *chassis_rec, > + struct ovsdb_idl_index *sbrec_chassis_by_name, > you need to add this to the definition of local_nonvif_data_run() in controller/local_data.h > struct simap *patch_ofports, > struct hmap *chassis_tunnels, > struct flow_based_tunnel *flow_tunnels) > @@ -525,6 +527,8 @@ local_nonvif_data_run(const struct ovsrec_bridge > *br_int, > if (!encaps_tunnel_id_parse(tunnel_id, &hash_id, &ip, > NULL)) { > continue; > } > + const struct sbrec_chassis *chassis = > + chassis_lookup_by_name(sbrec_chassis_by_name, > hash_id); > struct chassis_tunnel *tun = xmalloc(sizeof *tun); > hmap_insert(chassis_tunnels, &tun->hmap_node, > hash_string(hash_id, 0)); > @@ -532,7 +536,9 @@ local_nonvif_data_run(const struct ovsrec_bridge > *br_int, > tun->ofport = u16_to_ofp(ofport); > tun->type = tunnel_type; > tun->is_ipv6 = ip ? addr_is_ipv6(ip) : false; > - tun->is_ramp_tunnel = > is_ramp_tunnel(&iface_rec->other_config); > + tun->is_ramp_tunnel = chassis ? > + > is_ramp_tunnel(&chassis->other_config, > + "is-vtep", false) : > false; > not sure why you added `"is-vtep", false` as a second argument. is_ramp_tunnel() only has one argument which gets the "is-vtep" bool from the other_config smap. > free(hash_id); > free(ip); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 6551a4e30..1e9639216 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3730,7 +3730,7 @@ en_non_vif_data_run(struct engine_node *node, void > *data) > ed_non_vif_data->use_flow_based_tunnels = > is_flow_based_tunnels_enabled(ovs_table, chassis); > > - local_nonvif_data_run(br_int, chassis, > + local_nonvif_data_run(br_int, chassis, sbrec_chassis_by_name, > &ed_non_vif_data->patch_ofports, > &ed_non_vif_data->chassis_tunnels, > ed_non_vif_data->flow_tunnels); > -- > 2.48.1 > > _______________________________________________ > 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
