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.
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
