Hi Vladislav,
On 5/3/23 02:55, Vladislav Odintsov wrote:
> For large OVN_Southbound (or other) databases the default interval of 5000 ms
> could be not sufficient to run. This patch adds configuration of OVSDB
> inactivity probes for ovn-*ctl utilities.
>
> Initially, on the utility start the hardcoded value of 120000 ms is set.
> For daemon-mode it is possible to configure intervals in OVN Northbound
> database:
> OVN_Northbound.NB_Global.options.dbctl_inactivity_probe for ovn-nbctl and
> ovn-sbctl utilities.
>
> If no DB configuration option was provided, the initial (120000 ms) interval
> is left.
Nit: I would add here something like "A non-zero inactivity probe
interval is required in order to allow DB clients to detect connection
failures due to network issues."
>
> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com>
> ---
> lib/ovn-util.h | 2 ++
> ovn-nb.xml | 20 ++++++++++++++++++++
> utilities/ovn-dbctl.c | 18 ++++++++++++++++++
> utilities/ovn-dbctl.h | 1 +
> utilities/ovn-ic-nbctl.c | 4 ++++
> utilities/ovn-ic-sbctl.c | 4 ++++
> utilities/ovn-nbctl.c | 15 +++++++++++++++
> utilities/ovn-sbctl.c | 15 +++++++++++++++
I'm not sure whether we should we mention this user visible change in
the NEWS file.
> 8 files changed, 79 insertions(+)
>
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 7cf861dbc..47e53ca74 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -154,6 +154,8 @@ void set_idl_probe_interval(struct ovsdb_idl *idl, const
> char *remote,
> #define OVN_MAX_DP_VXLAN_KEY ((1u << 12) - 1)
> #define OVN_MAX_DP_VXLAN_KEY_LOCAL (OVN_MAX_DP_KEY - OVN_MAX_DP_GLOBAL_NUM)
>
> +#define DEFAULT_UTILS_PROBE_INTERVAL_MSEC 120000
I think this should go to ovn-dbctl.h instead but whoever applies the
patch can move this. So I don't think there's a need for v3, therefore:
Acked-by: Dumitru Ceara <dce...@redhat.com>
Ilya, Mark, Numan, Han, what do you guys think?
Regards,
Dumitru
> +
> struct hmap;
> void ovn_destroy_tnlids(struct hmap *tnlids);
> bool ovn_add_tnlid(struct hmap *set, uint32_t tnlid);
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index fd32070f2..a82f31a92 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -215,6 +215,26 @@
> </p>
> </column>
>
> + <column name="options" key="dbctl_probe_interval">
> + <p>
> + The inactivity probe interval of the connection to the OVN
> Northbound
> + and Southbound databases from <code>ovn-nbctl</code> and
> + <code>ovn-sbctl</code> utilities respectively, in milliseconds.
> + If the value is zero, it disables the connection keepalive feature.
> + </p>
> +
> + <p>
> + If the value is nonzero, then it will be forced to a value of
> + at least 1000 ms.
> + </p>
> +
> + <p>
> + If the value less then zero, then the default inactivity probe
> + interval for <code>ovn-nbctl</code> and <code>ovn-sbctl</code>
> would
> + be left intact (120000 ms).
> + </p>
> + </column>
> +
> <column name="options" key="northd_trim_timeout">
> <p>
> When used, this configuration value specifies the time, in
> diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
> index 369a6a663..c6aebcbbb 100644
> --- a/utilities/ovn-dbctl.c
> +++ b/utilities/ovn-dbctl.c
> @@ -205,6 +205,9 @@ ovn_dbctl_main(int argc, char *argv[],
> ovsdb_idl_set_remote(idl, db, daemon_mode);
> ovsdb_idl_set_leader_only(idl, leader_only);
>
> + /* Set reasonable high probe interval. */
> + set_idl_probe_interval(idl, db, DEFAULT_UTILS_PROBE_INTERVAL_MSEC);
> +
> if (daemon_mode) {
> server_loop(dbctl_options, idl, argc, argv_);
> } else {
> @@ -1094,6 +1097,17 @@ out:
> free(argv);
> }
>
> +static void
> +update_inactivity_probe(struct server_cmd_run_ctx *ctx)
> +{
> + int inactivity_probe =
> ctx->dbctl_options->get_inactivity_probe(ctx->idl);
> + if (inactivity_probe < 0) {
> + inactivity_probe = DEFAULT_UTILS_PROBE_INTERVAL_MSEC;
> + }
> +
> + set_idl_probe_interval(ctx->idl, db, inactivity_probe);
> +}
> +
> static void
> server_loop(const struct ovn_dbctl_options *dbctl_options,
> struct ovsdb_idl *idl, int argc, char *argv[])
> @@ -1125,6 +1139,10 @@ server_loop(const struct ovn_dbctl_options
> *dbctl_options,
>
> for (;;) {
> update_ssl_config();
> +
> + /* Configure inactivity probe from connected DB. */
> + update_inactivity_probe(&server_cmd_run_ctx);
> +
> memory_run();
> if (memory_should_report()) {
> struct simap usage = SIMAP_INITIALIZER(&usage);
> diff --git a/utilities/ovn-dbctl.h b/utilities/ovn-dbctl.h
> index a1fbede6b..54c232dd6 100644
> --- a/utilities/ovn-dbctl.h
> +++ b/utilities/ovn-dbctl.h
> @@ -52,6 +52,7 @@ struct ovn_dbctl_options {
> const struct timer *wait_timeout,
> long long int start_time, bool print_wait_time);
>
> + int (*get_inactivity_probe)(struct ovsdb_idl *);
> struct ctl_context *(*ctx_create)(void);
> void (*ctx_destroy)(struct ctl_context *);
> };
> diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c
> index f3d8039a8..19d8b054f 100644
> --- a/utilities/ovn-ic-nbctl.c
> +++ b/utilities/ovn-ic-nbctl.c
> @@ -116,6 +116,10 @@ main(int argc, char *argv[])
> ovsdb_idl_set_remote(idl, db, false);
> ovsdb_idl_set_db_change_aware(idl, false);
> ovsdb_idl_set_leader_only(idl, leader_only);
> +
> + /* Set reasonable high probe interval. */
> + set_idl_probe_interval(idl, db, DEFAULT_UTILS_PROBE_INTERVAL_MSEC);
> +
> run_prerequisites(commands, n_commands, idl);
>
> /* Execute the commands.
> diff --git a/utilities/ovn-ic-sbctl.c b/utilities/ovn-ic-sbctl.c
> index 3060b48b9..215d09d30 100644
> --- a/utilities/ovn-ic-sbctl.c
> +++ b/utilities/ovn-ic-sbctl.c
> @@ -115,6 +115,10 @@ main(int argc, char *argv[])
> ovsdb_idl_set_remote(idl, db, false);
> ovsdb_idl_set_db_change_aware(idl, false);
> ovsdb_idl_set_leader_only(idl, leader_only);
> +
> + /* Set reasonable high probe interval. */
> + set_idl_probe_interval(idl, db, DEFAULT_UTILS_PROBE_INTERVAL_MSEC);
> +
> run_prerequisites(commands, n_commands, idl);
>
> /* Execute the commands.
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 45572fd30..49c5b294a 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -153,6 +153,20 @@ nbctl_post_execute(struct ovsdb_idl *idl, struct
> ovsdb_idl_txn *txn,
> }
> }
>
> +static int
> +get_inactivity_probe(struct ovsdb_idl *idl)
> +{
> + const struct nbrec_nb_global *nb = nbrec_nb_global_first(idl);
> + int interval = -1;
> +
> + if (nb) {
> + interval = smap_get_int(&nb->options, "dbctl_probe_interval",
> + interval);
> + }
> +
> + return interval;
> +}
> +
> static char * OVS_WARN_UNUSED_RESULT dhcp_options_get(
> struct ctl_context *ctx, const char *id, bool must_exist,
> const struct nbrec_dhcp_options **);
> @@ -7935,6 +7949,7 @@ main(int argc, char *argv[])
> .add_base_prerequisites = nbctl_add_base_prerequisites,
> .pre_execute = nbctl_pre_execute,
> .post_execute = nbctl_post_execute,
> + .get_inactivity_probe = get_inactivity_probe,
>
> .ctx_create = nbctl_ctx_create,
> .ctx_destroy = nbctl_ctx_destroy,
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 542ab9ffa..76465fe7e 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -150,6 +150,20 @@ Other options:\n\
> * gracefully. */
> #define ctl_fatal dont_use_ctl_fatal_use_ctl_error_and_return
>
> +static int
> +get_inactivity_probe(struct ovsdb_idl *idl)
> +{
> + const struct sbrec_sb_global *sb = sbrec_sb_global_first(idl);
> + int interval = -1;
> +
> + if (sb) {
> + interval = smap_get_int(&sb->options, "dbctl_probe_interval",
> + interval);
> + }
> +
> + return interval;
> +}
> +
> /* ovs-sbctl specific context. Inherits the 'struct ctl_context' as base. */
> struct sbctl_context {
> struct ctl_context base;
> @@ -1590,6 +1604,7 @@ main(int argc, char *argv[])
> .add_base_prerequisites = sbctl_add_base_prerequisites,
> .pre_execute = sbctl_pre_execute,
> .post_execute = NULL,
> + .get_inactivity_probe = get_inactivity_probe,
>
> .ctx_create = sbctl_ctx_create,
> .ctx_destroy = sbctl_ctx_destroy,
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
