On 6/29/22 21:49, Han Zhou wrote:
> On Wed, Jun 29, 2022 at 1:37 AM Dumitru Ceara <[email protected]> wrote:
>>
>> On 6/28/22 19:18, Han Zhou wrote:
>>> On Tue, Jun 28, 2022 at 8:20 AM Dumitru Ceara <[email protected]> wrote:
>>>>
>>>> On 6/28/22 02:49, Han Zhou wrote:
>>>>> When ovn-controller is restarted, it may need multiple iterations of
>>>>> main loop before completely download all related data from SB DB,
>>>>> especially when ovn-monitor-all=false, so after restart, before it
> sees
>>>>> the related localnet ports from SB DB, it treats the related patch
>>>>> ports on the chassis as not needed, and deleted them. Later when it
>>>>> downloads thoses port-bindings it recreates them. For a graceful
>>>>> upgrade, we don't this to happen, because it would break the traffic.
>>>>>
>>>>> This is especially problematic at ovn-k8s setups because the external
>>>>> side of the patch port is used to program some flows for external
>>>>> network communication. When the patch ports are recreated, the OF port
>>>>> number changes and those flows are broken. The CMS would detect and
> fix
>>>>> the flows but it would result in even longer downtime.
>>>>>
>>>>> This patch postpone the deletion of the patch ports, with the
> assumption
>>>>> that leaving the unused ports on the chassis for little longer is not
>>>>> harmful.
>>>>>
>>>>> Signed-off-by: Han Zhou <[email protected]>
>>>>> ---
>>>>> controller/patch.c | 15 ++++++++-
>>>>> tests/ovn-controller.at | 71
> +++++++++++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 85 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/controller/patch.c b/controller/patch.c
>>>>> index ed831302c..9faae5879 100644
>>>>> --- a/controller/patch.c
>>>>> +++ b/controller/patch.c
>>>>> @@ -307,11 +307,24 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>>>>>
>>>>> /* Now 'existing_ports' only still contains patch ports that
> exist
>>> in the
>>>>> * database but shouldn't. Delete them from the database. */
>>>>> +
>>>>> + /* Wait for some iterations before really deleting any patch
>>> ports, because
>>>>> + * with conditional monitoring it is possible that related SB
> data
>>> is not
>>>>> + * completely downloaded yet after last restart of
> ovn-controller.
>>>>> + * Otherwise it may cause unncessary dataplane interruption
> during
>>>>> + * restart/upgrade. */
>>>>> + static int delete_patch_port_delay = 10;
>>>>
>>>> Hi Han,
>>>>
>>>> It's possible that ovn-controller wakes up 10 times in a row
> immediately
>>>> due to local OVSDB changes and doesn't process any new SB updates in
>>>> that time so 10 might not be enough in some cases.
>>>>
>>>
>>> Thanks Dumitru for the review!
>>> In theory I agree with you 10 times is not 100% guaranteeing SB update
>>> completed if other things are triggering the wakeups. However, in
> practice,
>>> the purpose here is for the start/restart scenario. I think it is very
>>> unlikely that local OVSDB is changing that frequently at the same time
> when
>>> ovn-controller is being restarted. We have some similar logic (not ideal
>>> for sure) at vif-plug.c:vif_plug_run() for similar reasons.
>>>
>>
>> Ah I didn't know we do that already for vif-plug. Thanks for pointing
>> it out!
>>
>>>> Does it make sense to wait a given amount of time instead? E.g., a few
>>>> seconds? Can we make this configurable somehow? Maybe an
>>>> ovn-controller command line argument to override the default?
>>>
>>> Waiting for a given amount of time is also not ideal. It is possible
> that
>>> when ovn-controller starts the SB IDL is not connected (due to server
> side
>>> problems/ control plane network problems, etc.) so we don't know how
> long
>>> it should wait at all.
>>>
>>> I am ok with adding command line arguments to adjust, but I'd really
> want
>>> to avoid that unless it is proved to be necessary. I'd rather use a
> bigger
>>> hardcoded value to avoid another knob which is not easy to understand by
>>> users - it should be something handled by the code itself.
>>>
>>
>> I understand your point against additional knobs. Maybe we don't need
>> a new one. What if we do a mixed approach? We already have the
>> ovn-controller startup timestamp so we could have a single (hardcoded)
>> delay counter but also ensure that at least X seconds elapsed. It might
>> be a bit over-engineered but what do you think of the following
>> incremental?
>>
>
> Thanks Dumitru. I am ok with adding a check for time passed in addition to
> the iterations. The function daemon_started_recently() you added below
> would have a problem because each caller of this function would trigger the
> decrement of the controller_startup_delay, so if there are 10 callers it
Oops, you're right.
> would decrease to 0 with just one iteration. But the overall approach
> demonstrated looks good. I will work on V2 with your idea incorporated.
>
Cool.
> In addition, it doesn't seem necessary to me for the vif_plug to reset the
> counter when DB is reconnected, because I don't expect it to lose old data
> in such cases - the monitor condition remains unchanged. So I think
> dbs_connected_recently() is not really needed. I will probably touch the
> vif_plug logic in a separate patch.
I didn't spend too much time on understanding why vif_plug cared about
DB reconnects but now after you mentioned it it does sound like it's not
really needed.
Thanks,
Dumitru
>
> Thanks,
> Han
>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 69615308e..153f742b1 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -863,11 +863,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,
>> @@ -3811,7 +3812,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();
>> @@ -3864,7 +3864,7 @@ main(int argc, char *argv[])
>> if (!new_ovnsb_cond_seqno) {
>> VLOG_INFO("OVNSB IDL reconnected, force recompute.");
>> engine_set_force_recompute(true);
>> - vif_plug_reset_idl_prime_counter();
>> + dbs_connected_record();
>> }
>> ovnsb_cond_seqno = new_ovnsb_cond_seqno;
>> }
>> @@ -4153,7 +4153,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 =
>> diff --git a/controller/patch.c b/controller/patch.c
>> index 9faae5879..d5879c580 100644
>> --- a/controller/patch.c
>> +++ b/controller/patch.c
>> @@ -313,16 +313,12 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>> * completely downloaded yet after last restart of ovn-controller.
>> * Otherwise it may cause unncessary dataplane interruption during
>> * restart/upgrade. */
>> - static int delete_patch_port_delay = 10;
>> - if (delete_patch_port_delay > 0) {
>> - delete_patch_port_delay--;
>> - }
>> -
>> + bool keep_patch_ports = daemon_started_recently();
>> struct shash_node *port_node;
>> SHASH_FOR_EACH_SAFE (port_node, &existing_ports) {
>> port = port_node->data;
>> shash_delete(&existing_ports, port_node);
>> - if (!delete_patch_port_delay) {
>> + if (!keep_patch_ports) {
>> remove_port(bridge_table, port);
>> }
>> }
>> diff --git a/controller/vif-plug.c b/controller/vif-plug.c
>> index c6fbe7e59..fc7a72a7d 100644
>> --- a/controller/vif-plug.c
>> +++ b/controller/vif-plug.c
>> @@ -532,22 +532,14 @@ vif_plug_handle_iface(const struct ovsrec_interface
> *iface_rec,
>> * completeness of the initial data downloading we need this counter so
> that we
>> * do not erronously unplug ports because the data is just not loaded
> yet.
>> */
>> -#define VIF_PLUG_PRIME_IDL_COUNT_SEEED 10
>> -static int vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
>> -
>> -void
>> -vif_plug_reset_idl_prime_counter(void)
>> -{
>> - vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
>> -}
>> -
>> void
>> vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
>> struct vif_plug_ctx_out *vif_plug_ctx_out)
>> {
>> - if (vif_plug_prime_idl_count && --vif_plug_prime_idl_count > 0) {
>> - VLOG_DBG("vif_plug_run: vif_plug_prime_idl_count=%d, will not
> unplug "
>> - "ports in this iteration.", vif_plug_prime_idl_count);
>> + bool delay_plug = daemon_started_recently() ||
> dbs_connected_recently();
>> + if (delay_plug) {
>> + VLOG_DBG("vif_plug_run: controller recently started, will not
> unplug "
>> + "ports in this iteration.");
>> }
>>
>> if (!vif_plug_ctx_in->chassis_rec) {
>> @@ -557,7 +549,7 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
>> OVSREC_INTERFACE_TABLE_FOR_EACH (iface_rec,
>> vif_plug_ctx_in->iface_table) {
>> vif_plug_handle_iface(iface_rec, vif_plug_ctx_in,
> vif_plug_ctx_out,
>> - !vif_plug_prime_idl_count);
>> + !delay_plug);
>> }
>>
>> struct sbrec_port_binding *target =
>> @@ -573,7 +565,7 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
>> enum en_lport_type lport_type = get_lport_type(pb);
>> if (lport_type == LP_VIF) {
>> vif_plug_handle_lport_vif(pb, vif_plug_ctx_in,
> vif_plug_ctx_out,
>> - !vif_plug_prime_idl_count);
>> + !delay_plug);
>> }
>> }
>> sbrec_port_binding_index_destroy_row(target);
>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
>> index 616999eab..7b859d440 100644
>> --- a/lib/ovn-util.c
>> +++ b/lib/ovn-util.c
>> @@ -883,3 +883,44 @@ get_bridge(const struct ovsrec_bridge_table
> *bridge_table, const char *br_name)
>> }
>> return NULL;
>> }
>> +
>> +static int64_t startup_ts;
>> +
>> +OVS_CONSTRUCTOR(startup_ts_initializer) {
>> + startup_ts = time_wall_msec();
>> +}
>> +
>> +int64_t
>> +daemon_startup_ts(void)
>> +{
>> + return startup_ts;
>> +}
>> +
>> +#define DAEMON_STARTUP_DELAY_SEED 10
>> +#define DAEMON_STARTUP_DELAY_MS 5000
>> +bool
>> +daemon_started_recently(void)
>> +{
>> + static int controller_startup_delay = DAEMON_STARTUP_DELAY_SEED;
>> +
>> + if (controller_startup_delay && --controller_startup_delay) {
>> + return true;
>> + }
>> +
>> + /* Ensure that at least an amount of time has passed. */
>> + return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS;
>> +}
>> +
>> +#define DBS_CONNECTED_RECENTLY_DELAY_SEED 10
>> +static int dbs_connected_recently_delay =
> DBS_CONNECTED_RECENTLY_DELAY_SEED;
>> +void
>> +dbs_connected_record(void)
>> +{
>> + dbs_connected_recently_delay = DBS_CONNECTED_RECENTLY_DELAY_SEED;
>> +}
>> +
>> +bool
>> +dbs_connected_recently(void)
>> +{
>> + return dbs_connected_recently_delay &&
> --dbs_connected_recently_delay;
>> +}
>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
>> index b3905ef7b..ed6b345a8 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);
>>
>> +int64_t daemon_startup_ts(void);
>> +bool daemon_started_recently(void);
>> +void dbs_connected_record(void);
>> +bool dbs_connected_recently(void);
>>
>> #endif /* OVN_UTIL_H */
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
