On 6/28/22 19:18, Han Zhou wrote:
> On Tue, Jun 28, 2022 at 8:20 AM Dumitru Ceara <dce...@redhat.com> 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 <hz...@ovn.org>
>>> ---
>>> 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?
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
