On Wed, Aug 17, 2022 at 8:27 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 7/25/22 23:34, Han Zhou wrote:
> > In some cases we need to know if ovn-controller started long enough and
> > has enough iterations of input processing, primarily to ensure it has
> > downloaded and handled a complete initial view of the SB DB (and of
> > course the local OVS DB), so that it won't delete things too early by
> > mistake based on incomplete data.
> >
> > The mechanism will be used in follow up patches.
> >
> > Suggested-by: Dumitru Ceara <dce...@redhat.com>
> > Signed-off-by: Han Zhou <hz...@ovn.org>
> > ---
>
> Hi Han,
>
> > controller/ovn-controller.c | 22 +++++++++++++---
> > lib/inc-proc-eng.c | 11 ++++++++
> > lib/inc-proc-eng.h | 4 +++
> > lib/ovn-util.c | 50 +++++++++++++++++++++++++++++++++++++
> > lib/ovn-util.h | 4 +++
> > 5 files changed, 88 insertions(+), 3 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 2e9138036..8fc554201 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -93,6 +93,7 @@ static unixctl_cb_func debug_dump_lflow_conj_ids;
> > static unixctl_cb_func lflow_cache_flush_cmd;
> > static unixctl_cb_func lflow_cache_show_stats_cmd;
> > static unixctl_cb_func debug_delay_nb_cfg_report;
> > +static unixctl_cb_func debug_ignore_startup_delay;
> >
> > #define DEFAULT_BRIDGE_NAME "br-int"
> > #define DEFAULT_DATAPATH "system"
> > @@ -863,11 +864,12 @@ static void
> > store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn
*ovs_txn,
> > const struct sbrec_chassis_private *chassis,
> > const struct ovsrec_bridge *br_int,
> > - unsigned int delay_nb_cfg_report, int64_t startup_ts)
> > + unsigned int delay_nb_cfg_report)
> > {
> > struct ofctrl_acked_seqnos *acked_nb_cfg_seqnos =
> > ofctrl_acked_seqnos_get(ofctrl_seq_type_nb_cfg);
> > uint64_t cur_cfg = acked_nb_cfg_seqnos->last_acked;
> > + int64_t startup_ts = daemon_startup_ts();
> >
> > if (ovs_txn && br_int
> > && startup_ts != smap_get_ullong(&br_int->external_ids,
> > @@ -3796,6 +3798,9 @@ main(int argc, char *argv[])
> > debug_dump_lflow_conj_ids,
> > &lflow_output_data->conj_ids);
> >
> > + unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
> > + debug_ignore_startup_delay, NULL);
> > +
> > unsigned int ovs_cond_seqno = UINT_MAX;
> > unsigned int ovnsb_cond_seqno = UINT_MAX;
> > unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
> > @@ -3817,7 +3822,6 @@ main(int argc, char *argv[])
> > /* Main loop. */
> > exiting = false;
> > restart = false;
> > - int64_t startup_ts = time_wall_msec();
> > bool sb_monitor_all = false;
> > while (!exiting) {
> > memory_run();
> > @@ -3997,6 +4001,10 @@ main(int argc, char *argv[])
> > }
> > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
> > time_msec());
> > + if (engine_has_updated()) {
> > + daemon_started_recently_countdown();
> > + }
> > +
> > ct_zones_data = engine_get_data(&en_ct_zones);
> > if (ovs_idl_txn) {
> > if (ct_zones_data) {
> > @@ -4159,7 +4167,7 @@ main(int argc, char *argv[])
> > }
> >
> > store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private,
> > - br_int, delay_nb_cfg_report, startup_ts);
> > + br_int, delay_nb_cfg_report);
> >
> > if (pending_pkt.conn) {
> > struct ed_type_addr_sets *as_data =
> > @@ -4633,3 +4641,11 @@ debug_dump_lflow_conj_ids(struct unixctl_conn
*conn, int argc OVS_UNUSED,
> > unixctl_command_reply(conn, ds_cstr(&conj_ids_dump));
> > ds_destroy(&conj_ids_dump);
> > }
> > +
> > +static void
> > +debug_ignore_startup_delay(struct unixctl_conn *conn, int argc
OVS_UNUSED,
> > + const char *argv[] OVS_UNUSED, void *arg
OVS_UNUSED)
> > +{
> > + daemon_started_recently_ignore();
> > + unixctl_command_reply(conn, NULL);
> > +}
> > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> > index 575b774ae..2e2b31033 100644
> > --- a/lib/inc-proc-eng.c
> > +++ b/lib/inc-proc-eng.c
> > @@ -312,6 +312,17 @@ engine_has_run(void)
> > return false;
> > }
> >
> > +bool
> > +engine_has_updated(void)
> > +{
> > + for (size_t i = 0; i < engine_n_nodes; i++) {
> > + if (engine_nodes[i]->state == EN_UPDATED) {
> > + return true;
> > + }
> > + }
> > + return false;
> > +}
> > +
> > bool
> > engine_aborted(void)
> > {
> > diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> > index 9bfab1f7c..dc365dc18 100644
> > --- a/lib/inc-proc-eng.h
> > +++ b/lib/inc-proc-eng.h
> > @@ -238,6 +238,10 @@ bool engine_node_changed(struct engine_node *node);
> > /* Return true if the engine has run in the last iteration. */
> > bool engine_has_run(void);
> >
> > +/* Return true if the engine has any update in any node, i.e. any input
> > + * has changed; false if nothing has changed. */
> > +bool engine_has_updated(void);
> > +
> > /* Returns true if during the last engine run we had to abort
processing. */
> > bool engine_aborted(void);
> >
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index 616999eab..693735841 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -883,3 +883,53 @@ get_bridge(const struct ovsrec_bridge_table
*bridge_table, const char *br_name)
> > }
> > return NULL;
> > }
> > +
> > +#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;
> > +static bool ignore_startup_delay = false;
> > +
> > +OVS_CONSTRUCTOR(startup_ts_initializer) {
> > + startup_ts = time_wall_msec();
>
> A bit of a nit, but if we moved the "debug/ignore-startup-delay" unixctl
> registration here, we wouldn't need the daemon_started_recently_ignore()
> api either.
Maybe, but I'd avoid register unix commands from the util module since it
doesn't seem to have a proper "initialize" place to do that. Now that I
register the command in main(), we will need an API anyway, either for the
command function, or for the current API, so I just left as is.
>
> I think I would also add a comment specifying that this is needed just
> for the sake of making the test reliable.
>
Ack. I added a comment for the ignore_startup_delay variable in v3.
Thanks,
Han
> > +}
> > +
> > +int64_t
> > +daemon_startup_ts(void)
> > +{
> > + return startup_ts;
> > +}
> > +
> > +void
> > +daemon_started_recently_countdown(void)
> > +{
> > + if (startup_delay > 0) {
> > + startup_delay--;
> > + }
> > +}
> > +
> > +void
> > +daemon_started_recently_ignore(void)
> > +{
> > + ignore_startup_delay = true;
> > +}
> > +
> > +bool
> > +daemon_started_recently(void)
> > +{
> > + if (ignore_startup_delay) {
> > + return false;
> > + }
> > +
> > + VLOG_DBG("startup_delay: %d, startup_ts: %"PRId64, startup_delay,
> > + startup_ts);
> > +
> > + /* Ensure that at least an amount of updates has been handled. */
> > + 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;
> > +}
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index b3905ef7b..145f974ed 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -309,5 +309,9 @@ struct ovsrec_bridge_table;
> > const struct ovsrec_bridge *get_bridge(const struct
ovsrec_bridge_table *,
> > const char *br_name);
> >
> > +void daemon_started_recently_countdown(void);
> > +void daemon_started_recently_ignore(void);
> > +bool daemon_started_recently(void);
> > +int64_t daemon_startup_ts(void);
> >
> > #endif /* OVN_UTIL_H */
>
>
> Thanks,
> Dumitru
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
