[ovs-dev] [ovn v2] ovn: Introduce ovn-controller.
Justin Pettit
jpettit at nicira.com
Sat Apr 11 06:59:48 UTC 2015
> On Apr 10, 2015, at 6:59 AM, Russell Bryant <rbryant at redhat.com> wrote:
>
> On 04/09/2015 03:53 AM, Justin Pettit wrote:
>> Add new ovn-controller daemon that runs locally on transport nodes.
>> This initial version registers itself in the Chassis table and registers
>> logical ports to the appropriate rows in the Bindings table.
>>
>> Signed-off-by: Justin Pettit <jpettit at nicira.com>
>> ---
>> v1->v2: Feedback from Ben.
>
> Very nice work. Let me know what you think about the comments. If
> there are parts you'd like to defer, I'd be happy to help if you'd like.
Thanks!
>> +EXTRA_DIST += ovn/controller/ovn-controller.8.xml
>
> I like the use of the controller subdir. I guess we should create
> similar dirs for ovn-nbctl and ovn-nbd, even though they're both just 1
> C file for now. I can do it, perhaps after my pending patches against
> those files go in.
Yeah, I was thinking we should especially do that for programs that require multiple source files. ovn-nbd seems likely to need that. I wonder if we should create a "utilities" directory for things like ovn-nbctl that only use a single file. I imagine we'll start getting other utilities, too.
>
>> +static void
>> +get_local_iface_ids(struct controller_ctx *ctx, struct sset *lports)
>> +{
>> + const struct ovsrec_open_vswitch *cfg;
>> + const struct ovsrec_bridge *bridge_rec;
>> + const char *bridge_name;
>> + int i;
>> +
>> + cfg = ovsrec_open_vswitch_first(ctx->ovs_idl);
>
> In chassis.c, there's a check here for cfg == NULL. Do you want to add
> the same here?
Good point for consistency. They shouldn't ever hit that case, since ovn-controller will bail out during startup if it doesn't exist, but better to be robust in case someone erases the row while it's running.
>> + bridge_name = smap_get(&cfg->external_ids, "ovn-bridge");
>> + if (!bridge_name) {
>> + bridge_name = "br-int";
>
> It might be helpful to define this as a constant at the top of the file,
> something like:
>
> static const char DEFAULT_BRIDGE_NAME = "br-int";
>
> or whatever the general convention used around ovs code is for constants.
Good idea.
>> + for (i = 0; i < bridge_rec->n_ports; i++) {
>> + const struct ovsrec_port *port_rec = bridge_rec->ports[i];
>> + const char *iface_id;
>> + int j;
>> +
>> + if (!strcmp(port_rec->name, bridge_rec->name)) {
>> + continue;
>> + }
>
> I don't think I understand the above condition. It's making the loop
> ignore all ports that aren't named the same as the bridge?
>
> It looks like the condition should just be removed, but maybe I just
> misunderstand it.
It should only be matching on ports that have the same name as the bridge. In OVS, we always create a port with the same name as the bridge, but it would not be part of the logical network, so the code just skips it.
>> +void
>> +bindings_run(struct controller_ctx *ctx)
>> +{
>> + const struct sbrec_bindings *bindings_rec;
>> + struct ovsdb_idl_txn *txn;
>> + struct sset lports;
>> + const char *name;
>> + int retval;
>> +
>> + sset_init(&lports);
>> + get_local_iface_ids(ctx, &lports);
>> +
>> + txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
>> + ovsdb_idl_txn_add_comment(txn,
>> + "ovn-controller: updating bindings for '%s'",
>> + ctx->chassis_name);
>
> This will probably end up with a lot of extra comments in ovsdb. This
> code is going to run a lot more often than when changes will actually be
> put in a transaction.
Most of the transactions should have no effect on the database, which will return TXN_UNCHANGED. The comment is associated with a transaction, so if the transaction gets dropped, there should be no entry in the database.
>> +
>> + SBREC_BINDINGS_FOR_EACH(bindings_rec, ctx->ovnsb_idl) {
>> + if (sset_contains(&lports, bindings_rec->logical_port)) {
>> + sset_find_and_delete(&lports, bindings_rec->logical_port);
>
> I think you can slightly optimize the above 2 lines by making it:
>
> if (sset_find_and_delete(&lports, bindings_rec->logical_port)) {
Excellent!
>> +
>> + if (!strcmp(bindings_rec->chassis, ctx->chassis_name)) {
>> + continue;
>> + }
>> + if (bindings_rec->chassis[0]) {
>> + VLOG_INFO("Changing chassis for lport %s from %s to %s",
>> + bindings_rec->logical_port, bindings_rec->chassis,
>> + ctx->chassis_name);
>> + }
>> + sbrec_bindings_set_chassis(bindings_rec, ctx->chassis_name);
>> + } else if (!strcmp(bindings_rec->chassis, ctx->chassis_name)) {
>> + sbrec_bindings_set_chassis(bindings_rec, "");
>> + }
>> + }
>> +
>> + retval = ovsdb_idl_txn_commit_block(txn);
>> + if (retval == TXN_ERROR) {
>> + VLOG_INFO("Problem committing bindings information: %s",
>> + ovsdb_idl_txn_status_to_string(retval));
>> + }
>
> It's worth tracking if a change was actually put into the transaction in
> the loop above and skip trying to commit if no changes were needed.
>
> Also see the comments regarding transaction committing and error
> handling that I put in chassis_run in chassis.c, as all of it applies
> here too.
As I mentioned above, I don't think a transaction that has no effect on the database should actually be committed, so I don't think it's a problem. However, let me know if I misunderstand what you're suggesting.
>> + ovsdb_idl_txn_destroy(txn);
>> +
>> + SSET_FOR_EACH (name, &lports) {
>> + VLOG_DBG("No binding record for lport %s", name);
>> + }
>> + sset_destroy(&lports);
>> +}
>> +
>> +void
>> +bindings_destroy(struct controller_ctx *ctx)
>> +{
>> + const struct sbrec_bindings *bindings_rec;
>> + struct ovsdb_idl_txn *txn;
>> + int retval = TXN_TRY_AGAIN;
>> +
>> + if (!ctx->ovnsb_idl) {
>> + return;
>> + }
>
> How about ovs_assert() here since we never expect this to happen?
Fair enough. Changed.
>> +void
>> +chassis_init(struct controller_ctx *ctx)
>> +{
>> + ovsdb_idl_add_table(ctx->ovs_idl, &ovsrec_table_open_vswitch);
>> + ovsdb_idl_add_column(ctx->ovs_idl, &ovsrec_open_vswitch_col_external_ids);
>
> This is already done in ovn-controller.c. Do you just want this not to
> assume the core code has added anything? I guess it's harmless and
> indeed helpful for this code to be explicit about what it cares about.
Yeah, I was just going for explicit. I just worry if someone changes how ovn-controller.c is written, we'll silently get incorrect behavior.
>> +}
>> +
>> +static void
>> +register_chassis(struct controller_ctx *ctx,
>> + const struct sbrec_chassis *chassis_rec,
>> + const char *encap_type, const char *encap_ip)
>> +{
>> + struct sbrec_encap *encap_rec;
>> + struct ovsdb_idl_txn *txn;
>> + int retval;
>> +
>> + txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
>> + ovsdb_idl_txn_add_comment(txn,
>> + "ovn-controller: registering chassis '%s'",
>> + ctx->chassis_name);
>> +
>> + if (!chassis_rec) {
>> + chassis_rec = sbrec_chassis_insert(txn);
>> + sbrec_chassis_set_name(chassis_rec, ctx->chassis_name);
>> + }
>> +
>> + encap_rec = sbrec_encap_insert(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 committing chassis information: %s",
>> + ovsdb_idl_txn_status_to_string(retval));
>> + }
>> + ovsdb_idl_txn_destroy(txn);
>
> Some thoughts on the transaction handling here:
>
> Another return value that isn't really an error is TXN_TRY_AGAIN. The
> docs for TRY_AGAIN say it happens if a "verify" operation fails, and
> none of those are being used here, so maybe it's never expected.
>
> There's also TXN_UNCOMMITTED, which sounds like txn_commit just needs to
> be called again.
>
> If it *does* fail, there's no attempt to make this try again right away.
> It's depending on the main loop to come back around after some changes
> have been detected. I suppose it's possible that could be a long delay
> we don't really want to wait for.
Yeah, I agree that it should be handled better. I've changed the logic so that it it will immediately wake if the commit doesn't fail. In that case, it just treat all the errors as a sign to try again.
> A bigger issue here to me though is that this is a blocking operation
> done outside of the main loop. I think it would be better to integrate
> committing transactions into the main loop. I tried to do this in ovn-nbd.
There's a trade-off here between creating too many transactions and too few. I was leaning towards more transactions, since they can be annotated through comments which can aid debugging. The argument against it is that it creates too many transactions that must be waded through and a lot of blocking. My thinking on this is changing towards creating fewer transactions, but my preference is to get it in as it is now, and then reassess later.
>> +}
>> +
>> +void
>> +chassis_run(struct controller_ctx *ctx)
>> +{
>> + const struct sbrec_chassis *chassis_rec;
>> + const struct ovsrec_open_vswitch *cfg;
>> + const char *encap_type, *encap_ip;
>> + static bool inited = false;
>> +
>> + SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->ovnsb_idl) {
>> + if (!strcmp(chassis_rec->name, ctx->chassis_name)) {
>> + break;
>> + }
>> + }
>
> I noted in some comments on ovn-controller.c that this code is actually
> getting called much more often than needed. It's going to do this
> chassis lookup every time OVN_Southbound changes (a lot), where we
> really only want to run this if the stuff we're watching in the
> Open_vSwitch db changes.
The IDL should prevent transactions that never commit from going to ovsdb-server, so this should essentially be a no-op in terms of taxing ovsdb-server. That said, it's obviously best to not needlessly waste unnecessary CPU cycles, but in this case, I'm planning to add a future update that creates the local tunnels based on new additions to the Chassis table.
>> + if (!ctx->ovnsb_idl) {
>> + return;
>> + }
>
> Maybe use ovs_assert() for this? We don't expect it to ever happen.
Sure.
>> +
>> + SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->ovnsb_idl) {
>> + if (!strcmp(chassis_rec->name, ctx->chassis_name)) {
>> + break;
>> + }
>> + }
>> +
>> + if (!chassis_rec) {
>> + return;
>> + }
>> +
>> + txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
>> + ovsdb_idl_txn_add_comment(txn,
>> + "ovn-controller: unregistering chassis '%s'",
>> + ctx->chassis_name);
>> + sbrec_chassis_delete(chassis_rec);
>> +
>> + retval = ovsdb_idl_txn_commit_block(txn);
>> + if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
>> + VLOG_INFO("Problem committing chassis information: %s",
>> + ovsdb_idl_txn_status_to_string(retval));
>> + }
>> + ovsdb_idl_txn_destroy(txn);
>
> Similar transaction handling comments apply here, but this is outside of
> the main loop and when ovn-controller is shutting down anyway so I'm not
> quite as worried about the blocking happening.
This shouldn't bail out just because the first commit failed, so I rewrote it to be more like bindings_destroy(), which keeps trying.
>> new file mode 100644
>> index 0000000..d172c08
>> --- /dev/null
>> +++ b/ovn/controller/ovn-controller.8.xml
>> @@ -0,0 +1,115 @@
>> +<?xml version="1.0" encoding="utf-8"?>
>> +<manpage program="ovn-controller" section="8" title="ovn-controller">
>> + <h1>Name</h1>
>> + <p>ovn-controller -- Open Virtual Network local controller</p>
>> +
>> + <h1>Synopsis</h1>
>> + <p><code>ovn-controller</code> [<var>options</var>] [<var>ovs-database</var>]</p>
>> +
>> + <h1>Description</h1>
>> + <p>
>> + <code>ovn-controller</code> is the local controller daemon for
>> + OVN, the Open Virtual Network. It connects northbound to the OVN
>> + database (see <code>ovn</code>(5)) over the OVSDB protocol, and
>
> OVN_Southbound database.
Whoops. I forgot to do a thorough OVN->OVN Southbound scrub on these patches.
>> +static char *ovs_remote;
>> +static char *ovn_remote;
>
> I suppose ovnsb_remote would be a little more consistent.
Thanks. Fixed.
>> +
>> +
>> +static void
>> +get_initial_snapshot(struct ovsdb_idl *idl)
>> +{
>> + while (1) {
>> + ovsdb_idl_run(idl);
>> + if (ovsdb_idl_get_seqno(idl)) {
>
> It looks like ovsdb_idl_has_ever_connected() would be a more explicit
> way to do this check.
Fair enough.
>> + /* xxx This does not support changing OVN OVSDB mid-run. */
>
> OVN_Southbound
Fixed.
>> + struct unixctl_server *unixctl;
>> + struct controller_ctx ctx;
>
> Instead of the memset() further below, I tend to prefer something like:
>
> struct controller_ctx ctx = {
> .chassis_name = NULL,
> };
I don't have a strong preference, so I went with your suggestion.
>> + /* Connect to OVS OVSDB instance. We do not monitor any tables or
>> + * columns be default, so modules must register their interest
>> + * explicitly. */
>
> Technically we register for external_ids in the ovs table by default,
> right? :-)
As I mentioned above, I don't think modules should assume that any particular table or column is registered. I agree it could cause confusion, though, so I softened the language.
>> + ctx.ovs_idl = ovsdb_idl_create(ovs_remote, &ovsrec_idl_class, false, true);
>> +
>> + /* Register interest in "external_ids" column in "Open_vSwitch" table,
>> + * since we'll need to get the OVN OVSDB remote. */
>> + ovsdb_idl_add_table(ctx.ovs_idl, &ovsrec_table_open_vswitch);
>> + ovsdb_idl_add_column(ctx.ovs_idl, &ovsrec_open_vswitch_col_external_ids);
>> +
>> + chassis_init(&ctx);
>> + bindings_init(&ctx);
>
> I think it would be nice to create the ovnsb_idl before calling the
> sub-module init functions. That just seems to match up better to having
> ovs_idl already created.
>
> However, it's really not that straight forward to do without rewriting
> the get_initial_snapshot() and get_core_config() functions to handle
> servicing both IDLs, so it can just be done later if the need actually
> arises.
Yeah, I see your point, but let's change that when it becomes an issue.
>> +
>> + get_initial_snapshot(ctx.ovs_idl);
>> +
>> + get_core_config(&ctx);
>> +
>> + ctx.ovnsb_idl = ovsdb_idl_create(ovn_remote, &sbrec_idl_class, true, true);
>> +
>> + get_initial_snapshot(ctx.ovnsb_idl);
>
> There's a slight downside here in that get_initial_snapshot() blocks and
> leaves ovs_idl unserviced in the meantime. It's just during the initial
> startup though, so I'm OK leaving it.
I don't understand the issue of leaving ovs_idl unserviced during startup.
>> +
>> + exiting = false;
>> + while (!exiting) {
>> + ovsdb_idl_run(ctx.ovs_idl);
>> + ovsdb_idl_run(ctx.ovnsb_idl);
>> +
>> + if (!ovsdb_idl_is_alive(ctx.ovnsb_idl)) {
>> + int retval = ovsdb_idl_get_last_error(ctx.ovnsb_idl);
>> + VLOG_ERR("%s: database connection failed (%s)",
>> + ovn_remote, ovs_retval_to_string(retval));
>> + retval = EXIT_FAILURE;
>> + break;
>> + }
>> +
>> + if (!ovsdb_idl_is_alive(ctx.ovs_idl)) {
>> + int retval = ovsdb_idl_get_last_error(ctx.ovs_idl);
>> + VLOG_ERR("%s: database connection failed (%s)",
>> + ovs_remote, ovs_retval_to_string(retval));
>> + retval = EXIT_FAILURE;
>> + break;
>> + }
>> +
>> + chassis_run(&ctx);
>> + bindings_run(&ctx);
>> + unixctl_server_run(unixctl);
>> +
>> + unixctl_server_wait(unixctl);
>> + if (exiting) {
>> + poll_immediate_wake();
>> + }
>> +
>> + ovsdb_idl_wait(ctx.ovs_idl);
>> + ovsdb_idl_wait(ctx.ovnsb_idl);
>> + poll_block();
>> + }
>
> It would be handy to track the seqno for each IDL so that we know which
> (if any) db has changes on each loop iteration. That could let
> foo_run() optimize out some processing.
>
> For example, chassis_run() only cares about changes to the ovs db, and
> we're only watching the minimal set of that db that we care about. The
> OVN_Southbound db is going to change a *LOT* more often.
I can see your point. but I don't think it's a huge problem, since chassis_run() will actually be monitoring all the Chassis tables in a near future patch to create all the various tunnels. Let me know if you still think it's an issue knowing that's going to happen, though.
>> +
>> + unixctl_server_destroy(unixctl);
>> + bindings_destroy(&ctx);
>> + chassis_destroy(&ctx);
>> +
>> + ovsdb_idl_destroy(ctx.ovs_idl);
>> + ovsdb_idl_destroy(ctx.ovnsb_idl);
>> +
>> + return retval;
>
> I'd change this to:
>
> exit(retval);
Good point.
>> + case 'V':
>> + ovs_print_version(OFP13_VERSION, OFP13_VERSION);
>
> I'm curious how you decided on specifying OFP13_VERSION in the version
> output here.
We wanted to use some of the features available in OpenFlow 1.3.
> I see ovs-vswitchd claims:
>
> ovs_print_version(OFP10_VERSION, OFP10_VERSION);
>
> but perhaps that is just out of date?
>
> and ovs-ofctl says OFP10 through OFP13 and provides an option to let you
> pick which version to use.
Yeah, I suspect that's out of date. We should fix that up.
>> + exit(EXIT_SUCCESS);
>> +
>> + VLOG_OPTION_HANDLERS
>> + DAEMON_OPTION_HANDLERS
>> + STREAM_SSL_OPTION_HANDLERS
>> +
>> + case OPT_PEER_CA_CERT:
>> + stream_ssl_set_peer_ca_cert_file(optarg);
>> + break;
>> +
>> + case OPT_ENABLE_DUMMY:
>> + timeval_dummy_register();
>> + break;
>
> It's not clear to my why --enable-dummy is needed for ovn-controller.
> Could it just be dropped?
>
> If not, should ovs-sandbox be specifying it?
I don't think we have any need for time in ovn-controller right now, so I'll remove it.
>> +
>> + case '?':
>> + exit(EXIT_FAILURE);
>
> I see that this exists in ovs-vsctl, as well. What's the reason for it?
>
> If '?' exists, I guess I'd expect it to call usage().
This is how getopt() indicates an unsupported option. It then prints an error to stderr.
>> +
>> + default:
>> + abort();
>
> Maybe log an error about an unuexpected option or something if this were
> to happen?
As mentioned, an unexpected option should be caught in getopt(), so this should be just an invalid condition.
I'll send out v3 in a minute. There's an incremental at the end of this message.
Thanks for the thorough review!
--Justin
-=-=-=-=-=-=-=-=-
diff --git a/ovn/controller/bindings.c b/ovn/controller/bindings.c
index 1beb8ed..bea4c38 100644
--- a/ovn/controller/bindings.c
+++ b/ovn/controller/bindings.c
@@ -17,6 +17,7 @@
#include "bindings.h"
#include "lib/sset.h"
+#include "lib/util.h"
#include "lib/vswitch-idl.h"
#include "openvswitch/vlog.h"
#include "ovn/ovn-sb-idl.h"
@@ -24,6 +25,8 @@
VLOG_DEFINE_THIS_MODULE(bindings);
+#define DEFAULT_BRIDGE_NAME "br-int"
+
void
bindings_init(struct controller_ctx *ctx)
{
@@ -52,10 +55,14 @@ get_local_iface_ids(struct controller_ctx *ctx, struct sset *lports)
int i;
cfg = ovsrec_open_vswitch_first(ctx->ovs_idl);
+ if (!cfg) {
+ VLOG_INFO("No Open_vSwitch row defined.");
+ return;
+ }
bridge_name = smap_get(&cfg->external_ids, "ovn-bridge");
if (!bridge_name) {
- bridge_name = "br-int";
+ bridge_name = DEFAULT_BRIDGE_NAME;
}
OVSREC_BRIDGE_FOR_EACH(bridge_rec, ctx->ovs_idl) {
@@ -110,9 +117,7 @@ bindings_run(struct controller_ctx *ctx)
ctx->chassis_name);
SBREC_BINDINGS_FOR_EACH(bindings_rec, ctx->ovnsb_idl) {
- if (sset_contains(&lports, bindings_rec->logical_port)) {
- sset_find_and_delete(&lports, bindings_rec->logical_port);
-
+ if (sset_find_and_delete(&lports, bindings_rec->logical_port)) {
if (!strcmp(bindings_rec->chassis, ctx->chassis_name)) {
continue;
}
@@ -144,15 +149,14 @@ bindings_run(struct controller_ctx *ctx)
void
bindings_destroy(struct controller_ctx *ctx)
{
- const struct sbrec_bindings *bindings_rec;
- struct ovsdb_idl_txn *txn;
int retval = TXN_TRY_AGAIN;
- if (!ctx->ovnsb_idl) {
- return;
- }
+ ovs_assert(ctx->ovnsb_idl);
while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
+ const struct sbrec_bindings *bindings_rec;
+ struct ovsdb_idl_txn *txn;
+
txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
ovsdb_idl_txn_add_comment(txn,
"ovn-controller: removing all bindings for '%s'",
@@ -166,7 +170,7 @@ bindings_destroy(struct controller_ctx *ctx)
retval = ovsdb_idl_txn_commit_block(txn);
if (retval == TXN_ERROR) {
- VLOG_INFO("Problem committing bindings information: %s",
+ VLOG_INFO("Problem removing bindings: %s",
ovsdb_idl_txn_status_to_string(retval));
}
diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
index 8fa2624..2d6e5e6 100644
--- a/ovn/controller/chassis.c
+++ b/ovn/controller/chassis.c
@@ -16,6 +16,8 @@
#include <config.h>
#include "chassis.h"
+#include "lib/poll-loop.h"
+#include "lib/util.h"
#include "lib/vswitch-idl.h"
#include "openvswitch/vlog.h"
#include "ovn/ovn-sb-idl.h"
@@ -36,8 +38,8 @@ register_chassis(struct controller_ctx *ctx,
const char *encap_type, const char *encap_ip)
{
struct sbrec_encap *encap_rec;
+ int retval = TXN_TRY_AGAIN;
struct ovsdb_idl_txn *txn;
- int retval;
txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
ovsdb_idl_txn_add_comment(txn,
@@ -58,8 +60,9 @@ register_chassis(struct controller_ctx *ctx,
retval = ovsdb_idl_txn_commit_block(txn);
if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- VLOG_INFO("Problem committing chassis information: %s",
+ VLOG_INFO("Problem registering chassis: %s",
ovsdb_idl_txn_status_to_string(retval));
+ poll_immediate_wake();
}
ovsdb_idl_txn_destroy(txn);
}
@@ -120,34 +123,35 @@ chassis_run(struct controller_ctx *ctx)
void
chassis_destroy(struct controller_ctx *ctx)
{
- const struct sbrec_chassis *chassis_rec;
- struct ovsdb_idl_txn *txn;
- int retval;
+ int retval = TXN_TRY_AGAIN;
- if (!ctx->ovnsb_idl) {
- return;
- }
+ ovs_assert(ctx->ovnsb_idl);
- SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->ovnsb_idl) {
- if (!strcmp(chassis_rec->name, ctx->chassis_name)) {
- break;
+ while (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
+ const struct sbrec_chassis *chassis_rec;
+ struct ovsdb_idl_txn *txn;
+
+ SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->ovnsb_idl) {
+ if (!strcmp(chassis_rec->name, ctx->chassis_name)) {
+ break;
+ }
}
- }
- if (!chassis_rec) {
- return;
- }
+ if (!chassis_rec) {
+ return;
+ }
- txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
- ovsdb_idl_txn_add_comment(txn,
- "ovn-controller: unregistering chassis '%s'",
- ctx->chassis_name);
- sbrec_chassis_delete(chassis_rec);
+ txn = ovsdb_idl_txn_create(ctx->ovnsb_idl);
+ ovsdb_idl_txn_add_comment(txn,
+ "ovn-controller: unregistering chassis '%s'",
+ ctx->chassis_name);
+ sbrec_chassis_delete(chassis_rec);
- retval = ovsdb_idl_txn_commit_block(txn);
- if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {
- VLOG_INFO("Problem committing chassis information: %s",
- ovsdb_idl_txn_status_to_string(retval));
+ 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);
}
- ovsdb_idl_txn_destroy(txn);
}
diff --git a/ovn/controller/ovn-controller.8.xml b/ovn/controller/ovn-controller.8.xml
index d172c08..ca7fa43 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -9,14 +9,14 @@
<h1>Description</h1>
<p>
<code>ovn-controller</code> is the local controller daemon for
- OVN, the Open Virtual Network. It connects northbound to the OVN
- database (see <code>ovn</code>(5)) over the OVSDB protocol, and
- southbound to the Open vSwitch database (see
+ OVN, the Open Virtual Network. It connects up to the OVN
+ Southbound database (see <code>ovn-sb</code>(5)) over the OVSDB
+ protocol, and down to the Open vSwitch database (see
<code>ovs-vswitchd.conf.db</code>(5)) over the OVSDB protocol and
to <code>ovs-vswitchd</code>(8) via OpenFlow. Each hypervisor and
software gateway in an OVN deployment runs its own independent
copy of <code>ovn-controller</code>; thus,
- <code>ovn-controller</code>'s southbound connections are
+ <code>ovn-controller</code>'s downward connections are
machine-local and do not run over a physical network.
</p>
@@ -111,5 +111,4 @@
</li>
</ul>
</p>
-.so lib/vlog-unixctl.man
</manpage>
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index d76af0b..35ab6ed 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -25,7 +25,6 @@
#include "compiler.h"
#include "daemon.h"
#include "dirs.h"
-#include "dummy.h"
#include "openvswitch/vconn.h"
#include "openvswitch/vlog.h"
#include "ovn/ovn-sb-idl.h"
@@ -50,7 +49,7 @@ static void parse_options(int argc, char *argv[]);
OVS_NO_RETURN static void usage(void);
static char *ovs_remote;
-static char *ovn_remote;
+static char *ovnsb_remote;
static void
@@ -58,7 +57,7 @@ get_initial_snapshot(struct ovsdb_idl *idl)
{
while (1) {
ovsdb_idl_run(idl);
- if (ovsdb_idl_get_seqno(idl)) {
+ if (ovsdb_idl_has_ever_connected(idl)) {
return;
}
ovsdb_idl_wait(idl);
@@ -86,7 +85,7 @@ get_core_config(struct controller_ctx *ctx)
ovsdb_idl_run(ctx->ovs_idl);
- /* xxx This does not support changing OVN OVSDB mid-run. */
+ /* xxx This does not support changing OVN Southbound OVSDB mid-run. */
remote = smap_get(&cfg->external_ids, "ovn-remote");
if (!remote) {
VLOG_INFO("OVN OVSDB remote not specified. Waiting...");
@@ -99,7 +98,7 @@ get_core_config(struct controller_ctx *ctx)
goto try_again;
}
- ovn_remote = xstrdup(remote);
+ ovnsb_remote = xstrdup(remote);
ctx->chassis_name = xstrdup(system_id);
return;
@@ -114,7 +113,7 @@ int
main(int argc, char *argv[])
{
struct unixctl_server *unixctl;
- struct controller_ctx ctx;
+ struct controller_ctx ctx = { .chassis_name = NULL };
bool exiting;
int retval;
@@ -125,8 +124,6 @@ main(int argc, char *argv[])
daemonize_start();
- memset(&ctx, '\0', sizeof ctx);
-
retval = unixctl_server_create(NULL, &unixctl);
if (retval) {
exit(EXIT_FAILURE);
@@ -138,9 +135,8 @@ main(int argc, char *argv[])
ovsrec_init();
sbrec_init();
- /* Connect to OVS OVSDB instance. We do not monitor any tables or
- * columns be default, so modules must register their interest
- * explicitly. */
+ /* Connect to OVS OVSDB instance. We do not monitor all tables by
+ * default, so modules must register their interest explicitly. */
ctx.ovs_idl = ovsdb_idl_create(ovs_remote, &ovsrec_idl_class, false, true);
/* Register interest in "external_ids" column in "Open_vSwitch" table,
@@ -155,7 +151,8 @@ main(int argc, char *argv[])
get_core_config(&ctx);
- ctx.ovnsb_idl = ovsdb_idl_create(ovn_remote, &sbrec_idl_class, true, true);
+ ctx.ovnsb_idl = ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class,
+ true, true);
get_initial_snapshot(ctx.ovnsb_idl);
@@ -167,7 +164,7 @@ main(int argc, char *argv[])
if (!ovsdb_idl_is_alive(ctx.ovnsb_idl)) {
int retval = ovsdb_idl_get_last_error(ctx.ovnsb_idl);
VLOG_ERR("%s: database connection failed (%s)",
- ovn_remote, ovs_retval_to_string(retval));
+ ovnsb_remote, ovs_retval_to_string(retval));
retval = EXIT_FAILURE;
break;
}
@@ -201,7 +198,7 @@ main(int argc, char *argv[])
ovsdb_idl_destroy(ctx.ovs_idl);
ovsdb_idl_destroy(ctx.ovnsb_idl);
- return retval;
+ exit(retval);
}
static void
@@ -210,8 +207,7 @@ parse_options(int argc, char *argv[])
enum {
OPT_PEER_CA_CERT = UCHAR_MAX + 1,
VLOG_OPTION_ENUMS,
- DAEMON_OPTION_ENUMS,
- OPT_ENABLE_DUMMY
+ DAEMON_OPTION_ENUMS
};
static struct option long_options[] = {
@@ -221,7 +217,6 @@ parse_options(int argc, char *argv[])
DAEMON_LONG_OPTIONS,
STREAM_SSL_LONG_OPTIONS,
{"peer-ca-cert", required_argument, NULL, OPT_PEER_CA_CERT},
- {"enable-dummy", no_argument, NULL, OPT_ENABLE_DUMMY},
{NULL, 0, NULL, 0}
};
char *short_options = ovs_cmdl_long_options_to_short_options(long_options);
@@ -250,10 +245,6 @@ parse_options(int argc, char *argv[])
stream_ssl_set_peer_ca_cert_file(optarg);
break;
- case OPT_ENABLE_DUMMY:
- timeval_dummy_register();
- break;
-
case '?':
exit(EXIT_FAILURE);
More information about the dev
mailing list