On 5/3/23 16:12, Vladislav Odintsov wrote:
> Hi Dumitru and Mark,
>
> thanks for the review!
>
>> On 3 May 2023, at 16:56, Dumitru Ceara <dce...@redhat.com> wrote:
>>
>> 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.
>
> I’m also fine with this change and have no objection to fix this on the apply
> time if needed.
>
>>>
>>>>
>>>> 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.
>
> Yes, this is due to the facts of northd logic for propagation options from
> NB_Global to SB_Global table.
>
>>
>>> 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.
>
> This was my first approach, where I tried to read nbctl_… and sbctl_… options
> from appropriate databases, but faced with northd syncing (and deletion
> option from SB).
> I decided to make a common option thinking that for northd there is similar
> logic where NB and SB connections are configured from one option
> (northd_probe_interval).
>
> You you think it’s worth to split options, so I need some time find out how
> to prevent northd from syncing custom option from NB to SB and how to make it
> not to delete new option from SB.
>
It's not really great but we already do that for
"lb_hairpin_use_ct_mark". Maybe something similar?
>>
>>>>> +
>>>>> + <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?
>
> Yeap, you’re right.
>
>>>
>>>>> +
>>>>> + 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
>
>
> Regards,
> Vladislav Odintsov
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
