From: Ben Pfaff Date: Thu, 17 Dec 2009 00:26:17 +0000 (-0800) Subject: Make ovs-vswitchd report when it is done configuring; make ovs-vsctl wait. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b54e22e91eee43eb04ad53e2fa919be44f34e731;p=openvswitch Make ovs-vswitchd report when it is done configuring; make ovs-vsctl wait. Until now the ovsdb-based vswitch has provided no way to know when it has finished applying the configuration from the database. This commit introduces a way: * The client who wants to wait increments the "next_cfg" column of the Open_vSwitch record. * When ovs-vswitchd finishes reconfiguring, it sets the value of the "cur_cfg" column to that of the "next_cfg" column. * The client waits until the "cur_cfg" column is at least as great as the value it set into "next_cfg". This allows us to drop the 5-second sleep in interface-reconfigure. --- diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index f72e1876..877fa3e2 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -82,6 +83,13 @@ struct ovsdb_idl_txn { enum ovsdb_idl_txn_status status; bool dry_run; struct ds comment; + + /* Increments. */ + char *inc_table; + char *inc_column; + struct json *inc_where; + unsigned int inc_index; + int64_t inc_new_value; }; static struct vlog_rate_limit syntax_rl = VLOG_RATE_LIMIT_INIT(1, 5); @@ -770,6 +778,8 @@ const char * ovsdb_idl_txn_status_to_string(enum ovsdb_idl_txn_status status) { switch (status) { + case TXN_UNCHANGED: + return "unchanged"; case TXN_INCOMPLETE: return "incomplete"; case TXN_ABORTED: @@ -796,6 +806,9 @@ ovsdb_idl_txn_create(struct ovsdb_idl *idl) hmap_init(&txn->txn_rows); txn->dry_run = false; ds_init(&txn->comment); + txn->inc_table = NULL; + txn->inc_column = NULL; + txn->inc_where = NULL; return txn; } @@ -814,6 +827,16 @@ ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *txn) txn->dry_run = true; } +void +ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn, const char *table, + const char *column, const struct json *where) +{ + assert(!txn->inc_table); + txn->inc_table = xstrdup(table); + txn->inc_column = xstrdup(column); + txn->inc_where = where ? json_clone(where) : json_array_create_empty(); +} + void ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *txn) { @@ -822,6 +845,9 @@ ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *txn) } ovsdb_idl_txn_abort(txn); ds_destroy(&txn->comment); + free(txn->inc_table); + free(txn->inc_column); + json_destroy(txn->inc_where); free(txn); } @@ -1052,6 +1078,36 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) } } + /* Add increment. */ + if (txn->inc_table && any_updates) { + struct json *op; + + txn->inc_index = operations->u.array.n; + + op = json_object_create(); + json_object_put_string(op, "op", "mutate"); + json_object_put_string(op, "table", txn->inc_table); + json_object_put(op, "where", + substitute_uuids(json_clone(txn->inc_where), txn)); + json_object_put(op, "mutations", + json_array_create_1( + json_array_create_3( + json_string_create(txn->inc_column), + json_string_create("+="), + json_integer_create(1)))); + json_array_add(operations, op); + + op = json_object_create(); + json_object_put_string(op, "op", "select"); + json_object_put_string(op, "table", txn->inc_table); + json_object_put(op, "where", + substitute_uuids(json_clone(txn->inc_where), txn)); + json_object_put(op, "columns", + json_array_create_1(json_string_create( + txn->inc_column))); + json_array_add(operations, op); + } + if (txn->comment.length) { struct json *op = json_object_create(); json_object_put_string(op, "op", "comment"); @@ -1066,7 +1122,7 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) } if (!any_updates) { - txn->status = TXN_SUCCESS; + txn->status = TXN_UNCHANGED; } else if (!jsonrpc_session_send( txn->idl->session, jsonrpc_create_request( @@ -1082,6 +1138,13 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) return txn->status; } +int64_t +ovsdb_idl_txn_get_increment_new_value(const struct ovsdb_idl_txn *txn) +{ + assert(txn->status == TXN_SUCCESS); + return txn->inc_new_value; +} + void ovsdb_idl_txn_abort(struct ovsdb_idl_txn *txn) { @@ -1206,6 +1269,75 @@ ovsdb_idl_txn_find(struct ovsdb_idl *idl, const struct json *id) return NULL; } +static bool +check_json_type(const struct json *json, enum json_type type, const char *name) +{ + if (!json) { + VLOG_WARN_RL(&syntax_rl, "%s is missing", name); + return false; + } else if (json->type != type) { + VLOG_WARN_RL(&syntax_rl, "%s is %s instead of %s", + name, json_type_to_string(json->type), + json_type_to_string(type)); + return false; + } else { + return true; + } +} + +static bool +ovsdb_idl_txn_process_inc_reply(struct ovsdb_idl_txn *txn, + const struct json_array *results) +{ + struct json *count, *rows, *row, *column; + struct shash *mutate, *select; + + if (txn->inc_index + 2 > results->n) { + VLOG_WARN_RL(&syntax_rl, "reply does not contain enough operations " + "for increment (has %u, needs %u)", + results->n, txn->inc_index + 2); + return false; + } + + /* We know that this is a JSON objects because the loop in + * ovsdb_idl_txn_process_reply() checked. */ + mutate = json_object(results->elems[txn->inc_index]); + count = shash_find_data(mutate, "count"); + if (!check_json_type(count, JSON_INTEGER, "\"mutate\" reply \"count\"")) { + return false; + } + if (count->u.integer != 1) { + VLOG_WARN_RL(&syntax_rl, + "\"mutate\" reply \"count\" is %"PRId64" instead of 1", + count->u.integer); + return false; + } + + select = json_object(results->elems[txn->inc_index + 1]); + rows = shash_find_data(select, "rows"); + if (!check_json_type(rows, JSON_ARRAY, "\"select\" reply \"rows\"")) { + return false; + } + if (rows->u.array.n != 1) { + VLOG_WARN_RL(&syntax_rl, "\"select\" reply \"rows\" has %u elements " + "instead of 1", + rows->u.array.n); + return false; + } + row = rows->u.array.elems[0]; + if (!check_json_type(row, JSON_OBJECT, "\"select\" reply row")) { + return false; + } + column = shash_find_data(json_object(row), txn->inc_column); + if (!check_json_type(column, JSON_INTEGER, + "\"select\" reply inc column")) { + return false; + } + txn->inc_new_value = column->u.integer; + return true; +} + + static bool ovsdb_idl_txn_process_reply(struct ovsdb_idl *idl, const struct jsonrpc_msg *msg) @@ -1259,6 +1391,14 @@ ovsdb_idl_txn_process_reply(struct ovsdb_idl *idl, } } + if (txn->inc_table + && !soft_errors + && !hard_errors + && !ovsdb_idl_txn_process_inc_reply(txn, + json_array(msg->result))) { + hard_errors++; + } + status = (hard_errors ? TXN_ERROR : soft_errors ? TXN_TRY_AGAIN : TXN_SUCCESS); diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index 0a1fbc6b..fb551b48 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -16,6 +16,9 @@ #ifndef OVSDB_IDL_H #define OVSDB_IDL_H 1 +#include + +struct json; struct ovsdb_idl_class; struct ovsdb_idl *ovsdb_idl_create(const char *remote, @@ -29,6 +32,7 @@ unsigned int ovsdb_idl_get_seqno(const struct ovsdb_idl *); void ovsdb_idl_force_reconnect(struct ovsdb_idl *); enum ovsdb_idl_txn_status { + TXN_UNCHANGED, /* Transaction didn't include any changes. */ TXN_INCOMPLETE, /* Commit in progress, please wait. */ TXN_ABORTED, /* ovsdb_idl_txn_abort() called. */ TXN_SUCCESS, /* Commit successful. */ @@ -43,9 +47,12 @@ const char *ovsdb_idl_txn_status_to_string(enum ovsdb_idl_txn_status); struct ovsdb_idl_txn *ovsdb_idl_txn_create(struct ovsdb_idl *); void ovsdb_idl_txn_add_comment(struct ovsdb_idl_txn *, const char *); void ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *); +void ovsdb_idl_txn_increment(struct ovsdb_idl_txn *, const char *table, + const char *column, const struct json *where); void ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *); void ovsdb_idl_txn_wait(const struct ovsdb_idl_txn *); enum ovsdb_idl_txn_status ovsdb_idl_txn_commit(struct ovsdb_idl_txn *); +int64_t ovsdb_idl_txn_get_increment_new_value(const struct ovsdb_idl_txn *); void ovsdb_idl_txn_abort(struct ovsdb_idl_txn *); #endif /* ovsdb-idl.h */ diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index cd4cab3d..b1e79484 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -23,17 +23,17 @@ dnl RUN_OVS_VSCTL(COMMAND, ...) dnl dnl Executes each ovs-vsctl COMMAND. m4_define([RUN_OVS_VSCTL], - [m4_foreach([command], [$@], [ovs-vsctl -vreconnect:ANY:emer --db=unix:socket -- command + [m4_foreach([command], [$@], [ovs-vsctl --no-wait -vreconnect:ANY:emer --db=unix:socket -- command ])]) m4_define([RUN_OVS_VSCTL_ONELINE], - [m4_foreach([command], [$@], [ovs-vsctl -vreconnect:ANY:emer --db=unix:socket --oneline -- command + [m4_foreach([command], [$@], [ovs-vsctl --no-wait -vreconnect:ANY:emer --db=unix:socket --oneline -- command ])]) dnl RUN_OVS_VSCTL_TOGETHER(COMMAND, ...) dnl dnl Executes each ovs-vsctl COMMAND in a single run of ovs-vsctl. m4_define([RUN_OVS_VSCTL_TOGETHER], - [ovs-vsctl -vreconnect:ANY:emer --db=unix:socket --oneline dnl + [ovs-vsctl --no-wait -vreconnect:ANY:emer --db=unix:socket --oneline dnl m4_foreach([command], [$@], [ -- command])]) dnl CHECK_BRIDGES([BRIDGE, PARENT, VLAN], ...) diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index 65914194..973ccac8 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -167,6 +167,17 @@ OVSDB_CHECK_IDL([simple idl, writing via IDL], 005: done ]]) +OVSDB_CHECK_IDL([simple idl, increment operation], + [['[{"op": "insert", + "table": "simple", + "row": {}}]']], + [['set 0 r 2.0, increment simple i']], + [[000: i=0 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> +001: commit, status=success, increment=1 +002: i=1 r=2 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> +003: done +]]) + OVSDB_CHECK_IDL([self-linking idl, consistent ops], [], [['[{"op": "insert", diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index a0476a92..ede79226 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -1553,6 +1553,7 @@ idl_set(struct ovsdb_idl *idl, char *commands, int step) char *cmd, *save_ptr1 = NULL; struct ovsdb_idl_txn *txn; enum ovsdb_idl_txn_status status; + bool increment = false; txn = ovsdb_idl_txn_create(idl); for (cmd = strtok_r(commands, ",", &save_ptr1); cmd; @@ -1614,6 +1615,12 @@ idl_set(struct ovsdb_idl *idl, char *commands, int step) "i=%d", atoi(arg1)); } idltest_simple_delete(s); + } else if (!strcmp(name, "increment")) { + if (!arg2 || arg3) { + ovs_fatal(0, "\"set\" command requires 2 arguments"); + } + ovsdb_idl_txn_increment(txn, arg1, arg2, NULL); + increment = true; } else { ovs_fatal(0, "unknown command %s", name); } @@ -1625,8 +1632,13 @@ idl_set(struct ovsdb_idl *idl, char *commands, int step) ovsdb_idl_txn_wait(txn); poll_block(); } - printf("%03d: commit, status=%s\n", + printf("%03d: commit, status=%s", step, ovsdb_idl_txn_status_to_string(status)); + if (increment) { + printf(", increment=%"PRId64, + ovsdb_idl_txn_get_increment_new_value(txn)); + } + putchar('\n'); ovsdb_idl_txn_destroy(txn); } diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index bc6e4dbb..941e6b7c 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -29,6 +29,7 @@ #include "compiler.h" #include "dirs.h" #include "dynamic-string.h" +#include "json.h" #include "ovsdb-idl.h" #include "poll-loop.h" #include "svec.h" @@ -48,6 +49,9 @@ static bool oneline; /* --dry-run: Do not commit any changes. */ static bool dry_run; +/* --no-wait: Wait for ovs-vswitchd to reload its configuration? */ +static bool wait_for_reload = true; + /* --timeout: Time to wait for a connection to 'db'. */ static int timeout = 5; @@ -188,7 +192,7 @@ parse_options(int argc, char *argv[]) break; case OPT_NO_WAIT: - /* XXX not yet implemented */ + wait_for_reload = false; break; case OPT_DRY_RUN: @@ -1208,6 +1212,20 @@ static void run_vsctl_command(int argc, char *argv[], const struct ovsrec_open_vswitch *ovs, struct ds *output); +static struct json * +where_uuid_equals(const struct uuid *uuid) +{ + return + json_array_create_1( + json_array_create_3( + json_string_create("_uuid"), + json_string_create("=="), + json_array_create_2( + json_string_create("uuid"), + json_string_create_nocopy( + xasprintf(UUID_FMT, UUID_ARGS(uuid)))))); +} + static void do_vsctl(int argc, char *argv[], struct ovsdb_idl *idl) { @@ -1215,6 +1233,7 @@ do_vsctl(int argc, char *argv[], struct ovsdb_idl *idl) const struct ovsrec_open_vswitch *ovs; enum ovsdb_idl_txn_status status; struct ds comment, *output; + int64_t next_cfg; int n_output; int i, start; @@ -1237,6 +1256,13 @@ do_vsctl(int argc, char *argv[], struct ovsdb_idl *idl) ovs = ovsrec_open_vswitch_insert(txn); } + if (wait_for_reload) { + struct json *where = where_uuid_equals(&ovs->header_.uuid); + ovsdb_idl_txn_increment(txn, "Open_vSwitch", "next_cfg", + where); + json_destroy(where); + } + output = xmalloc(argc * sizeof *output); n_output = 0; for (start = i = 0; i <= argc; i++) { @@ -1256,6 +1282,9 @@ do_vsctl(int argc, char *argv[], struct ovsdb_idl *idl) ovsdb_idl_txn_wait(txn); poll_block(); } + if (wait_for_reload && status == TXN_SUCCESS) { + next_cfg = ovsdb_idl_txn_get_increment_new_value(txn); + } ovsdb_idl_txn_destroy(txn); switch (status) { @@ -1266,6 +1295,7 @@ do_vsctl(int argc, char *argv[], struct ovsdb_idl *idl) /* Should not happen--we never call ovsdb_idl_txn_abort(). */ vsctl_fatal("transaction aborted"); + case TXN_UNCHANGED: case TXN_SUCCESS: break; @@ -1308,6 +1338,23 @@ do_vsctl(int argc, char *argv[], struct ovsdb_idl *idl) fputs(ds_cstr(ds), stdout); } } + + if (wait_for_reload && status != TXN_UNCHANGED) { + for (;;) { + const struct ovsrec_open_vswitch *ovs; + + ovsdb_idl_run(idl); + OVSREC_OPEN_VSWITCH_FOR_EACH (ovs, idl) { + if (ovs->cur_cfg >= next_cfg) { + goto done; + } + } + ovsdb_idl_wait(idl); + poll_block(); + } + done: ; + } + exit(EXIT_SUCCESS); } diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 2b4a3f0c..040c7f38 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -754,6 +754,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) iterate_and_prune_ifaces(br, set_iface_properties, NULL); } + ovsrec_open_vswitch_set_cur_cfg(ovs_cfg, ovs_cfg->next_cfg); + ovsdb_idl_txn_commit(txn); ovsdb_idl_txn_destroy(txn); /* XXX */ } diff --git a/vswitchd/vswitch-idl.ovsidl b/vswitchd/vswitch-idl.ovsidl index 9fd07dc4..3fcf385d 100644 --- a/vswitchd/vswitch-idl.ovsidl +++ b/vswitchd/vswitch-idl.ovsidl @@ -25,7 +25,13 @@ "type": {"key": "uuid", "keyRefTable": "Controller", "min": 0, "max": 1}}, "ssl": { "comment": "SSL used globally by the daemon.", - "type": {"key": "uuid", "keyRefTable": "SSL", "min": 0, "max": 1}}}}, + "type": {"key": "uuid", "keyRefTable": "SSL", "min": 0, "max": 1}}, + "next_cfg": { + "comment": "Sequence number for client to increment when it modifies the configuration and wishes to wait for Open vSwitch to finish applying the changes.", + "type": "integer"}, + "cur_cfg": { + "comment": "Sequence number that Open vSwitch sets to the current value of 'next_cfg' after it finishing applying a set of configuration changes.", + "type": "integer"}}}, "Bridge": { "comment": "Configuration for a bridge within an Open_vSwitch.", "columns": { diff --git a/xenserver/opt_xensource_libexec_interface-reconfigure b/xenserver/opt_xensource_libexec_interface-reconfigure index de6043d1..d346cdc4 100755 --- a/xenserver/opt_xensource_libexec_interface-reconfigure +++ b/xenserver/opt_xensource_libexec_interface-reconfigure @@ -1257,7 +1257,6 @@ def datapath_modify_config(commands): + [c for c in commands if not c.startswith('#')]) if not rc: raise Error("Failed to modify vswitch configuration") - run_command(['/bin/sleep', '5']) # XXX return True #