On Thu, Aug 27, 2026 at 11:42 AM Dumitru Ceara <[email protected]> wrote:
> Hi Ales, > > On 8/27/26 11:23 AM, Dumitru Ceara via dev wrote: > > Note: This review was produced with the help of an AI assistant. > > A human reviewer has examined the feedback below and considered it > > worth sharing. However, some observations may be inaccurate or miss > > context. Please use your own judgment when evaluating these comments. > > > > The feedback below is not really something I'd normally share as review > feedback, it's too nitpicky. I was just experimenting a bit with a > "sidekick" though so I decided to let it go through this one time. > > For the patch itself, it looks good to me! Applied to main, 26.09 and > 26.03. > > Regards, > Dumitru > > > Commit: eee8cb3e3b38 > > Author: Ales Musil <[email protected]> > > Subject: controller: Add missing I-P handler for MAC binding sync. > > > > The patch adds a port binding I-P handler for the > > en_evpn_mac_binding_sync engine node. The handler triggers a > > recompute when a port binding that is relevant to EVPN MAC binding > > sync is created, deleted, or has its type/options updated. It > > also adds a new uuidset (lsp_peers) to track the switch-side port > > binding UUIDs that participate in the EVPN sync, populated during > > the full run in evpn_mac_binding_sync_run(). > > > > The test changes replace flow-level checks (dump-flows) with > > SB MAC_Binding row-count assertions, which is more robust and > > decoupled from the OVS flow table encoding. > > > >> diff --git a/controller/evpn-mac-binding-sync.c > b/controller/evpn-mac-binding-sync.c > >> index 7387f91ca..be62c0f9a 100644 > >> --- a/controller/evpn-mac-binding-sync.c > >> +++ b/controller/evpn-mac-binding-sync.c > >> [ ... ] > >> + uuidset_insert(&data->lsp_peers, > &peers->local->header_.uuid); > >> + > >> int64_t remaining = > >> sync_evpn_mb_for_router_port(ovnsb_idl_txn, > >> > sbrec_mac_binding_by_lport_ip, > >> [ ... ] > > > >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > >> index 31cb30cfd..be02d2864 100644 > >> --- a/controller/ovn-controller.c > >> +++ b/controller/ovn-controller.c > >> [ ... ] > >> + uuidset_clear(&data->lsp_peers); > >> data->sb_changes_pending = false; > >> [ ... ] > >> +static enum engine_input_handler_result > >> +evpn_mac_binding_sync_sb_port_binding_handler(struct engine_node *node, > >> + void *data_) > >> +{ > >> + struct ed_type_evpn_mac_binding_sync *data = data_; > >> + struct ed_type_runtime_data *rt_data = > >> + engine_get_input_data("runtime_data", node); > >> + const struct sbrec_port_binding_table *port_binding_table = > >> + EN_OVSDB_GET(engine_get_input("SB_port_binding", node)); > >> + struct ovsdb_idl_index *sbrec_port_binding_by_name = > >> + engine_ovsdb_node_get_index( > >> + engine_get_input("SB_port_binding", node), > >> + "name"); > >> + > >> + const struct sbrec_port_binding *pb; > >> + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, port_binding_table) > { > >> + if (sbrec_port_binding_is_deleted(pb) && > >> + uuidset_contains(&data->lsp_peers, &pb->header_.uuid)) { > >> + return EN_UNHANDLED; > >> + } > >> + > >> + if (!sbrec_port_binding_is_new(pb) && > >> + uuidset_contains(&data->lsp_peers, &pb->header_.uuid) && > >> + (sbrec_port_binding_is_updated(pb, > >> + > SBREC_PORT_BINDING_COL_OPTIONS) || > >> + sbrec_port_binding_is_updated(pb, > SBREC_PORT_BINDING_COL_TYPE))) { > >> + return EN_UNHANDLED; > >> + } > > > > Nit: the second sbrec_port_binding_is_updated() call looks like > > it is indented one space short of the first one inside the > > parenthesized OR expression. Should be aligned to the opening > > paren: > > > > (sbrec_port_binding_is_updated(pb, > > SBREC_PORT_BINDING_COL_OPTIONS) || > > sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) > > > >> + > >> + if (!sbrec_port_binding_is_new(pb)) { > >> + continue; > >> + } > >> + > >> + struct local_datapath *ld = get_local_datapath( > >> + &rt_data->local_datapaths, pb->datapath->tunnel_key); > >> + if (!ld || !ld->is_switch) { > >> + continue; > >> + } > >> + > >> + int64_t vni = ovn_smap_get_llong(&ld->datapath->external_ids, > >> + "dynamic-routing-vni", -1); > >> + if (!ovn_is_valid_vni(vni)) { > >> + continue; > >> + } > >> + > >> + enum en_lport_type type = get_lport_type(pb); > >> + if (type != LP_L3GATEWAY && type != LP_PATCH) { > >> + continue; > >> + } > >> + > >> + const struct sbrec_port_binding *peer = > >> + lport_get_peer(pb, sbrec_port_binding_by_name); > >> + if (!peer) { > >> + continue; > >> + } > >> + > >> + struct local_datapath *peer_ld = > >> + get_local_datapath(&rt_data->local_datapaths, > >> + peer->datapath->tunnel_key); > >> + if (!peer_ld || peer_ld->is_switch) { > >> + continue; > >> + } > >> + > >> + return EN_UNHANDLED; > >> + } > >> + > >> + return EN_HANDLED_UNCHANGED; > >> +} > > > > The handler checks is_deleted before is_new, which is the right > > ordering. The logic for new port bindings mirrors the filtering > > in evpn_mac_binding_sync_run() (switch datapath with valid VNI, > > patch/l3gateway type, peer on a router datapath), which is good. > > > > I notice that sbrec_port_binding_by_name is obtained but only > > used in the new-port-binding path (via lport_get_peer()). It > > would be slightly cheaper to move that index lookup inside the > > "is_new" block, but that is cosmetic and the current structure > > is clear enough. > > > >> [ ... ] > >> + engine_add_input(&en_evpn_mac_binding_sync, &en_sb_port_binding, > >> + evpn_mac_binding_sync_sb_port_binding_handler); > >> [ ... ] > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > Thank you Dumitru! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
