On Tue, May 27, 2025 at 4:56 PM Felix Huettner <felix.huettner@stackit.cloud> wrote:
> On Tue, May 27, 2025 at 07:18:46AM +0200, Ales Musil wrote: > > On Wed, May 7, 2025 at 4:21 PM Felix Huettner via dev < > > ovs-dev@openvswitch.org> wrote: > > > > > When starting the ovn-controller without monitor_all we need a few > > > iterations of the engine node and updating monitoring conditions until > > > we have loaded all data. > > > Previously this was handled by the number of main loop iterations of > > > ovn-controller and an arbitrary timeout. > > > However this is was always best effort and there was no guarantee that > > > we actually had loaded all data. > > > > > > To solve this we now track the monitoring requests we sent out to > > > southbound and when that data becomes available. > > > > > > For monitor_all we just need to have the initial monitoring conditions > > > fullfilled and are afterwards sure that we have all required data. > > > > > > For non monitor_all we need at least one more iteration of the engine > > > node and updating sb monitoring conditions. We limit this to 20 rounds > > > for now, which should be sufficient and is at least the value of what > > > the previous implementation provided. > > > However if we notice that we do not update monitoring condition anymore > > > after a engine run we can also assume that we have finished loading all > > > data. > > > > > > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> > > > --- > > > > > > > Hi Felix, > > > > sorry for the recent silence on this one. There is related discussion > > happening on > > > https://patchwork.ozlabs.org/project/ovn/patch/20250318100817.117922-2-alekssmirnov@k2.cloud/ > > . > > It would be nice to get your input there too so we can move on > > with this. > > Hi Ales, > Hi Felix, > no worries i was also busy on other topics. > > I think Alexander has a valid point there. > I would wait for his further input and then send a new version that is > also rebased against main. > Based on this I'll mark the series as changes requested for now and we will continue on the next revision when the conversation is settled. > > Thanks a lot, > Felix > > > > > > > > v8->v9: addressed reviews > > > v7->v8: introduced > > > > > > controller/ovn-controller.c | 27 ++++++++++++++++++++++++++- > > > lib/ovn-util.c | 19 ++++++++++++++----- > > > tests/ovn-controller.at | 1 - > > > tests/ovn-performance.at | 3 --- > > > tests/ovn.at | 10 ---------- > > > 5 files changed, 40 insertions(+), 20 deletions(-) > > > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 13a566d26..946e6146d 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -6029,6 +6029,7 @@ main(int argc, char *argv[]) > > > unsigned int ovs_cond_seqno = UINT_MAX; > > > unsigned int ovnsb_cond_seqno = UINT_MAX; > > > unsigned int ovnsb_expected_cond_seqno = UINT_MAX; > > > + bool has_computed_once = false; > > > > > > struct controller_engine_ctx ctrl_engine_ctx = { > > > .lflow_cache = lflow_cache_create(), > > > @@ -6103,6 +6104,20 @@ main(int argc, char *argv[]) > > > ovnsb_cond_seqno = new_ovnsb_cond_seqno; > > > } > > > > > > + /* Check if we have received all initial dumps of the > southbound > > > + * based on the monitor condtions we set. > > > + * If we have sb_monitor_all that means we have all data that > we > > > would > > > + * ever need. > > > + * If we monitor cases individually we need at least one > engine > > > run. */ > > > + if (ovnsb_cond_seqno == ovnsb_expected_cond_seqno > > > + && ovnsb_expected_cond_seqno != UINT_MAX) { > > > + if (sb_monitor_all) { > > > + daemon_started_recently_ignore(); > > > + } else if (has_computed_once) { > > > + daemon_started_recently_countdown(); > > > + } > > > + } > > > + > > > struct engine_context eng_ctx = { > > > .ovs_idl_txn = ovs_idl_txn, > > > .ovnsb_idl_txn = ovnsb_idl_txn, > > > @@ -6260,8 +6275,9 @@ main(int argc, char *argv[]) > > > > > > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME, > > > time_msec()); > > > + > > > if (engine_has_updated()) { > > > - daemon_started_recently_countdown(); > > > + has_computed_once = true; > > > } > > > > > > ct_zones_data = engine_get_data(&en_ct_zones); > > > @@ -6380,6 +6396,15 @@ main(int argc, char *argv[]) > > > &runtime_data->local_datapaths, > > > sb_monitor_all); > > > } > > > + /* If there is no new expected seqno we have > > > + * finished loading all needed data from > > > + * southbound. We then need to run one more > time > > > since > > > + * we might behave differently. */ > > > + if (daemon_started_recently() > > > + && ovnsb_cond_seqno == > > > ovnsb_expected_cond_seqno) { > > > + daemon_started_recently_ignore(); > > > + poll_immediate_wake(); > > > + } > > > if (ovs_idl_txn) { > > > update_qos(sbrec_port_binding_by_name, > > > ovs_idl_txn, > > > ovsrec_port_by_qos, > > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > > > index af17082a9..153a69ff6 100644 > > > --- a/lib/ovn-util.c > > > +++ b/lib/ovn-util.c > > > @@ -933,13 +933,24 @@ get_bridge(const struct ovsrec_bridge_table > > > *bridge_table, const char *br_name) > > > return NULL; > > > } > > > > > > +/* This counts the amount of iterations of monitoring condition > updates > > > and > > > + * their respective update towards us. > > > + * This only needs to handle the case of non monitor_all since that > will > > > use > > > + * the ignore feature below. > > > + * In this case we need at least 2 iterations: > > > + * 1. the initial iteration where we pull the sb content based on the > few > > > + * conditions we now have. > > > + * 2. after the first engine run we have enough information to update > the > > > + * monitoring conditions to their final values. > > > + * However based on the structure of the datapaths we might need > more. The > > > + * value below is just a hard limit of iterations. We detect if we are > > > done > > > + * earlier and then skip further iterations. */ > > > #define DAEMON_STARTUP_DELAY_SEED 20 > > > -#define DAEMON_STARTUP_DELAY_MS 10000 > > > > > > static int64_t startup_ts; > > > static int startup_delay = DAEMON_STARTUP_DELAY_SEED; > > > > > > -/* Used by debug command only, for tests. */ > > > +/* Used if we do not need the startup delay (e.g. when using > > > monitor_all). */ > > > static bool ignore_startup_delay = false; > > > > > > OVS_CONSTRUCTOR(startup_ts_initializer) { > > > @@ -980,9 +991,7 @@ daemon_started_recently(void) > > > if (startup_delay) { > > > return true; > > > } > > > - > > > - /* Ensure that at least an amount of time has passed. */ > > > - return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS; > > > + return false; > > > } > > > > > > /* Builds a unique address set compatible name > ([a-zA-Z_.][a-zA-Z_.0-9]*) > > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > > index a878a440b..781ee242a 100644 > > > --- a/tests/ovn-controller.at > > > +++ b/tests/ovn-controller.at > > > @@ -161,7 +161,6 @@ check_patches \ > > > 'br-eth0 patch-localnet2-to-br-int patch-br-int-to-localnet2' > > > > > > # Delete the mapping and the ovn-bridge-mapping patch ports should go > > > away. > > > -check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > AT_CHECK([ovs-vsctl remove Open_vSwitch . external-ids > > > ovn-bridge-mappings]) > > > check_bridge_mappings > > > check_patches > > > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at > > > index 44bd4f7e4..4ddb1bd81 100644 > > > --- a/tests/ovn-performance.at > > > +++ b/tests/ovn-performance.at > > > @@ -238,9 +238,6 @@ for i in `seq 1 5`; do > > > as hv$i > > > ovs-vsctl add-br br-phys > > > ovn_attach n1 br-phys 192.168.0.$i > > > - # Do not postpone patch ports related changes, as they may occur > more > > > or less anywere within the test, > > > - # hence causing unexpected recomputes. > > > - check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > if [[ $i -ge 3 ]] ; then > > > ovs-vsctl add-br br-ex > > > ovs-vsctl set open . > > > external_ids:ovn-bridge-mappings="public:br-ex" > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index 596a7be2b..9af74774f 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -35301,7 +35301,6 @@ OVS_WAIT_UNTIL([ > > > test x430 = x$(as hv1 ovs-vsctl get Interface ${iface1_uuid} > > > mtu_request) > > > ]) > > > > > > -as hv1 check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > # Check that pointing requested-chassis somewhere else will unplug the > > > port > > > check ovn-nbctl --wait=hv set Logical_Switch_Port lsp1 \ > > > options:requested-chassis=non-existent-chassis > > > @@ -35309,7 +35308,6 @@ OVS_WAIT_UNTIL([ > > > ! as hv1 ovs-vsctl get Interface ${iface1_uuid} _uuid > > > ]) > > > > > > -as hv2 check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > # Check that removing an lport will unplug it > > > AT_CHECK([test x${iface2_uuid} = x$(as hv2 ovs-vsctl get Interface > > > ${iface2_uuid} _uuid)], [0], []) > > > check ovn-nbctl --wait=hv lsp-del ${lsp2_uuid} > > > @@ -38073,9 +38071,6 @@ ovs-vsctl -- add-port br-phys-2 > > > fake-phys-2-to-int-2 -- \ > > > > > > ovn_attach n1 br-phys 192.168.1.1 24 > > > > > > -# allow controller to immediately clean patch ports up > > > -check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > - > > > # check that patch port on br-int is cleaned up (and also its peer) > > > OVS_WAIT_UNTIL([ > > > test 0 = $(ovs-vsctl --columns _uuid --bare find Port > > > name=fake-int-to-phys | wc -l) > > > @@ -38141,9 +38136,6 @@ check ovn-nbctl lsp-set-options ln_port > > > network_name=phys-1 > > > wait_for_ports_up > > > check ovn-nbctl --wait=hv sync > > > > > > -# Allow controller to immediately clean patch ports up if any. > > > -check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > - > > > # check that patch port created on br-int and br-phys-1. > > > OVS_WAIT_UNTIL([ > > > test 1 = $(ovs-vsctl --columns _uuid --bare find Port > > > name=patch-br-int-to-ln_port | wc -l) > > > @@ -41407,8 +41399,6 @@ check ovn-nbctl lsp-add sw0 sw0p1 > > > > > > check ovn-nbctl --wait=hv sync > > > > > > -check ovn-appctl -t ovn-controller debug/ignore-startup-delay > > > - > > > # Waits until the OVS database contains exactly the specified patch > ports. > > > # Each argument should be of the form BRIDGE PORT PEER. > > > check_patch_ports () { > > > -- > > > 2.43.0 > > > > > > > > > _______________________________________________ > > > dev mailing list > > > d...@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > Thanks, > > Ales > > Thanks, Ales _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
