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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
