Until now, ovn-controller has been full of loops that commit a transaction
to the OVS or OVN Southbound database. These blocking loops delay other
work within ovn-controller. They also make it unsafe to keep pointers to
database records within a single ovn-controller main loop, since calls
to ovsdb_idl_run() can cause IDL records to be destroyed. This commit
drops all of the blocking calls, instead doing a single commit to the
databases at the end of each main loop.
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
ovn/controller/binding.c | 61 ++++++++------------
ovn/controller/binding.h | 4 +-
ovn/controller/chassis.c | 117 ++++++++++++++------------------------
ovn/controller/chassis.h | 4 +-
ovn/controller/ovn-controller.c | 123 +++++++++++++++++++++++++++++++++++++---
ovn/controller/ovn-controller.h | 4 ++
6 files changed, 188 insertions(+), 125 deletions(-)
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 0a4a39e..6af216c 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -76,10 +76,12 @@ binding_run(struct controller_ctx *ctx)
{
const struct sbrec_chassis *chassis_rec;
const struct sbrec_binding *binding_rec;
- struct ovsdb_idl_txn *txn;
struct sset lports, all_lports;
const char *name;
- int retval;
+
+ if (!ctx->ovnsb_idl_txn) {
+ return;
+ }
chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
if (!chassis_rec) {
@@ -91,8 +93,7 @@ binding_run(struct controller_ctx *ctx)
get_local_iface_ids(ctx, &lports);
sset_clone(&all_lports, &lports);
- txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
- ovsdb_idl_txn_add_comment(txn,
+ ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
"ovn-controller: updating bindings for '%s'",
ctx->chassis_id);
@@ -115,14 +116,6 @@ binding_run(struct controller_ctx *ctx)
}
}
- retval = ovsdb_idl_txn_commit_block(txn);
- if (retval == TXN_ERROR) {
- VLOG_INFO("Problem committing binding information: %s",
- ovsdb_idl_txn_status_to_string(retval));
- }
-
- ovsdb_idl_txn_destroy(txn);
-
SSET_FOR_EACH (name, &lports) {
VLOG_DBG("No binding record for lport %s", name);
}
@@ -130,40 +123,32 @@ binding_run(struct controller_ctx *ctx)
sset_destroy(&all_lports);
}
-void
-binding_destroy(struct controller_ctx *ctx)
+/* Returns true if the database is all cleaned up, false if more work is
+ * required. */
+bool
+binding_cleanup(struct controller_ctx *ctx)
{
- const struct sbrec_chassis *chassis_rec;
- int retval = TXN_TRY_AGAIN;
-
- ovs_assert(ctx->ovnsb_idl);
+ if (!ctx->ovnsb_idl_txn) {
+ return false;
+ }
- chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
+ const struct sbrec_chassis *chassis_rec
+ = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
if (!chassis_rec) {
- return;
+ return true;
}
- while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- const struct sbrec_binding *binding_rec;
- struct ovsdb_idl_txn *txn;
-
- txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
- ovsdb_idl_txn_add_comment(txn,
+ ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
"ovn-controller: removing all bindings for '%s'",
ctx->chassis_id);
- SBREC_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
- if (binding_rec->chassis == chassis_rec) {
- sbrec_binding_set_chassis(binding_rec, NULL);
- }
- }
-
- retval = ovsdb_idl_txn_commit_block(txn);
- if (retval == TXN_ERROR) {
- VLOG_INFO("Problem removing bindings: %s",
- ovsdb_idl_txn_status_to_string(retval));
+ const struct sbrec_binding *binding_rec;
+ bool any_changes = false;
+ SBREC_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
+ if (binding_rec->chassis == chassis_rec) {
+ sbrec_binding_set_chassis(binding_rec, NULL);
+ any_changes = true;
}
-
- ovsdb_idl_txn_destroy(txn);
}
+ return !any_changes;
}
diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
index 2611173..e73c1d1 100644
--- a/ovn/controller/binding.h
+++ b/ovn/controller/binding.h
@@ -17,10 +17,12 @@
#ifndef OVN_BINDING_H
#define OVN_BINDING_H 1
+#include <stdbool.h>
+
struct controller_ctx;
void binding_init(struct controller_ctx *);
void binding_run(struct controller_ctx *);
-void binding_destroy(struct controller_ctx *);
+bool binding_cleanup(struct controller_ctx *);
#endif /* ovn/binding.h */
diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
index cf18dd0..9d2959a 100644
--- a/ovn/controller/chassis.c
+++ b/ovn/controller/chassis.c
@@ -52,8 +52,6 @@ register_chassis(struct controller_ctx *ctx)
const char *encap_type, *encap_ip;
struct sbrec_encap *encap_rec;
static bool inited = false;
- int retval = TXN_TRY_AGAIN;
- struct ovsdb_idl_txn *txn;
chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
@@ -92,31 +90,22 @@ register_chassis(struct controller_ctx *ctx)
}
}
- txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
- ovsdb_idl_txn_add_comment(txn,
+ ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
"ovn-controller: registering chassis '%s'",
ctx->chassis_id);
if (!chassis_rec) {
- chassis_rec = sbrec_chassis_insert(txn);
+ chassis_rec = sbrec_chassis_insert(ctx->ovnsb_idl_txn);
sbrec_chassis_set_name(chassis_rec, ctx->chassis_id);
}
- encap_rec = sbrec_encap_insert(txn);
+ encap_rec = sbrec_encap_insert(ctx->ovnsb_idl_txn);
sbrec_encap_set_type(encap_rec, encap_type);
sbrec_encap_set_ip(encap_rec, encap_ip);
sbrec_chassis_set_encaps(chassis_rec, &encap_rec, 1);
- retval = ovsdb_idl_txn_commit_block(txn);
- if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- VLOG_INFO("Problem registering chassis: %s",
- ovsdb_idl_txn_status_to_string(retval));
- poll_immediate_wake();
- }
- ovsdb_idl_txn_destroy(txn);
-
inited = true;
}
@@ -311,7 +300,6 @@ update_encaps(struct controller_ctx *ctx)
{
const struct sbrec_chassis *chassis_rec;
const struct ovsrec_bridge *br;
- int retval;
struct tunnel_ctx tc = {
.tunnel_hmap = HMAP_INITIALIZER(&tc.tunnel_hmap),
@@ -319,7 +307,7 @@ update_encaps(struct controller_ctx *ctx)
.br_int = ctx->br_int
};
- tc.ovs_txn = ovsdb_idl_txn_create(ctx->ovs_idl);
+ tc.ovs_txn = ctx->ovs_idl_txn;
ovsdb_idl_txn_add_comment(tc.ovs_txn,
"ovn-controller: modifying OVS tunnels '%s'",
ctx->chassis_id);
@@ -366,81 +354,58 @@ update_encaps(struct controller_ctx *ctx)
}
hmap_destroy(&tc.tunnel_hmap);
sset_destroy(&tc.port_names);
-
- retval = ovsdb_idl_txn_commit_block(tc.ovs_txn);
- if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- VLOG_INFO("Problem modifying OVS tunnels: %s",
- ovsdb_idl_txn_status_to_string(retval));
- poll_immediate_wake();
- }
- ovsdb_idl_txn_destroy(tc.ovs_txn);
}
void
chassis_run(struct controller_ctx *ctx)
{
- register_chassis(ctx);
- update_encaps(ctx);
+ if (ctx->ovnsb_idl_txn) {
+ register_chassis(ctx);
+ }
+
+ if (ctx->ovs_idl_txn) {
+ update_encaps(ctx);
+ }
}
-void
-chassis_destroy(struct controller_ctx *ctx)
+/* Returns true if the database is all cleaned up, false if more work is
+ * required. */
+bool
+chassis_cleanup(struct controller_ctx *ctx)
{
- int retval = TXN_TRY_AGAIN;
-
- ovs_assert(ctx->ovnsb_idl);
-
- while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- const struct sbrec_chassis *chassis_rec;
- struct ovsdb_idl_txn *txn;
+ if (!ctx->ovnsb_idl_txn || !ctx->ovs_idl_txn) {
+ return false;
+ }
- chassis_rec = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
- if (!chassis_rec) {
- break;
- }
+ bool any_changes = false;
- txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
- ovsdb_idl_txn_add_comment(txn,
+ /* Delete Chassis row. */
+ const struct sbrec_chassis *chassis_rec
+ = get_chassis_by_name(ctx->ovnsb_idl, ctx->chassis_id);
+ if (chassis_rec) {
+ ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
"ovn-controller: unregistering chassis '%s'",
ctx->chassis_id);
sbrec_chassis_delete(chassis_rec);
-
- retval = ovsdb_idl_txn_commit_block(txn);
- if (retval == TXN_ERROR) {
- VLOG_INFO("Problem unregistering chassis: %s",
- ovsdb_idl_txn_status_to_string(retval));
- }
- ovsdb_idl_txn_destroy(txn);
+ any_changes = true;
}
- retval = TXN_TRY_AGAIN;
- while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- struct ovsrec_port **ports;
- struct ovsdb_idl_txn *txn;
- size_t i, n;
-
- txn = ovsdb_idl_txn_create(ctx->ovs_idl);
- ovsdb_idl_txn_add_comment(txn,
- "ovn-controller: destroying tunnels");
-
- /* Delete all the OVS-created tunnels from the integration
- * bridge. */
- ports = xmalloc(sizeof *ctx->br_int->ports * ctx->br_int->n_ports);
- for (i = n = 0; i < ctx->br_int->n_ports; i++) {
- if (!smap_get(&ctx->br_int->ports[i]->external_ids,
- "ovn-chassis-id")) {
- ports[n++] = ctx->br_int->ports[i];
- }
- }
- ovsrec_bridge_verify_ports(ctx->br_int);
- ovsrec_bridge_set_ports(ctx->br_int, ports, n);
- free(ports);
-
- retval = ovsdb_idl_txn_commit_block(txn);
- if (retval == TXN_ERROR) {
- VLOG_INFO("Problem destroying tunnels: %s",
- ovsdb_idl_txn_status_to_string(retval));
+ /* Delete all the OVS-created tunnels from the integration bridge. */
+ ovsdb_idl_txn_add_comment(ctx->ovs_idl_txn,
+ "ovn-controller: destroying tunnels");
+ struct ovsrec_port **ports
+ = xmalloc(sizeof *ctx->br_int->ports * ctx->br_int->n_ports);
+ size_t n = 0;
+ for (size_t i = 0; i < ctx->br_int->n_ports; i++) {
+ if (!smap_get(&ctx->br_int->ports[i]->external_ids,
+ "ovn-chassis-id")) {
+ ports[n++] = ctx->br_int->ports[i];
+ any_changes = true;
}
- ovsdb_idl_txn_destroy(txn);
}
+ ovsrec_bridge_verify_ports(ctx->br_int);
+ ovsrec_bridge_set_ports(ctx->br_int, ports, n);
+ free(ports);
+
+ return !any_changes;
}
diff --git a/ovn/controller/chassis.h b/ovn/controller/chassis.h
index aee701b..851a300 100644
--- a/ovn/controller/chassis.h
+++ b/ovn/controller/chassis.h
@@ -16,10 +16,12 @@
#ifndef OVN_CHASSIS_H
#define OVN_CHASSIS_H 1
+#include <stdbool.h>
+
struct controller_ctx;
void chassis_init(struct controller_ctx *);
void chassis_run(struct controller_ctx *);
-void chassis_destroy(struct controller_ctx *);
+bool chassis_cleanup(struct controller_ctx *);
#endif /* ovn/chassis.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index cccea24..7b165a1 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -145,6 +145,78 @@ try_again:
}
+struct idl_loop {
+ struct ovsdb_idl *idl;
+ unsigned int skip_seqno;
+
+ struct ovsdb_idl_txn *committing_txn;
+ unsigned int precommit_seqno;
+
+ struct ovsdb_idl_txn *open_txn;
+};
+
+#define IDL_LOOP_INITIALIZER(IDL) { .idl = (IDL) }
+
+static void
+idl_loop_destroy(struct idl_loop *loop)
+{
+ if (loop) {
+ ovsdb_idl_destroy(loop->idl);
+ }
+}
+
+static struct ovsdb_idl_txn *
+idl_loop_run(struct idl_loop *loop)
+{
+ ovsdb_idl_run(loop->idl);
+ loop->open_txn = (loop->committing_txn
+ || ovsdb_idl_get_seqno(loop->idl) == loop->skip_seqno
+ ? NULL
+ : ovsdb_idl_txn_create(loop->idl));
+ return loop->open_txn;
+}
+
+static void
+idl_loop_wait(struct idl_loop *loop)
+{
+ if (loop->open_txn) {
+ loop->committing_txn = loop->open_txn;
+ loop->open_txn = NULL;
+
+ loop->precommit_seqno = ovsdb_idl_get_seqno(loop->idl);
+ }
+
+ struct ovsdb_idl_txn *txn = loop->committing_txn;
+ if (txn) {
+ enum ovsdb_idl_txn_status status = ovsdb_idl_txn_commit(txn);
+ switch (status) {
+ case TXN_INCOMPLETE:
+ break;
+
+ case TXN_TRY_AGAIN:
+ loop->skip_seqno = loop->precommit_seqno;
+ if (ovsdb_idl_get_seqno(loop->idl) != loop->skip_seqno) {
+ poll_immediate_wake();
+ }
+ /* Fall through. */
+ case TXN_UNCHANGED:
+ case TXN_ABORTED:
+ case TXN_SUCCESS:
+ case TXN_NOT_LOCKED:
+ case TXN_ERROR:
+ ovsdb_idl_txn_destroy(txn);
+ loop->committing_txn = NULL;
+ break;
+
+ case TXN_UNCOMMITTED:
+ OVS_NOT_REACHED();
+
+ }
+ }
+
+ ovsdb_idl_wait(loop->idl);
+}
+
int
main(int argc, char *argv[])
{
@@ -195,10 +267,14 @@ main(int argc, char *argv[])
true, true);
get_initial_snapshot(ctx.ovnsb_idl);
+ struct idl_loop ovnsb_idl_loop = IDL_LOOP_INITIALIZER(ctx.ovnsb_idl);
+ struct idl_loop ovs_idl_loop = IDL_LOOP_INITIALIZER(ctx.ovs_idl);
+
+ /* Main loop. */
exiting = false;
while (!exiting) {
- ovsdb_idl_run(ctx.ovs_idl);
- ovsdb_idl_run(ctx.ovnsb_idl);
+ ctx.ovnsb_idl_txn = idl_loop_run(&ovnsb_idl_loop);
+ ctx.ovs_idl_txn = idl_loop_run(&ovs_idl_loop);
/* xxx If run into any surprising changes, we exit. We should
* xxx handle this more gracefully. */
@@ -207,7 +283,7 @@ main(int argc, char *argv[])
VLOG_ERR("Integration bridge '%s' disappeared",
ctx.br_int_name);
retval = EXIT_FAILURE;
- break;
+ goto exit;
}
ofctrl_clear_flows();
@@ -224,20 +300,49 @@ main(int argc, char *argv[])
poll_immediate_wake();
}
- ovsdb_idl_wait(ctx.ovs_idl);
- ovsdb_idl_wait(ctx.ovnsb_idl);
+ idl_loop_wait(&ovnsb_idl_loop);
+ idl_loop_wait(&ovs_idl_loop);
+
ofctrl_wait();
poll_block();
}
+ /* It's time to exit. Clean up the databases. */
+ bool done = false;
+ while (!done) {
+ ctx.ovnsb_idl_txn = idl_loop_run(&ovnsb_idl_loop);
+ ctx.ovs_idl_txn = idl_loop_run(&ovs_idl_loop);
+
+ /* xxx If run into any surprising changes, we exit. We should
+ * xxx handle this more gracefully. */
+ ctx.br_int = get_bridge(&ctx, ctx.br_int_name);
+ if (!ctx.br_int) {
+ VLOG_ERR("Integration bridge '%s' disappeared",
+ ctx.br_int_name);
+ retval = EXIT_FAILURE;
+ goto exit;
+ }
+
+ /* Run both the binding and chassis cleanup, even if one of them
+ * returns false. We're done if both return true. */
+ done = binding_cleanup(&ctx);
+ done = chassis_cleanup(&ctx) && done;
+ if (done) {
+ poll_immediate_wake();
+ }
+
+ idl_loop_wait(&ovnsb_idl_loop);
+ idl_loop_wait(&ovs_idl_loop);
+ poll_block();
+ }
+
+exit:
unixctl_server_destroy(unixctl);
pipeline_destroy(&ctx);
ofctrl_destroy();
- binding_destroy(&ctx);
- chassis_destroy(&ctx);
- ovsdb_idl_destroy(ctx.ovs_idl);
- ovsdb_idl_destroy(ctx.ovnsb_idl);
+ idl_loop_destroy(&ovs_idl_loop);
+ idl_loop_destroy(&ovnsb_idl_loop);
free(ctx.br_int_name);
free(ctx.chassis_id);
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index 6f98658..b72d891 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -22,8 +22,12 @@
struct controller_ctx {
char *chassis_id; /* ID for this chassis. */
char *br_int_name; /* Name of local integration bridge. */
+
struct ovsdb_idl *ovnsb_idl;
+ struct ovsdb_idl_txn *ovnsb_idl_txn;
+
struct ovsdb_idl *ovs_idl;
+ struct ovsdb_idl_txn *ovs_idl_txn;
const struct ovsrec_bridge *br_int;
};
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
