On 7/2/25 6:26 PM, Mark Michelson via dev wrote: > This is similar to a recent change that refactored datapath syncing. > This works in a very similar way. > > * Input nodes create type-agnostic ovn_unpaired_port_bindings. These are > fed into the en-port-binding-pair node. > * The en-port-binding-pair node ensures that a southbound Port_Binding > is created for each unpaired port binding. Any remaining soutbound > Port_Bindings are deleted. > * Type-specific nodes then convert the paired port bindings into > type-specific paired port bindings that can be consumed by other > nodes. > > However, there are some important differences to note between this and > the datapath sync patch: > > * This patch opts for the word "pair" instead of "sync". This is because > en-port-binding-pair ensures that all southbound Port_Bindings are > paired with some northbound structure. However, the columns in the > southobund Port_Binding are not all synced with their northd > counterpart. Other engine nodes are responsible for fully syncing the > data. > * Not all southbound Port_Bindings have a corresponding northbound row. > Therefore, the engine nodes that create unpaired port bindings pass an > opaque cookie pointer to the pairing node instead of a database row. > * Port bindings tend to be identified by name. This means the code has > to have several safeguards in place to catch scenarios such as a port > "moving" from one datapath to another, or a port being deleted and > re-added quickly. > * Unlike with the datapath syncing code, this required changes to > northd's incremental processing since northd was already incrementally > processing some northbound logical switch port changes. > > Signed-off-by: Mark Michelson <[email protected]> > ---
Hi Mark, Thanks for this new revision! This is not the actual review though, just an update with some scale testing results we got with ovn-heater. > v9 -> v10: > * Rebased. > * Used designated initializers after allocating structures. > * Added a new "free_cookie" callback to the > ovn_unsynced_port_binding_map callbacks. All port binding engine > nodes now provide this callback. This means there is no longer a > fallback to the default callbacks if none are provided. > * Some formatting fixes. > > v8 -> v9: > * Rebased. > * Changed some file names to be in alignment with others in the > directory. > * Fixed memory leak of port tunnel ids. > * Added a missing "static" to a structure in > en-port-binding-logical-switch-port.c. > > v7 -> v8: > * Rebased. > > v6 -> v7: > * Rebased. > * Made some recommended changes to formatting (combining if statements, > fixing indentation, etc.) > * Switched from an array of router DPGs in the chassisredirect code > to a vector. > * Fixed several memory leaks in the chassisredirect code. > * Fixed a crash in northd.c in the incremental case where a northbound > logical switch port did not have a corresponding paired logical > switch port to find due to tunnel key issues. > * Ensured that we only compare a requested tunnel key against the > VXLAN multicast minimum when VXLAN is actually enabled. > > v5 -> v6: > * Rebased. > * All calls to xzalloc() in this patch have been changed to xmalloc() > or xcalloc(). > * Candidate paired port bindings use a vector instead of a list now. > * Chassisredirect ports have been simplified a great deal. > * This version introduces an enum for port binding types to make type > checking more efficient. > * This patch abandons the attempt to blindly pull in unpaired port > binding maps based on their positions in the engine node inputs > array. Instead, we now explicitly pull them in by input name and > assign them to array positions based on their port binding type. > > v4 -> v5: > * Rebased. > * Fixed some formatting anomalies in mirror port syncing. > * Fixed checkpatch warnings. > > v3 -> v4: > * Rebased. > * Addressed newly-added mirror port functionality. > * Fixed many memory leaks. > > v3: This is the first version with this patch present. > --- [...] > > diff --git a/TODO.rst b/TODO.rst > index 78962bb92..60ae155c5 100644 > --- a/TODO.rst > +++ b/TODO.rst > @@ -168,3 +168,15 @@ OVN To-do List > ovn\_synced\_logical_router and ovn\_synced\_logical\_switch. This will > allow for the eventual removal of the ovn\_datapath structure from the > codebase. > + > +* Port Binding sync nodes > + > + * Southbound Port bindings are synced across three engine nodes: > + - en_port_binding_pair > + - en_northd > + - en_sync_to_sb > + It would be easier to work with if these were combined into a > + single node instead. > + > + * Add incremental processing to the en-port-binding-pair node, as > + well as derivative nodes. Unfortunately, it seems that the lack of incremental processing for these northd nodes introduces a measurable performance regression. With ovn-heater, I ran a 500 node density-heavy (simulating realistic ovn-kubernetes workloads) test. That showed an increase of ~50% in time it takes to run the simulated workloads. Most of that seems to be due to these new nodes performing recomputation on every input change. For ease of debugging I uploaded the NB database extracted from that test here: https://people.redhat.com/~dceara/ovn-northd-datapath-port-binding-refactor-v10/ovnnb_db.db.gz To make it easier to debug directly on my machine I hacked the ovn-sandbox script in the following way: diff --git a/tutorial/ovn-sandbox b/tutorial/ovn-sandbox index ed334d1c31..a64bb930dd 100755 --- a/tutorial/ovn-sandbox +++ b/tutorial/ovn-sandbox @@ -661,14 +661,6 @@ else run ovs-vsctl set open . external-ids:ovn-remote=$OVN_SB_DB OVN_CTRLR_PKI="" fi -for i in $(seq $n_ics); do - if [ $i -eq 1 ]; then inst=""; else inst=$i; fi - rungdb $gdb_ovn_ic $gdb_ovn_ic_ex ovn-ic --detach \ - --no-chdir --pidfile=ovn-ic${inst}.pid -vconsole:off \ - --log-file=ovn-ic${inst}.log -vsyslog:off \ - --ovnsb-db="$OVN_SB_DB" --ovnnb-db="$OVN_NB_DB" \ - --ic-sb-db="$OVN_IC_SB_DB" --ic-nb-db="$OVN_IC_NB_DB" -done northd_args= OVN_NORTHD=ovn-northd @@ -680,17 +672,11 @@ for i in $(seq $n_northds); do --log-file=$OVN_NORTHD$inst.log -vsyslog:off \ --ovnsb-db="$OVN_SB_DB" --ovnnb-db="$OVN_NB_DB" $northd_args done -for i in $(seq $n_controllers); do - if [ $i -eq 1 ]; then inst=""; else inst=$i; fi - rungdb $gdb_ovn_controller $gdb_ovn_controller_ex ovn-controller \ - $OVN_CTRLR_PKI --detach --no-chdir -vsyslog:off \ - --log-file=ovn-controller${inst}.log \ - --pidfile=ovn-controller${inst}.pid -vconsole:off -done -rungdb $gdb_ovn_controller_vtep $gdb_ovn_controller_vtep_ex \ - ovn-controller-vtep --detach --no-chdir --pidfile -vconsole:off \ - $OVN_CTRLR_PKI --log-file -vsyslog:off \ - --ovnsb-db="$OVN_SB_DB" + +run sleep 10 +run ovn-sbctl set connection . inactivity_probe=180000 +run ovn-nbctl set connection . inactivity_probe=180000 +run ovn-nbctl set NB_GLOBAL . options:northd_probe_interval=180000 cat <<EOF --- Then ran a sandbox with the test NB database: $ make -j4 sandbox SANDBOXFLAGS="--nbdb-source=/tmp/ovnnb_db.db" Decreased the threshold for logging I-P node compute times: $ ovn-appctl -t ovn-northd inc-engine/compute-log-timeout 10 I waited for things to settle down and then I added a new logical switch port: $ ovn-nbctl lsp-add lswitch-ovn-scale-99 lsp-foo \ -- lsp-set-addresses lsp-foo "00:00:00:00:00:01 42.42.42.42" ovn-northd.log shows: 2025-07-03T11:49:59.869Z|00113|inc_proc_eng|INFO|node: port_binding_logical_switch_port, recompute (missing handler for input datapath_synced_logical_switch) took 101ms 2025-07-03T11:49:59.895Z|00114|inc_proc_eng|INFO|node: port_binding_mirror, recompute (missing handler for input datapath_synced_logical_switch) took 15ms 2025-07-03T11:50:00.059Z|00115|inc_proc_eng|INFO|node: port_binding_pair, recompute (missing handler for input port_binding_logical_switch_port) took 164ms 2025-07-03T11:50:00.126Z|00116|inc_proc_eng|INFO|node: port_binding_paired_logical_switch_port, recompute (missing handler for input port_binding_pair) took 62ms 2025-07-03T11:50:00.255Z|00117|inc_proc_eng|INFO|node: port_binding_logical_switch_port, recompute (missing handler for input datapath_synced_logical_switch) took 99ms 2025-07-03T11:50:00.290Z|00118|inc_proc_eng|INFO|node: port_binding_mirror, recompute (missing handler for input datapath_synced_logical_switch) took 24ms 2025-07-03T11:50:00.480Z|00119|inc_proc_eng|INFO|node: port_binding_pair, recompute (missing handler for input port_binding_logical_switch_port) took 190ms 2025-07-03T11:50:00.573Z|00120|inc_proc_eng|INFO|node: port_binding_paired_logical_switch_port, recompute (missing handler for input port_binding_pair) took 85ms 2025-07-03T11:50:00.609Z|00121|inc_proc_eng|INFO|node: sync_from_sb, recompute (missing handler for input SB_port_binding) took 27ms While on main most of this is done incrementally (only the sync_from_sb node takes some time). I plan to still review the series but I'm afraid that we cannot merge it until we figure out how to reduce performance impact due to these these new nodes. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
