daemon_started_recently() concept is flawed in terms that it uses
fixed number of iterations for a countdown and a fixed timeout, so
the undesired removal of configured resources can still happen under
certain conditions even with this mechanism in place.
The root cause of the original problem is that conditional monitoring
in ovn-controller works in stages. ovn-controller doesn't know all
the information it needs to monitor until the first monitor reply
arrives, then it adjusts conditions and receives all the data. That
doesn't work well with the ovsdb_idl_has_ever_connected() check that
will return 'true' right after the first monitor reply.
This leads to situation where resources can be removed after the
first monitor reply and re-added back after condition change.
Instead of waiting a fixed number of iterations/seconds, the correct
solution should be to always start with unconditional monitoring.
When the full initial snapshot is received, switch to conditional
monitoring as data is complete. This way ovsdb_idl_has_ever_connected()
check will provide an accurate answer to the question: if we have
a sufficient amount of data to make decisions about resource removal?
Once switched to conditional monitoring, southbound database will
remove all the unnecessary data from the ovn-controller, so it will
not need to keep it for long. And since setup is using conditional
monitoring, it's likely not that big anyway to be concerned about
a brief spike in memory consumption on startup.
This change will also remove unnecessary waiting on startup when all
the data is already available.
The main change is in the ovsdb_idl_has_ever_connected() check in the
update_sb_monitors(), the rest is mostly a revert of commits that
introduced the startup delay.
Test enhanced to run with and w/o conditional monitoring, since the
issue is closely related it it.
Alternative solution might be to store the first condition change
sequence number and replace all ovsdb_idl_has_ever_connected() calls
with a function that also checks for the current sequence number being
higher than condition change one, but that will require slightly more
code. Can be implemented in the future, if the first unconditional
monitor reply will become a problem some day.
Fixes: 2b27eb3482a3 ("patch.c: Avoid patch interface deletion & recreation
during restart.")
Signed-off-by: Ilya Maximets <[email protected]>
---
TODO.rst | 7 -----
controller/ovn-controller.c | 31 ++++++++--------------
controller/patch.c | 9 +------
controller/vif-plug.c | 33 +++++------------------
controller/vif-plug.h | 1 +
lib/inc-proc-eng.c | 11 --------
lib/inc-proc-eng.h | 4 ---
lib/ovn-util.c | 52 -------------------------------------
lib/ovn-util.h | 4 ---
tests/ovn-controller.at | 3 ++-
tests/ovn.at | 2 --
11 files changed, 22 insertions(+), 135 deletions(-)
diff --git a/TODO.rst b/TODO.rst
index 12738280b..d54817011 100644
--- a/TODO.rst
+++ b/TODO.rst
@@ -172,10 +172,3 @@ OVN To-do List
* Multi-threaded logical flow computation was optimized for the case
when datapath groups are disabled. Datpath groups are always enabled
now so northd parallel processing should be revisited.
-
-* ovn-controller daemon module
-
- * Dumitru Ceara: Add a new module e.g. ovn/lib/daemon-ovn.c that wraps
- OVS' daemonize_start() call and initializes the additional things, like
- the unixctl commands. Or, we should move the APIs such as
- daemon_started_recently() to OVS's lib/daemon.
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 0b0ccc48a..2ac6409bc 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -94,7 +94,6 @@ static unixctl_cb_func debug_dump_lflow_conj_ids;
static unixctl_cb_func lflow_cache_flush_cmd;
static unixctl_cb_func lflow_cache_show_stats_cmd;
static unixctl_cb_func debug_delay_nb_cfg_report;
-static unixctl_cb_func debug_ignore_startup_delay;
#define DEFAULT_BRIDGE_NAME "br-int"
#define DEFAULT_DATAPATH "system"
@@ -190,7 +189,14 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
* avoid the unnecessarily extra wake-ups of ovn-controller. */
ovsdb_idl_condition_add_clause_true(&ldpg);
- if (monitor_all) {
+ /* Don't enable conditional monitoring until we have a full view on a
+ * data that we need to monitor. Without that we will send a first
+ * monitor request with incomplete conditions, receive a reply, update
+ * conditions and receive the rest of data. Such a split may result in
+ * removal and re-addition of certain resources (e.g. patch ports)
+ * on restart of ovn-controller. Database will send deletion requiests
+ * for all the data that we don't actually need. */
+ if (monitor_all || !ovsdb_idl_has_ever_connected(ovnsb_idl)) {
ovsdb_idl_condition_add_clause_true(&pb);
ovsdb_idl_condition_add_clause_true(&lf);
ovsdb_idl_condition_add_clause_true(&mb);
@@ -867,12 +873,11 @@ 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)
+ unsigned int delay_nb_cfg_report, int64_t startup_ts)
{
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,
@@ -3860,9 +3865,6 @@ main(int argc, char *argv[])
debug_dump_lflow_conj_ids,
&lflow_output_data->conj_ids);
- unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
- debug_ignore_startup_delay, NULL);
-
unsigned int ovs_cond_seqno = UINT_MAX;
unsigned int ovnsb_cond_seqno = UINT_MAX;
unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
@@ -3884,6 +3886,7 @@ 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();
@@ -4062,10 +4065,6 @@ main(int argc, char *argv[])
}
stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
time_msec());
- if (engine_has_updated()) {
- daemon_started_recently_countdown();
- }
-
ct_zones_data = engine_get_data(&en_ct_zones);
if (ovs_idl_txn) {
if (ct_zones_data) {
@@ -4228,7 +4227,7 @@ main(int argc, char *argv[])
}
store_nb_cfg(ovnsb_idl_txn, ovs_idl_txn, chassis_private,
- br_int, delay_nb_cfg_report);
+ br_int, delay_nb_cfg_report, startup_ts);
if (pending_pkt.conn) {
struct ed_type_addr_sets *as_data =
@@ -4705,11 +4704,3 @@ debug_dump_lflow_conj_ids(struct unixctl_conn *conn, int
argc OVS_UNUSED,
unixctl_command_reply(conn, ds_cstr(&conj_ids_dump));
ds_destroy(&conj_ids_dump);
}
-
-static void
-debug_ignore_startup_delay(struct unixctl_conn *conn, int argc OVS_UNUSED,
- const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED)
-{
- daemon_started_recently_ignore();
- unixctl_command_reply(conn, NULL);
-}
diff --git a/controller/patch.c b/controller/patch.c
index 12e0b6f7c..ed831302c 100644
--- a/controller/patch.c
+++ b/controller/patch.c
@@ -311,14 +311,7 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
SHASH_FOR_EACH_SAFE (port_node, &existing_ports) {
port = port_node->data;
shash_delete(&existing_ports, port_node);
- /* 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. */
- if (!daemon_started_recently()) {
- remove_port(bridge_table, port);
- }
+ remove_port(bridge_table, port);
}
shash_destroy(&existing_ports);
}
diff --git a/controller/vif-plug.c b/controller/vif-plug.c
index 38348bf54..eafc513f5 100644
--- a/controller/vif-plug.c
+++ b/controller/vif-plug.c
@@ -469,8 +469,7 @@ vif_plug_iface_touched_this_txn(
static bool
vif_plug_handle_lport_vif(const struct sbrec_port_binding *pb,
struct vif_plug_ctx_in *vif_plug_ctx_in,
- struct vif_plug_ctx_out *vif_plug_ctx_out,
- bool can_unplug)
+ struct vif_plug_ctx_out *vif_plug_ctx_out)
{
if (vif_plug_iface_touched_this_txn(vif_plug_ctx_out, pb->logical_port)) {
return true;
@@ -482,7 +481,7 @@ vif_plug_handle_lport_vif(const struct sbrec_port_binding
*pb,
if (lport_can_bind_on_this_chassis(vif_plug_ctx_in->chassis_rec, pb)) {
handled &= consider_plug_lport(pb, lbinding,
vif_plug_ctx_in, vif_plug_ctx_out);
- } else if (can_unplug && lbinding && lbinding->iface) {
+ } else if (lbinding && lbinding->iface) {
handled &= consider_unplug_iface(lbinding->iface, pb,
vif_plug_ctx_in, vif_plug_ctx_out);
}
@@ -492,8 +491,7 @@ vif_plug_handle_lport_vif(const struct sbrec_port_binding
*pb,
static bool
vif_plug_handle_iface(const struct ovsrec_interface *iface_rec,
struct vif_plug_ctx_in *vif_plug_ctx_in,
- struct vif_plug_ctx_out *vif_plug_ctx_out,
- bool can_unplug)
+ struct vif_plug_ctx_out *vif_plug_ctx_out)
{
bool handled = true;
const char *vif_plug_type = smap_get(&iface_rec->external_ids,
@@ -513,10 +511,8 @@ vif_plug_handle_iface(const struct ovsrec_interface
*iface_rec,
* consider updating it */
handled &= consider_plug_lport(pb, lbinding,
vif_plug_ctx_in, vif_plug_ctx_out);
- } else if (can_unplug
- && (!pb
- || !lport_can_bind_on_this_chassis(
- vif_plug_ctx_in->chassis_rec, pb))) {
+ } else if (!pb || !lport_can_bind_on_this_chassis(
+ vif_plug_ctx_in->chassis_rec, pb)) {
/* No lport for this interface or it is destined for different chassis,
* consuder unplugging it */
handled &= consider_unplug_iface(iface_rec, pb,
@@ -525,31 +521,17 @@ vif_plug_handle_iface(const struct ovsrec_interface
*iface_rec,
return handled;
}
-/* On initial startup or on IDL reconnect, several rounds of the main loop may
- * run before data is actually loaded in the IDL, primarily depending on
- * conditional monitoring status and other events that could trigger main loop
- * runs during this period. Until we find a reliable way to determine the
- * 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.
- */
void
vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
struct vif_plug_ctx_out *vif_plug_ctx_out)
{
- bool delay_plug = daemon_started_recently();
- if (delay_plug) {
- VLOG_DBG("vif_plug_run: daemon started recently, will not unplug "
- "ports in this iteration.");
- }
-
if (!vif_plug_ctx_in->chassis_rec) {
return;
}
const struct ovsrec_interface *iface_rec;
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,
- !delay_plug);
+ vif_plug_handle_iface(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out);
}
struct sbrec_port_binding *target =
@@ -564,8 +546,7 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
vif_plug_ctx_in->sbrec_port_binding_by_requested_chassis) {
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,
- !delay_plug);
+ vif_plug_handle_lport_vif(pb, vif_plug_ctx_in, vif_plug_ctx_out);
}
}
sbrec_port_binding_index_destroy_row(target);
diff --git a/controller/vif-plug.h b/controller/vif-plug.h
index 7a1978e38..76063591b 100644
--- a/controller/vif-plug.h
+++ b/controller/vif-plug.h
@@ -71,6 +71,7 @@ void vif_plug_clear_changed(struct shash *deleted_iface_ids);
void vif_plug_finish_changed(struct shash *changed_iface_ids);
void vif_plug_clear_deleted(struct shash *deleted_iface_ids);
void vif_plug_finish_deleted(struct shash *changed_iface_ids);
+void vif_plug_reset_idl_prime_counter(void);
#ifdef __cplusplus
}
diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
index 2e2b31033..575b774ae 100644
--- a/lib/inc-proc-eng.c
+++ b/lib/inc-proc-eng.c
@@ -312,17 +312,6 @@ engine_has_run(void)
return false;
}
-bool
-engine_has_updated(void)
-{
- for (size_t i = 0; i < engine_n_nodes; i++) {
- if (engine_nodes[i]->state == EN_UPDATED) {
- return true;
- }
- }
- return false;
-}
-
bool
engine_aborted(void)
{
diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
index dc365dc18..9bfab1f7c 100644
--- a/lib/inc-proc-eng.h
+++ b/lib/inc-proc-eng.h
@@ -238,10 +238,6 @@ bool engine_node_changed(struct engine_node *node);
/* Return true if the engine has run in the last iteration. */
bool engine_has_run(void);
-/* Return true if the engine has any update in any node, i.e. any input
- * has changed; false if nothing has changed. */
-bool engine_has_updated(void);
-
/* Returns true if during the last engine run we had to abort processing. */
bool engine_aborted(void);
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index d80db179a..616999eab 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -883,55 +883,3 @@ get_bridge(const struct ovsrec_bridge_table *bridge_table,
const char *br_name)
}
return NULL;
}
-
-#define DAEMON_STARTUP_DELAY_SEED 20
-#define DAEMON_STARTUP_DELAY_MS 10000
-
-static int64_t startup_ts;
-static int startup_delay = DAEMON_STARTUP_DELAY_SEED;
-
-/* Used by debug command only, for tests. */
-static bool ignore_startup_delay = false;
-
-OVS_CONSTRUCTOR(startup_ts_initializer) {
- startup_ts = time_wall_msec();
-}
-
-int64_t
-daemon_startup_ts(void)
-{
- return startup_ts;
-}
-
-void
-daemon_started_recently_countdown(void)
-{
- if (startup_delay > 0) {
- startup_delay--;
- }
-}
-
-void
-daemon_started_recently_ignore(void)
-{
- ignore_startup_delay = true;
-}
-
-bool
-daemon_started_recently(void)
-{
- if (ignore_startup_delay) {
- return false;
- }
-
- VLOG_DBG("startup_delay: %d, startup_ts: %"PRId64, startup_delay,
- startup_ts);
-
- /* Ensure that at least an amount of updates has been handled. */
- if (startup_delay) {
- return true;
- }
-
- /* Ensure that at least an amount of time has passed. */
- return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS;
-}
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index 145f974ed..b3905ef7b 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -309,9 +309,5 @@ struct ovsrec_bridge_table;
const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *,
const char *br_name);
-void daemon_started_recently_countdown(void);
-void daemon_started_recently_ignore(void);
-bool daemon_started_recently(void);
-int64_t daemon_startup_ts(void);
#endif /* OVN_UTIL_H */
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 3c3fb31c7..2d765f31a 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -114,7 +114,6 @@ check_patches \
'br-eth0 patch-localnet2-to-br-int patch-br-int-to-localnet2'
# Delete the mapping and the ovn-bridge-mapping patch ports should go away.
-check ovn-appctl -t ovn-controller debug/ignore-startup-delay
AT_CHECK([ovs-vsctl remove Open_vSwitch . external-ids ovn-bridge-mappings])
check_bridge_mappings
check_patches
@@ -2268,6 +2267,7 @@ OVS_WAIT_UNTIL([test 6 = $(as hv1 ovs-ofctl dump-flows
br-int | grep -c $(port_b
OVN_CLEANUP([hv1])
AT_CLEANUP
+OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn-controller - restart should not delete patch ports])
ovn_start
@@ -2337,3 +2337,4 @@ done
AT_CHECK([grep "deleted interface patch" hv1/ovs-vswitchd.log], [1], [ignore])
OVN_CLEANUP([hv1])
AT_CLEANUP
+])
diff --git a/tests/ovn.at b/tests/ovn.at
index bba2c9c1d..21a99089f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -31681,7 +31681,6 @@ OVS_WAIT_UNTIL([
test x43 = x$(as hv1 ovs-vsctl get Interface ${iface1_uuid} mtu_request)
])
-as hv1 check ovn-appctl -t ovn-controller debug/ignore-startup-delay
# Check that pointing requested-chassis somewhere else will unplug the port
check ovn-nbctl --wait=hv set Logical_Switch_Port lsp1 \
options:requested-chassis=non-existent-chassis
@@ -31689,7 +31688,6 @@ OVS_WAIT_UNTIL([
! as hv1 ovs-vsctl get Interface ${iface1_uuid} _uuid
])
-as hv2 check ovn-appctl -t ovn-controller debug/ignore-startup-delay
# Check that removing an lport will unplug it
AT_CHECK([test x${iface2_uuid} = x$(as hv2 ovs-vsctl get Interface
${iface2_uuid} _uuid)], [0], [])
check ovn-nbctl --wait=hv lsp-del ${lsp2_uuid}
--
2.34.3
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
