[ovs-dev] [PATCH 14/15] ovsdb-idl: Mark ovsdb_idl_get_row_arc() parameter const.

[Ben Pfaff]

This function doesn't modify its 'dst_table' parameter, so it might as well
be marked const.

Signed-off-by: Ben Pfaff 
---
 lib/ovsdb-idl-provider.h | 2 +-
 lib/ovsdb-idl.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 1e375de..3104f2c 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -129,7 +129,7 @@ struct ovsdb_idl_class {
 
 struct ovsdb_idl_row *ovsdb_idl_get_row_arc(
 struct ovsdb_idl_row *src,
-struct ovsdb_idl_table_class *dst_table,
+const struct ovsdb_idl_table_class *dst_table,
 const struct uuid *dst_uuid);
 
 void ovsdb_idl_txn_verify(const struct ovsdb_idl_row *,
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index e1df634..b4d3c42 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2041,7 +2041,7 @@ ovsdb_idl_table_from_class(const struct ovsdb_idl *idl,
 /* Called by ovsdb-idlc generated code. */
 struct ovsdb_idl_row *
 ovsdb_idl_get_row_arc(struct ovsdb_idl_row *src,
-  struct ovsdb_idl_table_class *dst_table_class,
+  const struct ovsdb_idl_table_class *dst_table_class,
   const struct uuid *dst_uuid)
 {
 struct ovsdb_idl *idl = src->table->idl;
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 15/15] ovsdb-idl: Check internal graph in OVSDB tests.

[Ben Pfaff]

Some upcoming tests will add extra trickiness to the IDL internal graph.
This worries me, because the IDL doesn't have any checks for its graph
consistency.  This commit adds some.

Signed-off-by: Ben Pfaff 
---
 lib/ovsdb-idl.c| 122 +
 lib/ovsdb-idl.h|   2 +
 tests/test-ovsdb.c |   7 +++
 3 files changed, 131 insertions(+)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index b4d3c42..8ce228d 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -629,6 +629,128 @@ ovsdb_idl_set_probe_interval(const struct ovsdb_idl *idl, 
int probe_interval)
 {
 jsonrpc_session_set_probe_interval(idl->session, probe_interval);
 }
+
+static size_t
+find_uuid_in_array(const struct uuid *target,
+   const struct uuid *array, size_t n)
+{
+for (size_t i = 0; i < n; i++) {
+if (uuid_equals([i], target)) {
+return i;
+}
+}
+return SIZE_MAX;
+}
+
+static size_t
+array_contains_uuid(const struct uuid *target,
+const struct uuid *array, size_t n)
+{
+return find_uuid_in_array(target, array, n) != SIZE_MAX;
+}
+
+static bool
+remove_uuid_from_array(const struct uuid *target,
+   struct uuid *array, size_t *n)
+{
+size_t i = find_uuid_in_array(target, array, *n);
+if (i != SIZE_MAX) {
+array[i] = array[--*n];
+return true;
+} else {
+return false;
+}
+}
+
+static void
+add_row_references(const struct ovsdb_base_type *type,
+   const union ovsdb_atom *atoms, size_t n_atoms,
+   const struct uuid *exclude_uuid,
+   struct uuid **dstsp, size_t *n_dstsp,
+   size_t *allocated_dstsp)
+{
+if (type->type != OVSDB_TYPE_UUID || !type->u.uuid.refTableName) {
+return;
+}
+
+for (size_t i = 0; i < n_atoms; i++) {
+const struct uuid *uuid = [i].uuid;
+if (!uuid_equals(uuid, exclude_uuid)
+&& !array_contains_uuid(uuid, *dstsp, *n_dstsp)) {
+if (*n_dstsp >= *allocated_dstsp) {
+*dstsp = x2nrealloc(*dstsp, allocated_dstsp,
+sizeof **dstsp);
+
+}
+(*dstsp)[*n_dstsp] = *uuid;
+++*n_dstsp;
+}
+}
+}
+
+/* Checks for consistency in 'idl''s graph of arcs between database rows.  Each
+ * reference from one row to a different row should be reflected as a "struct
+ * ovsdb_idl_arc" between those rows.
+ *
+ * This function is slow, big-O wise, and aborts if it finds an inconsistency,
+ * thus it is only for use in test programs. */
+void
+ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
+{
+/* Consistency is broken while a transaction is in progress. */
+if (!idl->txn) {
+return;
+}
+
+bool ok = true;
+
+struct uuid *dsts = NULL;
+size_t allocated_dsts = 0;
+
+for (size_t i = 0; i < idl->class->n_tables; i++) {
+const struct ovsdb_idl_table *table = >tables[i];
+const struct ovsdb_idl_table_class *class = table->class;
+
+const struct ovsdb_idl_row *row;
+HMAP_FOR_EACH (row, hmap_node, >rows) {
+size_t n_dsts = 0;
+if (row->new) {
+size_t n_columns = shash_count(>table->columns);
+for (size_t j = 0; j < n_columns; j++) {
+const struct ovsdb_type *type = >columns[j].type;
+const struct ovsdb_datum *datum = >new[j];
+add_row_references(>key,
+   datum->keys, datum->n, >uuid,
+   , _dsts, _dsts);
+add_row_references(>value,
+   datum->values, datum->n, >uuid,
+   , _dsts, _dsts);
+}
+}
+const struct ovsdb_idl_arc *arc;
+LIST_FOR_EACH (arc, src_node, >src_arcs) {
+if (!remove_uuid_from_array(>dst->uuid,
+dsts, _dsts)) {
+VLOG_ERR("unexpected arc from %s row "UUID_FMT" to %s "
+ "row "UUID_FMT,
+ table->class->name,
+ UUID_ARGS(>uuid),
+ arc->dst->table->class->name,
+ UUID_ARGS(>dst->uuid));
+ok = false;
+}
+}
+for (size_t i = 0; i < n_dsts; i++) {
+VLOG_ERR("%s row "UUID_FMT" missing arc to row "UUID_FMT,
+ table->class->name, UUID_ARGS(>uuid),
+ UUID_ARGS([i]));
+ok = false;
+}
+}
+}
+free(dsts);
+ovs_assert(ok);
+}
 
 static unsigned char *
 ovsdb_idl_get_mode(struct ovsdb_idl *idl,
diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index 

[ovs-dev] [PATCH 13/15] ovsdb-idl: Add some more implementation comments.

[Ben Pfaff]

I wrote this code and if I have to rediscover how it works, it's time to
improve the commnts.

Signed-off-by: Ben Pfaff 
---
 lib/ovsdb-idl-provider.h | 36 
 lib/ovsdb-idl.c  |  4 ++--
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index bac7754..1e375de 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -25,6 +25,42 @@
 #include "openvswitch/shash.h"
 #include "uuid.h"
 
+/* A local copy of a row in an OVSDB table, replicated from an OVSDB server.
+ * This structure is used as a header for a larger structure that translates
+ * the "struct ovsdb_datum"s into easier-to-use forms, via the ->parse() and
+ * ->unparse functions in struct ovsdb_idl_column.  (Those functions are
+ * generated automatically via ovsdb-idlc.)
+ *
+ * When no transaction is in progress:
+ *
+ * - 'old' points to the data committed to the database and currently
+ *   in the row.
+ *
+ * - 'new == old'.
+ *
+ * When a transaction is in progress, the situation is a little different.  For
+ * a row inserted in the transaction, 'old' is NULL and 'new' points to the
+ * row's initial contents.  Otherwise:
+ *
+ * - 'old' points to the data committed to the database and currently in
+ *   the row.  (This is the same as when no transaction is in progress.)
+ *
+ * - If the transaction does not modify the row, 'new == old'.
+ *
+ * - If the transaction modifies the row, 'new' points to the modified
+ *   data.
+ *
+ * - If the transaction deletes the row, 'new' is NULL.
+ *
+ * Thus:
+ *
+ * - 'old' always points to committed data, except that it is NULL if the
+ *   row is inserted within the current transaction.
+ *
+ * - 'new' always points to the newest, possibly uncommitted version of the
+ *   row's data, except that it is NULL if the row is deleted within the
+ *   current transaction.
+ */
 struct ovsdb_idl_row {
 struct hmap_node hmap_node; /* In struct ovsdb_idl_table's 'rows'. */
 struct uuid uuid;   /* Row "_uuid" field. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index a1fcd19..e1df634 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -88,8 +88,8 @@ struct ovsdb_idl {
 const struct ovsdb_idl_class *class;
 struct jsonrpc_session *session;
 struct uuid uuid;
-struct shash table_by_name;
-struct ovsdb_idl_table *tables; /* Contains "struct ovsdb_idl_table *"s.*/
+struct shash table_by_name; /* Contains "struct ovsdb_idl_table *"s.*/
+struct ovsdb_idl_table *tables; /* Array of ->class->n_tables elements. */
 unsigned int change_seqno;
 bool verify_write_only;
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 11/15] ovsdb-idl: Sort and unique-ify datum in ovsdb_idl_txn_write().

[Ben Pfaff]

I noticed that there were lots of calls to ovsdb_datum_sort_unique() from
"set" functions in generated IDL code.  This moves that call into common
code, reducing redundancy.

There are more calls to the same function that are a little harder to
remove.

Signed-off-by: Ben Pfaff 
---
 lib/ovsdb-idl.c | 17 -
 ovsdb/ovsdb-idlc.in |  2 --
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index ffee4b6..a1fcd19 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -3159,9 +3159,10 @@ discard_datum:
  * itself and the structs derived from it (e.g. the "struct ovsrec_*", for
  * ovs-vswitchd).
  *
- * 'datum' must have the correct type for its column.  The IDL does not check
- * that it meets schema constraints, but ovsdb-server will do so at commit time
- * so it had better be correct.
+ * 'datum' must have the correct type for its column, but it needs not be
+ * sorted or unique because this function will take care of that.  The IDL does
+ * not check that it meets schema constraints, but ovsdb-server will do so at
+ * commit time so it had better be correct.
  *
  * A transaction must be in progress.  Replication of 'column' must not have
  * been disabled (by calling ovsdb_idl_omit()).
@@ -3177,11 +3178,17 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row,
 const struct ovsdb_idl_column *column,
 struct ovsdb_datum *datum)
 {
+ovsdb_datum_sort_unique(datum,
+column->type.key.type, column->type.value.type);
 ovsdb_idl_txn_write__(row, column, datum, true);
 }
 
-/* Similar to ovsdb_idl_txn_write(), except that the caller retains ownership
- * of 'datum' and what it points to. */
+/* Similar to ovsdb_idl_txn_write(), except:
+ *
+ * - The caller retains ownership of 'datum' and what it points to.
+ *
+ * - The caller must ensure that 'datum' is sorted and unique (e.g. via
+ *   ovsdb_datum_sort_unique().) */
 void
 ovsdb_idl_txn_write_clone(const struct ovsdb_idl_row *row,
   const struct ovsdb_idl_column *column,
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 17b4453..680a205 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -763,8 +763,6 @@ void
 valueType = type.value.toAtomicType()
 else:
 valueType = "OVSDB_TYPE_VOID"
-print "ovsdb_datum_sort_unique(, %s, %s);" % (
-type.key.toAtomicType(), valueType)
 txn_write_func = "ovsdb_idl_txn_write"
 print "%(f)s(>header_, &%(s)s_col_%(c)s, );" \
 % {'f': txn_write_func,
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 12/15] ovsdb-idlc: Remove special case for "sizeof bool".

[Ben Pfaff]

The "sparse" checker used to warn about sizeof(bool).  These days, it does
not warn (without -Wsizeof-bool), so remove this ugly special case.

If you have a version of "sparse" that still warns by default, please
upgrade to a version that includes commit 2667c2d4ab33 (sparse: Allow
override of sizeof(bool) warning).

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 27 ---
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 680a205..cf6cd58 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -311,13 +311,6 @@ def printCIDLSource(schemaFile):
 #include "ovsdb-error.h"
 #include "util.h"
 
-#ifdef __CHECKER__
-/* Sparse dislikes sizeof(bool) ("warning: expression using sizeof bool"). */
-enum { sizeof_bool = 1 };
-#else
-enum { sizeof_bool = sizeof(bool) };
-#endif
-
 ''' % schema.idlHeader
 
 # Cast functions.
@@ -411,23 +404,11 @@ static void
 valueSrc = "datum->values[i].%s" % 
type.value.type.to_string()
 print "if (!row->n_%s) {" % (columnName)
 
-# Special case for boolean types.  This is only here because
-# sparse does not like the "normal" case ("warning: expression
-# using sizeof bool").
-if type.key.type == ovs.db.types.BooleanType:
-sizeof = "sizeof_bool"
-else:
-sizeof = "sizeof *%s" % keyVar
-print "%s = xmalloc(%s * %s);" % (keyVar, nMax,
-  sizeof)
+print "%s = xmalloc(%s * sizeof *%s);" % (
+keyVar, nMax, keyVar)
 if valueVar:
-# Special case for boolean types (see above).
-if type.value.type == ovs.db.types.BooleanType:
-sizeof = " * sizeof_bool"
-else:
-sizeof = "sizeof *%s" % valueVar
-print "%s = xmalloc(%s * %s);" % (valueVar,
-  nMax, sizeof)
+print "%s = xmalloc(%s * sizeof *%s);" % (
+valueVar, nMax, valueVar)
 print "}"
 print "%s[row->n_%s] = %s;" % (keyVar, columnName, 
keySrc)
 if valueVar:
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 10/15] ovsdb-idl: Improve comment on ovsdb_idl_txn_write[_clone]().

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 lib/ovsdb-idl.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 3dbf0f7..ffee4b6 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -3080,23 +3080,6 @@ ovsdb_idl_txn_complete(struct ovsdb_idl_txn *txn,
 hmap_remove(>idl->outstanding_txns, >hmap_node);
 }
 
-/* Writes 'datum' to the specified 'column' in 'row_'.  Updates both 'row_'
- * itself and the structs derived from it (e.g. the "struct ovsrec_*", for
- * ovs-vswitchd).
- *
- * 'datum' must have the correct type for its column.  The IDL does not check
- * that it meets schema constraints, but ovsdb-server will do so at commit time
- * so it had better be correct.
- *
- * A transaction must be in progress.  Replication of 'column' must not have
- * been disabled (by calling ovsdb_idl_omit()).
- *
- * Usually this function is used indirectly through one of the "set" functions
- * generated by ovsdb-idlc.
- *
- * Takes ownership of what 'datum' points to (and in some cases destroys that
- * data before returning) but makes a copy of 'datum' itself.  (Commonly
- * 'datum' is on the caller's stack.) */
 static void
 ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
   const struct ovsdb_idl_column *column,
@@ -3172,6 +3155,23 @@ discard_datum:
 }
 }
 
+/* Writes 'datum' to the specified 'column' in 'row_'.  Updates both 'row_'
+ * itself and the structs derived from it (e.g. the "struct ovsrec_*", for
+ * ovs-vswitchd).
+ *
+ * 'datum' must have the correct type for its column.  The IDL does not check
+ * that it meets schema constraints, but ovsdb-server will do so at commit time
+ * so it had better be correct.
+ *
+ * A transaction must be in progress.  Replication of 'column' must not have
+ * been disabled (by calling ovsdb_idl_omit()).
+ *
+ * Usually this function is used indirectly through one of the "set" functions
+ * generated by ovsdb-idlc.
+ *
+ * Takes ownership of what 'datum' points to (and in some cases destroys that
+ * data before returning) but makes a copy of 'datum' itself.  (Commonly
+ * 'datum' is on the caller's stack.) */
 void
 ovsdb_idl_txn_write(const struct ovsdb_idl_row *row,
 const struct ovsdb_idl_column *column,
@@ -3180,6 +3180,8 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row,
 ovsdb_idl_txn_write__(row, column, datum, true);
 }
 
+/* Similar to ovsdb_idl_txn_write(), except that the caller retains ownership
+ * of 'datum' and what it points to. */
 void
 ovsdb_idl_txn_write_clone(const struct ovsdb_idl_row *row,
   const struct ovsdb_idl_column *column,
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 08/15] ovsdb-idlc: Consolidate assertions.

[Ben Pfaff]

There were lots of bits of code emitting "assert(inited);".  This combines
many of them.

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 33 -
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 6a2dc6b..f6f00b3 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -345,6 +345,7 @@ static struct %(s)s *
 static void
 %(s)s_parse_%(c)s(struct ovsdb_idl_row *row_, const struct ovsdb_datum *datum)
 {
+ovs_assert(inited);
 struct %(s)s *row = %(s)s_cast(row_);''' % {'s': structName,
 'c': columnName}
 type = column.type
@@ -356,7 +357,6 @@ static void
 valueVar = None
 
 if type.is_smap():
-print "ovs_assert(inited);"
 print "smap_init(>%s);" % columnName
 print "for (size_t i = 0; i < datum->n; i++) {"
 print "smap_add(>%s," % columnName
@@ -365,7 +365,6 @@ static void
 print "}"
 elif (type.n_min == 1 and type.n_max == 1) or 
type.is_optional_pointer():
 print
-print "ovs_assert(inited);"
 print "if (datum->n >= 1) {"
 if not type.key.ref_table:
 print "%s = datum->keys[0].%s;" % (keyVar, 
type.key.type.to_string())
@@ -388,7 +387,6 @@ static void
 nMax = "n"
 else:
 nMax = "datum->n"
-print "ovs_assert(inited);"
 print "%s = NULL;" % keyVar
 if valueVar:
 print "%s = NULL;" % valueVar
@@ -706,18 +704,20 @@ const struct ovsdb_datum *
 nVar = members[1]['name']
 
 print comment
-print 'void'
-print '%(s)s_set_%(c)s(const struct %(s)s *row, %(args)s)' % \
-{'s': structName, 'c': columnName,
- 'args': ', '.join(['%(type)s%(name)s' % m for m in members])}
-print "{"
-print "struct ovsdb_datum datum;"
+print """\
+void
+%(s)s_set_%(c)s(const struct %(s)s *row, %(args)s)
+{
+ovs_assert(inited);
+struct ovsdb_datum datum;""" % {'s': structName,
+  'c': columnName,
+  'args': ', '.join(['%(type)s%(name)s'
+ % m for m in members])}
 if type.n_min == 1 and type.n_max == 1:
 print "union ovsdb_atom key;"
 if type.value:
 print "union ovsdb_atom value;"
 print
-print "ovs_assert(inited);"
 print "datum.n = 1;"
 print "datum.keys = "
 print "" + 
type.key.assign_c_value_casting_away_const("key.%s" % 
type.key.type.to_string(), keyVar)
@@ -730,7 +730,6 @@ const struct ovsdb_datum *
 elif type.is_optional_pointer():
 print "union ovsdb_atom key;"
 print
-print "ovs_assert(inited);"
 print "if (%s) {" % keyVar
 print "datum.n = 1;"
 print "datum.keys = "
@@ -744,7 +743,6 @@ const struct ovsdb_datum *
 elif type.n_max == 1:
 print "union ovsdb_atom key;"
 print
-print "ovs_assert(inited);"
 print "if (%s) {" % nVar
 print "datum.n = 1;"
 print "datum.keys = "
@@ -757,7 +755,6 @@ const struct ovsdb_datum *
 txn_write_func = "ovsdb_idl_txn_write_clone"
 else:
 print
-print "ovs_assert(inited);"
 print "datum.n = %s;" % nVar
 print "datum.keys = %s ? xmalloc(%s * sizeof *datum.keys) 
: NULL;" % (nVar, nVar)
 if type.value:
@@ -955,6 +952,7 @@ void
 {'s': structName, 'c': columnName,
  'args': ', '.join(['%(type)s%(name)s' % m for m in members])}
 print "{"
+print "ovs_assert(inited);"
 print "struct ovsdb_datum datum;"
 free = []
 if type.n_min == 1 and type.n_max == 1:
@@ -962,7 +960,6 @@ void
 if type.value:
 print "union ovsdb_atom value;"
 print
-print "ovs_assert(inited);"
 print "datum.n = 1;"
 print "datum.keys = "
 print "" + 
type.key.assign_c_value_casting_away_const("key.%s" % 
type.key.type.to_string(), keyVar, refTable=False)
@@ -974,7 +971,6 @@ void
 elif type.is_optional_pointer():
 print "union ovsdb_atom 

[ovs-dev] [PATCH 07/15] ovsdb-idlc: Declare loop variables in for statements in generated code.

[Ben Pfaff]

This changes several instances of
size_t i;
for (i = 0; i < ...; i++)
into:
for (size_t i = 0; i < ...; i++)
in generated code, making it slightly more compact and easier to read.

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 19 +--
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 89bd8f9..6a2dc6b 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -356,11 +356,9 @@ static void
 valueVar = None
 
 if type.is_smap():
-print "size_t i;"
-print
 print "ovs_assert(inited);"
 print "smap_init(>%s);" % columnName
-print "for (i = 0; i < datum->n; i++) {"
+print "for (size_t i = 0; i < datum->n; i++) {"
 print "smap_add(>%s," % columnName
 print " datum->keys[i].string,"
 print " datum->values[i].string);"
@@ -390,14 +388,12 @@ static void
 nMax = "n"
 else:
 nMax = "datum->n"
-print "size_t i;"
-print
 print "ovs_assert(inited);"
 print "%s = NULL;" % keyVar
 if valueVar:
 print "%s = NULL;" % valueVar
 print "row->n_%s = 0;" % columnName
-print "for (i = 0; i < %s; i++) {" % nMax
+print "for (size_t i = 0; i < %s; i++) {" % nMax
 if type.key.ref_table:
 print """\
 struct %s%s *keyRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_%s, >keys[i].uuid));
@@ -760,7 +756,6 @@ const struct ovsdb_datum *
 print "datum.values = NULL;"
 txn_write_func = "ovsdb_idl_txn_write_clone"
 else:
-print "size_t i;"
 print
 print "ovs_assert(inited);"
 print "datum.n = %s;" % nVar
@@ -769,7 +764,7 @@ const struct ovsdb_datum *
 print "datum.values = xmalloc(%s * sizeof 
*datum.values);" % nVar
 else:
 print "datum.values = NULL;"
-print "for (i = 0; i < %s; i++) {" % nVar
+print "for (size_t i = 0; i < %s; i++) {" % nVar
 print "" + type.key.copyCValue("datum.keys[i].%s" % 
type.key.type.to_string(), "%s[i]" % keyVar)
 if type.value:
 print "" + 
type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), 
"%s[i]" % valueVar)
@@ -1003,8 +998,6 @@ void
 print "}"
 print "datum.values = NULL;"
 else:
-print "size_t i;"
-print
 print "ovs_assert(inited);"
 print "datum.n = %s;" % nVar
 print "datum.keys = %s ? xmalloc(%s * sizeof *datum.keys) 
: NULL;" % (nVar, nVar)
@@ -1014,7 +1007,7 @@ void
 free += ['datum.values']
 else:
 print "datum.values = NULL;"
-print "for (i = 0; i < %s; i++) {" % nVar
+print "for (size_t i = 0; i < %s; i++) {" % nVar
 print "" + 
type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % 
type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)
 if type.value:
 print "" + 
type.value.assign_c_value_casting_away_const("datum.values[i].%s" % 
type.value.type.to_string(), "%s[i]" % valueVar, refTable=False)
@@ -1162,8 +1155,6 @@ void
 print "}"
 print "datum.values = NULL;"
 else:
-print "size_t i;"
-print
 print "ovs_assert(inited);"
 print "datum.n = %s;" % nVar
 print "datum.keys = %s ? xmalloc(%s * sizeof *datum.keys) 
: NULL;" % (nVar, nVar)
@@ -1173,7 +1164,7 @@ void
 print "datum.values = xmalloc(%s * sizeof 
*datum.values);" % nVar
 else:
 print "datum.values = NULL;"
-print "for (i = 0; i < %s; i++) {" % nVar
+print "for (size_t i = 0; i < %s; i++) {" % nVar
 print "" + 
type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % 
type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)
 if type.value:
 print "" + 
type.value.assign_c_value_casting_away_const("datum.values[i].%s" % 
type.value.type.to_string(), "%s[i]" % valueVar, refTable=False)
-- 
2.1.3

___
dev mailing list

[ovs-dev] [PATCH 09/15] ovsdb-idlc: Eliminate _init() function from generated code.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 ovn/controller-vtep/ovn-controller-vtep.c |  3 --
 ovn/controller/ovn-controller.c   |  3 --
 ovn/northd/ovn-northd.c   |  3 --
 ovn/utilities/ovn-nbctl.c |  1 -
 ovn/utilities/ovn-sbctl.c |  1 -
 ovn/utilities/ovn-trace.c |  1 -
 ovsdb/ovsdb-idlc.in   | 87 +++---
 python/ovs/db/data.py | 54 +--
 python/ovs/db/types.py| 88 ++-
 python/ovs/ovsuuid.py | 11 ++--
 tests/test-ovsdb.c|  4 --
 utilities/ovs-vsctl.c |  1 -
 vswitchd/ovs-vswitchd.c   |  3 +-
 vtep/vtep-ctl.c   |  1 -
 14 files changed, 113 insertions(+), 148 deletions(-)

diff --git a/ovn/controller-vtep/ovn-controller-vtep.c 
b/ovn/controller-vtep/ovn-controller-vtep.c
index baee789..d73f624 100644
--- a/ovn/controller-vtep/ovn-controller-vtep.c
+++ b/ovn/controller-vtep/ovn-controller-vtep.c
@@ -76,9 +76,6 @@ main(int argc, char *argv[])
 
 daemonize_complete();
 
-vteprec_init();
-sbrec_init();
-
 /* Connect to VTEP database. */
 struct ovsdb_idl_loop vtep_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
 ovsdb_idl_create(vtep_remote, _idl_class, true, true));
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 00392ca..7e97cec 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -432,9 +432,6 @@ main(int argc, char *argv[])
 
 daemonize_complete();
 
-ovsrec_init();
-sbrec_init();
-
 ofctrl_init(_table);
 pinctrl_init();
 lflow_init();
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4668d9e..56c15c7 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -4744,9 +4744,6 @@ main(int argc, char *argv[])
 
 daemonize_complete();
 
-nbrec_init();
-sbrec_init();
-
 /* We want to detect (almost) all changes to the ovn-nb db. */
 struct ovsdb_idl_loop ovnnb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
 ovsdb_idl_create(ovnnb_db, _idl_class, true, true));
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index ad2d2f8..df1c405 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -99,7 +99,6 @@ main(int argc, char *argv[])
 fatal_ignore_sigpipe();
 vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
 vlog_set_levels_from_string_assert("reconnect:warn");
-nbrec_init();
 
 nbctl_cmd_init();
 
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index b894b8b..afc350a 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -97,7 +97,6 @@ main(int argc, char *argv[])
 fatal_ignore_sigpipe();
 vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
 vlog_set_levels_from_string_assert("reconnect:warn");
-sbrec_init();
 
 sbctl_cmd_init();
 
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index a35909f..788809e 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -76,7 +76,6 @@ main(int argc, char *argv[])
 service_start(, );
 fatal_ignore_sigpipe();
 vlog_set_levels_from_string_assert("reconnect:warn");
-sbrec_init();
 
 /* Parse command line. */
 parse_options(argc, argv);
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index f6f00b3..17b4453 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -283,7 +283,6 @@ bool %(s)s_is_updated(const struct %(s)s *, enum 
%(s)s_column_id);
 print "\nextern struct ovsdb_idl_table_class %stable_classes[%sN_TABLES];" 
% (prefix, prefix.upper())
 
 print "\nextern struct ovsdb_idl_class %sidl_class;" % prefix
-print "\nvoid %sinit(void);" % prefix
 
 print "\nconst char * %sget_db_version(void);" % prefix
 print "\n#endif /* %(prefix)sIDL_HEADER */" % {'prefix': prefix.upper()}
@@ -319,7 +318,6 @@ enum { sizeof_bool = 1 };
 enum { sizeof_bool = sizeof(bool) };
 #endif
 
-static bool inited;
 ''' % schema.idlHeader
 
 # Cast functions.
@@ -345,7 +343,6 @@ static struct %(s)s *
 static void
 %(s)s_parse_%(c)s(struct ovsdb_idl_row *row_, const struct ovsdb_datum *datum)
 {
-ovs_assert(inited);
 struct %(s)s *row = %(s)s_cast(row_);''' % {'s': structName,
 'c': columnName}
 type = column.type
@@ -447,9 +444,8 @@ static void
 static void
 %(s)s_unparse_%(c)s(struct ovsdb_idl_row *row_)
 {
-struct %(s)s *row = %(s)s_cast(row_);
-
-ovs_assert(inited);''' % {'s': structName, 'c': columnName}
+struct %(s)s *row = %(s)s_cast(row_);''' % {'s': structName,
+'c': columnName}
 
 if type.is_smap():
 print "smap_destroy(>%s);" % columnName
@@ -614,7 +610,6 @@ bool
 void
 

[ovs-dev] [PATCH 04/15] ovsdb-idlc: Simplify code generation to parse sets and maps of references.

[Ben Pfaff]

This switches from code that looks like:
if (keyRow) {
...
}
to:
if (!keyRow) {
continue;
}
...
which is a little easier to generate because the indentation of ... is
constant.

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 42 +-
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 0031636..90a5ddb 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -398,25 +398,27 @@ static void
 print "%s = NULL;" % valueVar
 print "row->n_%s = 0;" % columnName
 print "for (i = 0; i < %s; i++) {" % nMax
-refs = []
 if type.key.ref_table:
-print "struct %s%s *keyRow = 
%s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_classes[%sTABLE_%s], 
>keys[i].uuid));" % (prefix, type.key.ref_table.name.lower(), prefix, 
type.key.ref_table.name.lower(), prefix, prefix.upper(), 
type.key.ref_table.name.upper())
+print """\
+struct %s%s *keyRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_classes[%sTABLE_%s], >keys[i].uuid));
+if (!keyRow) {
+continue;
+}\
+""" % (prefix, type.key.ref_table.name.lower(), prefix, 
type.key.ref_table.name.lower(), prefix, prefix.upper(), 
type.key.ref_table.name.upper())
 keySrc = "keyRow"
-refs.append('keyRow')
 else:
 keySrc = "datum->keys[i].%s" % type.key.type.to_string()
 if type.value and type.value.ref_table:
-print "struct %s%s *valueRow = 
%s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_classes[%sTABLE_%s], 
>values[i].uuid));" % (prefix, type.value.ref_table.name.lower(), 
prefix, type.value.ref_table.name.lower(), prefix, prefix.upper(), 
type.value.ref_table.name.upper())
+print """\
+struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_classes[%sTABLE_%s], >values[i].uuid));
+if (!valueRow) {
+continue;
+}\
+""" % (prefix, type.value.ref_table.name.lower(), prefix, 
type.value.ref_table.name.lower(), prefix, prefix.upper(), 
type.value.ref_table.name.upper())
 valueSrc = "valueRow"
-refs.append('valueRow')
 elif valueVar:
 valueSrc = "datum->values[i].%s" % 
type.value.type.to_string()
-if refs:
-print "if (%s) {" % ' && '.join(refs)
-indent = ""
-else:
-indent = ""
-print "%sif (!row->n_%s) {" % (indent, columnName)
+print "if (!row->n_%s) {" % (columnName)
 
 # Special case for boolean types.  This is only here because
 # sparse does not like the "normal" case ("warning: expression
@@ -425,23 +427,21 @@ static void
 sizeof = "sizeof_bool"
 else:
 sizeof = "sizeof *%s" % keyVar
-print "%s%s = xmalloc(%s * %s);" % (indent, keyVar, nMax,
-sizeof)
+print "%s = xmalloc(%s * %s);" % (keyVar, nMax,
+  sizeof)
 if valueVar:
 # Special case for boolean types (see above).
 if type.value.type == ovs.db.types.BooleanType:
 sizeof = " * sizeof_bool"
 else:
 sizeof = "sizeof *%s" % valueVar
-print "%s%s = xmalloc(%s * %s);" % (indent, valueVar,
-nMax, sizeof)
-print "%s}" % indent
-print "%s%s[row->n_%s] = %s;" % (indent, keyVar, columnName, 
keySrc)
+print "%s = xmalloc(%s * %s);" % (valueVar,
+  nMax, sizeof)
+print "}"
+print "%s[row->n_%s] = %s;" % (keyVar, columnName, 
keySrc)
 if valueVar:
-print "%s%s[row->n_%s] = %s;" % (indent, valueVar, 
columnName, valueSrc)
-print "%srow->n_%s++;" % (indent, columnName)
-if refs:
-print "}"
+print "%s[row->n_%s] = %s;" % (valueVar, 
columnName, valueSrc)
+print "row->n_%s++;" % columnName
 print "}"
 print "}"
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 05/15] ovsdb-idlc: Make generated references to table classes easier to read.

[Ben Pfaff]

This replaces _table_classes[OVSREC_TABLE_OPEN_VSWITCH] by the
easier to read and equivalent _table_open_vswitch in generated code.

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 57 +++--
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 90a5ddb..73e7cf8 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -372,13 +372,13 @@ static void
 if not type.key.ref_table:
 print "%s = datum->keys[0].%s;" % (keyVar, 
type.key.type.to_string())
 else:
-print "%s = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_classes[%sTABLE_%s], >keys[0].uuid));" % (keyVar, prefix, 
type.key.ref_table.name.lower(), prefix, prefix.upper(), 
type.key.ref_table.name.upper())
+print "%s = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_%s, >keys[0].uuid));" % (keyVar, prefix, 
type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower())
 
 if valueVar:
 if not type.value.ref_table:
 print "%s = datum->values[0].%s;" % (valueVar, 
type.value.type.to_string())
 else:
-print "%s = 
%s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_classes[%sTABLE_%s], 
>values[0].uuid));" % (valueVar, prefix, 
type.value.ref_table.name.lower(), prefix, prefix.upper(), 
type.value.ref_table.name.upper())
+print "%s = 
%s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, >values[0].uuid));" % 
(valueVar, prefix, type.value.ref_table.name.lower(), prefix, 
type.value.ref_table.name.lower())
 print "} else {"
 print "%s" % type.key.initCDefault(keyVar, type.n_min 
== 0)
 if valueVar:
@@ -400,21 +400,21 @@ static void
 print "for (i = 0; i < %s; i++) {" % nMax
 if type.key.ref_table:
 print """\
-struct %s%s *keyRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_classes[%sTABLE_%s], >keys[i].uuid));
+struct %s%s *keyRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_%s, >keys[i].uuid));
 if (!keyRow) {
 continue;
 }\
-""" % (prefix, type.key.ref_table.name.lower(), prefix, 
type.key.ref_table.name.lower(), prefix, prefix.upper(), 
type.key.ref_table.name.upper())
+""" % (prefix, type.key.ref_table.name.lower(), prefix, 
type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower())
 keySrc = "keyRow"
 else:
 keySrc = "datum->keys[i].%s" % type.key.type.to_string()
 if type.value and type.value.ref_table:
 print """\
-struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_classes[%sTABLE_%s], >values[i].uuid));
+struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, 
&%stable_%s, >values[i].uuid));
 if (!valueRow) {
 continue;
 }\
-""" % (prefix, type.value.ref_table.name.lower(), prefix, 
type.value.ref_table.name.lower(), prefix, prefix.upper(), 
type.value.ref_table.name.upper())
+""" % (prefix, type.value.ref_table.name.lower(), prefix, 
type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower())
 valueSrc = "valueRow"
 elif valueVar:
 valueSrc = "datum->values[i].%s" % 
type.value.type.to_string()
@@ -505,7 +505,7 @@ void
 const struct %(s)s *
 %(s)s_get_for_uuid(const struct ovsdb_idl *idl, const struct uuid *uuid)
 {
-return %(s)s_cast(ovsdb_idl_get_row_for_uuid(idl, 
&%(p)stable_classes[%(P)sTABLE_%(T)s], uuid));
+return %(s)s_cast(ovsdb_idl_get_row_for_uuid(idl, &%(p)stable_%(tl)s, 
uuid));
 }
 
 /* Returns a row in table "%(t)s" in 'idl', or a null pointer if that
@@ -517,7 +517,7 @@ const struct %(s)s *
 const struct %(s)s *
 %(s)s_first(const struct ovsdb_idl *idl)
 {
-return %(s)s_cast(ovsdb_idl_first_row(idl, 
&%(p)stable_classes[%(P)sTABLE_%(T)s]));
+return %(s)s_cast(ovsdb_idl_first_row(idl, &%(p)stable_%(tl)s));
 }
 
 /* Returns a row following 'row' within its table, or a null pointer if 'row'
@@ -530,7 +530,7 @@ const struct %(s)s *
 
 unsigned int %(s)s_get_seqno(const struct ovsdb_idl *idl)
 {
-return ovsdb_idl_table_get_seqno(idl, 
&%(p)stable_classes[%(P)sTABLE_%(T)s]);
+return ovsdb_idl_table_get_seqno(idl, &%(p)stable_%(tl)s);
 }
 
 unsigned int %(s)s_row_get_seqno(const struct %(s)s *row, enum 
ovsdb_idl_change change)
@@ -541,7 +541,7 @@ unsigned int %(s)s_row_get_seqno(const struct %(s)s *row, 
enum ovsdb_idl_change
 const struct %(s)s *
 %(s)s_track_get_first(const struct ovsdb_idl *idl)
 {
-return %(s)s_cast(ovsdb_idl_track_get_first(idl, 
&%(p)stable_classes[%(P)sTABLE_%(T)s]));
+

[ovs-dev] [PATCH 06/15] ovsdb-idlc: Make generated references to columns easier to read.

[Ben Pfaff]

This replaces ovsrec_open_vswitch_columns[OVSREC_OPEN_VSWITCH_COL_CUR_CFG]
by the easier to read and equivalent ovsrec_open_vswitch_col_cur_cfg in
generated code.

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 66 +++--
 1 file changed, 28 insertions(+), 38 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 73e7cf8..89bd8f9 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -621,7 +621,7 @@ void
 %(s)s_verify_%(c)s(const struct %(s)s *row)
 {
 ovs_assert(inited);
-ovsdb_idl_txn_verify(>header_, &%(s)s_columns[%(S)s_COL_%(C)s]);
+ovsdb_idl_txn_verify(>header_, &%(s)s_col_%(c)s);
 }''' % {'s': structName,
 'S': structName.upper(),
 'c': columnName,
@@ -688,7 +688,7 @@ const struct ovsdb_datum *
 ovsdb_datum_init_empty();
 }
 ovsdb_idl_txn_write(>header_,
-&%(s)s_columns[%(S)s_COL_%(C)s],
+&%(s)s_col_%(c)s,
 );
 }
 """ % {'t': tableName,
@@ -781,11 +781,11 @@ const struct ovsdb_datum *
 print "ovsdb_datum_sort_unique(, %s, %s);" % (
 type.key.toAtomicType(), valueType)
 txn_write_func = "ovsdb_idl_txn_write"
-print "%(f)s(>header_, &%(s)s_columns[%(S)s_COL_%(C)s], 
);" \
+print "%(f)s(>header_, &%(s)s_col_%(c)s, );" \
 % {'f': txn_write_func,
's': structName,
'S': structName.upper(),
-   'C': columnName.upper()}
+   'c': columnName}
 print "}"
 # Update/Delete of partial map column functions
 for columnName, column in sorted_columns(table):
@@ -815,11 +815,10 @@ void
 print ""+ type.value.copyCValue("datum->values[0].%s" % 
type.value.type.to_string(), "new_value")
 print '''
 ovsdb_idl_txn_write_partial_map(>header_,
-&%(s)s_columns[%(S)s_COL_%(C)s],
+&%(s)s_col_%(c)s,
 datum);
-}''' % {'s': structName, 'c': 
columnName,'coltype':column.type.key.to_const_c_type(prefix),
-'valtype':column.type.value.to_const_c_type(prefix), 'S': 
structName.upper(),
-'C': columnName.upper()}
+}''' % {'s': structName, 'c': 
columnName,'coltype':column.type.key.toCType(prefix),
+'valtype':column.type.value.to_const_c_type(prefix), 'S': 
structName.upper()}
 print '''
 /* Deletes an element of the "%(c)s" map column from the "%(t)s" table in 'row'
  * given the key value 'delete_key'.
@@ -843,11 +842,10 @@ void
 print ""+ type.key.copyCValue("datum->keys[0].%s" % 
type.key.type.to_string(), "delete_key")
 print '''
 ovsdb_idl_txn_delete_partial_map(>header_,
-&%(s)s_columns[%(S)s_COL_%(C)s],
+&%(s)s_col_%(c)s,
 datum);
-}''' % {'s': structName, 'c': 
columnName,'coltype':column.type.key.to_const_c_type(prefix),
-'valtype':column.type.value.to_const_c_type(prefix), 'S': 
structName.upper(),
-'C': columnName.upper()}
+}''' % {'s': structName, 'c': 
columnName,'coltype':column.type.key.toCType(prefix),
+'valtype':column.type.value.to_const_c_type(prefix), 'S': 
structName.upper()}
 # End Update/Delete of partial maps
 # Update/Delete of partial set column functions
 if type.is_set():
@@ -873,11 +871,10 @@ void
 print ""+ type.key.copyCValue("datum->keys[0].%s" % 
type.key.type.to_string(), "new_value")
 print '''
 ovsdb_idl_txn_write_partial_set(>header_,
-&%(s)s_columns[%(S)s_COL_%(C)s],
+&%(s)s_col_%(c)s,
 datum);
-}''' % {'s': structName, 'c': 
columnName,'coltype':column.type.key.to_const_c_type(prefix),
-'valtype':column.type.key.to_const_c_type(prefix), 'S': 
structName.upper(),
-'C': columnName.upper()}
+}''' % {'s': structName, 'c': 
columnName,'coltype':column.type.key.toCType(prefix),
+'valtype':column.type.key.to_const_c_type(prefix), 'S': 
structName.upper()}
 print '''
 /* Deletes the value 'delete_value' from the "%(c)s" set column from the
  * "%(t)s" table in 'row'.
@@ -901,11 +898,10 @@ void
 print ""+ type.key.copyCValue("datum->keys[0].%s" % 
type.key.type.to_string(), "delete_value")
 print '''
 ovsdb_idl_txn_delete_partial_set(>header_,
-&%(s)s_columns[%(S)s_COL_%(C)s],
+&%(s)s_col_%(c)s,
 datum);
-}''' % {'s': structName, 'c': 

[ovs-dev] [PATCH 01/15] ovsdb-idlc: Use ovsdb_datum_from_smap() instead of open-coding it.

[Ben Pfaff]

There's no reason to have three copies of this code for every smap-type
column.

The code wasn't a perfect match for ovsdb_datum_from_smap(), so this commit
also changes ovsdb_datum_from_smap() to better suit it.  It only had one
caller and the new design is adequate for that caller.

Signed-off-by: Ben Pfaff 
---
 lib/ovsdb-data.c| 21 +
 lib/ovsdb-data.h|  4 ++--
 ovsdb/ovsdb-idlc.in | 45 +++--
 vswitchd/bridge.c   |  1 +
 4 files changed, 15 insertions(+), 56 deletions(-)

diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
index 266a3e4..0dda73a 100644
--- a/lib/ovsdb-data.c
+++ b/lib/ovsdb-data.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2014, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1542,27 +1542,24 @@ ovsdb_datum_to_bare(const struct ovsdb_datum *datum,
 }
 }
 
-/* Initializes 'datum' as a string-to-string map whose contents are taken from
- * 'smap'.  Destroys 'smap'. */
+/* Initializes 'datum' as a string-to-string map whose contents are copied from
+ * 'smap', which is not modified. */
 void
-ovsdb_datum_from_smap(struct ovsdb_datum *datum, struct smap *smap)
+ovsdb_datum_from_smap(struct ovsdb_datum *datum, const struct smap *smap)
 {
-struct smap_node *node, *next;
-size_t i;
-
 datum->n = smap_count(smap);
 datum->keys = xmalloc(datum->n * sizeof *datum->keys);
 datum->values = xmalloc(datum->n * sizeof *datum->values);
 
-i = 0;
-SMAP_FOR_EACH_SAFE (node, next, smap) {
-smap_steal(smap, node,
-   >keys[i].string, >values[i].string);
+struct smap_node *node;
+size_t i = 0;
+SMAP_FOR_EACH (node, smap) {
+datum->keys[i].string = xstrdup(node->key);
+datum->values[i].string = xstrdup(node->value);
 i++;
 }
 ovs_assert(i == datum->n);
 
-smap_destroy(smap);
 ovsdb_datum_sort_unique(datum, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
 }
 
diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 98633ef..ae2672e 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2015 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -178,7 +178,7 @@ void ovsdb_datum_to_string(const struct ovsdb_datum *,
 void ovsdb_datum_to_bare(const struct ovsdb_datum *,
  const struct ovsdb_type *, struct ds *);
 
-void ovsdb_datum_from_smap(struct ovsdb_datum *, struct smap *);
+void ovsdb_datum_from_smap(struct ovsdb_datum *, const struct smap *);
 
 /* Comparison. */
 uint32_t ovsdb_datum_hash(const struct ovsdb_datum *,
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 0037b90..fb2241b 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -678,20 +678,7 @@ const struct ovsdb_datum *
 
 ovs_assert(inited);
 if (%(c)s) {
-struct smap_node *node;
-size_t i;
-
-datum.n = smap_count(%(c)s);
-datum.keys = xmalloc(datum.n * sizeof *datum.keys);
-datum.values = xmalloc(datum.n * sizeof *datum.values);
-
-i = 0;
-SMAP_FOR_EACH (node, %(c)s) {
-datum.keys[i].string = xstrdup(node->key);
-datum.values[i].string = xstrdup(node->value);
-i++;
-}
-ovsdb_datum_sort_unique(, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
+ovsdb_datum_from_smap(, %(c)s);
 } else {
 ovsdb_datum_init_empty();
 }
@@ -932,20 +919,7 @@ void
 
 ovs_assert(inited);
 if (%(c)s) {
-struct smap_node *node;
-size_t i;
-
-datum.n = smap_count(%(c)s);
-datum.keys = xmalloc(datum.n * sizeof *datum.keys);
-datum.values = xmalloc(datum.n * sizeof *datum.values);
-
-i = 0;
-SMAP_FOR_EACH (node, %(c)s) {
-datum.keys[i].string = xstrdup(node->key);
-datum.values[i].string = xstrdup(node->value);
-i++;
-}
-ovsdb_datum_sort_unique(, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
+ovsdb_datum_from_smap(, %(c)s);
 } else {
 ovsdb_datum_init_empty();
 }
@@ -1107,20 +1081,7 @@ void
 
 ovs_assert(inited);
 if (%(c)s) {
-struct smap_node *node;
-size_t i;
-
-datum.n = smap_count(%(c)s);
-datum.keys = xmalloc(datum.n * sizeof *datum.keys);
-datum.values = xmalloc(datum.n * sizeof *datum.values);
-
-i = 0;
-SMAP_FOR_EACH (node, %(c)s) {
-datum.keys[i].string = xstrdup(node->key);
-datum.values[i].string = xstrdup(node->value);
-i++;
-}
-ovsdb_datum_sort_unique(, 

[ovs-dev] [PATCH 03/15] ovsdb-idlc: Factor out sorting columns.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.in | 37 -
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 2900bd9..0031636 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -117,6 +117,9 @@ def cMembers(prefix, tableName, columnName, column, const, 
refTable=True):
 
 return (comment, members)
 
+def sorted_columns(table):
+return sorted(table.columns.items())
+
 def printCIDLHeader(schemaFile):
 schema = parseSchema(schemaFile)
 prefix = schema.idlPrefix
@@ -141,7 +144,7 @@ def printCIDLHeader(schemaFile):
 print "/* %s table. */" % tableName
 print "struct %s {" % structName
 print "\tstruct ovsdb_idl_row header_;"
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 print "\n\t/* %s column. */" % columnName
 comment, members = cMembers(prefix, tableName,
 columnName, column, False)
@@ -151,7 +154,7 @@ def printCIDLHeader(schemaFile):
 
 # Column indexes.
 printEnum("%s_column_id" % structName.lower(), ["%s_COL_%s" % 
(structName.upper(), columnName.upper())
-   for columnName in sorted(table.columns)]
+for columnName, column 
in sorted_columns(table)]
   + ["%s_N_COLUMNS" % structName.upper()])
 
 print
@@ -204,11 +207,11 @@ struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *);
 bool %(s)s_is_updated(const struct %(s)s *, enum %(s)s_column_id);
 ''' % {'s': structName, 'S': structName.upper()}
 
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 print 'void %(s)s_verify_%(c)s(const struct %(s)s *);' % {'s': 
structName, 'c': columnName}
 
 print
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 if column.type.value:
 valueParam = ', enum ovsdb_atomic_type value_type'
 else:
@@ -217,7 +220,7 @@ bool %(s)s_is_updated(const struct %(s)s *, enum 
%(s)s_column_id);
 's': structName, 'c': columnName, 'v': valueParam}
 
 print
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 print 'void %(s)s_set_%(c)s(const struct %(s)s *,' % {'s': 
structName, 'c': columnName},
 if column.type.is_smap():
 args = ['const struct smap *']
@@ -228,7 +231,7 @@ bool %(s)s_is_updated(const struct %(s)s *, enum 
%(s)s_column_id);
 print '%s);' % ', '.join(args)
 
 print
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 if column.type.is_map():
 print 'void %(s)s_update_%(c)s_setkey(const struct %(s)s *, ' 
% {'s': structName, 'c': columnName},
 print '%(coltype)s, %(valtype)s);' % 
{'coltype':column.type.key.to_const_c_type(prefix), 
'valtype':column.type.value.to_const_c_type(prefix)}
@@ -253,7 +256,7 @@ bool %(s)s_is_updated(const struct %(s)s *, enum 
%(s)s_column_id);
 print 'void %s_add_clause_false(struct ovsdb_idl *idl);' % structName
 
 print
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 print 'void %(s)s_remove_clause_%(c)s(struct ovsdb_idl *idl, enum 
ovsdb_function function,' % {'s': structName, 'c': columnName},
 if column.type.is_smap():
 args = ['const struct smap *']
@@ -337,7 +340,7 @@ static struct %(s)s *
 print "/* %s table. */" % (tableName)
 
 # Parse functions.
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 print '''
 static void
 %(s)s_parse_%(c)s(struct ovsdb_idl_row *row_, const struct ovsdb_datum *datum)
@@ -443,7 +446,7 @@ static void
 print "}"
 
 # Unparse functions.
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 type = column.type
 if type.is_smap() or (type.n_min != 1 or type.n_max != 1) and not 
type.is_optional_pointer():
 print '''
@@ -490,7 +493,7 @@ void
 %(s)s_init(struct %(s)s *row)
 {
 memset(row, 0, sizeof *row); """ % {'s': structName, 't': tableName}
-for columnName, column in sorted(table.columns.iteritems()):
+for columnName, column in sorted_columns(table):
 if column.type.is_smap():
 print "smap_init(>%s);" % columnName
 print "}"
@@ -587,7 

[ovs-dev] [PATCH 02/15] ovsdb-idlc: Remove obsolete documentation and usage.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 ovsdb/ovsdb-idlc.1  | 5 -
 ovsdb/ovsdb-idlc.in | 1 -
 2 files changed, 6 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.1 b/ovsdb/ovsdb-idlc.1
index 4a33261..b44757b 100644
--- a/ovsdb/ovsdb-idlc.1
+++ b/ovsdb/ovsdb-idlc.1
@@ -56,11 +56,6 @@ defines a structure for each table defined by the schema.
 Reads \fIidl\fR and prints on standard output a C source file that
 implements C bindings for the database defined by the schema.
 .
-.IP "\fBdoc\fI idl\fR"
-Reads \fIidl\fR and prints on standard output a text file that
-documents the schema.  The output may have very long lines, so it
-makes sense to pipe it through, e.g. \fBfmt \-s\fR.
-.
 .SS "Options"
 .so lib/common.man
 .
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index fb2241b..2900bd9 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -1329,7 +1329,6 @@ The following commands are supported:
   annotate SCHEMA ANNOTATIONS print SCHEMA combined with ANNOTATIONS
   c-idl-header IDLprint C header file for IDL
   c-idl-source IDLprint C source file for IDL implementation
-  nroff IDL   print schema documentation in nroff format
 
 The following options are also available:
   -h, --help  display this help message
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] tests: Get rid of overly specific --pidfile and --unixctl options.

[Ben Pfaff]

At an early point in OVS development, OVS was built with fixed default
directories for pidfiles and sockets.  This meant that it was necessary to
use lots of --pidfile and --unixctl options in the testsuite, to point the
daemons to where they should put these files (since the testsuite cannot
and generally should not touch the real system /var/run).  Later on,
the environment variables OVS_RUNDIR, OVS_LOGDIR, etc. were introduced
to override these defaults, and even later the testsuite was changed to
always set these variables correctly in every test.  Thus, these days it
isn't usually necessary to specify a filename on --pidfile or to specify
--unixctl at all.  However, many of the tests are built by cut-and-paste,
so they tended to keep appearing anyhow.  This commit drops most of them,
making the testsuite easier to read and understand.

This commit also sweeps away some other historical detritus.  In
particular, in early days of the testsuite there was no way to
automatically kill daemons when a test failed (or otherwise ended).  This
meant that some tests were littered with calls to "kill `cat pidfile`" on
almost every line (or m4 macros that expanded to the same thing) so that if
a test failed partway through the testsuite would not hang waiting for a
daemon to die that was never going to die without manual intervention.
However, a long time ago we introduced the "on_exit" mechanism that
obsoletes this.  This commit eliminates a lot of the old litter of kill
invocations, which also makes those tests easier to read.

Signed-off-by: Ben Pfaff 
---
 tests/daemon-py.at   | 128 +++
 tests/daemon.at  | 120 +++-
 tests/jsonrpc-py.at  |  41 --
 tests/jsonrpc.at |  42 +-
 tests/ofproto.at |  10 ++--
 tests/ovn-controller-vtep.at |   6 +-
 tests/ovn-sbctl.at   |   4 +-
 tests/ovs-vsctl.at   |  27 -
 tests/ovs-vswitchd.at|  20 +++
 tests/ovs-xapi-sync.at   |   2 +-
 tests/ovsdb-idl.at   |  88 +++--
 tests/ovsdb-monitor.at   |  46 +++-
 tests/ovsdb-server.at|  62 ++---
 tests/ovsdb-tool.at  |   4 +-
 tests/vtep-ctl.at|   2 +-
 15 files changed, 262 insertions(+), 340 deletions(-)

diff --git a/tests/daemon-py.at b/tests/daemon-py.at
index a32762b..d0e65ad 100644
--- a/tests/daemon-py.at
+++ b/tests/daemon-py.at
@@ -10,7 +10,7 @@ m4_define([DAEMON_PYN],
AT_CAPTURE_FILE([expected])
# Start the daemon and wait for the pidfile to get created
# and that its contents are the correct pid.
-   AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=`pwd`/pid& echo $! > 
expected], [0])
+   AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=pid& echo $! > expected], [0])
OVS_WAIT_UNTIL([test -s pid], [kill `cat expected`])
AT_CHECK(
  [pid=`cat pid` && expected=`cat expected` && test "$pid" = "$expected"],
@@ -35,40 +35,35 @@ m4_define([DAEMON_MONITOR_PYN],
AT_CAPTURE_FILE([parentpid])
AT_CAPTURE_FILE([newpid])
# Start the daemon and wait for the pidfile to get created.
-   AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=`pwd`/pid --monitor& echo $! 
> parent], [0])
-   OVS_WAIT_UNTIL([test -s pid], [kill `cat parent`])
+   AT_CHECK([$3 $srcdir/test-daemon.py --pidfile=pid --monitor& echo $! > 
parent], [0])
+   on_exit 'kill `cat parent`'
+   OVS_WAIT_UNTIL([test -s pid])
# Check that the pidfile names a running process,
# and that the parent process of that process is our child process.
-   AT_CHECK([kill -0 `cat pid`], [0], [], [], [kill `cat parent`])
-   AT_CHECK([parent_pid `cat pid` > parentpid],
- [0], [], [], [kill `cat parent`])
+   AT_CHECK([kill -0 `cat pid`])
+   AT_CHECK([parent_pid `cat pid` > parentpid])
AT_CHECK(
  [parentpid=`cat parentpid` &&
   parent=`cat parent` &&
-  test $parentpid = $parent],
- [0], [], [], [kill `cat parent`])
+  test $parentpid = $parent])
# Kill the daemon process, making it look like a segfault,
# and wait for a new child process to get spawned.
-   AT_CHECK([cp pid oldpid], [0], [], [], [kill `cat parent`])
-   AT_CHECK([kill -SEGV `cat pid`], [0], [], [ignore], [kill `cat parent`])
-   OVS_WAIT_WHILE([kill -0 `cat oldpid`], [kill `cat parent`])
-   OVS_WAIT_UNTIL([test -s pid && test `cat pid` != `cat oldpid`],
- [kill `cat parent`])
-   AT_CHECK([cp pid newpid], [0], [], [], [kill `cat parent`])
+   AT_CHECK([cp pid oldpid])
+   AT_CHECK([kill -SEGV `cat pid`], [0], [], [ignore])
+   OVS_WAIT_WHILE([kill -0 `cat oldpid`])
+   OVS_WAIT_UNTIL([test -s pid && test `cat pid` != `cat oldpid`])
+   AT_CHECK([cp pid newpid])
# Check that the pidfile names a running process,
# and that the parent process of that process is our child process.
-   AT_CHECK([parent_pid `cat pid` > parentpid],
- [0], [], [], [kill 

Re: [ovs-dev] [PATCH] json: Fix non-static json char lookup table.

[Ben Pfaff]

On Thu, Oct 06, 2016 at 11:33:39AM +0900, Joe Stringer wrote:
> This warning breaks the build on travis:
> lib/json.c:1627:12: error: symbol 'chars_escaping' was not declared.
> Should it be static?
> 
> CC: Esteban Rodriguez Betancourt 
> Reported-At: https://travis-ci.org/openvswitch/ovs/jobs/165300417
> Fixes: 644ecb10a661 ("json: Serialize strings using a lookup table")
> Signed-off-by: Joe Stringer 

I noticed this earlier too and forgot to send a patch.  Thanks!

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] json: Fix non-static json char lookup table.

[Joe Stringer]

This warning breaks the build on travis:
lib/json.c:1627:12: error: symbol 'chars_escaping' was not declared.
Should it be static?

CC: Esteban Rodriguez Betancourt 
Reported-At: https://travis-ci.org/openvswitch/ovs/jobs/165300417
Fixes: 644ecb10a661 ("json: Serialize strings using a lookup table")
Signed-off-by: Joe Stringer 
---
 lib/json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/json.c b/lib/json.c
index e052a6a0af0d..40c8f718db72 100644
--- a/lib/json.c
+++ b/lib/json.c
@@ -1624,7 +1624,7 @@ json_serialize_array(const struct json_array *array, 
struct json_serializer *s)
 ds_put_char(ds, ']');
 }
 
-const char *chars_escaping[256] = {
+static const char *chars_escaping[256] = {
 "\\u", "\\u0001", "\\u0002", "\\u0003", "\\u0004", "\\u0005", 
"\\u0006", "\\u0007",
 "\\b", "\\t", "\\n", "\\u000b", "\\f", "\\r", "\\u000e", "\\u000f",
 "\\u0010", "\\u0011", "\\u0012", "\\u0013", "\\u0014", "\\u0015", 
"\\u0016", "\\u0017",
-- 
2.9.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH ovs RFC 0/9] Introducing HW offload support for openvswitch

[Joe Stringer]

On 27 September 2016 at 21:45, Paul Blakey  wrote:
> Openvswitch currently configures the kerenel datapath via netlink over an 
> internal ovs protocol.
>
> This patch series offers a new provider: dpif-netlink-tc that uses the tc 
> flower protocol
> to offload ovs rules into HW data-path through netdevices that e.g represent 
> NIC e-switch ports.
>
> The user can create a bridge with type: datapath_type=dpif-hw-netlink in 
> order to use this provider.
> This provider can be used to pass the tc flower rules to the HW for HW 
> offloads.
>
> Also introducing in this patch series a policy module in which the user can 
> program a HW-offload
> policy. The policy module accept a ovs flow and returns a policy decision for 
> each
> flow:NO_OFFLOAD or HW_ONLY -- currently the policy is to HW offload all rules.
>
> If the HW_OFFLOAD rule assignment fails the provider will fallback to the 
> system datapath.
>
> Flower was chosen b/c its sort of natural to state OVS DP rules for this 
> classifier. However,
> the code can be extended to support other classifiers such as U32, eBPF, etc 
> which have
> HW offloads as well.
>
> The use-case we are currently addressing is the newly introduced SRIOV 
> switchdev mode in the
> Linux kernel which is introduced in version 4.8 [1][2]. This series was 
> tested against SRIOV VFs
> vports representors of the Mellanox 100G ConnectX-4 series exposed by the 
> mlx5 kernel driver.
>
> Paul and Shahar.
>
> [1] 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=513334e18a74f70c0be58c2eb73af1715325b870
> [2] 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=53d94892e27409bb2b48140207c0273b2ba65f61

Thanks for submitting the series. Clearly this is a topic of interest
for multiple parties, and it's a good starting point to discuss.

A few of us also discussed this topic today at netdev, so I'll list a
few points that we talked about and hopefully others can fill in the
bits I miss.

Positives
* Hardware offload decision is made in a module in userspace
* Layered dpif approach means that the tc-based hardware offload could
sit in front of kernel or userspace datapaths
* Separate dpif means that if you don't enable it, it doesn't affect
you. Doesn't litter another dpif implementation with offload logic.

Drawbacks
* Additional dpif to maintain. Another implementation to change when
modifying dpif interface. Maybe this doesn't change too often, but
there has been some discussions recently about whether the
flow_{put,get,del} should be converted to use internal flow structures
rather than OVS netlink representation. This is one example of
potential impact on development.
* Fairly limited support for OVS matches and actions. For instance, it
is not yet useful for OVN-style pipeline. But that's not a limitation
of the design, just the current implementation.

Other considerations
* Is tc flower filter setup rate and stats dump fast enough? How does
it compare to existing kernel datapath flow setup rate? Multiple
threads inserting at once? How many filters can be dumped per second?
etc.
* Currently for a given flow, it will exist in either the offloaded
implementation or the kernel datapath. Statistics are only drawn from
one location. This is consistent with how ofproto-dpif-upcall will
insert flows - one flow_put operation and one flow is inserted into
the datapath. Correspondingly there is one udpif_key which reflects
the most recently used stats for this datapath flow. There may be
situations where flows need to be in both datapaths, in which case
there either needs to be either one udpif_key per datapath
representation of the flow, or the dpif must hide the second flow and
aggregate stats.

Extra, not previously discussed
* Testing - we may want a mode where tc flower is used in software
mode, to test the tc netlink interface. It would be good to see
extension of kernel module testsuite to at least test some basics of
the interface, perhaps also the flower behaviour (though that may be
out of scope of the testsuite in the OVS tree).

Thanks,
Joe
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovs-dev,v2,2/4] ovn-controller: add quiet mode

[Ryan Moats]

"dev"  wrote on 10/05/2016 01:19:34 PM:

> From: Ryan Moats/Omaha/IBM@IBMUS
> To: Ben Pfaff 
> Cc: dev@openvswitch.org
> Date: 10/05/2016 01:20 PM
> Subject: Re: [ovs-dev] [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> Sent by: "dev" 
>
> Ben Pfaff  wrote on 10/05/2016 01:04:36 PM:
>
> > From: Ben Pfaff 
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: dev@openvswitch.org
> > Date: 10/05/2016 01:04 PM
> > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> >
> > On Wed, Oct 05, 2016 at 12:52:57PM -0500, Ryan Moats wrote:
> > > Ben Pfaff  wrote on 10/05/2016 12:37:26 PM:
> > >
> > > > From: Ben Pfaff 
> > > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > > Cc: dev@openvswitch.org
> > > > Date: 10/05/2016 12:37 PM
> > > > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> > > >
> > > > On Tue, Oct 04, 2016 at 05:11:37PM -0500, Ryan Moats wrote:
> > > > > Ben Pfaff  wrote on 10/04/2016 12:14:32 PM:
> > > > >
> > > > > > From: Ben Pfaff 
> > > > > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > > > > Cc: dev@openvswitch.org
> > > > > > Date: 10/04/2016 12:14 PM
> > > > > > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> > > > > >
> > > > > > On Wed, Aug 31, 2016 at 03:22:44PM +, Ryan Moats wrote:
> > > > > > > As discussed in [1], what the incremental processing code
> > > > > > > actually accomplished was that the ovn-controller would
> > > > > > > be "quiet" and not burn CPU when things weren't changing.
> > > > > > > This patch set recreates this state by calculating whether
> > > > > > > changes have occured that would require a full calculation
> > > > > > > to be performed.  It does this by persisting a copy of
> > > > > > > the localvif_to_ofport and tunnel information in the
> > > > > > > controller module, rather than in the physical.c module
> > > > > > > as was the case with previous commits.
> > > > > > >
> > > > > > > [1]
> http://openvswitch.org/pipermail/dev/2016-August/078272.html
> > > > > > >
> > > > > > > Signed-off-by: Ryan Moats 
> > > > > >
> > > > > > Hi Ryan.
> > > > > >
> > > > > > I like the idea behind this patch.  However, it no longer
applies
> to
> > > > > > master, so it needs a rebase.
> > > > >
> > > > > So done, but before submitting a new patch
> > > > >
> > > > > >
> > > > > > It also seems like this TODO should be addressed:
> > > > > > +/* TODO (regXboi): this next line is needed for the 3
> HVs, 3
> > > LS,
> > > > > > + * 3 lports/LS, 1 LR test case, but has the potential
> side
> > > > > effect
> > > > > > + * of defeating quiet mode once a logical router leads
> to
> > > > > creating
> > > > > > + * patch ports. Need to understand the failure mode
> better
> > > and
> > > > > > + * what is needed to remove this. */
> > > > > > +force_full_process();
> > > > >
> > > > > I've been looking at what happens here and I'm seeing some
> signatures
> > > > > that concern me.  The test case that fails is no longer the cited
> one
> > > above
> > > > > but is "2 HVs, 4 lports/HV, localnet ports" ...
> > > > >
> > > > > What I'm seeing when I peer in is that the information populating
> the
> > > > > local_datapath structures doesn't appear to be consistent on each
> pass
> > > > > through binding_run: Looking at the ovn-controller process under
> hv2
> > > > > for the above test (when it passes), I'll see signatures that
look
> > > > > like the following:
> > > > >
> > > > > 2016-10-04T21:57:58.257Z|00020|physical|INFO|looking at binding
> record
> > > > > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > > > > 2016-10-04T21:57:58.257Z|00021|physical|INFO|dp_key is 1
> > > > > 2016-10-04T21:57:58.257Z|00022|physical|INFO|looking for ofport
of
> lp11
> > > > > (LP)
> > > > > 2016-10-04T21:57:58.257Z|00023|physical|INFO|looking for ofport
of
> ln1
> > > > > (localnet)
> > > > > ...
> > > > > 2016-10-04T21:57:58.259Z|00034|physical|INFO|looking at binding
> record
> > > > > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > > > > 2016-10-04T21:57:58.259Z|00035|physical|INFO|dp_key is 1
> > > > > 2016-10-04T21:57:58.259Z|00036|physical|INFO|looking for ofport
of
> lp11
> > > > > (LP)
> > > > > 2016-10-04T21:57:58.259Z|00037|physical|INFO|looking for tunnel
to
> hv1
> > > > > ...
> > > >
> > > > This sort of oscillation seems super-weird to me.  I'd also like to
> > > > learn more.
> > > >
> > >
> > > I found the problem and fixed it in the v3 patch set - some of the
> methods
> > > (patch_run for example) now include a test of ovs_idl_txn before they
> run
> > > and some don't (physical_run for example), so you can see guess what
> was
> > > happening...
> > >
> > > (If we had a txn to work with everything is fine, but without one,
> > > physical_run was trying to calculate flows based on local datapaths
> > > with no localnet ports being defined, which 

[ovs-dev] [PATCH] ovn-controller: honor ovs_idl_txn when calculating and installing flows.

[Ryan Moats]

ovs_idl_txn is checked before various routines (like patch_run) execute.
However, flow calculation and installation does not also check this
variable, which can lead to oscillations as described in [1].

[1] http://openvswitch.org/pipermail/dev/2016-October/080247.html

Signed-off-by: Ryan Moats 
---
Note: This patch also applies to the 2.6 branch

 ovn/controller/ovn-controller.c | 45 +++--
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 00392ca..4ac1425 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -312,14 +312,9 @@ update_ct_zones(struct sset *lports, struct hmap 
*patched_datapaths,
 }
 
 static void
-commit_ct_zones(struct controller_ctx *ctx,
-const struct ovsrec_bridge *br_int,
+commit_ct_zones(const struct ovsrec_bridge *br_int,
 struct shash *pending_ct_zones)
 {
-if (!ctx->ovs_idl_txn) {
-return;
-}
-
 struct smap new_ids;
 smap_clone(_ids, _int->external_ids);
 
@@ -543,24 +538,26 @@ main(int argc, char *argv[])
 pinctrl_run(, , br_int, chassis_id, _datapaths);
 update_ct_zones(_lports, _datapaths, _zones,
 ct_zone_bitmap, _ct_zones);
-commit_ct_zones(, br_int, _ct_zones);
-
-struct hmap flow_table = HMAP_INITIALIZER(_table);
-lflow_run(, , , _datapaths,
-  _datapaths, _table, _zones,
-  _table);
-
-physical_run(, mff_ovn_geneve,
- br_int, chassis_id, _zones, _table,
- _datapaths, _datapaths);
-
-ofctrl_put(_table, _ct_zones,
-   get_nb_cfg(ctx.ovnsb_idl));
-hmap_destroy(_table);
-if (ctx.ovnsb_idl_txn) {
-int64_t cur_cfg = ofctrl_get_cur_cfg();
-if (cur_cfg && cur_cfg != chassis->nb_cfg) {
-sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+if (ctx.ovs_idl_txn) {
+commit_ct_zones(br_int, _ct_zones);
+
+struct hmap flow_table = HMAP_INITIALIZER(_table);
+lflow_run(, , , _datapaths,
+  _datapaths, _table, _zones,
+  _table);
+
+physical_run(, mff_ovn_geneve,
+ br_int, chassis_id, _zones, _table,
+ _datapaths, _datapaths);
+
+ofctrl_put(_table, _ct_zones,
+   get_nb_cfg(ctx.ovnsb_idl));
+hmap_destroy(_table);
+if (ctx.ovnsb_idl_txn) {
+int64_t cur_cfg = ofctrl_get_cur_cfg();
+if (cur_cfg && cur_cfg != chassis->nb_cfg) {
+sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+}
 }
 }
 mcgroup_index_destroy();
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] tests: Fix double-rebuild of testsuite for "check-valgrind".

[Ben Pfaff]

When I ran "make check-valgrind -j10" and the testsuite needed to be
rebuilt, two copies of it were rebuilt in parallel and sometimes they
raced with each other.  I don't have the full story on exactly why this
happened, but this commit, which eliminates redundant dependencies from
check-* targets, fixes the problem for me.  The dependencies are redundant
because these targets depend on "all", which also depends on them.

Signed-off-by: Ben Pfaff 
---
 tests/automake.mk | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tests/automake.mk b/tests/automake.mk
index a2b7786..f022feb 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -118,7 +118,7 @@ DISTCLEANFILES += tests/atconfig tests/atlocal
 
 AUTOTEST_PATH = 
utilities:vswitchd:ovsdb:vtep:tests:$(PTHREAD_WIN32_DIR_DLL):ovn/controller-vtep:ovn/northd:ovn/utilities:ovn/controller
 
-check-local: tests/atconfig tests/atlocal $(TESTSUITE)
+check-local:
set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH) 
$(TESTSUITEFLAGS); \
"$$@" || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
 
@@ -127,7 +127,7 @@ check-local: tests/atconfig tests/atlocal $(TESTSUITE)
 
 COVERAGE = coverage
 COVERAGE_FILE='$(abs_srcdir)/.coverage'
-check-pycov: all tests/atconfig tests/atlocal $(TESTSUITE) clean-pycov
+check-pycov: all clean-pycov
PYTHONDONTWRITEBYTECODE=yes COVERAGE_FILE=$(COVERAGE_FILE) 
PYTHON='$(COVERAGE) run -p' $(SHELL) '$(TESTSUITE)' -C tests 
AUTOTEST_PATH=$(AUTOTEST_PATH) $(TESTSUITEFLAGS)
@cd $(srcdir) && $(COVERAGE) combine && COVERAGE_FILE=$(COVERAGE_FILE) 
$(COVERAGE) annotate
@echo
@@ -145,7 +145,7 @@ clean-lcov:
 
 LCOV_OPTS = -b $(abs_top_builddir) -d $(abs_top_builddir) -q -c --rc 
lcov_branch_coverage=1
 GENHTML_OPTS = -q --branch-coverage --num-spaces 4
-check-lcov: all tests/atconfig tests/atlocal $(TESTSUITE) $(check_DATA) 
clean-lcov
+check-lcov: all $(check_DATA) clean-lcov
find . -name '*.gcda' | xargs -n1 rm -f
-set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH) 
$(TESTSUITEFLAGS); \
"$$@" || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
@@ -217,8 +217,7 @@ HELGRIND = valgrind --log-file=helgrind.%p --tool=helgrind \
--suppressions=$(abs_top_srcdir)/tests/glibc.supp \
--suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=20
 EXTRA_DIST += tests/glibc.supp tests/openssl.supp
-check-valgrind: all tests/atconfig tests/atlocal $(TESTSUITE) \
-$(valgrind_wrappers) $(check_DATA)
+check-valgrind: all $(valgrind_wrappers) $(check_DATA)
$(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true 
VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d 
$(TESTSUITEFLAGS)
@echo
@echo 
'--'
@@ -241,7 +240,7 @@ check-ryu: all
 EXTRA_DIST += tests/run-ryu
 
 # Run kmod tests. Assume kernel modules has been installed or linked into the 
kernel
-check-kernel: all tests/atconfig tests/atlocal $(SYSTEM_KMOD_TESTSUITE)
+check-kernel: all
set $(SHELL) '$(SYSTEM_KMOD_TESTSUITE)' -C tests  
AUTOTEST_PATH='$(AUTOTEST_PATH)' $(TESTSUITEFLAGS) -j1; \
"$$@" || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
 
@@ -251,7 +250,7 @@ check-kmod: all tests/atconfig tests/atlocal 
$(SYSTEM_KMOD_TESTSUITE)
modprobe -r -a vport-geneve vport-gre vport-lisp vport-stt vport-vxlan 
openvswitch
$(MAKE) check-kernel
 
-check-system-userspace: all tests/atconfig tests/atlocal 
$(SYSTEM_USERSPACE_TESTSUITE)
+check-system-userspace: all
set $(SHELL) '$(SYSTEM_USERSPACE_TESTSUITE)' -C tests  
AUTOTEST_PATH='$(AUTOTEST_PATH)' $(TESTSUITEFLAGS) -j1; \
"$$@" || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-northd: Drop redundant matching constraints in build_stateful().

[Ben Pfaff]

ip4.dst implies ip, udp.dst implies udp, and tcp.dst implies tcp.

Signed-off-by: Ben Pfaff 
---
 ovn/northd/ovn-northd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4668d9e..9bb49e8 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2708,12 +2708,12 @@ build_stateful(struct ovn_datapath *od, struct hmap 
*lflows)
 /* New connections in Ingress table. */
 char *action = xasprintf("ct_lb(%s);", node->value);
 struct ds match = DS_EMPTY_INITIALIZER;
-ds_put_format(, "ct.new && ip && ip4.dst == %s", ip_address);
+ds_put_format(, "ct.new && ip4.dst == %s", ip_address);
 if (port) {
 if (lb->protocol && !strcmp(lb->protocol, "udp")) {
-ds_put_format(, " && udp && udp.dst == %d", port);
+ds_put_format(, " && udp.dst == %d", port);
 } else {
-ds_put_format(, " && tcp && tcp.dst == %d", port);
+ds_put_format(, " && tcp.dst == %d", port);
 }
 ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL,
   120, ds_cstr(), action);
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 7/7] ovn: Remove weird or unneeded keywords from tests.

[Ben Pfaff]

AT_KEYWORDS are mostly there to make it easier to find the tests you're
looking for.  One might, for example, mark tests as "positive" or
"negative" so you can select the tests you want to run on that basis.
They're also useful for cases where Autotest just isn't good at splitting
words: for example, Autotest includes punctuation so that a test name
that has a word followed by a comma or colon won't be selected using a
keyword that lacks the comma or the colon.

But a lot of OVN tests had keywords that just didn't seem helpful in one
of these ways.  For example, it's hard to guess why running together
words into a longer word would help someone select a test, and it's not
helpful at all to repeat one of the words in the test name, since those
words are keywords by default anyway.

Therefore, this commit removes the keywords that don't seem helpful.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 24 ++--
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index c4a600a..cfc6a35 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1468,7 +1468,6 @@ AT_CLEANUP
 # 2 locally attached networks (one flat, one vlan tagged over same device)
 # 2 ports per HV on each network
 AT_SETUP([ovn -- 2 HVs, 4 lports/HV, localnet ports])
-AT_KEYWORDS([ovn-localnet])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -2375,7 +2374,6 @@ AT_CLEANUP
 
 # 3 hypervisors, one logical switch, 3 logical ports per hypervisor
 AT_SETUP([ovn -- portsecurity : 3 HVs, 1 LS, 3 lports/HV])
-AT_KEYWORDS([portsecurity])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -2753,7 +2751,6 @@ OVN_CLEANUP([hv1],[hv2],[hv3])
 AT_CLEANUP
 
 AT_SETUP([ovn -- 2 HVs, 2 LS, 1 lport/LS, 2 peer LRs])
-AT_KEYWORDS([ovnpeer])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -3121,7 +3118,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- 2 HVs, 3 LS, 1 lport/LS, 2 peer LRs, static routes])
-AT_KEYWORDS([ovnstaticroutespeer])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -3275,7 +3271,6 @@ OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 
 AT_SETUP([ovn -- send gratuitous arp on localnet])
-AT_KEYWORDS([ovn])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 ovn-nbctl ls-add lsw0
@@ -3317,7 +3312,6 @@ OVN_CLEANUP([hv])
 AT_CLEANUP
 
 AT_SETUP([ovn -- 2 HVs, 3 LRs connected via LS, static routes])
-AT_KEYWORDS([ovnstaticroutes])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -3495,7 +3489,6 @@ OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 
 AT_SETUP([ovn -- dhcpv4 : 1 HV, 2 LS, 2 LSPs/LS])
-AT_KEYWORDS([dhcpv4])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -3765,7 +3758,6 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 AT_CLEANUP
 
 AT_SETUP([ovn -- dhcpv6 : 1 HV, 2 LS, 5 LSPs])
-AT_KEYWORDS([dhcpv6])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -4027,7 +4019,6 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 AT_CLEANUP
 
 AT_SETUP([ovn -- 2 HVs, 2 LRs connected via LS, gateway router])
-AT_KEYWORDS([ovngatewayrouter])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -4353,7 +4344,6 @@ AT_CLEANUP
 # make sure that the port state is properly set to up and back down
 # when created and deleted.
 AT_SETUP([ovn -- port state up and down])
-AT_KEYWORDS([ovn])
 ovn_start
 
 ovn-nbctl ls-add ls1
@@ -4379,7 +4369,7 @@ AT_CLEANUP
 # make sure that the OF rules created to support a datapath are added/cleared
 # when logical switch is created and removed.
 AT_SETUP([ovn -- datapath rules added/removed])
-AT_KEYWORDS([ovn datapath cleanup])
+AT_KEYWORDS([cleanup])
 ovn_start
 
 net_add n1
@@ -4443,7 +4433,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- nd_na ])
-AT_KEYWORDS([ovn-nd_na])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -4516,7 +4505,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- address sets modification/removal smoke test])
-AT_KEYWORDS([ovn-addr])
 ovn_start
 
 net_add n1
@@ -4544,7 +4532,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- ipam])
-AT_KEYWORDS([ovnipam])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -4701,7 +4688,6 @@ OVS_APP_EXIT_AND_WAIT([ovn-northd])
 AT_CLEANUP
 
 AT_SETUP([ovn -- ipam connectivity])
-AT_KEYWORDS([ovnipamconnectivity])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -4830,7 +4816,7 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- ovs-vswitchd restart])
-AT_KEYWORDS([vswitchd restart])
+AT_KEYWORDS([vswitchd])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -4925,7 +4911,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- send arp for nexthop])
-AT_KEYWORDS([ovn])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 
@@ -5036,7 +5021,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- send gratuitous arp for nat ips in localnet])
-AT_KEYWORDS([ovn])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
 ovn_start
 # Create logical switch
@@ -5085,7 +5069,6 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 
 AT_SETUP([ovn -- delete mac bindings])
-AT_KEYWORDS([ovn])
 ovn_start
 net_add n1
 sim_add hv1
@@ 

[ovs-dev] [PATCH 5/7] ovn: Wait for ovn-northd to catch up in "ovn-sbctl" test.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 tests/ovn-sbctl.at | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
index 9393eef..26b05af 100644
--- a/tests/ovn-sbctl.at
+++ b/tests/ovn-sbctl.at
@@ -87,6 +87,7 @@ AT_CHECK([ovn-nbctl ls-add br-test])
 AT_CHECK([ovn-nbctl lsp-add br-test vif0])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:02])
 AT_CHECK([ovn-sbctl chassis-add ch0 stt 1.2.3.5])
+AT_CHECK([ovn-nbctl --wait=sb sync])
 AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
 
 AT_CHECK([ovn-sbctl show], [0], [dnl
@@ -98,7 +99,7 @@ Chassis "ch0"
 ])
 
 # adds another 'vif1'
-AT_CHECK([ovn-nbctl lsp-add br-test vif1])
+AT_CHECK([ovn-nbctl --wait=sb lsp-add br-test vif1])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif1 f0:ab:cd:ef:01:03])
 AT_CHECK([ovn-sbctl lsp-bind vif1 ch0])
 
@@ -113,6 +114,7 @@ Chassis "ch0"
 
 # deletes 'vif1'
 AT_CHECK([ovn-nbctl lsp-del vif1])
+AT_CHECK([ovn-nbctl --wait=sb sync])
 
 AT_CHECK([ovn-sbctl show], [0], [dnl
 Chassis "ch0"
@@ -130,12 +132,12 @@ chassis : ${uuid}
 ])
 
 # test the passing down of logical port type and options.
-AT_CHECK([ovn-nbctl lsp-add br-test vtep0])
+AT_CHECK([ovn-nbctl --wait=sb lsp-add br-test vtep0])
 AT_CHECK([ovn-nbctl lsp-set-type vtep0 vtep])
 AT_CHECK([ovn-nbctl lsp-set-options vtep0 vtep_physical_switch=p0 
vtep_logical_switch=l0])
 
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl --columns=logical_port list Port_Binding | 
grep vtep0`" ])
-AT_CHECK_UNQUOTED([ovn-sbctl --columns=logical_port,mac,type,options list 
Port_Binding vtep0], [0], [dnl
+AT_CHECK([ovn-sbctl --timeout=10 wait-until Port_Binding vtep0 options!={}])
+AT_CHECK([ovn-sbctl --columns=logical_port,mac,type,options list Port_Binding 
vtep0], [0], [dnl
 logical_port: "vtep0"
 mac : [[]]
 type: vtep
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 6/7] ovn: Fix some races in ovn-controller-vtep tests.

[Ben Pfaff]

This fixes a few races for port bindings appearing and being bound to
a chassis.  The ones changed to use "ovn-sbctl wait-until" were previously
only waiting until a Port_Binding record appeared (created by ovn-northd),
but not until the Port_Binding record's 'chassis' column was set (by
ovn-controller).

Signed-off-by: Ben Pfaff 
---
 tests/ovn-controller-vtep.at | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
index 654c212..aeb1ec1 100644
--- a/tests/ovn-controller-vtep.at
+++ b/tests/ovn-controller-vtep.at
@@ -190,7 +190,7 @@ OVN_CONTROLLER_VTEP_START
 AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0 -- 
bind-ls br-vtep p1 300 lswitch0])
 # adds logical switch port in ovn-nb database, and sets the type and options.
 OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep], [lswitch0])
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep 
br-vtep_lswitch0`"])
+ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0 chassis!='[[]]'
 # should see one binding, associated to chassis of 'br-vtep'.
 chassis_uuid=$(ovn-sbctl --columns=_uuid list Chassis br-vtep | cut -d ':' -f2 
| tr -d ' ')
 AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Port_Binding 
br-vtep_lswitch0 | cut -d ':' -f2 | tr -d ' '], [0], [dnl
@@ -201,7 +201,7 @@ ${chassis_uuid}
 AT_CHECK([vtep-ctl add-ls lswitch1 -- bind-ls br-vtep p0 200 lswitch1])
 # adds logical switch port in ovn-nb database for lswitch1.
 OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch1], [br-vtep], [lswitch1])
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding | grep -- 
br-vtep_lswitch1`"])
+ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch1 chassis!='[[]]'
 # This is allowed, but not recommended, to have two vlan_bindings (to 
different vtep logical switches)
 # from one vtep gateway physical port in one ovn-nb logical swithch.
 AT_CHECK_UNQUOTED([ovn-sbctl --columns=chassis list Port_Binding | cut -d ':' 
-f2 | tr -d ' ' | sort], [0], [dnl
@@ -212,7 +212,7 @@ ${chassis_uuid}
 
 # adds another logical switch port in ovn-nb database for lswitch0.
 OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0_dup], [br-vtep], [lswitch0])
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding | grep -- 
br-vtep_lswitch0_dup`"])
+ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0_dup 
chassis!='[[]]'
 # it is not allowed to have more than one ovn-nb logical port for the same
 # vtep logical switch on a vtep gateway chassis, so should still see only
 # two port_binding entries bound.
@@ -255,7 +255,7 @@ OVN_CONTROLLER_VTEP_START
 AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0])
 # adds logical switch port in ovn-nb database, and sets the type and options.
 OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep], [lswitch0])
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep 
br-vtep_lswitch0`"])
+ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0 chassis!='[[]]'
 
 # adds another lswitch 'br-void' in ovn-nb database.
 AT_CHECK([ovn-nbctl ls-add br-void])
@@ -264,7 +264,7 @@ AT_CHECK([vtep-ctl add-ps br-vtep-void -- add-port 
br-vtep-void p0-void -- bind-
 # adds a conflicting logical port (both br-vtep_lswitch0 and 
br-vtep-void_lswitch0
 # are bound to the same logical switch, but they are on different datapath).
 OVN_NB_ADD_VTEP_PORT([br-void], [br-vtep-void_lswitch0], [br-vtep-void], 
[lswitch0])
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep 
br-vtep-void_lswitch0`"])
+ovn-sbctl --timeout=10 wait-until Port_Binding br-vtep_lswitch0
 OVS_WAIT_UNTIL([test -n "`grep WARN ovn-controller-vtep.log`"])
 # confirms the warning log.
 AT_CHECK([sed -n 's/^.*\(|WARN|.*\)$/\1/p' ovn-controller-vtep.log | sed 
's/([[-_0-9a-z]][[-_0-9a-z]]*)/()/g;s/(with tunnel key [[0-9]][[0-9]]*)/()/g' | 
uniq], [0], [dnl
@@ -346,6 +346,7 @@ OVN_CONTROLLER_VTEP_START
 # 'ch0'.
 AT_CHECK([ovn-nbctl lsp-add br-test vif0])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:02])
+AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
 AT_CHECK([ovn-sbctl chassis-add ch0 vxlan 1.2.3.5])
 AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
 
@@ -360,9 +361,9 @@ AT_CHECK([ovn-nbctl ls-add br-void])
 # adds fake hv chassis 'ch1'.
 AT_CHECK([ovn-nbctl lsp-add br-void vif1])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif1 f0:ab:cd:ef:01:02])
+AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
 AT_CHECK([ovn-sbctl chassis-add ch1 vxlan 1.2.3.6])
 AT_CHECK([ovn-sbctl lsp-bind vif1 ch1])
-OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding | grep vif1`"])
 
 # checks Ucast_Macs_Remote creation.
 OVS_WAIT_UNTIL([test -n "`vtep-ctl list Ucast_Macs_Remote | grep _uuid`"])
@@ -417,12 +418,14 @@ OVN_CONTROLLER_VTEP_START
 # 'ch0'.
 AT_CHECK([ovn-nbctl lsp-add br-test vif0])
 AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:02])
+AT_CHECK([ovn-nbctl --timeout=10 --wait=sb 

[ovs-dev] [PATCH 4/7] ovn: Fix race in "ovn -- ipam" test.

[Ben Pfaff]

After setting the subnet, ovn-northd needs to process the changes before
setting the dynamic addresses.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 9baa1df..c4a600a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4552,7 +4552,7 @@ ovn_start
 # subnet which should result in an address being allocated for the port.
 ovn-nbctl ls-add sw0
 ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
-ovn-nbctl add Logical-Switch sw0 other_config subnet=192.168.1.0/24
+ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=192.168.1.0/24
 AT_CHECK([ovn-nbctl get Logical-Switch-Port p0 dynamic_addresses], [0],
 ["0a:00:00:00:00:01 192.168.1.2"
 ])
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/7] ovn: Fix bugs in port security test.

[Ben Pfaff]

A number of instances of "{i}" in this test should have been "${i}".

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 05e1349..4214a00 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -2626,13 +2626,13 @@ sip=`ip_to_hex 192 168 0 13`
 tip=`ip_to_hex 192 168 0 23`
 test_ip 13 f113 f023 $sip $tip
 
-# ipv6 packet should be received by lp[123]3 with mac f000{i}{i}3
-# and ip6.dst as fe80::ea2a:eaff:fe28:0{i}{i}3.
+# ipv6 packet should be received by lp[123]3 with mac f${i}${i}3
+# and ip6.dst as fe80::ea2a:eaff:fe28:0${i}${i}3.
 # lp11 can send ipv6 traffic as there is no port security
 sip=ee80
 for i in 1 2 3; do
-tip=fe80ea2aeafffe2800{i}3
-test_ipv6 11 f011 f0{i}${i}3 $sip $tip {i}3
+tip=fe80ea2aeafffe2800${i}3
+test_ipv6 11 f011 f${i}${i}3 $sip $tip ${i}3
 done
 
 
@@ -2645,8 +2645,8 @@ sip=ee80
 tip=fe80ea2aeafffe280023
 test_ipv6 11 f011 f333 $sip $tip
 
-# ipv6 packet should be allowed for lp[123]3 with mac f000{i}{i}3
-# and ip6.src fe80::ea2a:eaff:fe28:0{i}{i}3 and ip6.src ::.
+# ipv6 packet should be allowed for lp[123]3 with mac f000${i}${i}3
+# and ip6.src fe80::ea2a:eaff:fe28:0${i}${i}3 and ip6.src ::.
 # and should be dropped for any other ip6.src
 # lp21 can receive ipv6 traffic as there is no port security
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 3/7] ovn: Fix some races in IPAM connectivity test.

[Ben Pfaff]

It can take a way for dynamic addresses to propagate through ovn-northd,
so wait for it to happen.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 4214a00..9baa1df 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4724,23 +4724,18 @@ ovn-nbctl lsp-add alice rp-alice -- set 
Logical_Switch_Port rp-alice type=router
 # Create logical port foo1 in foo
 ovn-nbctl lsp-add foo foo1 \
 -- lsp-set-addresses foo1 "dynamic"
-AT_CHECK([ovn-nbctl get Logical-Switch-Port foo1 dynamic_addresses], [0],
- ["0a:00:00:00:00:01 192.168.1.2"
-])
+AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port foo1 
dynamic_addresses='"0a:00:00:00:00:01 192.168.1.2"'], [0])
 
 # Create logical port alice1 in alice
 ovn-nbctl lsp-add alice alice1 \
 -- lsp-set-addresses alice1 "dynamic"
-AT_CHECK([ovn-nbctl get Logical-Switch-Port alice1 dynamic_addresses], [0],
- ["0a:00:00:00:00:02 192.168.2.2"
-])
+AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port alice1 
dynamic_addresses='"0a:00:00:00:00:02 192.168.2.2"'])
 
 # Create logical port foo2 in foo
 ovn-nbctl lsp-add foo foo2 \
 -- lsp-set-addresses foo2 "dynamic"
-AT_CHECK([ovn-nbctl get Logical-Switch-Port foo2 dynamic_addresses], [0],
- ["0a:00:00:00:00:03 192.168.1.3"
-])
+sleep 1
+AT_CHECK([ovn-nbctl --timeout=10 wait-until Logical-Switch-Port foo2 
dynamic_addresses='"0a:00:00:00:00:03 192.168.1.3"'])
 
 # Create a hypervisor and create OVS ports corresponding to logical ports.
 net_add n1
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/7] ovn: Fix races in MAC_Binding deletion test.

[Ben Pfaff]

The test assumed that ovn-northd could delete the MAC_Binding rows
instantly, but it may take a while.

Signed-off-by: Ben Pfaff 
---
 tests/ovn.at | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 948716b..05e1349 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5108,15 +5108,14 @@ dp_uuid=`ovn-sbctl find datapath | grep uuid | cut -f2 
-d ":" | cut -f2 -d " "`
 ovn-sbctl create MAC_Binding ip=10.0.0.1 datapath=$dp_uuid logical_port=lp0 
mac="mac1"
 ovn-sbctl create MAC_Binding ip=10.0.0.1 datapath=$dp_uuid logical_port=lp1 
mac="mac2"
 ovn-sbctl find MAC_Binding
-#Delete port lp0
+# Delete port lp0 and check that its MAC_Binding is deleted.
 ovn-nbctl lsp-del lp0
 ovn-sbctl find MAC_Binding
-AT_CHECK([ovn-sbctl find MAC_Binding logical_port=lp0], [0], [])
-#Delete ls0. This will verify that the mac_bindings are cleaned up when a
-#datapath is deleted without explicitly removing the the logical ports
+OVS_WAIT_UNTIL([test `ovn-sbctl find MAC_Binding logical_port=lp0 | wc -l` = 
0])
+# Delete logical switch ls0 and check that its MAC_Binding is deleted.
 ovn-nbctl ls-del ls0
 ovn-sbctl find MAC_Binding
-AT_CHECK([ovn-sbctl find MAC_Binding], [0], [])
+OVS_WAIT_UNTIL([test `ovn-sbctl find MAC_Binding | wc -l` = 0])
 
 OVN_CLEANUP([hv1])
 
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [patch_v3] ovn: Add additional comments regarding arp responders.

[Darrell Ball]

This e-mail is a duplicate - ignore

On Wed, Oct 5, 2016 at 6:06 PM, Darrell Ball  wrote:

> There has been enough confusion regarding logical switch datapath
> arp responders in ovn to warrant some additional comments;
> hence add a general description regarding why they exist and
> document the special cases.
>
> Signed-off-by: Darrell Ball 
> ---
>  ovn/northd/ovn-northd.8.xml | 51 ++
> +--
>  1 file changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 77eb3d1..2104302 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -415,20 +415,59 @@
>  Ingress Table 9: ARP/ND responder
>
>  
> -  This table implements ARP/ND responder for known IPs.  It contains
> these
> -  logical flows:
> +  This table implements ARP/ND responder for known IPs.  The advantage
> +  of the arp responder flow is to limit arp broadcasts by locally
> +  responding to arp requests without the need to send to other
> +  hypervisors.  One common case is when the inport is a logical
> +  port associated with a VIF and the broadcast is responded to on the
> +  local hypervisor rather than broadcast across the whole network and
> +  responded to by the destination VM.  This behavior is proxy arp.
> +  Packets received by multiple hypervisors, as in the case of
> +  localnet and vtep logical inports need
> +  to skip these logical switch arp responders;  the reason being
> +  that northd downloads the same mac binding rules to all hypervisors
> +  and all hypervisors will receive the arp request from the external
> +  network and respond.  These skip rules are mentioned under
> +  priority-100 flows.  Arp requests arrive from VMs with a logical
> +  switch inport type of type empty, which is the default.  For this
> +  case, the logical switch proxy arp rules can be for other VMs or
> +  a logical router port.  In order to support proxy arp for logical
> +  router ports, an IP address must be configured on the logical
> +  switch router type port, with the same value as the peer of the
> +  logical router port.  The configured MAC addresses must match as
> +  well.  If the logical switch router type port does not have an
> +  IP address configured, arp requests will hit another arp responder
> +  on the logical router datapath itself, which is most commonly a
> +  distributed logical router.  The advantage of using the logical
> +  switch proxy arp rule for logical router ports is that this rule
> +  is hit before the logical switch L2 broadcast rule.  This means
> +  the arp request is not broadcast on this logical switch.  Logical
> +  switch arp responder proxy arp rules can also be hit when
> +  receiving arp requests externally on a L2 gateway port.  In this
> +  case, the hypervisor acting as an L2 gateway, responds to the arp
> +  request on behalf of a VM.  Note that arp requests received from
> +  localnet or vtep logical inports can
> +  either go directly to VMs, in which case the VM responds or can
> +  hit an arp responder for a logical router port if the packet is
> +  used to resolve a logical router port next hop address.
> +  It contains these logical flows:
>  
>
>  
>
> -Priority-100 flows to skip ARP responder if inport is of type
> -localnet, and advances directly to the next table.
> +Priority-100 flows to skip the ARP responder if inport is
> +of type localnet or vtep and
> +advances directly to the next table.  The inport being of type
> +router has no known use case for these arp
> +responders.  However, no skip flows are installed for these
> +packets, as there would be some additional flow cost for this
> +and the value appears limited.
>
>
>
>  
>Priority-50 flows that match ARP requests to each known IP
> address
> -  A of every logical router port, and respond with ARP
> +  A of every logical switch port, and respond with ARP
>replies directly with corresponding Ethernet address
> E:
>  
>
> @@ -455,7 +494,7 @@ output;
>  
>Priority-50 flows that match IPv6 ND neighbor solicitations to
>each known IP address A (and A's
> -  solicited node address) of every logical router port, and
> +  solicited node address) of every logical switch port, and
>respond with neighbor advertisements directly with
>corresponding Ethernet address E:
>  
> --
> 1.9.1
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [patch_v3] ovn: Add additional comments regarding arp responders.

[Darrell Ball]

There has been enough confusion regarding logical switch datapath
arp responders in ovn to warrant some additional comments;
hence add a general description regarding why they exist and
document the special cases.

Signed-off-by: Darrell Ball 
---
 ovn/northd/ovn-northd.8.xml | 51 +++--
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 77eb3d1..2104302 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -415,20 +415,59 @@
 Ingress Table 9: ARP/ND responder
 
 
-  This table implements ARP/ND responder for known IPs.  It contains these
-  logical flows:
+  This table implements ARP/ND responder for known IPs.  The advantage
+  of the arp responder flow is to limit arp broadcasts by locally
+  responding to arp requests without the need to send to other
+  hypervisors.  One common case is when the inport is a logical
+  port associated with a VIF and the broadcast is responded to on the
+  local hypervisor rather than broadcast across the whole network and
+  responded to by the destination VM.  This behavior is proxy arp.
+  Packets received by multiple hypervisors, as in the case of
+  localnet and vtep logical inports need
+  to skip these logical switch arp responders;  the reason being
+  that northd downloads the same mac binding rules to all hypervisors
+  and all hypervisors will receive the arp request from the external
+  network and respond.  These skip rules are mentioned under
+  priority-100 flows.  Arp requests arrive from VMs with a logical
+  switch inport type of type empty, which is the default.  For this
+  case, the logical switch proxy arp rules can be for other VMs or
+  a logical router port.  In order to support proxy arp for logical
+  router ports, an IP address must be configured on the logical
+  switch router type port, with the same value as the peer of the
+  logical router port.  The configured MAC addresses must match as
+  well.  If the logical switch router type port does not have an
+  IP address configured, arp requests will hit another arp responder
+  on the logical router datapath itself, which is most commonly a
+  distributed logical router.  The advantage of using the logical
+  switch proxy arp rule for logical router ports is that this rule
+  is hit before the logical switch L2 broadcast rule.  This means
+  the arp request is not broadcast on this logical switch.  Logical
+  switch arp responder proxy arp rules can also be hit when
+  receiving arp requests externally on a L2 gateway port.  In this
+  case, the hypervisor acting as an L2 gateway, responds to the arp
+  request on behalf of a VM.  Note that arp requests received from
+  localnet or vtep logical inports can
+  either go directly to VMs, in which case the VM responds or can
+  hit an arp responder for a logical router port if the packet is
+  used to resolve a logical router port next hop address.
+  It contains these logical flows:
 
 
 
   
-Priority-100 flows to skip ARP responder if inport is of type
-localnet, and advances directly to the next table.
+Priority-100 flows to skip the ARP responder if inport is
+of type localnet or vtep and
+advances directly to the next table.  The inport being of type
+router has no known use case for these arp
+responders.  However, no skip flows are installed for these
+packets, as there would be some additional flow cost for this
+and the value appears limited.
   
 
   
 
   Priority-50 flows that match ARP requests to each known IP address
-  A of every logical router port, and respond with ARP
+  A of every logical switch port, and respond with ARP
   replies directly with corresponding Ethernet address E:
 
 
@@ -455,7 +494,7 @@ output;
 
   Priority-50 flows that match IPv6 ND neighbor solicitations to
   each known IP address A (and A's
-  solicited node address) of every logical router port, and
+  solicited node address) of every logical switch port, and
   respond with neighbor advertisements directly with
   corresponding Ethernet address E:
 
-- 
1.9.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [CudaMailTagged] [patch_v3] ovn: Add additional comments regarding arp responders.

[Darrell Ball]

There has been enough confusion regarding logical switch datapath
arp responders in ovn to warrant some additional comments;
hence add a general description regarding why they exist and
document the special cases.

Signed-off-by: Darrell Ball 
---

Note this patch is meant to be merge with the code change for vtep
inport handling here.
https://patchwork.ozlabs.org/patch/675796/

v2->v3: Reword and further elaborate.

v1->v2: Dropped RFC code change for logical switch router
type ports

 ovn/northd/ovn-northd.8.xml | 51 +++--
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 77eb3d1..2104302 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -415,20 +415,59 @@
 Ingress Table 9: ARP/ND responder
 
 
-  This table implements ARP/ND responder for known IPs.  It contains these
-  logical flows:
+  This table implements ARP/ND responder for known IPs.  The advantage
+  of the arp responder flow is to limit arp broadcasts by locally
+  responding to arp requests without the need to send to other
+  hypervisors.  One common case is when the inport is a logical
+  port associated with a VIF and the broadcast is responded to on the
+  local hypervisor rather than broadcast across the whole network and
+  responded to by the destination VM.  This behavior is proxy arp.
+  Packets received by multiple hypervisors, as in the case of
+  localnet and vtep logical inports need
+  to skip these logical switch arp responders;  the reason being
+  that northd downloads the same mac binding rules to all hypervisors
+  and all hypervisors will receive the arp request from the external
+  network and respond.  These skip rules are mentioned under
+  priority-100 flows.  Arp requests arrive from VMs with a logical
+  switch inport type of type empty, which is the default.  For this
+  case, the logical switch proxy arp rules can be for other VMs or
+  a logical router port.  In order to support proxy arp for logical
+  router ports, an IP address must be configured on the logical
+  switch router type port, with the same value as the peer of the
+  logical router port.  The configured MAC addresses must match as
+  well.  If the logical switch router type port does not have an
+  IP address configured, arp requests will hit another arp responder
+  on the logical router datapath itself, which is most commonly a
+  distributed logical router.  The advantage of using the logical
+  switch proxy arp rule for logical router ports is that this rule
+  is hit before the logical switch L2 broadcast rule.  This means
+  the arp request is not broadcast on this logical switch.  Logical
+  switch arp responder proxy arp rules can also be hit when
+  receiving arp requests externally on a L2 gateway port.  In this
+  case, the hypervisor acting as an L2 gateway, responds to the arp
+  request on behalf of a VM.  Note that arp requests received from
+  localnet or vtep logical inports can
+  either go directly to VMs, in which case the VM responds or can
+  hit an arp responder for a logical router port if the packet is
+  used to resolve a logical router port next hop address.
+  It contains these logical flows:
 
 
 
   
-Priority-100 flows to skip ARP responder if inport is of type
-localnet, and advances directly to the next table.
+Priority-100 flows to skip the ARP responder if inport is
+of type localnet or vtep and
+advances directly to the next table.  The inport being of type
+router has no known use case for these arp
+responders.  However, no skip flows are installed for these
+packets, as there would be some additional flow cost for this
+and the value appears limited.
   
 
   
 
   Priority-50 flows that match ARP requests to each known IP address
-  A of every logical router port, and respond with ARP
+  A of every logical switch port, and respond with ARP
   replies directly with corresponding Ethernet address E:
 
 
@@ -455,7 +494,7 @@ output;
 
   Priority-50 flows that match IPv6 ND neighbor solicitations to
   each known IP address A (and A's
-  solicited node address) of every logical router port, and
+  solicited node address) of every logical switch port, and
   respond with neighbor advertisements directly with
   corresponding Ethernet address E:
 
-- 
1.9.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 4/4] ovn-nbctl: Improve ovn-nbctl manpage

[Ben Pfaff]

On Wed, Oct 05, 2016 at 04:20:45AM -0700, nickcooper-zhangtonghao wrote:
> Signed-off-by: nickcooper-zhangtonghao 
> 

Thanks for pointing out the omission.  I ended up doing significant
further work here.  I applied the following final patch.

--8<--cut here-->8--

From: nickcooper-zhangtonghao 
Date: Wed, 5 Oct 2016 04:20:45 -0700
Subject: [PATCH] ovn-nbctl: Improve ovn-nbctl manpage

Signed-off-by: nickcooper-zhangtonghao 
[b...@ovn.org added further improvements]
Signed-off-by: Ben Pfaff 
---
 ovn/utilities/ovn-nbctl.8.xml | 40 
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 7cd515f..2cbd6e0 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -169,14 +169,38 @@
 
   lsp-set-addresses port 
[address]...
   
-Sets the addresses associated with port to
-address.  Each address should be either an
-Ethernet address or an Ethernet address followed by IP addresses
-(separated by space and quoted to form a single command-line
-argument).  The special form unknown is also valid.
-Multiple Ethernet addresses or Ethernet+IPs combinations may be set.
-If no address argument is given, port will have
-no addresses associated with it.
+
+  Sets the addresses associated with port to
+  address.  Each address should be one of the
+  following:
+
+
+
+  an Ethernet address, optionally followed by a space and one or 
more IP addresses
+  
+OVN delivers packets for the Ethernet address to this port.
+  
+
+  unknown
+  
+OVN delivers unicast Ethernet packets whose destination MAC address
+is not in any logical port's addresses column to ports with address
+unknown.
+  
+
+  dynamic
+  
+Use this keyword to make ovn-northd generate a
+globally unique MAC address and choose an unused IPv4 address with
+the logical port's subnet and store them in the port's
+dynamic_addresses column.
+  
+
+
+
+  Multiple addresses may be set.  If no address argument is
+  given, port will have no addresses associated with it.
+
   
 
   lsp-get-addresses port
-- 
2.1.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/4] ovn-nbctl: Fix memory leak in nbctl_lr_route_add

[Ben Pfaff]

On Wed, Oct 05, 2016 at 04:20:44AM -0700, nickcooper-zhangtonghao wrote:
> Signed-off-by: nickcooper-zhangtonghao 
> 

Thanks, applied to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/4] ovn-nbctl: Check the length of MAC address

[Ben Pfaff]

On Wed, Oct 05, 2016 at 04:20:43AM -0700, nickcooper-zhangtonghao wrote:
> The command "ovn-nbctl lrp-add" should not set the MAC address
> which length is invalid to logical router port. This patch
> updates the eth_addr_from_string() to check trailing characters.
> We should use the ovs_scan() to check the "addresses" owned by
> the logical port, instead of eth_addr_from_string(). This patch
> also updates the ovn-nbctl tests.
> 
> Signed-off-by: nickcooper-zhangtonghao 
> 

Thanks, I applied this to master.  I folded in the following minor
incremental just because ovs_scan_len() is only really meant for
situations where the 'n' offset is being incremented over several calls.

--8<--cut here-->8--

diff --git a/lib/packets.c b/lib/packets.c
index f661c34..990c407 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -147,14 +147,13 @@ bool
 eth_addr_from_string(const char *s, struct eth_addr *ea)
 {
 int n = 0;
-if (ovs_scan_len(s, , ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(*ea))) {
-if (!s[n]) {
-return true;
-}
+if (ovs_scan(s, ETH_ADDR_SCAN_FMT"%n", ETH_ADDR_SCAN_ARGS(*ea), )
+&& !s[n]) {
+return true;
+} else {
+*ea = eth_addr_zero;
+return false;
 }
-
-*ea = eth_addr_zero;
-return false;
 }
 
 /* Fills 'b' with a Reverse ARP packet with Ethernet source address 'eth_src'.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/4] ovn: make ipam tests more reliable

[Ben Pfaff]

On Wed, Oct 05, 2016 at 04:20:42AM -0700, nickcooper-zhangtonghao wrote:
> Fixes: 8639f9be("ovn-northd, tests: Adding IPAM to ovn-northd.")
> The IPAM tests began to fail occasionally. Adding --wait=sb
> to commands triggering address allocation eliminated these failures.
> 
> Signed-off-by: nickcooper-zhangtonghao 
> 

Thanks for working on this.

When there are multiple ovn-nbctl calls in a row, it's ordinarily only
necessary to add --wait to the last call.  Can you make that change?

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PACTCH v14] DSCP marking on packets

[Ben Pfaff]

On Wed, Oct 05, 2016 at 01:57:47PM +0530, bscha...@redhat.com wrote:
> From: Babu Shanmugam 
> 
> This patch adds support for marking qos on IP packets based on arbitrary
> match criteria for a logical switch.
> 
> Signed-off-by: Babu Shanmugam 
> Suggested-by: Mickey Spiegel 
> Acked-by: Mickey Spiegel 

I figured out the problem I had with the test.  It was a race due to
lack of --wait on the ovn-nbctl calls.  I added --wait in the right
places and that fixed the problem.

While I was at it, I decided to improve the test by making it also check
with ovn-trace, since it's usually easier to understand failures from
ovn-trace than from physical packet traces.  I folded in the following
and applied this to master.

Thank you!

--8<--cut here-->8--

diff --git a/NEWS b/NEWS
index ee873d6..95cb2b2 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,7 @@ Post-v2.6.0
 -
- OVN:
  * QoS is now implemented via egress shaping rather than ingress policing.
+ * DSCP marking is now supported, via the new northbound QoS table.
- Fixed regression in table stats maintenance introduced in OVS
  2.3.0, wherein the number of OpenFlow table hits and misses was
  not accurate.
diff --git a/tests/ovn.at b/tests/ovn.at
index d9f9683..948716b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5367,9 +5367,7 @@ AT_SETUP([ovn -- DSCP marking check])
 AT_KEYWORDS([ovn])
 ovn_start
 
-# Configure the Northbound database
 ovn-nbctl ls-add lsw0
-
 ovn-nbctl --wait=sb lsp-add lsw0 lp1
 ovn-nbctl --wait=sb lsp-add lsw0 lp2
 ovn-nbctl lsp-set-addresses lp1 f0:00:00:00:00:01
@@ -5385,44 +5383,66 @@ ovn_attach n1 br-phys 192.168.0.1
 ovs-vsctl add-port br-int vif1 -- set Interface vif1 external-ids:iface-id=lp1 
options:tx_pcap=vif1-tx.pcap options:rxq_pcap=vif1-rx.pcap ofport-request=1
 ovs-vsctl add-port br-int vif2 -- set Interface vif2 external-ids:iface-id=lp2 
options:tx_pcap=vif2-tx.pcap options:rxq_pcap=vif2-rx.pcap ofport-request=2
 
-# Extracts the final flow from ofproto/trace output,
-# removing the irrelevant MFF_LOG_CT_ZONE (reg13) value.
-get_final_flow() {
-sed -n "/Final flow:/s/reg13=[[^,]]*,//p" stdout
+AT_CAPTURE_FILE([trace])
+ovn_trace () {
+ovn-trace --all "$@" | tee trace | sed '1,/Minimal trace/d'
+}
+
+# Extracts nw_tos from the final flow from ofproto/trace output and prints
+# it on stdout.  Prints "none" if no nw_tos was included.
+get_final_nw_tos() {
+if flow=$(grep '^Final flow:' stdout); then :; else
+   # The output didn't have a final flow.
+   return 99
+fi
+
+tos=$(echo "$flow" | sed -n 's/.*nw_tos=\([[0-9]]\{1,\}\).*/\1/p')
+case $tos in
+'') echo none ;;
+   *) echo $tos ;;
+esac
+}
+
+# check_tos TOS
+#
+# Checks that a packet from 1.1.1.1 to 1.1.1.2 gets its DSCP set to TOS.
+check_tos() {
+# First check with ovn-trace for logical flows.
+echo "checking for tos $1"
+(if test $1 != 0; then echo "ip.dscp = $1;"; fi;
+ echo 'output("lp2");') > expout
+AT_CHECK_UNQUOTED([ovn_trace lsw0 'inport == "lp1" && eth.src == 
f0:00:00:00:00:01 && eth.dst == f0:00:00:00:00:02 && ip4.src == 1.1.1.1 && 
ip4.dst == 1.1.1.2'], [0], [expout])
+
+# Then re-check with ofproto/trace for a physical packet.
+AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x800,nw_src=1.1.1.1,nw_dst=1.1.1.2'],
 [0], [stdout-nolog])
+AT_CHECK_UNQUOTED([get_final_nw_tos], [0], [`expr $1 \* 4`
+])
 }
 
 # check at L2
-AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02' -generate], [0], 
[stdout])
-AT_CHECK([get_final_flow], [0],[Final flow: 
reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x
+AT_CHECK([ovn_trace lsw0 'inport == "lp1" && eth.src == f0:00:00:00:00:01 && 
eth.dst == f0:00:00:00:00:02'], [0], [output("lp2");
+])
+AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02'], [0], 
[stdout-nolog])
+AT_CHECK([get_final_nw_tos], [0], [none
 ])
 
 # check at L3 without dscp marking
-AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x800,nw_src=1.1.1.1,nw_dst=1.1.1.2'
 -generate], [0], [stdout])
-AT_CHECK([get_final_flow], [0],[Final flow: 
ip,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=1.1.1.1,nw_dst=1.1.1.2,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0
-])
+check_tos 0
 
 # Mark DSCP with a valid value
-qos_id=$(ovn-nbctl -- --id=@lp1-qos create QoS priority=100 action=dscp=48 
match="inport\=\=\"lp1\"" direction="from-lport" -- set Logical_Switch lsw0 
qos_rules=@lp1-qos)
-AT_CHECK([ovs-appctl ofproto/trace br-int 

Re: [ovs-dev] [patch_v5] ovn: Add datapaths of interest filtering.

[Darrell Ball]

I made a correction inline

On Tue, Oct 4, 2016 at 10:04 PM, Darrell Ball  wrote:

> This patch adds datapaths of interest support where only datapaths of
> local interest are monitored by the ovn-controller ovsdb client.  The
> idea is to do a flood fill in ovn-controller of datapath associations
> calculated by northd. A new column is added to the SB database
> datapath_binding table - related_datapaths to facilitate this so all
> datapaths associations are known quickly in ovn-controller.  This
> allows monitoring to adapt quickly with a single new monitor setting
> for all datapaths of interest locally.
>
> To reduce risk and minimize latency on network churn, only logical
> flows are conditionally monitored for now.  This should provide
> the bulk of the benefit.  This will be re-evaluated later to
> possibly add additional conditional monitoring such as for
> logical ports.
>
> Liran Schour contributed enhancements to the conditional
> monitoring granularity in ovs and also submitted patches
> for ovn usage of conditional monitoring which aided this patch
> though sharing of concepts through code review work.
>
> Ben Pfaff suggested that northd could be used to pre-populate
> related datapaths for ovn-controller to use.  That idea is
> used as part of this patch.
>
> CC: Ben Pfaff 
> CC: Liran Schour 
> Signed-off-by: Darrell Ball 
> ---
>
> v4->v5: Correct cleanup of monitors.
> Fix warning.
>
> v3->v4: Refactor after incremental processing backout.
> Limit filtering to logical flows to limit risk.
>
> v2->v3: Line length violation fixups :/
>
> v1->v2: Added logical port removal monitoring handling, factoring
> in recent changes for incremental processing.  Added some
> comments in the code regarding initial conditions for
> database conditional monitoring.
>
>  ovn/controller/binding.c|  11 +++--
>  ovn/controller/ovn-controller.c | 103 ++
> ++
>  ovn/controller/ovn-controller.h |   5 ++
>  ovn/controller/patch.c  |   7 ++-
>  ovn/northd/ovn-northd.c |  76 +
>  ovn/ovn-sb.ovsschema|  11 +++--
>  ovn/ovn-sb.xml  |   9 
>  tests/ovn.at|   8 
>  8 files changed, 221 insertions(+), 9 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 13c51da..0561b6c 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -84,7 +84,8 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
>
>  static void
>  add_local_datapath(struct hmap *local_datapaths,
> -const struct sbrec_port_binding *binding_rec)
> +const struct sbrec_port_binding *binding_rec,
> +const struct controller_ctx *ctx)
>  {
>  if (get_local_datapath(local_datapaths,
> binding_rec->datapath->tunnel_key)) {
> @@ -96,6 +97,8 @@ add_local_datapath(struct hmap *local_datapaths,
>  memcpy(>uuid, _rec->header_.uuid, sizeof ld->uuid);
>  hmap_insert(local_datapaths, >hmap_node,
>  binding_rec->datapath->tunnel_key);
> +do_datapath_flood_fill(ctx, binding_rec->datapath,
> +   local_datapaths, NULL);
>  }
>
>  static void
> @@ -127,7 +130,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>  /* Add child logical port to the set of all local ports. */
>  sset_add(all_lports, binding_rec->logical_port);
>  }
> -add_local_datapath(local_datapaths, binding_rec);
> +add_local_datapath(local_datapaths, binding_rec, ctx);
>  if (iface_rec && ctx->ovs_idl_txn) {
>  update_qos(iface_rec, binding_rec);
>  }
> @@ -162,7 +165,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>  }
>
>  sset_add(all_lports, binding_rec->logical_port);
> -add_local_datapath(local_datapaths, binding_rec);
> +add_local_datapath(local_datapaths, binding_rec, ctx);
>  if (binding_rec->chassis == chassis_rec) {
>  return;
>  }
> @@ -176,7 +179,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>  const char *chassis = smap_get(_rec->options,
> "l3gateway-chassis");
>  if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) {
> -add_local_datapath(local_datapaths, binding_rec);
> +add_local_datapath(local_datapaths, binding_rec, ctx);
>  }
>  } else if (chassis_rec && binding_rec->chassis == chassis_rec) {
>  if (ctx->ovnsb_idl_txn) {
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-
> controller.c
> index 00392ca..02bafa0 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -69,6 +69,14 @@ OVS_NO_RETURN static void usage(void);
>
>  static char *ovs_remote;
>
> 

Re: [ovs-dev] [patch_v2] ovn: Add additional comments regarding arp responders.

[Darrell Ball]

On Wed, Oct 5, 2016 at 2:37 PM, Mickey Spiegel 
wrote:

> On Wed, Oct 5, 2016 at 10:08 AM, Darrell Ball  wrote:
>
>> There has been enough confusion regarding logical switch datapath
>> arp responders in ovn to warrant some additional comments;
>> hence add a general description regarding why they exist and
>> document the special cases.
>>
>> Signed-off-by: Darrell Ball 
>> ---
>>
>> v1->v2: Dropped RFC code change for logical switch router
>> type ports
>>
>>  ovn/northd/ovn-northd.8.xml | 37 ++---
>>  1 file changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
>> index 77eb3d1..7496284 100644
>> --- a/ovn/northd/ovn-northd.8.xml
>> +++ b/ovn/northd/ovn-northd.8.xml
>> @@ -415,14 +415,45 @@
>>  Ingress Table 9: ARP/ND responder
>>
>>  
>> -  This table implements ARP/ND responder for known IPs.  It contains
>> these
>> -  logical flows:
>> +  This table implements ARP/ND responder for known IPs.  The
>> advantage
>> +  of the arp responder flow is to limit arp broadcasts by locally
>> +  responding to arp requests without the need to send to other
>> +  hypervisors.  One common case is when the inport is a logical
>> +  port associated with a VIF and the broadcast is responded to on the
>> +  local hypervisor rather than broadcast across the whole network and
>> +  responded to by the destination VM.  This behavior is proxy arp.
>> +  It contains these logical flows:
>>  
>>
>>  
>>
>>  Priority-100 flows to skip ARP responder if inport is of type
>> -localnet, and advances directly to the next table.
>> +localnet or vtep and advances
>> +directly to the next table.  localnet and
>> +vtep port types are skipped, otherwise the arp
>>
> /otherwise/since
>
>> +request is received by multiple hypervisors, which all have the
>> +same mac binding downloaded from northd, which will cause
>> redundant
>> +arp replies, confusing the originator of the arp request.  The
>>
>
> Up to this point, I agree.
>
> The rest of the description below (with the exception of "l2gateway"
> at the bottom) is not about the priority 100 flows, and is not about
> inport.
> It is about the priority 50 flows, whether to program ARP/ND responder
> flows for IP/MAC combinations for logical switch ports of certain types.
>
> Note that inport is not part of the match criteria for the priority 50
> flows.
> If these ARP/ND responder flows are programmed, they work on all
> inports, unless overridden by the priority 100 flows above. For example,
> an ARP for a logical router IP address may be received on a VIF inport.
>
>
I agree the priority 50 flows are not inport specific but the overall
result, which
is what we really want to clarify is based on  the priority 100 skip
table flows
which are inport specific and the priority 50 arp responder flows proper.
Mentioning details regarding inport types is important to understand the
overall
behavior. I don't plan to limit important information, so I will not be
cutting the
description regarding inport types.

Also there are two descriptions for the priority 50 flows - one for V4 and
one for V6
and I don't want to repeat the overall explanation for both.
There are also two paragraphs references to "These flows are omitted..."

Alternatively, I will move the overall logic description to the
introductory paragraph
and briefly introduce the following flows, in one place; i.e. no repetition.
This location is not flow specific but rather about overall logic.




> The existing description of the priority 50 flows has an error. It should
> be referring to "logical switch port" rather than "logical router port",
> i.e.
> instead of:
>
>  Priority-50 flows that match ARP requests to each known IP address
>   A of every logical router port, and respond with ARP
> ...
>
> It should read:
>
>  Priority-50 flows that match ARP requests to each known IP address
>   A of every logical switch port, and respond with ARP
>

Actually, the existing description is incorrect for V6 as well as V4.



>
> The rest of the description below (with the exception of "l2gateway" at the
> bottom) should be attached to the paragraph that reads:
>
>   These flows are omitted for logical ports (other than router
> ports)
>   that are down.
>
> Something along the lines of:
>
> +Note that proxy arp for logical router ports is enabled, if a
> +corresponding IP address is configured on the peer logical
> +switch port of type router.
>

I can add this



> There is a
> +requirement that both MAC and IP (if configured) be manually
> +enforced to match on logical switch router type and logical
> +router port peers.  If the logical switch router type 

Re: [ovs-dev] [patch_v2] ovn: Add additional comments regarding arp responders.

[Mickey Spiegel]

On Wed, Oct 5, 2016 at 10:08 AM, Darrell Ball  wrote:

> There has been enough confusion regarding logical switch datapath
> arp responders in ovn to warrant some additional comments;
> hence add a general description regarding why they exist and
> document the special cases.
>
> Signed-off-by: Darrell Ball 
> ---
>
> v1->v2: Dropped RFC code change for logical switch router
> type ports
>
>  ovn/northd/ovn-northd.8.xml | 37 ++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 77eb3d1..7496284 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -415,14 +415,45 @@
>  Ingress Table 9: ARP/ND responder
>
>  
> -  This table implements ARP/ND responder for known IPs.  It contains
> these
> -  logical flows:
> +  This table implements ARP/ND responder for known IPs.  The advantage
> +  of the arp responder flow is to limit arp broadcasts by locally
> +  responding to arp requests without the need to send to other
> +  hypervisors.  One common case is when the inport is a logical
> +  port associated with a VIF and the broadcast is responded to on the
> +  local hypervisor rather than broadcast across the whole network and
> +  responded to by the destination VM.  This behavior is proxy arp.
> +  It contains these logical flows:
>  
>
>  
>
>  Priority-100 flows to skip ARP responder if inport is of type
> -localnet, and advances directly to the next table.
> +localnet or vtep and advances
> +directly to the next table.  localnet and
> +vtep port types are skipped, otherwise the arp
>
/otherwise/since

> +request is received by multiple hypervisors, which all have the
> +same mac binding downloaded from northd, which will cause
> redundant
> +arp replies, confusing the originator of the arp request.  The
>

Up to this point, I agree.

The rest of the description below (with the exception of "l2gateway"
at the bottom) is not about the priority 100 flows, and is not about inport.
It is about the priority 50 flows, whether to program ARP/ND responder
flows for IP/MAC combinations for logical switch ports of certain types.

Note that inport is not part of the match criteria for the priority 50
flows.
If these ARP/ND responder flows are programmed, they work on all
inports, unless overriden by the priority 100 flows above. For example,
an ARP for a logical router IP address may be received on a VIF inport.

The existing description of the priority 50 flows has an error. It should
be referring to "logical switch port" rather than "logical router port",
i.e.
instead of:

 Priority-50 flows that match ARP requests to each known IP address
  A of every logical router port, and respond with ARP
...

It should read:

 Priority-50 flows that match ARP requests to each known IP address
  A of every logical switch port, and respond with ARP

The rest of the description below (with the exception of "l2gateway" at the
bottom) should be attached to the paragraph that reads:

  These flows are omitted for logical ports (other than router
ports)
  that are down.

Something along the lines of:

+Note that proxy arp for logical router ports is enabled, if a
+corresponding IP address is configured on the peer logical
+switch port of type router. There is a
+requirement that both MAC and IP (if configured) be manually
+enforced to match on logical switch router type and logical
+router port peers.  If the logical switch router type port does
+not have an IP address configured, arp requests will hit another
+arp responder on the logical router datapath itself, which is
+most commonly a distributed logical router.  When the logical
+switch datapath arp responder is used for ports of type
+router, then arp requests from VMs would hit
+the logical switch datapath broadcast rule.  These arp requests
+would not hit the logical router datapath arp responder.  If arp
+requests are received on localnet or
+vtep logical switch ports, those arp requests
+would be handled by the logical router datapath arp responder.

Mickey


+inport being of type router has no valid use case.
> +No special filtering is done for these packets, as there would
> +be some additional flow cost for this and the value appears
> +limited, at this time.  The inport of type empty is the most
> +common case and includes both proxy arp for other VMs and also
> +logical router ports (if a corresponding IP address is configured
> +on the peer logical switch router type port).  There is a
> +requirement that both MAC and 

[ovs-dev] [PATCH] datapath_windows: Set isActivated flag only on success

[Shashank Ram]

@Switch.c: Modifies OvsActivateSwitch() function
to mark the switch as activated only if the
the status is success. The callers itself
only call this method when the isActivated
flag is unset.

Signed-off-by: Shashank Ram 
---
 datapath-windows/ovsext/Switch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c
index 825fa3c..7913cd5 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -556,7 +556,6 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
 OVS_LOG_TRACE("Enter: activate switch %p, dpNo: %ld",
   switchContext, switchContext->dpNo);

-switchContext->isActivated = TRUE;
 status = OvsAddConfiguredSwitchPorts(switchContext);

 if (status != NDIS_STATUS_SUCCESS) {
@@ -573,7 +572,7 @@ OvsActivateSwitch(POVS_SWITCH_CONTEXT switchContext)
 }

 cleanup:
-if (status != NDIS_STATUS_SUCCESS) {
+if (status == NDIS_STATUS_SUCCESS) {
 switchContext->isActivated = TRUE;
 }

--
2.6.2

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-controller: Container can have connection to a hosting VM.

[Guru Shetty]

On 5 October 2016 at 10:44, Ben Pfaff  wrote:

> On Tue, Oct 04, 2016 at 09:56:15AM -0700, Gurucharan Shetty wrote:
> > A Container running inside a VM can have a connection to the
> > hosting VM (parent port) in the logical topology (for e.g via a router).
> > So we should be able to loop-back into the same VM, even if the
> > final packet delivered does not have any tags in it.
> >
> > Reported-by: Dustin Spinhirne 
> > Signed-off-by: Gurucharan Shetty 
>
> It looks to me like, after this patch, we're effectively acting as if
> flags.allow_loopback is always set to 1.  Is that right?


You are right. It did not flash to me while making the code change that
table 65 is also used for logical to physical translation for patch ports.


>   If so, then we
> can simplify the code a bit.
>

I sent a V2 which I think reduces the scope of the change of v1 using the
flags.loopback flag. You probably had a better idea.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2] ovn-controller: Container can have connection to a hosting VM.

[Gurucharan Shetty]

A Container running inside a VM can have a connection to the
hosting VM (parent port) in the logical topology (for e.g via a router).
So we should be able to loop-back into the same VM, even if the
final packet delivered does not have any tags in it.

Reported-by: Dustin Spinhirne 
Signed-off-by: Gurucharan Shetty 
---
 AUTHORS   |   1 +
 ovn/controller/physical.c |  27 ---
 tests/ovn.at  | 195 ++
 3 files changed, 211 insertions(+), 12 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index a30a5d8..2525265 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -324,6 +324,7 @@ Derek Cormier   derek.corm...@lab.ntt.co.jp
 Dhaval Badiani  dbadi...@vmware.com
 DK Moon dkm...@nicira.com
 Ding Zhizhi.d...@6wind.com
+Dustin Spinhirnedspinhi...@vmware.com
 Edwin Chiu  ec...@vmware.com
 Eivind Bulie Haanaes
 Enas Ahmad  enas.ah...@kaust.edu.sa
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 1a91531..c5866b4 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -211,6 +211,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
  */
 
 int tag = 0;
+bool nested_container = false;
 ofp_port_t ofport;
 bool is_remote = false;
 if (binding->parent_port && *binding->parent_port) {
@@ -221,6 +222,7 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
   binding->parent_port));
 if (ofport) {
 tag = *binding->tag;
+nested_container = true;
 }
 } else {
 ofport = u16_to_ofp(simap_get(_to_ofport,
@@ -288,6 +290,12 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
 if (tag || !strcmp(binding->type, "localnet")
 || !strcmp(binding->type, "l2gateway")) {
 match_set_dl_vlan(, htons(tag));
+if (nested_container) {
+/* When a packet comes from a container sitting behind a
+ * parent_port, we should let it loopback to other containers
+ * or the parent_port itself. */
+put_load(MLF_ALLOW_LOOPBACK, MFF_LOG_FLAGS, 0, 1, ofpacts_p);
+}
 ofpact_put_STRIP_VLAN(ofpacts_p);
 }
 
@@ -385,15 +393,18 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
  * ===
  *
  * If the packet is supposed to hair-pin because the "loopback"
- * flag is set, temporarily set the in_port to zero, resubmit to
+ * flag is set (or if the destination is a nested container),
+ * temporarily set the in_port to zero, resubmit to
  * table 65 for logical-to-physical translation, then restore
  * the port number. */
 match_init_catchall();
 ofpbuf_clear(ofpacts_p);
 match_set_metadata(, htonll(dp_key));
-match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0,
- MLF_ALLOW_LOOPBACK, MLF_ALLOW_LOOPBACK);
 match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
+if (!nested_container) {
+match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0,
+ MLF_ALLOW_LOOPBACK, MLF_ALLOW_LOOPBACK);
+}
 
 put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
 put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
@@ -417,22 +428,14 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
 vlan_vid = ofpact_put_SET_VLAN_VID(ofpacts_p);
 vlan_vid->vlan_vid = tag;
 vlan_vid->push_vlan_if_needed = true;
-
-/* A packet might need to hair-pin back into its ingress
- * OpenFlow port (to a different logical port, which we already
- * checked back in table 34), so set the in_port to zero. */
-put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
-put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
 }
 ofpact_put_OUTPUT(ofpacts_p)->port = ofport;
 if (tag) {
 /* Revert the tag added to the packets headed to containers
  * in the previous step. If we don't do this, the packets
  * that are to be broadcasted to a VM in the same logical
- * switch will also contain the tag. Also revert the zero'd
- * in_port. */
+ * switch will also contain the tag. */
 ofpact_put_STRIP_VLAN(ofpacts_p);
-put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
 }
 ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
 , ofpacts_p);
diff --git a/tests/ovn.at b/tests/ovn.at
index 3b27581..1d2e4e5 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5362,3 +5362,198 @@ OVS_WAIT_UNTIL([test `ovs-vsctl show | grep "Port 
patch-br-int-to-ln_port" | wc
 
 OVN_CLEANUP([hv1],[hv2])
 

Re: [ovs-dev] [RFC 0/5] Integrate Sphinx documentation generator

[Stephen Finucane]

On 2016-10-05 17:29, Ben Pfaff wrote:

On Wed, Oct 05, 2016 at 10:05:50AM +, Stephen Finucane wrote:


[snip]


3) Rework docs into a series of guides

   I see the following guides as being helpful

   - Installation guide
   - Usage guide
   - Contributor guide

   I had a 'testing-guide', but this makes more sense split between 
the

middle two. Any other guides?


We probably want to have separate guides for OVS and for OVN, at least
for installation and usage.


Agreed. I'll scope this out a little further once the basic conversion 
is complete.


This would be three separate series so I don't have to do everything 
at
once. I'd also welcome assistance from anyone who wants to take the 
chance

to rework some aspect of the docs.


I can help with some MD->RST.  You mentioned that you've already done 
it

for about half the docs and hadn't posted all of those.


I've actually got a good deal more done over the weekend (it didn't stop 
raining) so there's only the below left:


- INSTALL.Fedora.md
- INSTALL.Libvirt.md
- INSTALL.RHEL.md
- INSTALL.SELinux.md
- INSTALL.SSL.md
- tutorial/Tutorial.md
- tutorial/OVN-Tutorial.md
- ovn/CONTAINERS.OpenStack.md
- ovn/OVN-GW-HA.md
- Documentation/OVSDB-replication.md
- Documentation/release-process.md

(I'm also half way through the FAQ, which is crazy long and should 
probable be broken up at some point)


I won't get a chance to work on these until next weekend at the 
earliest, so if you (or anyone) wants to take a look at any of that 
would be helpful.


As far as converting goes, I've found a lot of the Markdown is broken 
meaning pandoc doesn't handle it well. Doing it manually takes as much 
time as fixing the badly parsed text and gives you the chance to 
refactor some stuff.



Thanks a lot for taking this on!  It's something I've thought of before
(I was thinking of a DocBook or Texinfo route, although I wasn't really
satisfied with either of those; Sphinx seems fine), but I've never
actually taken it on.


No problem. reST is definitely the best choice based on my 
investigations and ties in well with related projects. Hopefully this 
helps lower the entry curve for OVS yet more.


Stephen
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 0/4] High availability support for OVN DB servers using pacemaker

[Andy Zhou]

On Tue, Oct 4, 2016 at 1:37 PM, Ben Pfaff  wrote:

> On Wed, Sep 28, 2016 at 10:00:19AM +0530, bscha...@redhat.com wrote:
> > v2 -> v3:
> > - Handling few edged cases in the OCF script as suggested by
> >   Andrew Beekhof
> >
> > This patch contains changes required to run a pacemaker resource agent
> > to manage OVN db servers in active/standby mode in a HA cluster.
>
> I'm not sure who should review this.  Andy?  Russell?  (me?  I'm
> willing, if requested.)
>

I will try to review them in the next few days.
Since I have not used heartbeat before, It may take some time for me to set
up an environment to try it out.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovs-dev,v2,2/4] ovn-controller: add quiet mode

[Ryan Moats]

Ben Pfaff  wrote on 10/05/2016 01:04:36 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 10/05/2016 01:04 PM
> Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
>
> On Wed, Oct 05, 2016 at 12:52:57PM -0500, Ryan Moats wrote:
> > Ben Pfaff  wrote on 10/05/2016 12:37:26 PM:
> >
> > > From: Ben Pfaff 
> > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > Cc: dev@openvswitch.org
> > > Date: 10/05/2016 12:37 PM
> > > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> > >
> > > On Tue, Oct 04, 2016 at 05:11:37PM -0500, Ryan Moats wrote:
> > > > Ben Pfaff  wrote on 10/04/2016 12:14:32 PM:
> > > >
> > > > > From: Ben Pfaff 
> > > > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > > > Cc: dev@openvswitch.org
> > > > > Date: 10/04/2016 12:14 PM
> > > > > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> > > > >
> > > > > On Wed, Aug 31, 2016 at 03:22:44PM +, Ryan Moats wrote:
> > > > > > As discussed in [1], what the incremental processing code
> > > > > > actually accomplished was that the ovn-controller would
> > > > > > be "quiet" and not burn CPU when things weren't changing.
> > > > > > This patch set recreates this state by calculating whether
> > > > > > changes have occured that would require a full calculation
> > > > > > to be performed.  It does this by persisting a copy of
> > > > > > the localvif_to_ofport and tunnel information in the
> > > > > > controller module, rather than in the physical.c module
> > > > > > as was the case with previous commits.
> > > > > >
> > > > > > [1]
http://openvswitch.org/pipermail/dev/2016-August/078272.html
> > > > > >
> > > > > > Signed-off-by: Ryan Moats 
> > > > >
> > > > > Hi Ryan.
> > > > >
> > > > > I like the idea behind this patch.  However, it no longer applies
to
> > > > > master, so it needs a rebase.
> > > >
> > > > So done, but before submitting a new patch
> > > >
> > > > >
> > > > > It also seems like this TODO should be addressed:
> > > > > +/* TODO (regXboi): this next line is needed for the 3
HVs, 3
> > LS,
> > > > > + * 3 lports/LS, 1 LR test case, but has the potential
side
> > > > effect
> > > > > + * of defeating quiet mode once a logical router leads
to
> > > > creating
> > > > > + * patch ports. Need to understand the failure mode
better
> > and
> > > > > + * what is needed to remove this. */
> > > > > +force_full_process();
> > > >
> > > > I've been looking at what happens here and I'm seeing some
signatures
> > > > that concern me.  The test case that fails is no longer the cited
one
> > above
> > > > but is "2 HVs, 4 lports/HV, localnet ports" ...
> > > >
> > > > What I'm seeing when I peer in is that the information populating
the
> > > > local_datapath structures doesn't appear to be consistent on each
pass
> > > > through binding_run: Looking at the ovn-controller process under
hv2
> > > > for the above test (when it passes), I'll see signatures that look
> > > > like the following:
> > > >
> > > > 2016-10-04T21:57:58.257Z|00020|physical|INFO|looking at binding
record
> > > > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > > > 2016-10-04T21:57:58.257Z|00021|physical|INFO|dp_key is 1
> > > > 2016-10-04T21:57:58.257Z|00022|physical|INFO|looking for ofport of
lp11
> > > > (LP)
> > > > 2016-10-04T21:57:58.257Z|00023|physical|INFO|looking for ofport of
ln1
> > > > (localnet)
> > > > ...
> > > > 2016-10-04T21:57:58.259Z|00034|physical|INFO|looking at binding
record
> > > > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > > > 2016-10-04T21:57:58.259Z|00035|physical|INFO|dp_key is 1
> > > > 2016-10-04T21:57:58.259Z|00036|physical|INFO|looking for ofport of
lp11
> > > > (LP)
> > > > 2016-10-04T21:57:58.259Z|00037|physical|INFO|looking for tunnel to
hv1
> > > > ...
> > >
> > > This sort of oscillation seems super-weird to me.  I'd also like to
> > > learn more.
> > >
> >
> > I found the problem and fixed it in the v3 patch set - some of the
methods
> > (patch_run for example) now include a test of ovs_idl_txn before they
run
> > and some don't (physical_run for example), so you can see guess what
was
> > happening...
> >
> > (If we had a txn to work with everything is fine, but without one,
> > physical_run was trying to calculate flows based on local datapaths
> > with no localnet ports being defined, which leads to the above
> > oscillation.)
> >
> > I added a txn check to the logic for whether to call things or not in
> > ovn-controller.c and I added a note in the email about thinking that a
> > follow-on patch to clean up all of the ovs_idl_txn gates might make
sense
> > (so that nobody trips on this in the future)
>
> It sounds like you found a bug that should be fixed independent of
> "quiet mode".  Should it be two patches, one of which we backport to
> branch-2.6?

I'd say *yes* to two patches, and *yes* to backport to branch-2.6, but
I'd not 

Re: [ovs-dev] [ovs-dev,v2,2/4] ovn-controller: add quiet mode

[Ben Pfaff]

On Wed, Oct 05, 2016 at 12:52:57PM -0500, Ryan Moats wrote:
> Ben Pfaff  wrote on 10/05/2016 12:37:26 PM:
> 
> > From: Ben Pfaff 
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: dev@openvswitch.org
> > Date: 10/05/2016 12:37 PM
> > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> >
> > On Tue, Oct 04, 2016 at 05:11:37PM -0500, Ryan Moats wrote:
> > > Ben Pfaff  wrote on 10/04/2016 12:14:32 PM:
> > >
> > > > From: Ben Pfaff 
> > > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > > Cc: dev@openvswitch.org
> > > > Date: 10/04/2016 12:14 PM
> > > > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> > > >
> > > > On Wed, Aug 31, 2016 at 03:22:44PM +, Ryan Moats wrote:
> > > > > As discussed in [1], what the incremental processing code
> > > > > actually accomplished was that the ovn-controller would
> > > > > be "quiet" and not burn CPU when things weren't changing.
> > > > > This patch set recreates this state by calculating whether
> > > > > changes have occured that would require a full calculation
> > > > > to be performed.  It does this by persisting a copy of
> > > > > the localvif_to_ofport and tunnel information in the
> > > > > controller module, rather than in the physical.c module
> > > > > as was the case with previous commits.
> > > > >
> > > > > [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html
> > > > >
> > > > > Signed-off-by: Ryan Moats 
> > > >
> > > > Hi Ryan.
> > > >
> > > > I like the idea behind this patch.  However, it no longer applies to
> > > > master, so it needs a rebase.
> > >
> > > So done, but before submitting a new patch
> > >
> > > >
> > > > It also seems like this TODO should be addressed:
> > > > +/* TODO (regXboi): this next line is needed for the 3 HVs, 3
> LS,
> > > > + * 3 lports/LS, 1 LR test case, but has the potential side
> > > effect
> > > > + * of defeating quiet mode once a logical router leads to
> > > creating
> > > > + * patch ports. Need to understand the failure mode better
> and
> > > > + * what is needed to remove this. */
> > > > +force_full_process();
> > >
> > > I've been looking at what happens here and I'm seeing some signatures
> > > that concern me.  The test case that fails is no longer the cited one
> above
> > > but is "2 HVs, 4 lports/HV, localnet ports" ...
> > >
> > > What I'm seeing when I peer in is that the information populating the
> > > local_datapath structures doesn't appear to be consistent on each pass
> > > through binding_run: Looking at the ovn-controller process under hv2
> > > for the above test (when it passes), I'll see signatures that look
> > > like the following:
> > >
> > > 2016-10-04T21:57:58.257Z|00020|physical|INFO|looking at binding record
> > > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > > 2016-10-04T21:57:58.257Z|00021|physical|INFO|dp_key is 1
> > > 2016-10-04T21:57:58.257Z|00022|physical|INFO|looking for ofport of lp11
> > > (LP)
> > > 2016-10-04T21:57:58.257Z|00023|physical|INFO|looking for ofport of ln1
> > > (localnet)
> > > ...
> > > 2016-10-04T21:57:58.259Z|00034|physical|INFO|looking at binding record
> > > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > > 2016-10-04T21:57:58.259Z|00035|physical|INFO|dp_key is 1
> > > 2016-10-04T21:57:58.259Z|00036|physical|INFO|looking for ofport of lp11
> > > (LP)
> > > 2016-10-04T21:57:58.259Z|00037|physical|INFO|looking for tunnel to hv1
> > > ...
> >
> > This sort of oscillation seems super-weird to me.  I'd also like to
> > learn more.
> >
> 
> I found the problem and fixed it in the v3 patch set - some of the methods
> (patch_run for example) now include a test of ovs_idl_txn before they run
> and some don't (physical_run for example), so you can see guess what was
> happening...
> 
> (If we had a txn to work with everything is fine, but without one,
> physical_run was trying to calculate flows based on local datapaths
> with no localnet ports being defined, which leads to the above
> oscillation.)
> 
> I added a txn check to the logic for whether to call things or not in
> ovn-controller.c and I added a note in the email about thinking that a
> follow-on patch to clean up all of the ovs_idl_txn gates might make sense
> (so that nobody trips on this in the future)

It sounds like you found a bug that should be fixed independent of
"quiet mode".  Should it be two patches, one of which we backport to
branch-2.6?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovs-dev,v2,2/4] ovn-controller: add quiet mode

[Ryan Moats]

Ben Pfaff  wrote on 10/05/2016 12:37:26 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 10/05/2016 12:37 PM
> Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
>
> On Tue, Oct 04, 2016 at 05:11:37PM -0500, Ryan Moats wrote:
> > Ben Pfaff  wrote on 10/04/2016 12:14:32 PM:
> >
> > > From: Ben Pfaff 
> > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > Cc: dev@openvswitch.org
> > > Date: 10/04/2016 12:14 PM
> > > Subject: Re: [ovs-dev,v2,2/4] ovn-controller: add quiet mode
> > >
> > > On Wed, Aug 31, 2016 at 03:22:44PM +, Ryan Moats wrote:
> > > > As discussed in [1], what the incremental processing code
> > > > actually accomplished was that the ovn-controller would
> > > > be "quiet" and not burn CPU when things weren't changing.
> > > > This patch set recreates this state by calculating whether
> > > > changes have occured that would require a full calculation
> > > > to be performed.  It does this by persisting a copy of
> > > > the localvif_to_ofport and tunnel information in the
> > > > controller module, rather than in the physical.c module
> > > > as was the case with previous commits.
> > > >
> > > > [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html
> > > >
> > > > Signed-off-by: Ryan Moats 
> > >
> > > Hi Ryan.
> > >
> > > I like the idea behind this patch.  However, it no longer applies to
> > > master, so it needs a rebase.
> >
> > So done, but before submitting a new patch
> >
> > >
> > > It also seems like this TODO should be addressed:
> > > +/* TODO (regXboi): this next line is needed for the 3 HVs, 3
LS,
> > > + * 3 lports/LS, 1 LR test case, but has the potential side
> > effect
> > > + * of defeating quiet mode once a logical router leads to
> > creating
> > > + * patch ports. Need to understand the failure mode better
and
> > > + * what is needed to remove this. */
> > > +force_full_process();
> >
> > I've been looking at what happens here and I'm seeing some signatures
> > that concern me.  The test case that fails is no longer the cited one
above
> > but is "2 HVs, 4 lports/HV, localnet ports" ...
> >
> > What I'm seeing when I peer in is that the information populating the
> > local_datapath structures doesn't appear to be consistent on each pass
> > through binding_run: Looking at the ovn-controller process under hv2
> > for the above test (when it passes), I'll see signatures that look
> > like the following:
> >
> > 2016-10-04T21:57:58.257Z|00020|physical|INFO|looking at binding record
> > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > 2016-10-04T21:57:58.257Z|00021|physical|INFO|dp_key is 1
> > 2016-10-04T21:57:58.257Z|00022|physical|INFO|looking for ofport of lp11
> > (LP)
> > 2016-10-04T21:57:58.257Z|00023|physical|INFO|looking for ofport of ln1
> > (localnet)
> > ...
> > 2016-10-04T21:57:58.259Z|00034|physical|INFO|looking at binding record
> > 3736404b-c69d-4878-8d45-81ad9be06f5f
> > 2016-10-04T21:57:58.259Z|00035|physical|INFO|dp_key is 1
> > 2016-10-04T21:57:58.259Z|00036|physical|INFO|looking for ofport of lp11
> > (LP)
> > 2016-10-04T21:57:58.259Z|00037|physical|INFO|looking for tunnel to hv1
> > ...
>
> This sort of oscillation seems super-weird to me.  I'd also like to
> learn more.
>

I found the problem and fixed it in the v3 patch set - some of the methods
(patch_run for example) now include a test of ovs_idl_txn before they run
and some don't (physical_run for example), so you can see guess what was
happening...

(If we had a txn to work with everything is fine, but without one,
physical_run was trying to calculate flows based on local datapaths
with no localnet ports being defined, which leads to the above
oscillation.)

I added a txn check to the logic for whether to call things or not in
ovn-controller.c and I added a note in the email about thinking that a
follow-on patch to clean up all of the ovs_idl_txn gates might make sense
(so that nobody trips on this in the future)

Ryan
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH ovs RFC 9/9] netdev-linux: add tc ingress qdisc support

[Pravin Shelar]

On Wed, Oct 5, 2016 at 8:58 AM, Ben Pfaff  wrote:
> On Wed, Oct 05, 2016 at 05:53:14PM +0300, Or Gerlitz wrote:
>> On 10/5/2016 3:27 AM, Ben Pfaff wrote:
>> >On Tue, Sep 27, 2016 at 03:46:04PM +0300, Paul Blakey wrote:
>> >>>Add tc ingress qdisc support so we can add qdisc
>> >>>as a qos on port or through config.
>> >>>usage:
>> >>>ovs-vsctl -- set port  qos=@newq -- --id=@newq create \
>> >>>qos type=linux-ingress
>> >>>where  is a already added port using vsctl add-port.
>> >>>
>> >>>Signed-off-by: Paul Blakey
>> >>>Signed-off-by: Shahar Klein
>> >I don't plan to review all of these patches but this one caught my eye.
>>
>> any specail reason for not looking on this series?
>
> I was under the impression that Pravin was planning to make preliminary
> comments.  Pravin, is that true?

I am planing on reviewing it. But I have not looked it in details yet.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-controller: Container can have connection to a hosting VM.

[Ben Pfaff]

On Tue, Oct 04, 2016 at 09:56:15AM -0700, Gurucharan Shetty wrote:
> A Container running inside a VM can have a connection to the
> hosting VM (parent port) in the logical topology (for e.g via a router).
> So we should be able to loop-back into the same VM, even if the
> final packet delivered does not have any tags in it.
> 
> Reported-by: Dustin Spinhirne 
> Signed-off-by: Gurucharan Shetty 

It looks to me like, after this patch, we're effectively acting as if
flags.allow_loopback is always set to 1.  Is that right?  If so, then we
can simplify the code a bit.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 0/5] Integrate Sphinx documentation generator

[Ben Pfaff]

On Wed, Oct 05, 2016 at 10:05:50AM +, Stephen Finucane wrote:
> On 2016-10-04 22:01, Ben Pfaff wrote:
> >On Sat, Oct 01, 2016 at 08:01:29PM +0100, Stephen Finucane wrote:
> >>Since the move to GitHub, OVS has increasingly used Markdown for all of
> >>its documentation. This seems like a natural fit, given Markdown's low
> >>overhead and support in the GitHub web UI. However, Markdown is a
> >>documentation format, not a documentation system, and it starts to fall
> >>over as you add more and more documents and see the need for thing like
> >>cross-referencing, or insertion of partial documents into other docs.
> >>
> >>I'd like to propose integrating the Sphinx documentation tool into the
> >>OVS documentation. This would mean converting most or all of the
> >>documentation to reStructuredText, but it would bring significant
> >>advantages including:
> >>
> >>* native cross-referencing
> >>* multiple output formats (including pretty HTML)
> >>* automatic indexing/searching of docs
> >>* hierarchial structuring of the docs
> >>* DRY documents, through inclusion of partial docs in multiple files
> >>* ...
> >>
> >>I chose rST+Sphinx due to the overwhelming popularity of the tool
> >>within other open source communities like the Linux kernel and
> >>OpenStack, which have all migrated documentation from other tools
> >>(DocBook, mostly) in recent times.
> >>
> >>Documents located in the top-level directory should also be converted
> >>to rST to be consistent, but these should not include any Sphinx
> >>extensions to ensure that these continue to be readable on GitHub.
> >>
> >>I've included some patches that show the kind of changes that would be
> >>necessary. I've actually converted far more of the docs (~40%?) at this
> >>point, but I'd like to gauge interest before continuing with the
> >>remainder. I'd also appreciate help from folks who would take a stab at
> >>coverting docs so I don't have to do it all by myself.
> >>
> >>Feel free to direct any questions at me.
> >
> >This seems quite nice.  I played with it for a few minutes and looked at
> >the generated HTML output, which is nicer than the current "make
> >dist-docs" output.
> >
> >I assume that, if this were to be fully converted, then the generated
> >webpage would replace "make dist-docs" and point to all the
> >documentation that that currently outputs?
> 
> Yes, that would be the intention. We could keep the 'dist-docs' target to
> build offline copies of the documentation but it would eventually use sphinx
> instead of the 'dist-docs' script.

OK.

> I would also like to provide a custom stylesheet/Sphinx theme so we could
> display docs on the current documentation page [1] with a consistent style.
> For an example of what's possible here, look at the OpenStack docs [2], the
> Nova developer docs [3] and the Python docs [4]. Despite varying
> appearances, all are generated using Sphinx.

Sounds good to me.

> >In particular, it would be
> >good to make sure that the manpages make it into the output.
> 
> manpages would be included. We may wish to rewrite these in reST like
> OpenStack do [5], but this is totally optional and should be done at another
> time.

For me, at least, that's something of a separate issue, and I agree that
it should be deferred.

> >I had to fold in the appended patch to get "make" to pass, to support
> >out-of-tree builds, and to make the license disappear from the "unit
> >tests" page.
> 
> Yeah, the Makefile was a (poor) first pass, so thanks for the patch. The
> proper version will be actually tested.
> 
> Assuming we're good with the idea, here's my current POA.

I'm happy with the idea.

> 1) Convert all docs in-place to reStructuredText
> 
>Update the 'dist-docs' script to use 'rst2html' [6] for '*.rst' files.
> The documentation output will remain essentially the same for now. This will
> be a slog, but it's rather trivial to review (cursory visual inspection) and
> is necessary for (2)

OK.  I can help with the md->rst conversion.

> 2) Integrate Sphinx
> 
>Move all documents except traditional top-level docs (README, CodingStyle
> etc.) to 'Documentation' 

That's probably overdue anyway.

> and build with sphinx instead of 'rst2html'. The openvswitch.org docs
> page [1] will start using the sphinx output. Files would retain their
> existing names and structure meaning the docs index page will simply
> be a dumb list of these files (like today, really).

OK.

> 3) Rework docs into a series of guides
> 
>I see the following guides as being helpful
> 
>- Installation guide
>- Usage guide
>- Contributor guide
> 
>I had a 'testing-guide', but this makes more sense split between the
> middle two. Any other guides?

We probably want to have separate guides for OVS and for OVN, at least
for installation and usage.

> This would be three separate series so I don't have to do everything at
> once. I'd also welcome assistance from anyone who wants to take the chance
> to rework some 

Re: [ovs-dev] [PATCH v2 1/1] json: Serialize strings using a lookup table

[Ben Pfaff]

On Wed, Oct 05, 2016 at 04:47:21PM +, Rodriguez Betancourt, Esteban wrote:
> The existing implementation uses a switch with
> many conditions, that when compiled is translated
> to a not optimal series of conditional jumps.
> 
> With a lookup table the generated code has less conditional jumps,
> that should translate in improving the CPU ability to predict the
> jumps.

Thanks, applied to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [patch_v2] ovn: Add additional comments regarding arp responders.

[Darrell Ball]

There has been enough confusion regarding logical switch datapath
arp responders in ovn to warrant some additional comments;
hence add a general description regarding why they exist and
document the special cases.

Signed-off-by: Darrell Ball 
---

v1->v2: Dropped RFC code change for logical switch router
type ports

 ovn/northd/ovn-northd.8.xml | 37 ++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 77eb3d1..7496284 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -415,14 +415,45 @@
 Ingress Table 9: ARP/ND responder
 
 
-  This table implements ARP/ND responder for known IPs.  It contains these
-  logical flows:
+  This table implements ARP/ND responder for known IPs.  The advantage
+  of the arp responder flow is to limit arp broadcasts by locally
+  responding to arp requests without the need to send to other
+  hypervisors.  One common case is when the inport is a logical
+  port associated with a VIF and the broadcast is responded to on the
+  local hypervisor rather than broadcast across the whole network and
+  responded to by the destination VM.  This behavior is proxy arp.
+  It contains these logical flows:
 
 
 
   
 Priority-100 flows to skip ARP responder if inport is of type
-localnet, and advances directly to the next table.
+localnet or vtep and advances
+directly to the next table.  localnet and
+vtep port types are skipped, otherwise the arp
+request is received by multiple hypervisors, which all have the
+same mac binding downloaded from northd, which will cause redundant
+arp replies, confusing the originator of the arp request.  The
+inport being of type router has no valid use case.
+No special filtering is done for these packets, as there would
+be some additional flow cost for this and the value appears
+limited, at this time.  The inport of type empty is the most
+common case and includes both proxy arp for other VMs and also
+logical router ports (if a corresponding IP address is configured
+on the peer logical switch router type port).  There is a
+requirement that both MAC and IP (if configured) be manually
+enforced to match on logical switch router type and logical
+router port peers.  If the logical switch router type port does
+not have an IP address configured, arp requests will hit another
+arp responder on the logical router datapath itself, which is
+most commonly a distributed logical router.  There is a
+disadvantage to using the logical router datapath arp responder,
+when both logical switch and logical router datapath arp
+responders can be used, in that the arp request from a VM would
+have already hit the logical switch datapath broadcast rule.
+There is a special case: north->south traffic using the l2gateway
+port will use an arp responder on the l2 gateway chassis in
+the context of a logical switch datapath.
   
 
   
-- 
1.9.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] python: Add SSL support to the python ovs client library

[Ben Pfaff]

On Wed, Oct 05, 2016 at 05:50:24PM +0530, Numan Siddique wrote:
> SSL support is added to the ovs/stream.py. pyOpenSSL library is used
> to support SSL. If this library is not present, then the SSL stream
> is not registered with the Stream class.
> 
> Signed-off-by: Numan Siddique 

Applied, thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2 1/1] json: Serialize strings using a lookup table

[Rodriguez Betancourt, Esteban]

The existing implementation uses a switch with
many conditions, that when compiled is translated
to a not optimal series of conditional jumps.

With a lookup table the generated code has less conditional jumps,
that should translate in improving the CPU ability to predict the
jumps.

Performance Comparison:
All the timings are in nanoseconds, "OVS Master" corresponds to 13a1d36.
N is the number of repetitions

Serialize vswitch.ovsschema
NOVS Master  Lookup TableDifferenceDiff per op
1   233182200369   3281332813
10 2724931   1919168  80576380576.3
100   22802794  24406648-1603854   -16038.54
1000 253645888 2062597604738612847386.128
1   23522457031906839780   44540592344540.5923
10 23967770920   19012178655  495559226549555.92265

Serialize echo example
NOVS Master  Lookup TableDifferenceDiff per op
1385712565 -8708-8708
10  17403 7312 10091 1009.1
100 5785956613  1246   12.46
1000   592310   528110 64200   64.2
1 6096334  5576109520225   52.0225
10   60970439 58477626   2492813   24.92813

Serialize mutate example
NOVS Master  Lookup TableDifferenceDiff per op
17115  19051 -11936  -11936
10  34110  39561  -5451-545.1
100296613 298645  -2032-20.32
1000  35104992930588 579911579.911
133898710   302786313620079362.0079
10  305069356  280622992   24446364244.46364

Signed-off-by: Esteban Rodriguez Betancourt 
---
I can't believe that I didn't send the patch in the previous mail, sorry about 
that.
Regards,
Esteban

 lib/json.c | 76 +-
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/lib/json.c b/lib/json.c
index 995f3c2..080e881 100644
--- a/lib/json.c
+++ b/lib/json.c
@@ -1610,49 +1610,53 @@ json_serialize_array(const struct json_array *array, 
struct json_serializer *s)
 ds_put_char(ds, ']');
 }
 
+const char *chars_escaping[256] = {
+"\\u", "\\u0001", "\\u0002", "\\u0003", "\\u0004", "\\u0005", 
"\\u0006", "\\u0007",
+"\\b", "\\t", "\\n", "\\u000b", "\\f", "\\r", "\\u000e", "\\u000f",
+"\\u0010", "\\u0011", "\\u0012", "\\u0013", "\\u0014", "\\u0015", 
"\\u0016", "\\u0017",
+"\\u0018", "\\u0019", "\\u001a", "\\u001b", "\\u001c", "\\u001d", 
"\\u001e", "\\u001f",
+" ", "!", "\\\"", "#", "$", "%", "&", "'",
+"(", ")", "*", "+", ",", "-", ".", "/",
+"0", "1", "2", "3", "4", "5", "6", "7",
+"8", "9", ":", ";", "<", "=", ">", "?",
+"@", "A", "B", "C", "D", "E", "F", "G",
+"H", "I", "J", "K", "L", "M", "N", "O",
+"P", "Q", "R", "S", "T", "U", "V", "W",
+"X", "Y", "Z", "[", "", "]", "^", "_",
+"`", "a", "b", "c", "d", "e", "f", "g",
+"h", "i", "j", "k", "l", "m", "n", "o",
+"p", "q", "r", "s", "t", "u", "v", "w",
+"x", "y", "z", "{", "|", "}", "~", "\x7f",
+"\x80", "\x81", "\x82", "\x83", "\x84", "\x85", "\x86", "\x87",
+"\x88", "\x89", "\x8a", "\x8b", "\x8c", "\x8d", "\x8e", "\x8f",
+"\x90", "\x91", "\x92", "\x93", "\x94", "\x95", "\x96", "\x97",
+"\x98", "\x99", "\x9a", "\x9b", "\x9c", "\x9d", "\x9e", "\x9f",
+"\xa0", "\xa1", "\xa2", "\xa3", "\xa4", "\xa5", "\xa6", "\xa7",
+"\xa8", "\xa9", "\xaa", "\xab", "\xac", "\xad", "\xae", "\xaf",
+"\xb0", "\xb1", "\xb2", "\xb3", "\xb4", "\xb5", "\xb6", "\xb7",
+"\xb8", "\xb9", "\xba", "\xbb", "\xbc", "\xbd", "\xbe", "\xbf",
+"\xc0", "\xc1", "\xc2", "\xc3", "\xc4", "\xc5", "\xc6", "\xc7",
+"\xc8", "\xc9", "\xca", "\xcb", "\xcc", "\xcd", "\xce", "\xcf",
+"\xd0", "\xd1", "\xd2", "\xd3", "\xd4", "\xd5", "\xd6", "\xd7",
+"\xd8", "\xd9", "\xda", "\xdb", "\xdc", "\xdd", "\xde", "\xdf",
+"\xe0", "\xe1", "\xe2", "\xe3", "\xe4", "\xe5", "\xe6", "\xe7",
+"\xe8", "\xe9", "\xea", "\xeb", "\xec", "\xed", "\xee", "\xef",
+"\xf0", "\xf1", "\xf2", "\xf3", "\xf4", "\xf5", "\xf6", "\xf7",
+"\xf8", "\xf9", "\xfa", "\xfb", "\xfc", "\xfd", "\xfe", "\xff"
+};
+
 static void
 json_serialize_string(const char *string, struct ds *ds)
 {
 uint8_t c;
+uint8_t c2;
+const char *escape;
 
 ds_put_char(ds, '"');
 while ((c = *string++) != '\0') {
-switch (c) {
-case '"':
-ds_put_cstr(ds, "\\\"");
-break;
-
-case '\\':
-ds_put_cstr(ds, "");
-break;
-
-case '\b':
-ds_put_cstr(ds, "\\b");
-break;
-
-case '\f':
-   

Re: [ovs-dev] [Backport Request] For branch-2.5, backport rhel-systemd integration

[Ben Pfaff]

On Wed, Oct 05, 2016 at 11:09:39AM -0400, Aaron Conole wrote:
> Ben Pfaff  writes:
> 
> > On Thu, Sep 08, 2016 at 07:58:21PM +0100, Markos Chandras wrote:
> >> On 09/08/2016 05:50 PM, Aaron Conole wrote:
> >> > 
> >> >> It sounds like new feature territory, but you do make a case for
> >> > it being considered a set of fixes ...
> >> > 
> >> > I agree - it straddles a line.  I was hesitant to even ask, but in RHEL
> >> > we probably need to backport these anyway, so I made an assumption
> >> > (maybe poor) that other systemd distros might use the rhel scripts and
> >> > run into this class of issues also.  Good net-izen, and all that :)
> >> > 
> >> > -Aaron
> >> 
> >> Hi Aaron,
> >> 
> >> Thanks for the backport request. I am also interested in these fixes and
> >> it's something I could use in the SUSE package as well. For my point of
> >> view I see no blockers for those updating their 2.5.0 installations with
> >> these fixes in but I will do some testing on my end as well.
> >
> > I don't know whether we came to a conclusion on this or whether the
> > discussion just dropped.  Is it still desirable?
> 
> It's definitely desirable from my PoV.  I'd like to see it, and have
> already gotten it packaged in a local copy of an RPM, just doing some
> testing.
> 
> I don't know if Russell or Flavio have any thoughts.

I'm OK with this backport, but I'll leave it to Russell since ultimately
it's to improve Red Hat integration.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] json: Serialize strings using a lookup table

[Ben Pfaff]

I just noticed this email.  It sounds beneficial, but no patch was
included.  Do you want to send the patch?

On Wed, Sep 07, 2016 at 08:58:02PM +, Rodriguez Betancourt, Esteban wrote:
> The existing implementation uses a switch with
> many conditions, that when compiled is translated
> to a not optimal series of conditional jumps.
> 
> With a lookup table the generated code has less conditional jumps,
> that should translate in improving the CPU ability to predict the
> jumps.
> 
> Performance Comparison:
> All the timings are in nanoseconds, "OVS Master" corresponds to 13a1d36.
> N is the number of repetitions
> 
> Serialize vswitch.ovsschema
> NOVS Master  Lookup TableDifferenceDiff per op
> 1   233182200369   3281332813
> 10 2724931   1919168  80576380576.3
> 100   22802794  24406648-1603854   -16038.54
> 1000 253645888 2062597604738612847386.128
> 1   23522457031906839780   44540592344540.5923
> 10 23967770920   19012178655  495559226549555.92265
> 
> Serialize echo example
> NOVS Master  Lookup TableDifferenceDiff per op
> 1385712565 -8708-8708
> 10  17403 7312 10091 1009.1
> 100 5785956613  1246   12.46
> 1000   592310   528110 64200   64.2
> 1 6096334  5576109520225   52.0225
> 10   60970439 58477626   2492813   24.92813
> 
> Serialize mutate example
> NOVS Master  Lookup TableDifferenceDiff per op
> 17115  19051 -11936  -11936
> 10  34110  39561  -5451-545.1
> 100296613 298645  -2032-20.32
> 1000  35104992930588 579911579.911
> 133898710   302786313620079362.0079
> 10  305069356  280622992   24446364244.46364
> 
> 
> Signed-off-by: Esteban Rodriguez Betancourt 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofproto: Always delete rules before deleting a meter.

[Ben Pfaff]

On Tue, Oct 04, 2016 at 06:17:08PM -0700, Jarno Rajahalme wrote:
> 
> > On Oct 4, 2016, at 2:32 PM, Ben Pfaff  wrote:
> > 
> > On Fri, Sep 30, 2016 at 11:08:19AM -0700, Jarno Rajahalme wrote:
> >> When deleting a bridge it is currently possible to delete a mater
> >> without deleting the rules using the meter first.  Fix this by moving
> >> the meter's rule deletion to meter_delete().
> >> 
> >> Signed-off-by: Jarno Rajahalme 
> >> Reported-by: Petr Machata 
> > 
> > Acked-by: Ben Pfaff 
> 
> Thanks for the review!
> 
> Pushed to master, but not sure if need to backport to earlier
> branches, as meters are not really supported by any release yet?

If I recall correctly it's not even possible to add a meter in any
release, so this is just a theoretical bug fix.  If so, then I agree, no
need for backports.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] INSTALL.Docker: Explain how to set up a system-id.

[Guru Shetty]

On 4 October 2016 at 16:01, Ben Pfaff  wrote:

> Reported-by: Hui Kang 
> Signed-off-by: Ben Pfaff 
>

Acked-by: Gurucharan Shetty 

> ---
>  INSTALL.Docker.md | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/INSTALL.Docker.md b/INSTALL.Docker.md
> index b62922d..097452f 100644
> --- a/INSTALL.Docker.md
> +++ b/INSTALL.Docker.md
> @@ -83,6 +83,19 @@ ovs-vsctl set Open_vSwitch .
> external_ids:ovn-remote="tcp:$CENTRAL_IP:6642" \
>external_ids:ovn-nb="tcp:$CENTRAL_IP:6641" 
> external_ids:ovn-encap-ip=$LOCAL_IP
> external_ids:ovn-encap-type="$ENCAP_TYPE"
>  ```
>
> +Each Open vSwitch instance in an OVN deployment needs a unique, persistent
> +identifier, called the "system-id".  If you install OVS from distribution
> +packaging for Open vSwitch (e.g. .deb or .rpm packages), or if you use the
> +ovs-ctl utility included with Open vSwitch, it automatically configures a
> +system-id.  If you start Open vSwitch manually, you should set one up
> yourself,
> +e.g.:
> +
> +```
> +id_file=/etc/openvswitch/system-id.conf
> +test -e $id_file || uuidgen > $id_file
> +ovs-vsctl set Open_vSwitch . external_ids:system-id=$(cat $id_file)
> +```
> +
>  And finally, start the ovn-controller.  (You need to run the below command
>  on every boot)
>
> --
> 2.1.3
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] Fix missing system-id in INSTALL.Docker

[Ben Pfaff]

Thanks!

On Tue, Oct 04, 2016 at 08:38:19PM -0400, Hui Kang wrote:
> I like your version better.
> I am ok abandoning this patch. Thanks.
> 
> - Hui
> 
> On Tue, Oct 4, 2016 at 7:01 PM, Ben Pfaff  wrote:
> > On Tue, Oct 04, 2016 at 04:33:02PM -0400, Hui Kang wrote:
> >> Signed-off-by: Hui Kang 
> >> ---
> >>  INSTALL.Docker.md | 8 
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/INSTALL.Docker.md b/INSTALL.Docker.md
> >> index b62922d..4606354 100644
> >> --- a/INSTALL.Docker.md
> >> +++ b/INSTALL.Docker.md
> >> @@ -83,6 +83,14 @@ ovs-vsctl set Open_vSwitch . 
> >> external_ids:ovn-remote="tcp:$CENTRAL_IP:6642" \
> >>external_ids:ovn-nb="tcp:$CENTRAL_IP:6641" 
> >> external_ids:ovn-encap-ip=$LOCAL_IP 
> >> external_ids:ovn-encap-type="$ENCAP_TYPE"
> >>  ```
> >>
> >> +Note that if Open vSwitch is started manually (without the assistance of 
> >> system
> >> +startup scripts provided by packages like .deb or .rpm), you should 
> >> provide a
> >> +unique identification, $SYSTEM_ID for this host:
> >> +
> >> +```
> >> +ovs-vsctl set Open_vSwitch . external_ids:system-id=$SYSTEM_ID
> >> +```
> >> +
> >>  And finally, start the ovn-controller.  (You need to run the below command
> >>  on every boot)
> >
> > Thanks for working on this.
> >
> > I'm concerned about this wording, because the system-id should be
> > persistent and unique and this isn't specific enough to encourage people
> > to do it the right way.
> >
> > I posted my version, see what you think:
> > https://patchwork.ozlabs.org/patch/678263/
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH ovs RFC 9/9] netdev-linux: add tc ingress qdisc support

[Ben Pfaff]

On Wed, Oct 05, 2016 at 05:53:14PM +0300, Or Gerlitz wrote:
> On 10/5/2016 3:27 AM, Ben Pfaff wrote:
> >On Tue, Sep 27, 2016 at 03:46:04PM +0300, Paul Blakey wrote:
> >>>Add tc ingress qdisc support so we can add qdisc
> >>>as a qos on port or through config.
> >>>usage:
> >>>ovs-vsctl -- set port  qos=@newq -- --id=@newq create \
> >>>qos type=linux-ingress
> >>>where  is a already added port using vsctl add-port.
> >>>
> >>>Signed-off-by: Paul Blakey
> >>>Signed-off-by: Shahar Klein
> >I don't plan to review all of these patches but this one caught my eye.
> 
> any specail reason for not looking on this series?

I was under the impression that Pravin was planning to make preliminary
comments.  Pravin, is that true?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 1/5] Add initial sphinx configuration

[Ben Pfaff]

On Wed, Oct 05, 2016 at 10:07:59AM +, Stephen Finucane wrote:
> Speaking of licenses, I've included the Apache 2.0 license header at the top
> of all reworked docs. Is this necessary?

I think that it is a good idea to do this.  A fair number of projects
use different source and documentation licenses, so being explicit is
helpful to downstreams.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [Backport Request] For branch-2.5, backport rhel-systemd integration

[Aaron Conole]

Ben Pfaff  writes:

> On Thu, Sep 08, 2016 at 07:58:21PM +0100, Markos Chandras wrote:
>> On 09/08/2016 05:50 PM, Aaron Conole wrote:
>> > 
>> >> It sounds like new feature territory, but you do make a case for
>> > it being considered a set of fixes ...
>> > 
>> > I agree - it straddles a line.  I was hesitant to even ask, but in RHEL
>> > we probably need to backport these anyway, so I made an assumption
>> > (maybe poor) that other systemd distros might use the rhel scripts and
>> > run into this class of issues also.  Good net-izen, and all that :)
>> > 
>> > -Aaron
>> 
>> Hi Aaron,
>> 
>> Thanks for the backport request. I am also interested in these fixes and
>> it's something I could use in the SUSE package as well. For my point of
>> view I see no blockers for those updating their 2.5.0 installations with
>> these fixes in but I will do some testing on my end as well.
>
> I don't know whether we came to a conclusion on this or whether the
> discussion just dropped.  Is it still desirable?

It's definitely desirable from my PoV.  I'd like to see it, and have
already gotten it packaged in a local copy of an RPM, just doing some
testing.

I don't know if Russell or Flavio have any thoughts.

Thanks, Ben!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v3] ovn-controller: add quiet mode

[Ryan Moats]

As discussed in [1], what the incremental processing code
actually accomplished was that the ovn-controller would
be "quiet" and not burn CPU when things weren't changing.
This patch set recreates this state by calculating whether
changes have occured that would require a full calculation
to be performed.  It does this by persisting a copy of
the localvif_to_ofport and tunnel information in the
controller module, rather than in the physical.c module
as was the case with previous commits.

[1] http://openvswitch.org/pipermail/dev/2016-August/078272.html

Signed-off-by: Ryan Moats 
---
Note: it may make sense to consider a follow on patch to clean
up all the tests for the existence of ovs_idl_txn into the
main ovn-controller loop...

 ovn/controller/ofctrl.c |   2 +
 ovn/controller/ovn-controller.c |  71 +++-
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c  |   5 ++
 ovn/controller/physical.c   | 100 
 ovn/controller/physical.h   |   5 ++
 6 files changed, 123 insertions(+), 61 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 1d8fbf3..a3f465f 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -377,6 +377,8 @@ run_S_CLEAR_FLOWS(void)
 queue_msg(encode_group_mod());
 ofputil_uninit_group_mod();
 
+force_full_process();
+
 /* Clear existing groups, to match the state of the switch. */
 if (groups) {
 ovn_group_table_clear(groups, true);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 00392ca..3c6b8b3 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -402,6 +402,23 @@ get_nb_cfg(struct ovsdb_idl *idl)
 return sb ? sb->nb_cfg : 0;
 }
 
+/* Contains mapping of localvifs to OF ports for detecting if the physical
+ * configuration of the switch has changed. */
+static struct simap localvif_to_ofport =
+SIMAP_INITIALIZER(_to_ofport);
+
+/* Contains the list of known tunnels. */
+static struct hmap tunnels = HMAP_INITIALIZER();
+
+/* The last sequence number seen from the southbound IDL. */
+static unsigned int seqno = 0;
+
+void
+force_full_process(void)
+{
+seqno = 0;
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -528,42 +545,52 @@ main(int argc, char *argv[])
 _lports);
 }
 
+bool physical_change = true;
 if (br_int && chassis) {
+enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
+ _ct_zones);
+physical_change = detect_and_save_physical_changes(
+_to_ofport, , mff_ovn_geneve, br_int,
+chassis_id);
+unsigned int cur_seqno = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl);
+
 patch_run(, br_int, chassis_id, _datapaths,
   _datapaths);
 
 static struct lport_index lports;
-static struct mcgroup_index mcgroups;
 lport_index_init(, ctx.ovnsb_idl);
-mcgroup_index_init(, ctx.ovnsb_idl);
-
-enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
- _ct_zones);
 
 pinctrl_run(, , br_int, chassis_id, _datapaths);
 update_ct_zones(_lports, _datapaths, _zones,
 ct_zone_bitmap, _ct_zones);
 commit_ct_zones(, br_int, _ct_zones);
 
-struct hmap flow_table = HMAP_INITIALIZER(_table);
-lflow_run(, , , _datapaths,
-  _datapaths, _table, _zones,
-  _table);
-
-physical_run(, mff_ovn_geneve,
- br_int, chassis_id, _zones, _table,
- _datapaths, _datapaths);
-
-ofctrl_put(_table, _ct_zones,
-   get_nb_cfg(ctx.ovnsb_idl));
-hmap_destroy(_table);
-if (ctx.ovnsb_idl_txn) {
-int64_t cur_cfg = ofctrl_get_cur_cfg();
-if (cur_cfg && cur_cfg != chassis->nb_cfg) {
-sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+if (ctx.ovs_idl_txn && (physical_change || seqno < cur_seqno)) {
+seqno = cur_seqno;
+
+static struct mcgroup_index mcgroups;
+mcgroup_index_init(, ctx.ovnsb_idl);
+
+struct hmap flow_table = HMAP_INITIALIZER(_table);
+lflow_run(, , , _datapaths,
+  _datapaths, _table, _zones,
+  _table);
+
+physical_run(, mff_ovn_geneve,
+ br_int, chassis_id, _zones, _table,
+ _datapaths, _datapaths);
+
+ofctrl_put(_table, _ct_zones,
+   get_nb_cfg(ctx.ovnsb_idl));
+hmap_destroy(_table);
+if 

Re: [ovs-dev] [PATCH ovs RFC 9/9] netdev-linux: add tc ingress qdisc support

[Or Gerlitz]

On 10/5/2016 3:27 AM, Ben Pfaff wrote:

On Tue, Sep 27, 2016 at 03:46:04PM +0300, Paul Blakey wrote:

>Add tc ingress qdisc support so we can add qdisc
>as a qos on port or through config.
>usage:
>ovs-vsctl -- set port  qos=@newq -- --id=@newq create \
>qos type=linux-ingress
>where  is a already added port using vsctl add-port.
>
>Signed-off-by: Paul Blakey
>Signed-off-by: Shahar Klein

I don't plan to review all of these patches but this one caught my eye.


any specail reason for not looking on this series?

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 02/13] netdev-nodpdk.c: Add missing copyright.

[Aaron Conole]

Daniele Di Proietto  writes:

> Looks like we forgot to add the copyright headers to netdev-dpdk.h.
> Looking at the contribution history of the file, this commit adds the
> header with Red Hat copyright.
>
> CC: Aaron Conole 
> Signed-off-by: Daniele Di Proietto 

ACK
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2] python: Add SSL support to the python ovs client library

[Numan Siddique]

SSL support is added to the ovs/stream.py. pyOpenSSL library is used
to support SSL. If this library is not present, then the SSL stream
is not registered with the Stream class.

Signed-off-by: Numan Siddique 
---
 python/ovs/poller.py |  8 +
 python/ovs/stream.py | 91 ++--
 tests/ovsdb-idl.at   | 30 +++--
 tests/test-ovsdb.py  |  7 
 4 files changed, 131 insertions(+), 5 deletions(-)

v1 -> v2

 * Fixed the TypeError exception seen with pyopenssl version 0.14

diff --git a/python/ovs/poller.py b/python/ovs/poller.py
index de6bf22..d7cb7d3 100644
--- a/python/ovs/poller.py
+++ b/python/ovs/poller.py
@@ -20,6 +20,11 @@ import socket
 import os
 
 try:
+from OpenSSL import SSL
+except ImportError:
+SSL = None
+
+try:
 import eventlet.patcher
 
 def _using_eventlet_green_select():
@@ -54,6 +59,9 @@ class _SelectSelect(object):
 def register(self, fd, events):
 if isinstance(fd, socket.socket):
 fd = fd.fileno()
+if SSL and isinstance(fd, SSL.Connection):
+fd = fd.fileno()
+
 assert isinstance(fd, int)
 if events & POLLIN:
 self.rlist.append(fd)
diff --git a/python/ovs/stream.py b/python/ovs/stream.py
index 97b22ac..59578b1 100644
--- a/python/ovs/stream.py
+++ b/python/ovs/stream.py
@@ -22,6 +22,11 @@ import ovs.poller
 import ovs.socket_util
 import ovs.vlog
 
+try:
+from OpenSSL import SSL
+except ImportError:
+SSL = None
+
 vlog = ovs.vlog.Vlog("stream")
 
 
@@ -39,7 +44,7 @@ def stream_or_pstream_needs_probes(name):
 
 
 class Stream(object):
-"""Bidirectional byte stream.  Currently only Unix domain sockets
+"""Bidirectional byte stream.  Unix domain sockets, tcp and ssl
 are implemented."""
 
 # States.
@@ -54,6 +59,10 @@ class Stream(object):
 
 _SOCKET_METHODS = {}
 
+_SSL_private_key_file = None
+_SSL_certificate_file = None
+_SSL_ca_cert_file = None
+
 @staticmethod
 def register_method(method, cls):
 Stream._SOCKET_METHODS[method + ":"] = cls
@@ -68,7 +77,7 @@ class Stream(object):
 @staticmethod
 def is_valid_name(name):
 """Returns True if 'name' is a stream name in the form "TYPE:ARGS" and
-TYPE is a supported stream type (currently only "unix:" and "tcp:"),
+TYPE is a supported stream type ("unix:", "tcp:" and "ssl:"),
 otherwise False."""
 return bool(Stream._find_method(name))
 
@@ -116,7 +125,7 @@ class Stream(object):
 return error, None
 else:
 status = ovs.socket_util.check_connection_completion(sock)
-return 0, Stream(sock, name, status)
+return 0, cls(sock, name, status)
 
 @staticmethod
 def _open(suffix, dscp):
@@ -264,6 +273,18 @@ class Stream(object):
 # Don't delete the file: we might have forked.
 self.socket.close()
 
+@staticmethod
+def ssl_set_private_key_file(file_name):
+Stream._SSL_private_key_file = file_name
+
+@staticmethod
+def ssl_set_certificate_file(file_name):
+Stream._SSL_certificate_file = file_name
+
+@staticmethod
+def ssl_set_ca_cert_file(file_name):
+Stream._SSL_ca_cert_file = file_name
+
 
 class PassiveStream(object):
 @staticmethod
@@ -362,6 +383,7 @@ def usage(name):
 Active %s connection methods:
   unix:FILE   Unix domain socket named FILE
   tcp:IP:PORT TCP socket to IP with port no of PORT
+  ssl:IP:PORT SSL socket to IP with port no of PORT
 
 Passive %s connection methods:
   punix:FILE  Listen on Unix domain socket FILE""" % (name, name)
@@ -385,3 +407,66 @@ class TCPStream(Stream):
 sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
 return error, sock
 Stream.register_method("tcp", TCPStream)
+
+
+class SSLStream(Stream):
+
+@staticmethod
+def verify_cb(conn, cert, errnum, depth, ok):
+return ok
+
+@staticmethod
+def _open(suffix, dscp):
+error, sock = TCPStream._open(suffix, dscp)
+if error:
+return error, None
+
+# Create an SSL context
+ctx = SSL.Context(SSL.SSLv23_METHOD)
+ctx.set_verify(SSL.VERIFY_PEER, SSLStream.verify_cb)
+ctx.set_options(SSL.OP_NO_SSLv2 | SSL.OP_NO_SSLv3)
+ctx.set_session_cache_mode(SSL.SESS_CACHE_OFF)
+# If the client has not set the SSL configuration files
+# exception would be raised.
+ctx.use_privatekey_file(Stream._SSL_private_key_file)
+ctx.use_certificate_file(Stream._SSL_certificate_file)
+ctx.load_verify_locations(Stream._SSL_ca_cert_file)
+
+ssl_sock = SSL.Connection(ctx, sock)
+ssl_sock.set_connect_state()
+return error, ssl_sock
+
+def connect(self):
+retval = super(SSLStream, self).connect()
+
+if retval:
+return retval
+
+# TCP 

[ovs-dev] RETURNED MAIL: SEE TRANSCRIPT FOR DETAILS

[MAILER-DAEMON]

The original message was received at Wed, 5 Oct 2016 17:19:44 +0530
from [5.250.163.206]

- The following addresses had permanent fatal errors -




___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v10] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Thanks very much.

> On Oct 4, 2016, at 5:12 AM, Guru Shetty  wrote:
> 
> I applied the patch. For future, when you version, please have some sort of 
> versioning information that tells the difference between the versions below 
> the "---" line.

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 3/4] ovn-nbctl: Fix memory leak in nbctl_lr_route_add

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c |  6 ++
 tests/ovn-nbctl.at| 16 
 2 files changed, 22 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 453ff72..572370f 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2039,17 +2039,22 @@ nbctl_lr_route_add(struct ctl_context *ctx)
 
 next_hop = normalize_prefix_str(ctx->argv[3]);
 if (!next_hop) {
+free(prefix);
 ctl_fatal("bad next hop argument: %s", ctx->argv[3]);
 }
 
 if (strchr(prefix, '.')) {
 ovs_be32 hop_ipv4;
 if (!ip_parse(ctx->argv[3], _ipv4)) {
+free(prefix);
+free(next_hop);
 ctl_fatal("bad IPv4 nexthop argument: %s", ctx->argv[3]);
 }
 } else {
 struct in6_addr hop_ipv6;
 if (!ipv6_parse(ctx->argv[3], _ipv6)) {
+free(prefix);
+free(next_hop);
 ctl_fatal("bad IPv6 nexthop argument: %s", ctx->argv[3]);
 }
 }
@@ -2072,6 +2077,7 @@ nbctl_lr_route_add(struct ctl_context *ctx)
 }
 
 if (!may_exist) {
+free(next_hop);
 free(rt_prefix);
 ctl_fatal("duplicate prefix: %s", prefix);
 }
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 115d781..af00dad 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -640,6 +640,22 @@ dnl Add overlapping route with 10.0.0.1/24
 AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1], [1], [],
   [ovn-nbctl: duplicate prefix: 10.0.0.0/24
 ])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111a/24 11.0.0.1], [1], [],
+  [ovn-nbctl: bad prefix argument: 10.0.0.111a/24
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24a 11.0.0.1], [1], [],
+  [ovn-nbctl: bad prefix argument: 10.0.0.111/24a
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1a], [1], [],
+  [ovn-nbctl: bad next hop argument: 11.0.0.1a
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1/24], [1], [],
+  [ovn-nbctl: bad IPv4 nexthop argument: 11.0.0.1/24
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::1/64], 
[1], [],
+  [ovn-nbctl: bad IPv6 nexthop argument: 2001:0db8:0:f103::1/64
+])
+
 AT_CHECK([ovn-nbctl --may-exist lr-route-add lr0 10.0.0.111/24 11.0.0.1])
 
 AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
-- 
1.8.3.1



___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/4] ovn: make ipam tests more reliable

[nickcooper-zhangtonghao]

Fixes: 8639f9be("ovn-northd, tests: Adding IPAM to ovn-northd.")
The IPAM tests began to fail occasionally. Adding --wait=sb
to commands triggering address allocation eliminated these failures.

Signed-off-by: nickcooper-zhangtonghao 
---
 tests/ovn.at | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 2b193dd..73f1694 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4543,8 +4543,8 @@ ovn_start
 # Add a port to a switch that does not have a subnet set, then set the
 # subnet which should result in an address being allocated for the port.
 ovn-nbctl ls-add sw0
-ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
-ovn-nbctl add Logical-Switch sw0 other_config subnet=192.168.1.0/24
+ovn-nbctl --wait=sb lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
+ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=192.168.1.0/24
 AT_CHECK([ovn-nbctl get Logical-Switch-Port p0 dynamic_addresses], [0],
 ["0a:00:00:00:00:01 192.168.1.2"
 ])
@@ -4584,7 +4584,7 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p9 
dynamic_addresses], [0],
 # Trying similar tests with a second switch. MAC addresses should be unique
 # across both switches but IP's only need to be unique within the same switch.
 ovn-nbctl ls-add sw1
-ovn-nbctl lsp-add sw1 p10 -- lsp-set-addresses p10 dynamic
+ovn-nbctl --wait=sb lsp-add sw1 p10 -- lsp-set-addresses p10 dynamic
 ovn-nbctl --wait=sb add Logical-Switch sw1 other_config subnet=192.168.1.0/24
 AT_CHECK([ovn-nbctl get Logical-Switch-Port p10 dynamic_addresses], [0],
  ["0a:00:00:00:00:0b 192.168.1.2"
@@ -4700,8 +4700,8 @@ ovn_start
 ovn-nbctl lr-add R1
 
 # Test for a ping using dynamically allocated addresses.
-ovn-nbctl ls-add foo -- add Logical_Switch foo other_config 
subnet=192.168.1.0/24
-ovn-nbctl ls-add alice -- add Logical_Switch alice other_config 
subnet=192.168.2.0/24
+ovn-nbctl --wait=sb ls-add foo -- add Logical_Switch foo other_config 
subnet=192.168.1.0/24
+ovn-nbctl --wait=sb ls-add alice -- add Logical_Switch alice other_config 
subnet=192.168.2.0/24
 
 # Connect foo to R1
 ovn-nbctl lrp-add R1 foo 00:00:00:01:02:03 192.168.1.1/24
@@ -4714,21 +4714,21 @@ ovn-nbctl lsp-add alice rp-alice -- set 
Logical_Switch_Port rp-alice type=router
   options:router-port=alice addresses=\"00:00:00:01:02:04\"
 
 # Create logical port foo1 in foo
-ovn-nbctl lsp-add foo foo1 \
+ovn-nbctl --wait=sb lsp-add foo foo1 \
 -- lsp-set-addresses foo1 "dynamic"
 AT_CHECK([ovn-nbctl get Logical-Switch-Port foo1 dynamic_addresses], [0],
  ["0a:00:00:00:00:01 192.168.1.2"
 ])
 
 # Create logical port alice1 in alice
-ovn-nbctl lsp-add alice alice1 \
+ovn-nbctl --wait=sb lsp-add alice alice1 \
 -- lsp-set-addresses alice1 "dynamic"
 AT_CHECK([ovn-nbctl get Logical-Switch-Port alice1 dynamic_addresses], [0],
  ["0a:00:00:00:00:02 192.168.2.2"
 ])
 
 # Create logical port foo2 in foo
-ovn-nbctl lsp-add foo foo2 \
+ovn-nbctl --wait=sb lsp-add foo foo2 \
 -- lsp-set-addresses foo2 "dynamic"
 AT_CHECK([ovn-nbctl get Logical-Switch-Port foo2 dynamic_addresses], [0],
  ["0a:00:00:00:00:03 192.168.1.3"
-- 
1.8.3.1



___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 4/4] ovn-nbctl: Improve ovn-nbctl manpage

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 7cd515f..70798dc 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -173,10 +173,17 @@
 address.  Each address should be either an
 Ethernet address or an Ethernet address followed by IP addresses
 (separated by space and quoted to form a single command-line
-argument).  The special form unknown is also valid.
+argument).  The special form unknown is valid.
+When an OVN logical switch processes a unicast Ethernet frame
+whose destination MAC address is not in any logical port's addresses
+column, it delivers it to the port (or ports) whose addresses columns
+include unknown.  The special form dynamic is also valid.
+Use this keyword to make ovn-northd generate a globally unique MAC
+address and choose an unused IPv4 address with the logical port's
+subnet and store them in the port's dynamic_addresses column.
 Multiple Ethernet addresses or Ethernet+IPs combinations may be set.
-If no address argument is given, port will have
-no addresses associated with it.
+If no address argument is given, port will
+have no addresses associated with it.
   
 
   lsp-get-addresses port
-- 
1.8.3.1



___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/4] ovn-nbctl: Check the length of MAC address

[nickcooper-zhangtonghao]

The command "ovn-nbctl lrp-add" should not set the MAC address
which length is invalid to logical router port. This patch
updates the eth_addr_from_string() to check trailing characters.
We should use the ovs_scan() to check the "addresses" owned by
the logical port, instead of eth_addr_from_string(). This patch
also updates the ovn-nbctl tests.

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/packets.c | 15 +--
 ovn/northd/ovn-northd.c   | 11 +++
 ovn/utilities/ovn-nbctl.c |  2 +-
 tests/ovn-nbctl.at|  7 +++
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index 11bb587..f661c34 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -142,16 +142,19 @@ eth_addr_is_reserved(const struct eth_addr ea)
 
 /* Attempts to parse 's' as an Ethernet address.  If successful, stores the
  * address in 'ea' and returns true, otherwise zeros 'ea' and returns
- * false.  */
+ * false.  This function checks trailing characters. */
 bool
 eth_addr_from_string(const char *s, struct eth_addr *ea)
 {
-if (ovs_scan(s, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(*ea))) {
-return true;
-} else {
-*ea = eth_addr_zero;
-return false;
+int n = 0;
+if (ovs_scan_len(s, , ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(*ea))) {
+if (!s[n]) {
+return true;
+}
 }
+
+*ea = eth_addr_zero;
+return false;
 }
 
 /* Fills 'b' with a Reverse ARP packet with Ethernet source address 'eth_src'.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index eeeb41d..42b5423 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2912,9 +2912,12 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 }
 
 for (size_t i = 0; i < op->nbsp->n_addresses; i++) {
+/* Addresses are owned by the logical port.
+ * Ethernet address followed by zero or more IPv4
+ * or IPv6 addresses (or both). */
 struct eth_addr mac;
-
-if (eth_addr_from_string(op->nbsp->addresses[i], )) {
+if (ovs_scan(op->nbsp->addresses[i],
+ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
 ds_clear();
 ds_put_format(, "eth.dst == "ETH_ADDR_FMT,
   ETH_ADDR_ARGS(mac));
@@ -2930,8 +2933,8 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 }
 } else if (!strcmp(op->nbsp->addresses[i], "dynamic")) {
 if (!op->nbsp->dynamic_addresses
-|| !eth_addr_from_string(op->nbsp->dynamic_addresses,
-)) {
+|| !ovs_scan(op->nbsp->dynamic_addresses,
+ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
 continue;
 }
 ds_clear();
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 7643241..453ff72 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2299,7 +2299,7 @@ nbctl_lrp_add(struct ctl_context *ctx)
 }
 
 struct eth_addr ea;
-if (!ovs_scan(mac, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+if (!eth_addr_from_string(mac, )) {
 ctl_fatal("%s: invalid mac address %s", lrp_name, mac);
 }
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index d8331f8..115d781 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -530,6 +530,13 @@ AT_SETUP([ovn-nbctl - basic logical router port commands])
 OVN_NBCTL_TEST_START
 
 AT_CHECK([ovn-nbctl lr-add lr0])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02 192.168.1.1/24], [1], [],
+  [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02
+])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03:04 192.168.1.1/24], 
[1], [],
+  [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02:03:04
+])
+
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
 
 AT_CHECK([ovn-nbctl show lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
-- 
1.8.3.1



___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] python: Add SSL support to the python ovs client library

[Numan Siddique]

>
>
>
> stderr:
> Traceback (most recent call last):
>   File "../../../../tests/test-ovsdb.py", line 835, in 
> main(sys.argv)
>   File "../../../../tests/test-ovsdb.py", line 830, in main
> func(*args)
>   File "../../../../tests/test-ovsdb.py", line 592, in do_idl
> while idl.change_seqno == seqno and not idl.run():
>   File "/home/blp/nicira/ovs/python/ovs/db/idl.py", line 188, in run
> self.__send_monitor_request()
>   File "/home/blp/nicira/ovs/python/ovs/db/idl.py", line 431, in
> __send_monitor_request
> self._session.send(msg)
>   File "/home/blp/nicira/ovs/python/ovs/jsonrpc.py", line 527, in send
> return self.rpc.send(msg)
>   File "/home/blp/nicira/ovs/python/ovs/jsonrpc.py", line 243, in send
> self.run()
>   File "/home/blp/nicira/ovs/python/ovs/jsonrpc.py", line 202, in run
> retval = self.stream.send(self.output)
>   File "/home/blp/nicira/ovs/python/ovs/stream.py", line 461, in send
> return super(SSLStream, self).send(buf)
>   File "/home/blp/nicira/ovs/python/ovs/stream.py", line 239, in send
> return self.socket.send(buf)
>   File "/usr/lib/python2.7/dist-packages/OpenSSL/SSL.py", line 947, in
> send
> raise TypeError("data must be a byte string")
> TypeError: data must be a byte string
>


​Thanks Ben for the review and testing it out.
I was able to reproduce the failure with pyOpenSSL version 0.14. This issue
is not seen with
the latest version 16 of pyOpenSSL. The send funciton of latest version
works because it checks and converts the buffer to byte string where as
0.14 doesn't.
https://github.com/pyca/pyopenssl/blob/master/src/OpenSSL/SSL.py#L1246

I will re-spin another patch with the fix.

Thanks
Numan

​


> stdout:
> ../../tests/ovsdb-idl.at:1232: exit code was 1, expected 0
> 2078. ovsdb-idl.at:1232: 2078. simple idl verify notify - SSL (
> ovsdb-idl.at:1232): FAILED (ovsdb-idl.at:1232)
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 1/5] Add initial sphinx configuration

[Stephen Finucane]

On 2016-10-05 00:32, Ben Pfaff wrote:

On Wed, Oct 05, 2016 at 09:25:11AM +0900, Joe Stringer wrote:
On 2 October 2016 at 04:01, Stephen Finucane  
wrote:

> This is essentially the output of 'sphinx-quickstart' but with parts of
> the Makefile merged into the existing Makefile.am and a license added
> to the index.rst file.
>
> Signed-off-by: Stephen Finucane 
> ---
> I need to know who to assign copyright to. I've used "Open vSwitch
> Developers" as a placeholder until I get better information.

...

> +# General information about the project.
> +project = u'Open vSwitch'
> +copyright = u'2016, Open vSwitch Developers'
> +author = u'Open vSwitch Developers'

I think Ben's response is addressing your contributions to the
project; but I suppose that you're asking how this should be written
in the general information section above. Ben?


Oh, I misunderstood then ;-)

The documentation is the work of many people with many employers.  I
don't know of a better way to express that, in a maintainable way, than
the above.  (I hope that doesn't make any lawyers cringe.)


Sounds good to me. OpenStack docs use 'OpenStack Foundation', but 'Linux 
Foundation' seemed like the wrong choice here.


Speaking of licenses, I've included the Apache 2.0 license header at the 
top of all reworked docs. Is this necessary?


Stephen
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 0/5] Integrate Sphinx documentation generator

[Stephen Finucane]

On 2016-10-04 22:01, Ben Pfaff wrote:

On Sat, Oct 01, 2016 at 08:01:29PM +0100, Stephen Finucane wrote:
Since the move to GitHub, OVS has increasingly used Markdown for all 
of

its documentation. This seems like a natural fit, given Markdown's low
overhead and support in the GitHub web UI. However, Markdown is a
documentation format, not a documentation system, and it starts to 
fall
over as you add more and more documents and see the need for thing 
like

cross-referencing, or insertion of partial documents into other docs.

I'd like to propose integrating the Sphinx documentation tool into the
OVS documentation. This would mean converting most or all of the
documentation to reStructuredText, but it would bring significant
advantages including:

* native cross-referencing
* multiple output formats (including pretty HTML)
* automatic indexing/searching of docs
* hierarchial structuring of the docs
* DRY documents, through inclusion of partial docs in multiple files
* ...

I chose rST+Sphinx due to the overwhelming popularity of the tool
within other open source communities like the Linux kernel and
OpenStack, which have all migrated documentation from other tools
(DocBook, mostly) in recent times.

Documents located in the top-level directory should also be converted
to rST to be consistent, but these should not include any Sphinx
extensions to ensure that these continue to be readable on GitHub.

I've included some patches that show the kind of changes that would be
necessary. I've actually converted far more of the docs (~40%?) at 
this

point, but I'd like to gauge interest before continuing with the
remainder. I'd also appreciate help from folks who would take a stab 
at

coverting docs so I don't have to do it all by myself.

Feel free to direct any questions at me.


This seems quite nice.  I played with it for a few minutes and looked 
at

the generated HTML output, which is nicer than the current "make
dist-docs" output.

I assume that, if this were to be fully converted, then the generated
webpage would replace "make dist-docs" and point to all the
documentation that that currently outputs?


Yes, that would be the intention. We could keep the 'dist-docs' target 
to build offline copies of the documentation but it would eventually use 
sphinx instead of the 'dist-docs' script.


I would also like to provide a custom stylesheet/Sphinx theme so we 
could display docs on the current documentation page [1] with a 
consistent style. For an example of what's possible here, look at the 
OpenStack docs [2], the Nova developer docs [3] and the Python docs [4]. 
Despite varying appearances, all are generated using Sphinx.



In particular, it would be
good to make sure that the manpages make it into the output.


manpages would be included. We may wish to rewrite these in reST like 
OpenStack do [5], but this is totally optional and should be done at 
another time.



I had to fold in the appended patch to get "make" to pass, to support
out-of-tree builds, and to make the license disappear from the "unit
tests" page.


Yeah, the Makefile was a (poor) first pass, so thanks for the patch. The 
proper version will be actually tested.


Assuming we're good with the idea, here's my current POA.

1) Convert all docs in-place to reStructuredText

   Update the 'dist-docs' script to use 'rst2html' [6] for '*.rst' 
files. The documentation output will remain essentially the same for 
now. This will be a slog, but it's rather trivial to review (cursory 
visual inspection) and is necessary for (2)


2) Integrate Sphinx

   Move all documents except traditional top-level docs (README, 
CodingStyle etc.) to 'Documentation' and build with sphinx instead of 
'rst2html'. The openvswitch.org docs page [1] will start using the 
sphinx output. Files would retain their existing names and structure 
meaning the docs index page will simply be a dumb list of these files 
(like today, really).


3) Rework docs into a series of guides

   I see the following guides as being helpful

   - Installation guide
   - Usage guide
   - Contributor guide

   I had a 'testing-guide', but this makes more sense split between the 
middle two. Any other guides?


This would be three separate series so I don't have to do everything at 
once. I'd also welcome assistance from anyone who wants to take the 
chance to rework some aspect of the docs.


Cheers,

Stephen

[1] http://openvswitch.org/support/dist-docs/
[2] http://docs.openstack.org/admin-guide/compute-cpu-topologies.html
[3] http://docs.openstack.org/developer/nova/testing/libvirt-numa.html
[4] https://docs.python.org/3/library/mailbox.html
[5] 
https://github.com/openstack/python-openstackclient/blob/3.2.0/doc/source/man/openstack.rst

[6] http://docutils.sourceforge.net/docs/user/tools.html#rst2html-py
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v13 2/2] DSCP marking on packets

[Babu Shanmugam]

On Wednesday 05 October 2016 12:05 AM, Ben Pfaff wrote:

On Wed, Sep 07, 2016 at 11:40:12AM +0530,bscha...@redhat.com  wrote:

>From: Babu Shanmugam
>
>This patch adds support for marking qos on IP packets based on arbitrary
>match criteria for a logical switch.
>
>Signed-off-by: Babu Shanmugam
>Suggested-by: Mickey Spiegel
>Acked-by: Mickey Spiegel

I spent a little time with this, but I couldn't make the test pass.  The
first issue seems to be that MFF_LOG_CT_ZONE (in reg13) is
nondeterministic: sometimes I see 0x1, sometimes 0x2.  That's not a
problem with this patch, so I folded in the appended incremental, which
also makes sure that the logical port numbers get assigned in the
expected order, by adding --wait and sync.  But even with that, I get
the following failure, so i think that some more work is needed here.

Thanks,

Ben.


Ben, I rebased the patch and applied your changes. I could not see any 
failures in any test case. I have published a v14 which is the rebased 
version with your changes. Kindly try it and let me know if you are 
still seeing failures.


Thank you,
Babu
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PACTCH v14] DSCP marking on packets

[bschanmu]

From: Babu Shanmugam 

This patch adds support for marking qos on IP packets based on arbitrary
match criteria for a logical switch.

Signed-off-by: Babu Shanmugam 
Suggested-by: Mickey Spiegel 
Acked-by: Mickey Spiegel 
---
 ovn/lib/logical-fields.c|  2 +-
 ovn/northd/ovn-northd.8.xml | 47 --
 ovn/northd/ovn-northd.c | 80 ++---
 ovn/ovn-nb.ovsschema| 26 +--
 ovn/ovn-nb.xml  | 57 
 ovn/utilities/ovn-nbctl.c   |  5 +++
 tests/ovn.at| 66 -
 7 files changed, 242 insertions(+), 41 deletions(-)

diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
index 5229be3..2d3e217 100644
--- a/ovn/lib/logical-fields.c
+++ b/ovn/lib/logical-fields.c
@@ -137,7 +137,7 @@ ovn_init_symtab(struct shash *symtab)
 expr_symtab_add_predicate(symtab, "ip6", "eth.type == 0x86dd");
 expr_symtab_add_predicate(symtab, "ip", "ip4 || ip6");
 expr_symtab_add_field(symtab, "ip.proto", MFF_IP_PROTO, "ip", true);
-expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP, "ip", false);
+expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP_SHIFTED, "ip", false);
 expr_symtab_add_field(symtab, "ip.ecn", MFF_IP_ECN, "ip", false);
 expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
 
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 77eb3d1..df53d4c 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -362,7 +362,27 @@
   
 
 
-Ingress Table 7: LB
+Ingress Table 7: from-lport QoS marking
+
+
+  Logical flows in this table closely reproduce those in the
+  QoS table in the OVN_Northbound database
+  for the from-lport direction.
+
+
+
+  
+For every qos_rules for every logical switch a flow will be added at
+priorities mentioned in the QoS table.
+  
+
+  
+One priority-0 fallback flow that matches all packets and advances to
+the next table.
+  
+
+
+Ingress Table 8: LB
 
 
   It contains a priority-0 flow that simply moves traffic to the next
@@ -375,7 +395,7 @@
   connection.)
 
 
-Ingress Table 8: Stateful
+Ingress Table 9: Stateful
 
 
   
@@ -412,7 +432,7 @@
   
 
 
-Ingress Table 9: ARP/ND responder
+Ingress Table 10: ARP/ND responder
 
 
   This table implements ARP/ND responder for known IPs.  It contains these
@@ -507,7 +527,7 @@ nd_na {
   
 
 
-Ingress Table 10: DHCP option processing
+Ingress Table 11: DHCP option processing
 
 
   This table adds the DHCPv4 options to a DHCPv4 packet from the
@@ -567,7 +587,7 @@ next;
   
 
 
-Ingress Table 11: DHCP responses
+Ingress Table 12: DHCP responses
 
 
   This table implements DHCP responder for the DHCP replies generated by
@@ -649,7 +669,7 @@ output;
   
 
 
-Ingress Table 12: Destination Lookup
+Ingress Table 13 Destination Lookup
 
 
   This table implements switching behavior.  It contains these logical
@@ -716,7 +736,14 @@ output;
   to-lport ACLs.
 
 
-Egress Table 5: Stateful
+Egress Table 5: to-lport QoS marking
+
+
+  This is similar to ingress table QoS marking except for
+  to-lport qos rules.
+
+
+Egress Table 6: Stateful
 
 
   This is similar to ingress table Stateful except that
@@ -727,10 +754,10 @@ output;
   Also a priority 34000 logical flow is added for each logical port which
   has DHCPv4 options defined to allow the DHCPv4 reply packet and which has
   DHCPv6 options defined to allow the DHCPv6 reply packet from the
-  Ingress Table 11: DHCP responses.
+  Ingress Table 12: DHCP responses.
 
 
-Egress Table 6: Egress Port Security - IP
+Egress Table 7: Egress Port Security - IP
 
 
   This is similar to the port security logic in table
@@ -740,7 +767,7 @@ output;
   ip4.src and ip6.src
 
 
-Egress Table 7: Egress Port Security - L2
+Egress Table 8: Egress Port Security - L2
 
 
   This is similar to the ingress port security logic in ingress table
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 0150a8c..ad4e38d 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -96,21 +96,22 @@ enum ovn_datapath_type {
  * form the stage's full name, e.g. S_SWITCH_IN_PORT_SEC_L2,
  * S_ROUTER_OUT_DELIVERY. */
 enum ovn_stage {
-#define PIPELINE_STAGES   \
-/* Logical switch ingress stages. */  \
-PIPELINE_STAGE(SWITCH, IN,  PORT_SEC_L2,0, "ls_in_port_sec_l2") \
-PIPELINE_STAGE(SWITCH, IN,  PORT_SEC_IP,1, "ls_in_port_sec_ip") \
-