"dev" <dev-boun...@openvswitch.org> wrote on 05/23/2016 10:34:48 AM:
> From: Ryan Moats/Omaha/IBM@IBMUS > To: Amitabha Biswas/San Jose/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 05/23/2016 10:35 AM > Subject: Re: [ovs-dev] [v1 1/1] Fix "raceful" E2E ovn tests > Sent by: "dev" <dev-boun...@openvswitch.org> > > > > "dev" <dev-boun...@openvswitch.org> wrote on 05/20/2016 01:17:57 PM: > > > From: Amitabha Biswas/San Jose/IBM@IBMUS > > To: dev@openvswitch.org > > Date: 05/20/2016 01:18 PM > > Subject: [ovs-dev] [v1 1/1] Fix "raceful" E2E ovn tests > > Sent by: "dev" <dev-boun...@openvswitch.org> > > > > This patch fixes the "raceful" situation that occurs when ovn-nbctl > > configures the OVN Northbound database till the point when the > > ovn-controller picks up the corresponding logical flows and applies > > them as OpenFlow rules on the hypervisor. > > > > The algorithm is outlined in the email - > > http://openvswitch.org/pipermail/dev/2016-April/070042.html which > > began as a thread here - > > http://openvswitch.org/pipermail/dev/2016-April/069602.html > > > > A new table, Config, is added to OVN_Northbound and OVN_Southbound > > DBs. This table is restricted to a single row in both the DBs. > > The OVN_Northbound Config contains a single row, with next_cfg and > > cur_cfg. The next_cfg is updated by ovn-nbctl whenever it commits > > an update to the OVN_Northbound DB. The ovn-northd picks up the > > next_cfg from the OVN_Northbound DB and sets the same value in the > > OVN_Southbound's Config table next_cfg column. > > > > A new column (cur_cfg) is added to the Chassis table in the > > OVN_Southbound DB. Every ovn-controller updates the cur_cfg in its > > Chassis row (to the value in the OVN_Southbound DB cur_cfg column). > > This is done in the next iteration after the ovn-controller picks up > > the latest logical flows and pends them to be applied as OpenFlow > > rules. The ovn-northd computes the mimumum of all the cur_cfg values > > in the Chassis table and sets the OVN_Northbound's DB cur_cfg value. > > > > The ovn-nbctl utility increments the OVN_Northbound's next_cfg value > > whenever it commits a transaction that modifies the OVN_Northbound > > DB. A "--wait" option is added to the ovn-nbctl utility. When the > > "--wait" option is used, the ovn-nbctl waits for the > > OVN_Northbound's cur_cfg to equal next_cfg before it exits. A new > > operation "nop" is added to use when the ovn-nbctl does not have any > > read or write operation but wants to wait for all previous > > configurations to have been picked up by the ovn-controllers by > > using - "ovn-nbctl --wait nop" command. > > > > The ovn E2E test cases have been modified to use the > > "ovn-nbctl --wait nop" command instead of using "sleep 1" to assume > > synchronization between the OVN_Northbound settings and the > > ovn-controllers. > > > > Caveats: > > > > Not all ovn-nbctl operations that modify the OVN_Northbound DB and > > therefore increment the next_cfg value actually result in OpenFlow > > rules being modified in the hypervisors. So a --wait option must not > > be used under such circumstances. > > > > If a logical datapath doesn't exist on a particular hypervisor, the > > ovn-controller will not apply any OpenFlow rules associated with that > > datapath and not update the cur_cfg. Use caution with the "--wait" > > option. > > > > Signed-off-by: Amitabha Biswas <abis...@us.ibm.com> > > > > Reported-by: Ryan Moats <rmo...@us.ibm.com> > > > > Reported-at: http://openvswitch.org/pipermail/dev/2016-April/069602.html > > --- > > ovn/controller/ofctrl.c | 15 ++++++++--- > > ovn/controller/ofctrl.h | 2 +- > > ovn/controller/ovn-controller.c | 43 ++++++++++++++++++++++++++++++- > > ovn/northd/ovn-northd.c | 56 ++++++++++++++++++++++++++++++ > > +++++++++++ > > ovn/ovn-nb.ovsschema | 10 ++++++-- > > ovn/ovn-nb.xml | 21 +++++++++++++++- > > ovn/ovn-sb.ovsschema | 12 +++++++-- > > ovn/ovn-sb.xml | 19 ++++++++++++++ > > ovn/utilities/ovn-nbctl.8.xml | 29 +++++++++++++++++++++ > > ovn/utilities/ovn-nbctl.c | 56 ++++++++++++++++++++++++++++++ > > ++++++----- > > tests/ovn.at | 37 ++++++++++++++++----------- > > 11 files changed, 269 insertions(+), 31 deletions(-) > > > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c > > index f537bc0..9d211bb 100644 > > --- a/ovn/controller/ofctrl.c > > +++ b/ovn/controller/ofctrl.c > > @@ -597,10 +597,14 @@ queue_flow_mod(struct ofputil_flow_mod *fm) > > * flows from 'flow_table' and frees them. (The hmap itself isn't > > * destroyed.) > > * > > - * This called be called be ofctrl_run() within the main loop. */ > > -void > > + * This called be called be ofctrl_run() within the main loop. > > + * Returns true if the installed flows were updated (queued) false > otherwise. > > + */ > > +bool > > ofctrl_put(struct hmap *flow_table) > > { > > + bool flows_updated = false; > > + > > /* The flow table can be updated if the connection to the > > switch is up and > > * in the correct state and not backlogged with existing flow_mods. > (Our > > * criteria for being backlogged appear very conservative, but the > socket > > @@ -610,7 +614,7 @@ ofctrl_put(struct hmap *flow_table) > > if (state != S_UPDATE_FLOWS > > || rconn_packet_counter_n_packets(tx_counter)) { > > ovn_flow_table_clear(flow_table); > > - return; > > + return flows_updated; > > } > > > > /* Iterate through all of the installed flows. If any of them are > no > > @@ -629,6 +633,7 @@ ofctrl_put(struct hmap *flow_table) > > .command = OFPFC_DELETE_STRICT, > > }; > > queue_flow_mod(&fm); > > + flows_updated = true; > > ovn_flow_log(i, "removing"); > > > > hmap_remove(&installed_flows, &i->hmap_node); > > @@ -647,6 +652,7 @@ ofctrl_put(struct hmap *flow_table) > > }; > > queue_flow_mod(&fm); > > ovn_flow_log(i, "updating"); > > + flows_updated = true; > > > > /* Replace 'i''s actions by 'd''s. */ > > free(i->ofpacts); > > @@ -676,10 +682,13 @@ ofctrl_put(struct hmap *flow_table) > > .command = OFPFC_ADD, > > }; > > queue_flow_mod(&fm); > > + flows_updated = true; > > ovn_flow_log(d, "adding"); > > > > /* Move 'd' from 'flow_table' to installed_flows. */ > > hmap_remove(flow_table, &d->hmap_node); > > hmap_insert(&installed_flows, &d->hmap_node, d-> hmap_node.hash); > > } > > + > > + return flows_updated; > > } > > diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h > > index bc9cfba..dd26f6e 100644 > > --- a/ovn/controller/ofctrl.h > > +++ b/ovn/controller/ofctrl.h > > @@ -30,7 +30,7 @@ struct ovsrec_bridge; > > /* Interface for OVN main loop. */ > > void ofctrl_init(void); > > enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int); > > -void ofctrl_put(struct hmap *flows); > > +bool ofctrl_put(struct hmap *flows); > > void ofctrl_wait(void); > > void ofctrl_destroy(void); > > > > diff --git a/ovn/controller/ovn-controller.c > b/ovn/controller/ovn-controller.c > > index 511b184..2a06f7c 100644 > > --- a/ovn/controller/ovn-controller.c > > +++ b/ovn/controller/ovn-controller.c > > @@ -252,6 +252,34 @@ get_ovnsb_remote_probe_interval(struct > > ovsdb_idl *ovs_idl, int *value) > > return false; > > } > > > > +/* Set the current configuration of the local chassis to the OVN > Southbound > > + * next_cfg value. This indicates that the ovn-controller has picked up > and > > + * applied (queued) the latest flows. > > + */ > > +static bool > > +update_chassis_config(struct controller_ctx *ctx, const char > *chassis_id) > > +{ > > + const struct sbrec_chassis *chassis_rec; > > + > > + if (!ctx->ovnsb_idl || !ctx->ovnsb_idl_txn) { > > + return false; > > + } > > + > > + chassis_rec = get_chassis(ctx->ovnsb_idl, chassis_id); > > + if (!chassis_rec) { > > + return false; > > + } > > + > > + const struct sbrec_config *sb_config > > + = sbrec_config_first(ctx->ovnsb_idl); > > + if (!sb_config) { > > + return false; > > + } > > + > > + sbrec_chassis_set_cur_cfg(chassis_rec, sb_config->next_cfg); > > + return true; > > +} > > + > > int > > main(int argc, char *argv[]) > > { > > @@ -384,7 +412,20 @@ main(int argc, char *argv[]) > > br_int, chassis_id, &ct_zones, &flow_table, > > &local_datapaths, &patched_datapaths); > > } > > - ofctrl_put(&flow_table); > > + /* Update the cur_cfg of the local chassis if the flow table > > + * was (pended to be) modified in a previous iteration but > no > > + * modified in the current iteration. This indicates that > the > > + * flow modification has reached a steady state (for now). > > + */ > > + static bool new_flows_pended = false; > > + if (ofctrl_put(&flow_table)) { > > + new_flows_pended = true; > > + } > > + else if (new_flows_pended && chassis_id) { > > One nit: this is usually pulled together into one line: "} else if ..." > > > + if (update_chassis_config(&ctx, chassis_id)) { > > + new_flows_pended = false; > > + } > > + } > > hmap_destroy(&flow_table); > > mcgroup_index_destroy(&mcgroups); > > lport_index_destroy(&lports); > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index 44e9430..954978a 100644 > > --- a/ovn/northd/ovn-northd.c > > +++ b/ovn/northd/ovn-northd.c > > @@ -2382,6 +2382,54 @@ add_column_noalert(struct ovsdb_idl *idl, > > ovsdb_idl_omit_alert(idl, column); > > } > > > > +static void > > +cp_nextcfg_to_sb(struct northd_context *ctx) > > +{ > > + const struct nbrec_config *nb; > > + const struct sbrec_config *sb; > > + > > + nb = nbrec_config_first(ctx->ovnnb_idl); > > + if (!nb && ctx->ovnnb_txn) { > > + /* XXX add verification that table is empty */ > > + nb = nbrec_config_insert(ctx->ovnnb_txn); > > + } > > + > > + sb = sbrec_config_first(ctx->ovnsb_idl); > > + if (!sb && ctx->ovnsb_txn) { > > + /* XXX add verification that table is empty */ > > + sb = sbrec_config_insert(ctx->ovnsb_txn); > > + } > > + > > + if (sb && nb && ctx->ovnsb_txn && sb->next_cfg < nb->next_cfg) { > > + sbrec_config_set_next_cfg(sb, nb->next_cfg); > > + } > > +} > > + > > +/* Compute the cur_cfg of the OVN_Northbound as the minimum of all > cur_cfg > > + * values in the Chassis Table. > > + */ > > +static void > > +compute_curcfg(struct northd_context *ctx) > > +{ > > + const struct nbrec_config *nb; > > + const struct sbrec_chassis *chassis_rec; > > + int64_t cur_cfg_min = (((int64_t)1) << 62) - 1; > > + bool cur_cfg_modified = false; > > + > > + SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->ovnsb_idl) { > > + if (cur_cfg_min > chassis_rec->cur_cfg) { > > + cur_cfg_min = chassis_rec->cur_cfg; > > + cur_cfg_modified = true; > > + } > > + } > > + > > + nb = nbrec_config_first(ctx->ovnnb_idl); > > + if (nb && ctx->ovnnb_txn && cur_cfg_modified && > > + nb->cur_cfg < cur_cfg_min) { > > + nbrec_config_set_cur_cfg(nb, cur_cfg_min); > > + } > > +} > > + > > int > > main(int argc, char *argv[]) > > { > > @@ -2452,6 +2500,12 @@ main(int argc, char *argv[]) > > add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac); > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > &sbrec_port_binding_col_chassis); > > > > + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > &sbrec_chassis_col_cur_cfg); > > + > > + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_config); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > &sbrec_config_col_next_cfg); > > + > > /* Main loop. */ > > exiting = false; > > while (!exiting) { > > @@ -2462,6 +2516,8 @@ main(int argc, char *argv[]) > > .ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop), > > }; > > > > + cp_nextcfg_to_sb(&ctx); > > + compute_curcfg(&ctx); > > ovnnb_db_run(&ctx); > > ovnsb_db_run(&ctx); > > > > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema > > index 8163f6a..fd6e16f 100644 > > --- a/ovn/ovn-nb.ovsschema > > +++ b/ovn/ovn-nb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Northbound", > > "version": "2.1.1", > > - "cksum": "2615511875 5108", > > + "cksum": "3226126619 5306", > > "tables": { > > "Logical_Switch": { > > "columns": { > > @@ -99,6 +99,12 @@ > > "ip_prefix": {"type": "string"}, > > "nexthop": {"type": "string"}, > > "output_port": {"type": {"key": "string", "min": 0, > > "max": 1}}}, > > - "isRoot": false} > > + "isRoot": false}, > > + "Config": { > > + "columns": { > > + "cur_cfg": {"type": "integer"}, > > + "next_cfg": {"type": "integer"}}, > > + "isRoot": true, > > + "maxRows": 1} > > } > > } > > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml > > index c01455d..96dabd4 100644 > > --- a/ovn/ovn-nb.xml > > +++ b/ovn/ovn-nb.xml > > @@ -637,7 +637,7 @@ > > column is set to <code>false</code>, the router is disabled. A > disabled > > router has all ingress and egress traffic dropped. > > </column> > > - > > + > > <group title="Common Columns"> > > <column name="external_ids"> > > See <em>External IDs</em> at the beginning of this document. > > @@ -757,4 +757,23 @@ > > </column> > > </table> > > > > + <table name="Config" title="OVN Northbound configuration."> > > + Configuration for an OVN Northbound DB. There must be exactly > > + one record in the <ref table="Config"/> table. > > + > > + <group title="Status"> > > + <column name="next_cfg"> > > + Sequence number for client to increment. When a client > > + modifies any part of the database configuration and wishes to > > + wait for all the ovn-controllers to finish applying the > > + changes, it may increment this sequence number. > > + </column> > > + > > + <column name="cur_cfg"> > > + Sequence number that ovn-northd sets to the current value of > > + <ref column="next_cfg"/> after all the ovn-controllers have > > + applied a (configuration) sequence number. > > + </column> > > + </group> > > + </table> > > </database> > > diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema > > index 06e8a07..5d266e2 100644 > > --- a/ovn/ovn-sb.ovsschema > > +++ b/ovn/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > "version": "1.3.0", > > - "cksum": "654726257 5528", > > + "cksum": "329629017 5732", > > "tables": { > > "Chassis": { > > "columns": { > > @@ -13,6 +13,7 @@ > > "vtep_logical_switches" : {"type": {"key": "string", > > "min": 0, > > "max": > "unlimited"}}, > > + "cur_cfg": {"type": "integer"}, > > "external_ids": { > > "type": {"key": "string", "value": "string", > > "min": 0, "max": "unlimited"}}}, > > @@ -110,4 +111,11 @@ > > "ip": {"type": "string"}, > > "mac": {"type": "string"}}, > > "indexes": [["logical_port", "ip"]], > > - "isRoot": true}}} > > + "isRoot": true}, > > + "Config": { > > + "columns": { > > + "next_cfg": {"type": "integer"}}, > > + "isRoot": true, > > + "maxRows": 1} > > + } > > +} > > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml > > index efd2f9a..28938ee 100644 > > --- a/ovn/ovn-sb.xml > > +++ b/ovn/ovn-sb.xml > > @@ -179,6 +179,13 @@ > > information. > > </column> > > > > + <column name='cur_cfg'> > > + <code>ovn-controller</code> populates this value with the value > > + of <ref column="next_cfg"/> in <ref table='Config'/> after it > > + successfully applies all the logical flows in the OVN > > + Southbound database as open flow rules. > > + </column> > > + > > <group title="Common Columns"> > > The overall purpose of these columns is described under > <code>Common > > Columns</code> at the beginning of this document. > > @@ -1511,4 +1518,16 @@ tcp.flags = RST; > > The Ethernet address to which the IP is bound. > > </column> > > </table> > > + > > + <table name="Config" title="OVN Southbound configuration."> > > + Configuration for an OVN Northbound DB. There must be exactly > > + one record in the <ref table="Config"/> table. > > + > > + <group title="Status"> > > + <column name="next_cfg"> > > + Sequence number copied by <code>ovn-northd</code> from the > > + OVN Northbound database when that value changes. > > + </column> > > + </group> > > + </table> > > </database> > > diff --git a/ovn/utilities/ovn-nbctl.8.xml > b/ovn/utilities/ovn-nbctl.8.xml > > index 8375ab7..52b092a 100644 > > --- a/ovn/utilities/ovn-nbctl.8.xml > > +++ b/ovn/utilities/ovn-nbctl.8.xml > > @@ -12,6 +12,35 @@ > > <h1>General Commands</h1> > > > > <dl> > > + <dt><code>nop</code></dt> > > + <dd> > > + <p> > > + Implements an empty action. It should be used with the > > + <var>--wait</var> to wait for the ovn-controllers to sync up > with > > + the OVN Northbound DB. > > + </p> > > + <p> > > + The <code>ovn-nbctl</code> utility increments the > > <var>next_cfg</var> > > + in the <code>Config</code> Table of the OVN Northbound. When > > + a <code>ovn-controllers</code> picks up and pends the > corresponding > > + OpenFlow rules on the hypervisors, it sets its own > > + <code>Chassis's</code> <var>cur_cfg</var > to the > > <var>next_cfg</var> > > + of the OVN Southbound DB, which is essentially a copy of > > + <var>next_cfg</var> of the OVN Northbound DB. The > > + <code>ovn-northd</code> sets the <var>cur_cfg</var> of in the > > + OVN Northbound to the minimum value of <var>cur_cfg</var> > > + reported by the <code>Chassis'</code>. > > + </p> > > + <p> > > + The <var>--wait</var> option informs <code>ovn-nbctl</code> to > > + wait for <var>cur_cfg</var> to equal <var>next_cfg</var>.It > should > > + be noted that there may be <code>ovn-nbctl</code> actions that > > + modify the OVN Northbound <var>next_cfg</var> but do not > result in > > + OpenFlow rules. In such cases the <var>cur_cfg</var> > > will not equal > > + the <var>next_cfg</var> till a future operation updates > > the OpenFlow > > + rules in the hypervisor. > > + </p> > > + </dd> > > <dt><code>show [<var>lswitch</var>]</code></dt> > > <dd> > > Prints a brief overview of the database contents. If > > diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c > > index d267829..fd1620c 100644 > > --- a/ovn/utilities/ovn-nbctl.c > > +++ b/ovn/utilities/ovn-nbctl.c > > @@ -48,6 +48,9 @@ static bool oneline; > > /* --dry-run: Do not commit any changes. */ > > static bool dry_run; > > > > +/* --wait: Wait for all ovn-controllers to apply open flow rules. */ > > +static bool wait_for_controllers = false; > > + > > /* --timeout: Time to wait for a connection to 'db'. */ > > static int timeout; > > > > @@ -68,7 +71,7 @@ static const char *nbctl_default_db(void); > > static void run_prerequisites(struct ctl_command[], size_t n_commands, > > struct ovsdb_idl *); > > static bool do_nbctl(const char *args, struct ctl_command *, size_t n, > > - struct ovsdb_idl *); > > + struct ovsdb_idl *, bool might_write_to_db); > > > > int > > main(int argc, char *argv[]) > > @@ -79,6 +82,7 @@ main(int argc, char *argv[]) > > unsigned int seqno; > > size_t n_commands; > > char *args; > > + bool might_write_to_db; > > > > set_program_name(argv[0]); > > fatal_ignore_sigpipe(); > > @@ -90,8 +94,8 @@ main(int argc, char *argv[]) > > > > /* Log our arguments. This is often valuable for debugging systems. > */ > > args = process_escape_args(argv); > > - VLOG(ctl_might_write_to_db(argv) ? VLL_INFO : VLL_DBG, > > - "Called as %s", args); > > + might_write_to_db = ctl_might_write_to_db(argv); > > + VLOG(might_write_to_db ? VLL_INFO : VLL_DBG, "Called as %s", args); > > > > /* Parse command line. */ > > shash_init(&local_options); > > @@ -125,7 +129,8 @@ main(int argc, char *argv[]) > > > > if (seqno != ovsdb_idl_get_seqno(idl)) { > > seqno = ovsdb_idl_get_seqno(idl); > > - if (do_nbctl(args, commands, n_commands, idl)) { > > + if (do_nbctl(args, commands, n_commands, idl, > > + might_write_to_db)) { > > free(args); > > exit(EXIT_SUCCESS); > > } > > @@ -157,6 +162,7 @@ parse_options(int argc, char *argv[], struct > > shash *local_options) > > enum { > > OPT_DB = UCHAR_MAX + 1, > > OPT_NO_SYSLOG, > > + OPT_WAIT, > > OPT_DRY_RUN, > > OPT_ONELINE, > > OPT_LOCAL, > > @@ -168,6 +174,7 @@ parse_options(int argc, char *argv[], struct > > shash *local_options) > > static const struct option global_long_options[] = { > > {"db", required_argument, NULL, OPT_DB}, > > {"no-syslog", no_argument, NULL, OPT_NO_SYSLOG}, > > + {"wait", optional_argument, NULL, OPT_WAIT}, > > {"dry-run", no_argument, NULL, OPT_DRY_RUN}, > > {"oneline", no_argument, NULL, OPT_ONELINE}, > > {"timeout", required_argument, NULL, 't'}, > > @@ -224,6 +231,10 @@ parse_options(int argc, char *argv[], struct > > shash *local_options) > > vlog_set_levels(&this_module, VLF_SYSLOG, VLL_WARN); > > break; > > > > + case OPT_WAIT: > > + wait_for_controllers = true; > > + break; > > + > > case OPT_DRY_RUN: > > dry_run = true; > > break; > > @@ -339,6 +350,7 @@ Options:\n\ > > --db=DATABASE connect to DATABASE\n\ > > (default: %s)\n\ > > -t, --timeout=SECS wait at most SECS seconds\n\ > > + --wait wait for ovn-controllers to sync with NB \n > \ > > --dry-run do not commit changes to database\n\ > > --oneline print exactly one line of output per > > command\n", > > program_name, program_name, ctl_get_db_cmd_usage(), > > nbctl_default_db()); > > @@ -1162,22 +1174,36 @@ run_prerequisites(struct ctl_command > > *commands, size_t n_commands, > > > > static bool > > do_nbctl(const char *args, struct ctl_command *commands, size_t > n_commands, > > - struct ovsdb_idl *idl) > > + struct ovsdb_idl *idl, bool might_write_to_db) > > { > > struct ovsdb_idl_txn *txn; > > + const struct nbrec_config *nb; > > enum ovsdb_idl_txn_status status; > > struct ovsdb_symbol_table *symtab; > > struct ctl_context ctx; > > struct ctl_command *c; > > struct shash_node *node; > > char *error = NULL; > > + int64_t next_cfg = 0; > > > > txn = the_idl_txn = ovsdb_idl_txn_create(idl); > > if (dry_run) { > > ovsdb_idl_txn_set_dry_run(txn); > > } > > > > - ovsdb_idl_txn_add_comment(txn, "ovs-nbctl: %s", args); > > + ovsdb_idl_txn_add_comment(txn, "ovn-nbctl: %s", args); > > + > > + nb = nbrec_config_first(idl); > > + if (!nb) { > > + /* XXX add verification that table is empty */ > > + nb = nbrec_config_insert(txn); > > + } > > + > > + next_cfg = nb->next_cfg; > > + if (might_write_to_db) { > > + ovsdb_idl_txn_increment(txn, &nb->header_, > > + &nbrec_config_col_next_cfg); > > + } > > > > symtab = ovsdb_symbol_table_create(); > > for (c = commands; c < &commands[n_commands]; c++) { > > @@ -1220,6 +1246,9 @@ do_nbctl(const char *args, struct ctl_command > > *commands, size_t n_commands, > > } > > > > status = ovsdb_idl_txn_commit_block(txn); > > + if (status == TXN_SUCCESS && might_write_to_db) { > > + next_cfg = ovsdb_idl_txn_get_increment_new_value(txn); > > + } > > if (status == TXN_UNCHANGED || status == TXN_SUCCESS) { > > for (c = commands; c < &commands[n_commands]; c++) { > > if (c->syntax->postprocess) { > > @@ -1296,6 +1325,20 @@ do_nbctl(const char *args, struct ctl_command > > *commands, size_t n_commands, > > shash_destroy_free_data(&c->options); > > } > > free(commands); > > + > > + if (wait_for_controllers) { > > + for (;;) { > > + ovsdb_idl_run(idl); > > + NBREC_CONFIG_FOR_EACH(nb, idl) { > > + if (nb->cur_cfg >= next_cfg) { > > + goto done; > > + } > > + } > > + ovsdb_idl_wait(idl); > > + poll_block(); > > + } > > + done: ; > > + } > > ovsdb_idl_txn_destroy(txn); > > ovsdb_idl_destroy(idl); > > > > @@ -1337,6 +1380,7 @@ nbctl_exit(int status) > > } > > > > static const struct ctl_command_syntax nbctl_commands[] = { > > + { "nop", 0, 1, "[]", NULL, NULL, NULL, "", RO }, > > { "show", 0, 1, "[LSWITCH]", NULL, nbctl_show, NULL, "", RO }, > > > > /* lswitch commands. */ > > diff --git a/tests/ovn.at b/tests/ovn.at > > index e6ac1d7..1458134 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -589,8 +589,7 @@ ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type > > == 0x1236 && outport == "lp33"' d > > ovn_populate_arp > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > # Given the name of a logical port, prints the name of the hypervisor > > # on which it is located. > > @@ -762,6 +761,10 @@ done > > # set address for lp13 with invalid characters. > > # lp13 should be configured with only 192.168.0.13. > > ovn-nbctl lport-set-addresses lp13 "f0:00:00:00:00:13 192.168.0.13 > > invalid 192.169.0.13" > > + > > +# Allow some time for ovn-northd and ovn-controller to catch up. > > +ovn-nbctl --wait nop > > + > > sip=`ip_to_hex 192 168 0 11` > > tip=`ip_to_hex 192 168 0 13` > > test_arp 11 f00000000011 $sip $tip f00000000013 > > @@ -898,6 +901,9 @@ for i in 1 2; do > > done > > done > > > > +# Allow some time for ovn-northd and ovn-controller to catch up. > > +ovn-nbctl --wait nop > > + > > ovn_populate_arp > > > > # XXX This is now the 3rd copy of these functions in this file ... > > @@ -1108,8 +1114,7 @@ ovs-vsctl add-port br-phys vif3 -- set > > Interface vif3 options:tx_pcap=hv3/vif3-t > > ovn_populate_arp > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > # test_packet INPORT DST SRC ETHTYPE OUTPORT... > > # > > @@ -1339,8 +1344,7 @@ done > > ovn_populate_arp > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > # test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT... > > # > > @@ -1698,8 +1702,7 @@ done > > ovn_populate_arp > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > # Given the name of a logical port, prints the name of the hypervisor > > # on which it is located. > > @@ -1937,7 +1940,8 @@ done > > # configure lport13 to send and received IPv4 packets with an address > range > > ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168. > > 0.13 20.0.0.4/24 10.0.0.0/24" > > > > -sleep 2 > > +# Allow some time for ovn-northd and ovn-controller to catch up. > > +ovn-nbctl --wait nop > > > > sip=`ip_to_hex 10 0 0 13` > > tip=`ip_to_hex 192 168 0 22` > > @@ -2130,8 +2134,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > ovn_populate_arp > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > # Send ip packets between the two ports. > > ip_to_hex() { > > @@ -2262,8 +2265,7 @@ ovs-vsctl -- add-port br-int vif2 -- \ > > > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > # Send ip packets between the two ports. > > ip_to_hex() { > > @@ -2303,6 +2305,9 @@ as hv1 ovs-ofctl dump-flows br-int > > #Disable router R1 > > ovn-nbctl set Logical_Router R1 enabled=false > > > > +# Allow some time for ovn-northd and ovn-controller to catch up. > > +ovn-nbctl --wait nop > > + > > echo "---------SB dump-----" > > ovn-sbctl list datapath_binding > > echo "---------------------" > > @@ -2459,8 +2464,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > ovn_populate_arp > > > > # Allow some time for ovn-northd and ovn-controller to catch up. > > -# XXX This should be more systematic. > > -sleep 1 > > +ovn-nbctl --wait nop > > > > ip_to_hex() { > > printf "%02x%02x%02x%02x" "$@" > > @@ -2577,6 +2581,9 @@ AT_CHECK([ovn-nbctl lport-set-options ln_port > > network_name=physnet1]) > > > > AT_CHECK([ovs-vsctl add-port br-int localvif1 -- set Interface > > localvif1 external_ids:iface-id=localvif1]) > > > > +# Allow some time for ovn-northd and ovn-controller to catch up. > > +ovn-nbctl --wait nop > > + > > # Wait for packet to be received. > > OVS_WAIT_UNTIL([test `wc -c < "hv/snoopvif-tx.pcap"` -ge 50]) > > trim_zeros() { > > -- > > 2.7.4 (Apple Git-66) > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > Other than the above nit, this looks like what I had in mind... > > Still running through it in some more detail to make sure it > handles some of the nastier problems I've seen... Well, I'm going to stand corrected - I've run this through a bunch of different scenarios, and it's not up to snuff - I'm still seeing race conditions when running the following test cases: 3HVs, 1LS, 3 lports/HV portsecurity: 3 HVs, 1 LS, 3 lports/HV 2 HVs, 2 LS, 1 lport/LS, 2 peer LRs 2 HVs, 3 LS, 1 lport/LS, 2 peer LRs, static routes So, I think we are going to have to go back to the drawing board some more... Ryan _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
