On 5/3/23 15:47, Mark Michelson wrote:
> On 5/3/23 09:03, Dumitru Ceara wrote:
>> 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.
>
> I think it's worth mentioning 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:
>
> I also agree with this suggestion.
>
>>
>> Acked-by: Dumitru Ceara <dce...@redhat.com>
>>
>> Ilya, Mark, Numan, Han, what do you guys think?
>
> Before I can ack this patch, I want to bring up two points:
>
> 1) This patch updates ovn-nb.xml, but it does not update ovn-sb.xml to
> document the new SB option.
>
We already don't document everything that applies to both.
> 2) I'm not a big fan of the name "dbctl_probe_interval", because the
> term "dbctl" is not well-known to most OVN users. My suggestion would be
> to call the NB option "nbctl_probe_interval" or
> "ovn_nbctl_probe_interval" and to call the SB option
> "sbctl_probe_interval" or "ovn_sbctl_probe_interval". This makes it more
> clear that the option pertains to the "ovn-nbctl" and "ovn-sbctl"
> utilities. However, if others disagree with me on this, then we can use
> "dbctl_probe_interval" .
>
The problem is northd propagates all NB.NB_Global.options to
SB.SB_Global.options.
> I also have a couple of small findings below.
>
>>
>> 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>
>
> The documentation makes it sound as though the NB dbctl_probe_interval
> affects *both* ovn-nbctl and ovn-sbctl. However, this only affects
> ovn-nbctl. ovn-sbctl is controlled by the SB database.
>
Due to the fact that the option is propagated by northd it eventually
applies to both. But thinking some more about it, you're right, it's
probably better to add explicit nb and sb options.
>>> +
>>> + <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
>
> "If the value is less than zero"
>
>>> + 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;
>
> Why does this default to -1? Wouldn't it make more sense to use
> DEFAULT_UTILS_PROBE_INTERVAL_MSEC here?
>
>>> +
>>> + 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;
>
> Same comment here about using -1 as the default.
>
>>> +
>>> + 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
