Re: [ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

Yea I totally agree.

In the latest patch I sent out it skips these tests if the user doesn't
have the ovs python lib installed.  I'll try and see what I can do to fix
this.

On Wed, Jul 13, 2016 at 10:56 AM, Ben Pfaff  wrote:

> On Mon, Jul 04, 2016 at 12:17:54PM -0700, Aaron Rosen wrote:
> > On Sun, Jul 3, 2016 at 12:04 PM, Ben Pfaff  wrote:
> >
> > > On Thu, Jun 30, 2016 at 02:27:19PM -0700, Aaron Rosen wrote:
> > > > This patch exposes the c function expr_parse_string() to be called
> via
> > > > python. The motivation for this is so that clients interfacing with
> > > > ovn can call this method in order to validate the data they are
> writting
> > > > to ovn.
> > > >
> > > > Previously, there were several bugs in the neutron/ovn integration
> > > > that went unnoticed due to the client writing invalid data. This
> should
> > > > hopefully help catch errors like this earlier as it can now be
> detected
> > > on
> > > > the client side and an error can be raised.
>
> > > I don't yet understand how to use this, though.  When I run a normal
> > > "make check" I get failures like this:
> > >
> > > # -*- compilation -*-
> > > 2132. test-ovn-utils.at:12: testing test-ovn-utils - Python2 ...
> > > ../../tests/test-ovn-utils.at:12: $PYTHON
> $srcdir/test-ovn-utils.py
> > > stderr:
> > > Traceback (most recent call last):
> > >   File "../../../../tests/test-ovn-utils.py", line 17, in 
> > > from ovs import ovn_utils
> > > ImportError: cannot import name ovn_utils
> > > stdout:
> > > ../../tests/test-ovn-utils.at:12: exit code was 1, expected 0
> > > 2132. test-ovn-utils.at:12: 2132. test-ovn-utils - Python2 (
> > > test-ovn-utils.at:12): FAILED (test-ovn-utils.at:12)
> > >
> > > Is some extra step required?
> >
> >
> > Looks like you need to run:
> >
> > sudo python setup.py install
> > and
> > sudo python3 setup.py install (if you want the python3 tests to run).
> >
> > inside of  ovs/python first.
> >
> > Is there a good place were we could add a hook to make that occur?  I
> think
> > the other issue that we are facing before that is in order to build
> these C
> > extensions it currently is relying on the openvswitch-dev package being
> > installed on the system rather than able to use it from the current
> source
> > tree: https://github.com/openvswitch/ovs/blob/master/python/setup.py#L80
>
> These changes are going to break the OVS build process, then.  Building
> and testing OVS doesn't currently require root.  It sounds like a step
> backwards to start requiring it.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 0/2] Python JSON parser improvements

[Aaron Rosen]

I'm facing this same issue with the _ovn-utils.c Python C extension patch I
posted. If this gets approved I believe I need to so the same thing for the
following files:

ovn/lib/actions.h
ovn/lib/expr.h
ovn/lib/lex.h


On Tue, Jul 12, 2016 at 2:37 PM, Terry Wilson  wrote:

> These patches should address the previous issue with building the
> Python wrapper when builddir != srcdir. It also ensures that the
> wrapper can be properly built by pip from out-of-tree by ensuring
> that json.h is added to include/openvswitch/json.h.
>
> Terry Wilson (2):
>   Move lib/json.h to include/openvswitch
>   JSON serialization via Python's json lib
>
>  configure.ac|   2 +
>  include/openvswitch/automake.mk |   3 +
>  include/openvswitch/hmap.h  | 407
> 
>  include/openvswitch/json.h  | 141 ++
>  include/openvswitch/shash.h |  81 
>  lib/automake.mk |   3 -
>  lib/bfd.c   |   2 +-
>  lib/bundle.c|   1 +
>  lib/cfm.c   |   3 +-
>  lib/cfm.h   |   2 +-
>  lib/db-ctl-base.c   |   4 +-
>  lib/db-ctl-base.h   |   2 +-
>  lib/dpctl.c |   2 +-
>  lib/dpif-netdev.c   |   2 +-
>  lib/dpif-netlink.c  |   2 +-
>  lib/dpif.c  |   2 +-
>  lib/fat-rwlock.c|   2 +-
>  lib/fatal-signal.c  |   2 +-
>  lib/flow.c  |   1 +
>  lib/hmap.c  |   2 +-
>  lib/hmap.h  | 407
> 
>  lib/hmapx.h |   2 +-
>  lib/id-pool.c   |   2 +-
>  lib/json.c  |   4 +-
>  lib/json.h  | 140 --
>  lib/jsonrpc.c   |   2 +-
>  lib/lacp.c  |   5 +-
>  lib/learning-switch.c   |   4 +-
>  lib/lockfile.c  |   2 +-
>  lib/mac-learning.h  |   2 +-
>  lib/mcast-snooping.h|   2 +-
>  lib/meta-flow.c |   2 +-
>  lib/multipath.c |   1 +
>  lib/netdev-bsd.c|   2 +-
>  lib/netdev-dpdk.c   |   2 +-
>  lib/netdev-dummy.c  |   2 +-
>  lib/netdev-linux.c  |   5 +-
>  lib/netdev-provider.h   |   2 +-
>  lib/netdev-windows.c|   2 +-
>  lib/netdev.c|   2 +-
>  lib/netlink-conntrack.h |   2 +-
>  lib/netlink-socket.c|   2 +-
>  lib/netlink.c   |   1 +
>  lib/nx-match.c  |   4 +-
>  lib/odp-util.h  |   2 +-
>  lib/ofp-actions.c   |   2 +-
>  lib/ofp-msgs.c  |   3 +-
>  lib/ofp-parse.c |   1 +
>  lib/ofp-util.c  |   1 +
>  lib/ovs-lldp.h  |   2 +-
>  lib/ovs-numa.c  |   2 +-
>  lib/ovs-rcu.c   |   1 +
>  lib/ovsdb-data.c|   5 +-
>  lib/ovsdb-data.h|   2 +-
>  lib/ovsdb-error.c   |   2 +-
>  lib/ovsdb-idl-provider.h|   4 +-
>  lib/ovsdb-idl.c |   4 +-
>  lib/ovsdb-map-op.c  |   2 +-
>  lib/ovsdb-parser.h  |   2 +-
>  lib/ovsdb-types.c   |   3 +-
>  lib/packets.c   |   2 +-
>  lib/pcap-file.c |   3 +-
>  lib/perf-counter.c  |   2 +-
>  lib/poll-loop.c |   2 +-
>  lib/reconnect.c |   1 +
>  lib/rstp-common.h   |   2 +-
>  lib/seq.c   |   2 +-
>  lib/shash.c |   2 +-
>  lib/shash.h |  81 
>  lib/simap.h |   2 +-
>  lib/smap.c  |   2 +-
>  lib/smap.h  |   2 +-
>  lib/sset.h  |   2 +-
>  lib/stream-ssl.c|   2 +-
>  lib/table.c |   2 +-
>  lib/timeval.c   |   2 +-
>  lib/tun-metadata.c  |   3 +-
>  lib/unixctl.c   |   4 +-
>  lib/util.h  |   1 +
>  m4/openvswitch.m4   |  47 +
>  ofproto/bond.c  |   5 +-
>  ofproto/bundles.c   |   2 +-
>  ofproto/connmgr.c   |   3 +-
>  ofproto/connmgr.h   |   2 +-
>  ofproto/ofproto-dpif-ipfix.c|   2 +-
>  ofproto/ofproto-dpif-mirror.c   |   2 +-
>  ofproto/ofproto-dpif-monitor.c  |   2 +-
>  ofproto/ofproto-dpif-sflow.c|   2 +-
>  ofproto/ofproto-dpif-xlate.c|   1 +
>  ofproto/ofproto-dpif.c  |   1 +
>  ofproto/ofproto-provider.h  |   2 +-
>  ofproto/ofproto.c   |   5 +-
>  ofproto/pinsched.c  |   2 +-
>  ofproto/tunnel.c|   2 +-
>  ovn/controller-vtep/binding.c   |   2 +-
>  ovn/controller-vtep/vtep.c  |   4 +-
>  ovn/controller/binding.c

[ovs-dev] [PATCH] move ovn/lib/.h to include/ovn

[Aaron Rosen]

This patch is done to enable in tree building of the ovn-utils python
wrapper. This is similar to what was done in:
ee89ea7b477bb4fd05137de03b2e8443807ed9f4

Signed-off-by: Aaron Rosen 
---
 include/automake.mk |   1 +
 include/ovn/actions.h   | 141 
 include/ovn/automake.mk |   5 +
 include/ovn/expr.h  | 474 
 include/ovn/lex.h   | 139 
 ovn/controller/lflow.c  |   4 +-
 ovn/controller/ofctrl.c |   2 +-
 ovn/controller/ovn-controller.c |   2 +-
 ovn/controller/pinctrl.c|   2 +-
 ovn/lib/actions.c   |   6 +-
 ovn/lib/actions.h   | 141 
 ovn/lib/automake.mk |   3 -
 ovn/lib/expr.c  |   4 +-
 ovn/lib/expr.h  | 474 
 ovn/lib/lex.c   |   2 +-
 ovn/lib/lex.h   | 139 
 ovn/northd/ovn-northd.c |   2 +-
 tests/test-ovn.c|   6 +-
 18 files changed, 775 insertions(+), 772 deletions(-)
 create mode 100644 include/ovn/actions.h
 create mode 100644 include/ovn/automake.mk
 create mode 100644 include/ovn/expr.h
 create mode 100644 include/ovn/lex.h
 delete mode 100644 ovn/lib/actions.h
 delete mode 100644 ovn/lib/expr.h
 delete mode 100644 ovn/lib/lex.h

diff --git a/include/automake.mk b/include/automake.mk
index 6a4cf86..37903fd 100644
--- a/include/automake.mk
+++ b/include/automake.mk
@@ -6,6 +6,7 @@ include/odp-netlink.h: 
datapath/linux/compat/include/linux/openvswitch.h \
 EXTRA_DIST += build-aux/extract-odp-netlink-h
 CLEANFILES += include/odp-netlink.h
 
+include include/ovn/automake.mk
 include include/openflow/automake.mk
 include include/openvswitch/automake.mk
 include include/sparse/automake.mk
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
new file mode 100644
index 000..114c71e
--- /dev/null
+++ b/include/ovn/actions.h
@@ -0,0 +1,141 @@
+/*
+ * Copyright (c) 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.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef OVN_ACTIONS_H
+#define OVN_ACTIONS_H 1
+
+#include 
+#include 
+#include "compiler.h"
+#include "openvswitch/hmap.h"
+#include "openvswitch/dynamic-string.h"
+#include "util.h"
+
+struct expr;
+struct lexer;
+struct ofpbuf;
+struct shash;
+struct simap;
+
+#define MAX_OVN_GROUPS 65535
+
+struct group_table {
+unsigned long *group_ids;  /* Used as a bitmap with value set
+* for allocated group ids in either
+* desired_groups or existing_groups. */
+struct hmap desired_groups;
+struct hmap existing_groups;
+};
+
+struct group_info {
+struct hmap_node hmap_node;
+struct ds group;
+uint32_t group_id;
+};
+
+enum action_opcode {
+/* "arp { ...actions... }".
+ *
+ * The actions, in OpenFlow 1.3 format, follow the action_header.
+ */
+ACTION_OPCODE_ARP,
+
+/* "put_arp(port, ip, mac)"
+ *
+ * Arguments are passed through the packet metadata and data, as follows:
+ *
+ * MFF_REG0 = ip
+ * MFF_LOG_INPORT = port
+ * MFF_ETH_SRC = mac
+ */
+ACTION_OPCODE_PUT_ARP,
+
+/* "result = put_dhcp_opts(offer_ip, option, ...)".
+ *
+ * Arguments follow the action_header, in this format:
+ *   - A 32-bit or 64-bit OXM header designating the result field.
+ *   - A 32-bit integer specifying a bit offset within the result field.
+ *   - The 32-bit DHCP offer IP.
+ *   - Any number of DHCP options.
+ */
+ACTION_OPCODE_PUT_DHCP_OPTS,
+
+/* "na { ...actions... }".
+ *
+ * The actions, in OpenFlow 1.3 format, follow the action_header.
+ */
+ACTION_OPCODE_NA,
+};
+
+/* Header. */
+struct action_header {
+ovs_be32 opcode;/* One of ACTION_OPCODE_* */
+uint8_t pad[4];
+};
+BUILD_ASSERT_DECL(sizeof(struct action_header) == 8);
+
+struct action_params {
+/* A table of "struct expr_symbol"s to support (as one would provide to
+ * expr_parse()). */
+const struct shash *symtab;
+
+/* hmap of 'struct dhcp_opts_map'  to support 'put_dhcp_opts' action */
+const struct hmap *dhcp_opts;
+
+/* Looks up logical port 'port_name'.  If found, stores its po

[ovs-dev] [PATCH v4] ovn: expose c validator to python

[Aaron Rosen]

This patch exposes the c function expr_parse_string() to be called via
python. The motivation for this is so that clients interfacing with
ovn can call this method in order to validate the data they are writting
to ovn.

Previously, there were several bugs in the neutron/ovn integration
that went unnoticed due to the client writing invalid data. This should
hopefully help catch errors like this earlier as it can now be detected on
the client side and an error can be raised.

Signed-off-by: Aaron Rosen 
---
 include/ovn/expr.h  |   1 +
 ovn/lib/expr.c  | 107 +
 python/automake.mk  |   4 +-
 python/ovs/_ovn-utils.c | 104 
 python/setup.py |  31 +-
 tests/automake.mk   |   4 +-
 tests/test-ovn-utils.at |  15 +++
 tests/test-ovn-utils.py |  33 ++
 tests/test-ovn.c| 112 +---
 tests/testsuite.at  |   1 +
 10 files changed, 299 insertions(+), 113 deletions(-)
 create mode 100644 python/ovs/_ovn-utils.c
 create mode 100644 tests/test-ovn-utils.at
 create mode 100644 tests/test-ovn-utils.py

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index d790c49..381e32f 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -470,5 +470,6 @@ void expr_macros_add(struct shash *macros, const char *name,
  const char * const *values, size_t n_values);
 void expr_macros_remove(struct shash *macros, const char *name);
 void expr_macros_destroy(struct shash *macros);
+void create_symtab_helper(struct shash *symtab);
 
 #endif /* ovn/expr.h */
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 288aae2..94b93c5 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -3008,3 +3008,110 @@ expr_parse_constant_set(struct lexer *lexer, const 
struct shash *symtab,
 parse_constant_set(&ctx, cs);
 return ctx.error;
 }
+
+/* create_symtab_helper() is used for testing purposes and in the ovn_utils
+ * python package. */
+void
+create_symtab_helper(struct shash *symtab)
+{
+shash_init(symtab);
+
+/* Reserve a pair of registers for the logical inport and outport.  A full
+ * 32-bit register each is bigger than we need, but the expression code
+ * doesn't yet support string fields that occupy less than a full OXM. */
+expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
+expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
+
+expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
+expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
+expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
+
+expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
+expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
+expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
+expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
+expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
+expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
+
+expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
+expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
+expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
+
+expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
+expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
+expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
+ "vlan.tci[13..15]");
+expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
+ "vlan.tci[0..11]");
+
+expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
+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.ecn", MFF_IP_ECN, "ip", false);
+expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
+
+expr_symtab_add_field(symtab, "ip4.src", MFF_IPV4_SRC, "ip4", false);
+expr_symtab_add_field(symtab, "ip4.dst", MFF_IPV4_DST, "ip4", false);
+
+expr_symtab_add_predicate(symtab, "icmp4", "ip4 && ip.proto == 1");
+expr_symtab_add_field(symtab, "i

Re: [ovs-dev] [PATCH v4] ovn: expose c validator to python

[Aaron Rosen]

Note: this patch is rebased on top of : [PATCH] move
ovn/lib/.h to include/ovn


On Mon, Jul 25, 2016 at 3:07 PM, Aaron Rosen  wrote:

> This patch exposes the c function expr_parse_string() to be called via
> python. The motivation for this is so that clients interfacing with
> ovn can call this method in order to validate the data they are writting
> to ovn.
>
> Previously, there were several bugs in the neutron/ovn integration
> that went unnoticed due to the client writing invalid data. This should
> hopefully help catch errors like this earlier as it can now be detected on
> the client side and an error can be raised.
>
> Signed-off-by: Aaron Rosen 
> ---
>  include/ovn/expr.h  |   1 +
>  ovn/lib/expr.c  | 107
> +
>  python/automake.mk  |   4 +-
>  python/ovs/_ovn-utils.c | 104 
>  python/setup.py |  31 +-
>  tests/automake.mk   |   4 +-
>  tests/test-ovn-utils.at |  15 +++
>  tests/test-ovn-utils.py |  33 ++
>  tests/test-ovn.c| 112
> +---
>  tests/testsuite.at  |   1 +
>  10 files changed, 299 insertions(+), 113 deletions(-)
>  create mode 100644 python/ovs/_ovn-utils.c
>  create mode 100644 tests/test-ovn-utils.at
>  create mode 100644 tests/test-ovn-utils.py
>
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index d790c49..381e32f 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -470,5 +470,6 @@ void expr_macros_add(struct shash *macros, const char
> *name,
>   const char * const *values, size_t n_values);
>  void expr_macros_remove(struct shash *macros, const char *name);
>  void expr_macros_destroy(struct shash *macros);
> +void create_symtab_helper(struct shash *symtab);
>
>  #endif /* ovn/expr.h */
> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> index 288aae2..94b93c5 100644
> --- a/ovn/lib/expr.c
> +++ b/ovn/lib/expr.c
> @@ -3008,3 +3008,110 @@ expr_parse_constant_set(struct lexer *lexer, const
> struct shash *symtab,
>  parse_constant_set(&ctx, cs);
>  return ctx.error;
>  }
> +
> +/* create_symtab_helper() is used for testing purposes and in the
> ovn_utils
> + * python package. */
> +void
> +create_symtab_helper(struct shash *symtab)
> +{
> +shash_init(symtab);
> +
> +/* Reserve a pair of registers for the logical inport and outport.  A
> full
> + * 32-bit register each is bigger than we need, but the expression
> code
> + * doesn't yet support string fields that occupy less than a full
> OXM. */
> +expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
> +expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
> +
> +expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
> +expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
> +expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
> +
> +expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
> +expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
> +expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
> +expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
> +expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
> +expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
> +
> +expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
> +expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
> +expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
> +
> +expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
> +expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
> +expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
> + "vlan.tci[13..15]");
> +expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
> + "vlan.tci[0..11]");
> +
> +expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
> +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)

Re: [ovs-dev] [PATCH] move ovn/lib/.h to include/ovn

[Aaron Rosen]

FYI: this patch is rebased on top of (patch below) which is not yet merged
into ovs.

commit ee61acf6202c6575033ea050b63271af49936da5
Author: Terry Wilson 
Date:   Fri Jul 22 21:57:20 2016 -0500

JSON serialization via Python's json lib

There is no particularly good reason to use our own Python JSON
serialization implementation when serialization can be done faster
with Python's built-in JSON library.

A few tests were changed due to Python's default JSON library
returning slightly more precise floating point numbers.

Signed-off-by: Terry Wilson 




On Mon, Jul 25, 2016 at 3:04 PM, Aaron Rosen  wrote:

> This patch is done to enable in tree building of the ovn-utils python
> wrapper. This is similar to what was done in:
> ee89ea7b477bb4fd05137de03b2e8443807ed9f4
>
> Signed-off-by: Aaron Rosen 
> ---
>  include/automake.mk |   1 +
>  include/ovn/actions.h   | 141 
>  include/ovn/automake.mk |   5 +
>  include/ovn/expr.h  | 474
> 
>  include/ovn/lex.h   | 139 
>  ovn/controller/lflow.c  |   4 +-
>  ovn/controller/ofctrl.c |   2 +-
>  ovn/controller/ovn-controller.c |   2 +-
>  ovn/controller/pinctrl.c|   2 +-
>  ovn/lib/actions.c   |   6 +-
>  ovn/lib/actions.h   | 141 
>  ovn/lib/automake.mk |   3 -
>  ovn/lib/expr.c  |   4 +-
>  ovn/lib/expr.h  | 474
> 
>  ovn/lib/lex.c   |   2 +-
>  ovn/lib/lex.h   | 139 
>  ovn/northd/ovn-northd.c |   2 +-
>  tests/test-ovn.c|   6 +-
>  18 files changed, 775 insertions(+), 772 deletions(-)
>  create mode 100644 include/ovn/actions.h
>  create mode 100644 include/ovn/automake.mk
>  create mode 100644 include/ovn/expr.h
>  create mode 100644 include/ovn/lex.h
>  delete mode 100644 ovn/lib/actions.h
>  delete mode 100644 ovn/lib/expr.h
>  delete mode 100644 ovn/lib/lex.h
>
> diff --git a/include/automake.mk b/include/automake.mk
> index 6a4cf86..37903fd 100644
> --- a/include/automake.mk
> +++ b/include/automake.mk
> @@ -6,6 +6,7 @@ include/odp-netlink.h:
> datapath/linux/compat/include/linux/openvswitch.h \
>  EXTRA_DIST += build-aux/extract-odp-netlink-h
>  CLEANFILES += include/odp-netlink.h
>
> +include include/ovn/automake.mk
>  include include/openflow/automake.mk
>  include include/openvswitch/automake.mk
>  include include/sparse/automake.mk
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> new file mode 100644
> index 000..114c71e
> --- /dev/null
> +++ b/include/ovn/actions.h
> @@ -0,0 +1,141 @@
> +/*
> + * Copyright (c) 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.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef OVN_ACTIONS_H
> +#define OVN_ACTIONS_H 1
> +
> +#include 
> +#include 
> +#include "compiler.h"
> +#include "openvswitch/hmap.h"
> +#include "openvswitch/dynamic-string.h"
> +#include "util.h"
> +
> +struct expr;
> +struct lexer;
> +struct ofpbuf;
> +struct shash;
> +struct simap;
> +
> +#define MAX_OVN_GROUPS 65535
> +
> +struct group_table {
> +unsigned long *group_ids;  /* Used as a bitmap with value set
> +* for allocated group ids in either
> +* desired_groups or existing_groups. */
> +struct hmap desired_groups;
> +struct hmap existing_groups;
> +};
> +
> +struct group_info {
> +struct hmap_node hmap_node;
> +struct ds group;
> +uint32_t group_id;
> +};
> +
> +enum action_opcode {
> +/* "arp { ...actions... }".
> + *
> + * The actions, in OpenFlow 1.3 format, follow the action_header.
> + */
> +ACTION_OPCODE_ARP,
> +
> +/* "put_arp(port, ip, mac)"
> + *
> + * Arguments are passed through the packet metadata and data, as
> follows:
> + *
> + * MFF_REG0 = ip
> + * MFF_

[ovs-dev] [PATCH] Add additional files to datapath/linux/.gitignore

[Aaron Rosen]

After compiling ovs these files always show up as uncommitted.

Signed-off-by: Aaron Rosen 
---
 datapath/linux/.gitignore | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/datapath/linux/.gitignore b/datapath/linux/.gitignore
index a4a930d..25d9804 100644
--- a/datapath/linux/.gitignore
+++ b/datapath/linux/.gitignore
@@ -56,3 +56,14 @@
 /vport.c
 /vxlan.c
 /workqueue.c
+/conntrack.c
+/inet_fragment.c
+/ip6_output.c
+/ip_fragment.c
+/ip_gre.c
+/ip_tunnel.c
+/lisp.c
+/nf_conntrack_core.c
+/nf_conntrack_reasm.c
+/reassembly.c
+/socket.c
-- 
1.9.1

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

[ovs-dev] [PATCH] Add *.c *.h to datapath/linux/.gitignore

[Aaron Rosen]

This should prevent any additional files from sneaking in here.

Signed-off-by: Aaron Rosen 
---
 datapath/linux/.gitignore | 54 ++-
 1 file changed, 2 insertions(+), 52 deletions(-)

diff --git a/datapath/linux/.gitignore b/datapath/linux/.gitignore
index a4a930d..a59a560 100644
--- a/datapath/linux/.gitignore
+++ b/datapath/linux/.gitignore
@@ -2,57 +2,7 @@
 /Makefile
 /Makefile.main
 /Module.markers
-/actions.c
-/addrconf_core-openvswitch.c
-/checksum.c
-/dev-openvswitch.c
-/dp_sysfs_dp.c
-/dp_sysfs_if.c
-/datapath.c
-/dp_dev.c
-/dp_notify.c
-/exthdrs_core.c
-/flex_array.c
-/flow.c
-/flow_dissector.c
-/flow_netlink.c
-/flow_table.c
-/genetlink-openvswitch.c
-/geneve.c
-/genl_exec.c
-/gre.c
-/gso.c
-/hash.c
-/hash-x86.c
-/ip_output-openvswitch.c
-/ip_tunnels_core.c
-/kcompat.h
-/kmemdup.c
-/loop_counter.c
+/*.c
+/*.h
 /modules.order
-/netdevice.c
-/net_namespace.c
-/random32.c
-/reciprocal_div.c
-/skbuff-openvswitch.c
-/stt.c
-/table.c
-/time.c
 /tmp
-/tunnel.c
-/udp.c
-/udp_tunnel.c
-/utils.c
-/vlan.c
-/vport-generic.c
-/vport-geneve.c
-/vport-gre.c
-/vport-internal_dev.c
-/vport-lisp.c
-/vport-netdev.c
-/vport-patch.c
-/vport-stt.c
-/vport-vxlan.c
-/vport.c
-/vxlan.c
-/workqueue.c
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] Add additional files to datapath/linux/.gitignore

[Aaron Rosen]

This can be ignored I think: "[PATCH] Add *.c *.h to
datapath/linux/.gitignore" is a better solution.

On Thu, Jun 2, 2016 at 2:18 PM, Aaron Rosen  wrote:

> After compiling ovs these files always show up as uncommitted.
>
> Signed-off-by: Aaron Rosen 
> ---
>  datapath/linux/.gitignore | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/datapath/linux/.gitignore b/datapath/linux/.gitignore
> index a4a930d..25d9804 100644
> --- a/datapath/linux/.gitignore
> +++ b/datapath/linux/.gitignore
> @@ -56,3 +56,14 @@
>  /vport.c
>  /vxlan.c
>  /workqueue.c
> +/conntrack.c
> +/inet_fragment.c
> +/ip6_output.c
> +/ip_fragment.c
> +/ip_gre.c
> +/ip_tunnel.c
> +/lisp.c
> +/nf_conntrack_core.c
> +/nf_conntrack_reasm.c
> +/reassembly.c
> +/socket.c
> --
> 1.9.1
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] Add *.c to datapath/linux/.gitignore

[Aaron Rosen]

This should prevent any additional *.c files from sneaking in here.

Signed-off-by: Aaron Rosen 
---
 datapath/linux/.gitignore | 52 +--
 1 file changed, 1 insertion(+), 51 deletions(-)

diff --git a/datapath/linux/.gitignore b/datapath/linux/.gitignore
index a4a930d..8e9d781 100644
--- a/datapath/linux/.gitignore
+++ b/datapath/linux/.gitignore
@@ -2,57 +2,7 @@
 /Makefile
 /Makefile.main
 /Module.markers
-/actions.c
-/addrconf_core-openvswitch.c
-/checksum.c
-/dev-openvswitch.c
-/dp_sysfs_dp.c
-/dp_sysfs_if.c
-/datapath.c
-/dp_dev.c
-/dp_notify.c
-/exthdrs_core.c
-/flex_array.c
-/flow.c
-/flow_dissector.c
-/flow_netlink.c
-/flow_table.c
-/genetlink-openvswitch.c
-/geneve.c
-/genl_exec.c
-/gre.c
-/gso.c
-/hash.c
-/hash-x86.c
-/ip_output-openvswitch.c
-/ip_tunnels_core.c
 /kcompat.h
-/kmemdup.c
-/loop_counter.c
 /modules.order
-/netdevice.c
-/net_namespace.c
-/random32.c
-/reciprocal_div.c
-/skbuff-openvswitch.c
-/stt.c
-/table.c
-/time.c
 /tmp
-/tunnel.c
-/udp.c
-/udp_tunnel.c
-/utils.c
-/vlan.c
-/vport-generic.c
-/vport-geneve.c
-/vport-gre.c
-/vport-internal_dev.c
-/vport-lisp.c
-/vport-netdev.c
-/vport-patch.c
-/vport-stt.c
-/vport-vxlan.c
-/vport.c
-/vxlan.c
-/workqueue.c
+/*.c
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] Add *.c *.h to datapath/linux/.gitignore

[Aaron Rosen]

Done, see, "Subject: [PATCH] Add *.c to datapath/linux/.gitignore"




On Mon, Jun 6, 2016 at 11:06 AM, Jesse Gross  wrote:

> On Thu, Jun 2, 2016 at 6:02 PM, Aaron Rosen  wrote:
> > This should prevent any additional files from sneaking in here.
> >
> > Signed-off-by: Aaron Rosen 
>
> I agree that this is better since these files are just autogenerated
> and not very interesting.
>
> I think that I would leave kcompat.h in the list though and then drop
> *.h. Only new *.c files should continue to automatically pop up here.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] ovn: is it possible to add validation on acl match

[Aaron Rosen]

If we want to provide a validator to the caller (i.e the neutron plugin)
would it mostly be a matter of making:

actions_parse_string()  -
https://github.com/openvswitch/ovs/blob/master/ovn/lib/actions.c#L554

callable via python? I can start looking into making that work.

Aaron

On Fri, May 20, 2016 at 12:53 PM, Ben Pfaff  wrote:

> Well, there'd be a Conjunction (or And) class, a Disjunction (or Or)
> class, a Comparison class, and so on, and then a method on each class to
> render it to a string.  This is a pain to deal with in C, which is why
> ovn-northd generally doesn't do it, but it might be less painful in
> Python.
>
> On Fri, May 20, 2016 at 11:55:30AM -0700, Aaron Rosen wrote:
> > Would you mind giving an example of what this would look like?
> >
> > On Friday, May 20, 2016, Ben Pfaff  wrote:
> >
> > > Another way to make it harder to send bad matches would be to construct
> > > them in a structured way rather than as strings.
> > >
> > > On Fri, May 20, 2016 at 09:50:43AM -0700, Ben Pfaff wrote:
> > > > Would it be useful to provide a parser in Python for matches and
> > > > actions?  Then most issues could be found before anything is sent to
> the
> > > > database.
> > > >
> > > > (At this point I'm brainstorming.)
> > > >
> > > > On Fri, May 20, 2016 at 09:29:28AM -0700, Aaron Rosen wrote:
> > > > > Makes sense, getting the logging in OpenStack and in northd should
> > > > > definitely help improve visibility for us to detect this sooner.
> Even
> > > > > though we won't be able to completely prevent it from the openstack
> > > side I
> > > > > think this is still a good safe guard.
> > > > >
> > > > > On Fri, May 20, 2016 at 7:21 AM, Russell Bryant  > > > wrote:
> > > > >
> > > > > >
> > > > > >
> > > > > > On Thu, May 19, 2016 at 11:51 PM, Ben Pfaff  > > > wrote:
> > > > > >
> > > > > >> On Thu, May 19, 2016 at 08:42:15PM -0700, Aaron Rosen wrote:
> > > > > >> > I'm wondering if it would be possible to add any additional
> > > validation
> > > > > >> on
> > > > > >> > the match column in the ACL table (and potentially other
> places
> > > in the
> > > > > >> > future)?
> > > > > >> >
> > > > > >> > For example, we had a silly bug in the ovn plugin where if
> someone
> > > > > >> created
> > > > > >> > a security group rule and specified the protocol number as 6
> > > instead of
> > > > > >> > tcp,  we forgot to convert the protocol number 6 to tcp and
> ended
> > > up
> > > > > >> > pushing a rule that looked like this:
> > > > > >> >
> > > > > >> >   to-lport  1002 (outport ==
> > > "c48a1ff1-a184-491a-9ffd-3db06ebd18ee" &&
> > > > > >> ip4
> > > > > >> > && 6 && *6.dst *== 22) allow-related
> > > > > >>
> > > > > >> We could validate it in ovn-northd so that it doesn't get pushed
> > > down to
> > > > > >> the southbound database, either just logging it at northd or
> adding
> > > some
> > > > > >> kind of status or error column to the ACL table so that we could
> > > push
> > > > > >> the problem back up.  Is that the kind of thing you're looking
> for?
> > > > > >
> > > > > >
> > > > > > Validation in ovn-northd and reporting an error state in the ACL
> > > table
> > > > > > sounds good to me.
> > > > > >
> > > > > > We can watch events in our plugin for when ACL rows get updated
> and
> > > check
> > > > > > to see if the error column was set.  We can at least log an
> error on
> > > the
> > > > > > OpenStack side in that case.  It would be asynchronous from the
> > > OpenStack
> > > > > > API call, so we wouldn't be able to return an error in the API,
> > > though.
> > > > > >
> > > > > > --
> > > > > > Russell Bryant
> > > > > >
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] ovn: is it possible to add validation on acl match

[Aaron Rosen]

Sure!

On Tue, Jun 7, 2016 at 10:18 AM, Ben Pfaff  wrote:

> That's an interesting idea, do you want to take a shot at it?
>
> On Tue, Jun 07, 2016 at 10:09:42AM -0700, Aaron Rosen wrote:
> > If we want to provide a validator to the caller (i.e the neutron plugin)
> > would it mostly be a matter of making:
> >
> > actions_parse_string()  -
> > https://github.com/openvswitch/ovs/blob/master/ovn/lib/actions.c#L554
> >
> > callable via python? I can start looking into making that work.
> >
> > Aaron
> >
> > On Fri, May 20, 2016 at 12:53 PM, Ben Pfaff  wrote:
> >
> > > Well, there'd be a Conjunction (or And) class, a Disjunction (or Or)
> > > class, a Comparison class, and so on, and then a method on each class
> to
> > > render it to a string.  This is a pain to deal with in C, which is why
> > > ovn-northd generally doesn't do it, but it might be less painful in
> > > Python.
> > >
> > > On Fri, May 20, 2016 at 11:55:30AM -0700, Aaron Rosen wrote:
> > > > Would you mind giving an example of what this would look like?
> > > >
> > > > On Friday, May 20, 2016, Ben Pfaff  wrote:
> > > >
> > > > > Another way to make it harder to send bad matches would be to
> construct
> > > > > them in a structured way rather than as strings.
> > > > >
> > > > > On Fri, May 20, 2016 at 09:50:43AM -0700, Ben Pfaff wrote:
> > > > > > Would it be useful to provide a parser in Python for matches and
> > > > > > actions?  Then most issues could be found before anything is
> sent to
> > > the
> > > > > > database.
> > > > > >
> > > > > > (At this point I'm brainstorming.)
> > > > > >
> > > > > > On Fri, May 20, 2016 at 09:29:28AM -0700, Aaron Rosen wrote:
> > > > > > > Makes sense, getting the logging in OpenStack and in northd
> should
> > > > > > > definitely help improve visibility for us to detect this
> sooner.
> > > Even
> > > > > > > though we won't be able to completely prevent it from the
> openstack
> > > > > side I
> > > > > > > think this is still a good safe guard.
> > > > > > >
> > > > > > > On Fri, May 20, 2016 at 7:21 AM, Russell Bryant <
> russ...@ovn.org
> > > > > > wrote:
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, May 19, 2016 at 11:51 PM, Ben Pfaff  > > > > > wrote:
> > > > > > > >
> > > > > > > >> On Thu, May 19, 2016 at 08:42:15PM -0700, Aaron Rosen wrote:
> > > > > > > >> > I'm wondering if it would be possible to add any
> additional
> > > > > validation
> > > > > > > >> on
> > > > > > > >> > the match column in the ACL table (and potentially other
> > > places
> > > > > in the
> > > > > > > >> > future)?
> > > > > > > >> >
> > > > > > > >> > For example, we had a silly bug in the ovn plugin where if
> > > someone
> > > > > > > >> created
> > > > > > > >> > a security group rule and specified the protocol number
> as 6
> > > > > instead of
> > > > > > > >> > tcp,  we forgot to convert the protocol number 6 to tcp
> and
> > > ended
> > > > > up
> > > > > > > >> > pushing a rule that looked like this:
> > > > > > > >> >
> > > > > > > >> >   to-lport  1002 (outport ==
> > > > > "c48a1ff1-a184-491a-9ffd-3db06ebd18ee" &&
> > > > > > > >> ip4
> > > > > > > >> > && 6 && *6.dst *== 22) allow-related
> > > > > > > >>
> > > > > > > >> We could validate it in ovn-northd so that it doesn't get
> pushed
> > > > > down to
> > > > > > > >> the southbound database, either just logging it at northd or
> > > adding
> > > > > some
> > > > > > > >> kind of status or error column to the ACL table so that we
> could
> > > > > push
> > > > > > > >> the problem back up.  Is that the kind of thing you're
> looking
> > > for?
> > > > > > > >
> > > > > > > >
> > > > > > > > Validation in ovn-northd and reporting an error state in the
> ACL
> > > > > table
> > > > > > > > sounds good to me.
> > > > > > > >
> > > > > > > > We can watch events in our plugin for when ACL rows get
> updated
> > > and
> > > > > check
> > > > > > > > to see if the error column was set.  We can at least log an
> > > error on
> > > > > the
> > > > > > > > OpenStack side in that case.  It would be asynchronous from
> the
> > > > > OpenStack
> > > > > > > > API call, so we wouldn't be able to return an error in the
> API,
> > > > > though.
> > > > > > > >
> > > > > > > > --
> > > > > > > > Russell Bryant
> > > > > > > >
> > > > >
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 8/8] automake: Add ovs-bugtool.in to flake8-check.

[Aaron Rosen]

This patch seemed to break the build with some flake8 python3 errors for
me. I have a patch coming up to fix it.

On Thu, Jun 9, 2016 at 11:06 AM, Guru Shetty  wrote:

> >
> >
> >
> > All of these didn't appear to make the patchworks queue,
> > so the following is an ack of the whole series - I've
> > looked at all of them and they look good...
> >
> > Acked-by: Ryan Moats 
> >
> > Thank you Ryan! I applied the series.
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] Fix python 3 errors in ovs-bugtool.in

[Aaron Rosen]

The patch b00bdc728e7a0ae697b4fc59a4f9958b688c6789 enabled flake8-checking
to be run on ovs-bugtool.in which failed due to python3 issues. This
patch fixes those issues.

Signed-off-by: Aaron Rosen 
---
 utilities/bugtool/ovs-bugtool.in | 45 +---
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index ecf01f6..a666877 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -33,6 +33,7 @@
 # or func_output().
 #
 
+from __future__ import print_function
 import getopt
 import re
 import os
@@ -252,7 +253,7 @@ dev_null = open('/dev/null', 'r+')
 def output(x):
 global SILENT_MODE
 if not SILENT_MODE:
-print x
+print (x)
 
 
 def output_ts(x):
@@ -355,7 +356,7 @@ def collect_data():
 elif 'func' in v:
 try:
 s = v['func'](cap)
-except Exception, e:
+except Exception as e:
 s = str(e)
 if check_space(cap, k, len(s)):
 v['output'] = StringIOmtime(s)
@@ -373,7 +374,7 @@ def main(argv=None):
 collect_all_info = True
 
 if '--help' in sys.argv:
-print """\
+print ("""\
 %(argv0)s: create status report bundles to assist in problem diagnosis
 usage: %(argv0)s OPTIONS
 
@@ -398,12 +399,12 @@ Output options:
   --outfd=FD write output to FD (requires --output=tar)
   --unlimitedignore default limits on sizes of data collected
   --debugprint ovs-bugtool debug info on stdout\
-""" % {'argv0': sys.argv[0]}
+""" % {'argv0': sys.argv[0]})
 sys.exit(0)
 
 # we need access to privileged files, exit if we are not running as root
 if os.getuid() != 0:
-print >>sys.stderr, "Error: ovs-bugtool must be run as root"
+print ("Error: ovs-bugtool must be run as root", file=sys.stderr)
 return 1
 
 output_file = None
@@ -418,8 +419,8 @@ Output options:
 argv, 'sy', ['capabilities', 'silent', 'yestoall', 'entries=',
  'output=', 'outfd=', 'outfile=', 'all', 'unlimited',
  'debug', 'ovs', 'log-days='])
-except getopt.GetoptError, opterr:
-print >>sys.stderr, opterr
+except getopt.GetoptError as opterr:
+print (opterr, file=sys.stderr)
 return 2
 
 try:
@@ -439,7 +440,7 @@ Output options:
 if v in ['tar', 'tar.bz2', 'tar.gz', 'zip']:
 output_type = v
 else:
-print >>sys.stderr, "Invalid output format '%s'" % v
+print ("Invalid output format '%s'" % v, file=sys.stderr)
 return 2
 
 # "-s" or "--silent" means suppress output (except for the final
@@ -461,7 +462,8 @@ Output options:
 old = fcntl.fcntl(output_fd, fcntl.F_GETFD)
 fcntl.fcntl(output_fd, fcntl.F_SETFD, old | fcntl.FD_CLOEXEC)
 except:
-print >>sys.stderr, "Invalid output file descriptor", output_fd
+print ("Invalid output file descriptor", output_fd,
+   file=sys.stderr)
 return 2
 
 if k == '--outfile':
@@ -483,15 +485,16 @@ Output options:
 log_days = int(v)
 
 if len(params) != 1:
-print >>sys.stderr, "Invalid additional arguments", str(params)
+print ("Invalid additional arguments", str(params), file=sys.stderr)
 return 2
 
 if output_fd != -1 and output_type != 'tar':
-print >>sys.stderr, "Option '--outfd' only valid with '--output=tar'"
+print ("Option '--outfd' only valid with '--output=tar'",
+   file=sys.stderr)
 return 2
 
 if output_fd != -1 and output_file is not None:
-print >>sys.stderr, "Cannot set both '--outfd' and '--outfile'"
+print ("Cannot set both '--outfd' and '--outfile'", file=sys.stderr)
 return 2
 
 if output_file is not None and not unlimited_data:
@@ -713,10 +716,10 @@ exclude those logs from the archive.
 make_zip(subdir, output_file)
 
 if dbg:
-print >>sys.stderr, "Category sizes (max, actual):\n"
+print ("Category sizes (max, actual):\n", file=sys.stderr)
 fo

[ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

This patch exposes the c function expr_parse_string() to be called via
python. The motivation for this is so that clients interfacing with
ovn can call this method in order to validate the data they are writting
to ovn.

Previously, there were several bugs in the neutron/ovn integration
that went unnoticed due to the client writing invalid data. This should
hopefully help catch errors like this earlier as it can now be detected on
the client side and an error can be raised.

Signed-off-by: Aaron Rosen 
---
 ovn/lib/actions.c   | 106 
 ovn/lib/actions.h   |   3 ++
 python/automake.mk  |   4 +-
 python/ovs/_ovn-utils.c |  65 +
 python/setup.py |  31 +-
 tests/automake.mk   |   1 +
 tests/test-ovn.c| 104 ---
 tests/test-ovn_utils.py |  33 +++
 8 files changed, 241 insertions(+), 106 deletions(-)
 create mode 100644 python/ovs/_ovn-utils.c
 create mode 100644 tests/test-ovn_utils.py

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 5f0bf19..8ff06b0 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -26,6 +26,7 @@
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/ofp-actions.h"
 #include "openvswitch/ofpbuf.h"
+#include "shash.h"
 #include "simap.h"
 
 /* Context maintained during actions_parse(). */
@@ -564,3 +565,108 @@ actions_parse_string(const char *s, const struct 
action_params *ap,
 
 return error;
 }
+
+void
+create_symtab(struct shash *symtab)
+{
+shash_init(symtab);
+
+/* Reserve a pair of registers for the logical inport and outport.  A full
+ * 32-bit register each is bigger than we need, but the expression code
+ * doesn't yet support string fields that occupy less than a full OXM. */
+expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
+expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
+
+expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
+expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
+expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
+
+expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
+expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
+expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
+expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
+expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
+expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
+
+expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
+expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
+expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
+
+expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
+expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
+expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
+ "vlan.tci[13..15]");
+expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
+ "vlan.tci[0..11]");
+
+expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
+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.ecn", MFF_IP_ECN, "ip", false);
+expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
+
+expr_symtab_add_field(symtab, "ip4.src", MFF_IPV4_SRC, "ip4", false);
+expr_symtab_add_field(symtab, "ip4.dst", MFF_IPV4_DST, "ip4", false);
+
+expr_symtab_add_predicate(symtab, "icmp4", "ip4 && ip.proto == 1");
+expr_symtab_add_field(symtab, "icmp4.type", MFF_ICMPV4_TYPE, "icmp4",
+  false);
+expr_symtab_add_field(symtab, "icmp4.code", MFF_ICMPV4_CODE, "icmp4",
+  false);
+
+expr_symtab_add_field(symtab, "ip6.src", MFF_IPV6_SRC, "ip6", false);
+expr_symtab_add_field(symtab, "ip6.dst", MFF_IPV6_DST, "ip6", false);
+expr_symtab_add_field(symtab, "ip6.label", MFF_IPV6_LABEL, "ip6"

Re: [ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

Hi,

I figured I should post a WIP patch for this work just to gather some
feedback if anyone has any.

I still need to figure out how to test/add python3 support. I'm not sure
how to install the python ovs module so that I can import it via python3.
If anyone knows how to do that via the source tree that would be helpful.

I'm also wondering if we want to move the lib/ovn methods to the
openvswitch-dev package.  I think doing this will allow me to remove the
ovn/lib files in the setup.py file.

I'm also struggling writing the .at file in order to get my unit tests to
run. I think I should be able to sort through this though.


Aaron




On Mon, Jun 20, 2016 at 1:12 PM, Aaron Rosen  wrote:

> This patch exposes the c function expr_parse_string() to be called via
> python. The motivation for this is so that clients interfacing with
> ovn can call this method in order to validate the data they are writting
> to ovn.
>
> Previously, there were several bugs in the neutron/ovn integration
> that went unnoticed due to the client writing invalid data. This should
> hopefully help catch errors like this earlier as it can now be detected on
> the client side and an error can be raised.
>
> Signed-off-by: Aaron Rosen 
> ---
>  ovn/lib/actions.c   | 106
> 
>  ovn/lib/actions.h   |   3 ++
>  python/automake.mk  |   4 +-
>  python/ovs/_ovn-utils.c |  65 +
>  python/setup.py |  31 +-
>  tests/automake.mk   |   1 +
>  tests/test-ovn.c| 104
> ---
>  tests/test-ovn_utils.py |  33 +++
>  8 files changed, 241 insertions(+), 106 deletions(-)
>  create mode 100644 python/ovs/_ovn-utils.c
>  create mode 100644 tests/test-ovn_utils.py
>
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 5f0bf19..8ff06b0 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -26,6 +26,7 @@
>  #include "openvswitch/dynamic-string.h"
>  #include "openvswitch/ofp-actions.h"
>  #include "openvswitch/ofpbuf.h"
> +#include "shash.h"
>  #include "simap.h"
>
>  /* Context maintained during actions_parse(). */
> @@ -564,3 +565,108 @@ actions_parse_string(const char *s, const struct
> action_params *ap,
>
>  return error;
>  }
> +
> +void
> +create_symtab(struct shash *symtab)
> +{
> +shash_init(symtab);
> +
> +/* Reserve a pair of registers for the logical inport and outport.  A
> full
> + * 32-bit register each is bigger than we need, but the expression
> code
> + * doesn't yet support string fields that occupy less than a full
> OXM. */
> +expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
> +expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
> +
> +expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
> +expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
> +expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
> +
> +expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
> +expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
> +expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
> +expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
> +expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
> +expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
> +
> +expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
> +expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
> +expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
> +
> +expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
> +expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
> +expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
> + "vlan.tci[13..15]");
> +expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
> + "vlan.tci[0..11]");
> +
> +expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
> +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_

[ovs-dev] [PATCH] update .gitignore files for debian build artifacts

[Aaron Rosen]

After running: `fakeroot debian/rules binary`

These files are left uncommitted to the source tree and should be ignored.

Signed-off-by: Aaron Rosen 
---
 .gitignore| 1 +
 debian/.gitignore | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/.gitignore b/.gitignore
index 077a178..5965a1e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,3 +72,4 @@ testsuite.tmp.orig
 /rpm/
 /openvswitch-*.tar.gz
 /tests/lcov/
+openvswitch.tar.gz
diff --git a/debian/.gitignore b/debian/.gitignore
index 2a509ca..88fb00e 100644
--- a/debian/.gitignore
+++ b/debian/.gitignore
@@ -24,3 +24,6 @@
 /ovn-docker
 /python-openvswitch
 /tmp
+/autoreconf.after
+/autoreconf.before
+/openvswitch-dev
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] update .gitignore files for debian build artifacts

[Aaron Rosen]

These have been getting in my away because I need to rebuild openvswitch
for it's packages for openvswitch-dev which is required for the python C
bindings in the tree. Would be good to figure out how to be able to install
the python bindings using the intree code rather the package.


On Wed, Jun 22, 2016 at 1:04 PM, Aaron Rosen  wrote:

> After running: `fakeroot debian/rules binary`
>
> These files are left uncommitted to the source tree and should be ignored.
>
> Signed-off-by: Aaron Rosen 
> ---
>  .gitignore| 1 +
>  debian/.gitignore | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/.gitignore b/.gitignore
> index 077a178..5965a1e 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -72,3 +72,4 @@ testsuite.tmp.orig
>  /rpm/
>  /openvswitch-*.tar.gz
>  /tests/lcov/
> +openvswitch.tar.gz
> diff --git a/debian/.gitignore b/debian/.gitignore
> index 2a509ca..88fb00e 100644
> --- a/debian/.gitignore
> +++ b/debian/.gitignore
> @@ -24,3 +24,6 @@
>  /ovn-docker
>  /python-openvswitch
>  /tmp
> +/autoreconf.after
> +/autoreconf.before
> +/openvswitch-dev
> --
> 1.9.1
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

This patch exposes the c function expr_parse_string() to be called via
python. The motivation for this is so that clients interfacing with
ovn can call this method in order to validate the data they are writting
to ovn.

Previously, there were several bugs in the neutron/ovn integration
that went unnoticed due to the client writing invalid data. This should
hopefully help catch errors like this earlier as it can now be detected on
the client side and an error can be raised.
---
 ovn/lib/actions.c   | 105 
 ovn/lib/actions.h   |   3 ++
 python/automake.mk  |   4 +-
 python/ovs/_ovn-utils.c | 104 +++
 python/setup.py |  31 +-
 tests/automake.mk   |   4 +-
 tests/test-ovn-utils.at |  13 ++
 tests/test-ovn-utils.py |  33 +++
 tests/test-ovn.c| 104 ---
 tests/testsuite.at  |   1 +
 10 files changed, 295 insertions(+), 107 deletions(-)
 create mode 100644 python/ovs/_ovn-utils.c
 create mode 100644 tests/test-ovn-utils.at
 create mode 100644 tests/test-ovn-utils.py

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 52c74e6..06f413d 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -1002,3 +1002,108 @@ actions_parse_string(const char *s, const struct 
action_params *ap,
 
 return error;
 }
+
+void
+create_symtab(struct shash *symtab)
+{
+shash_init(symtab);
+
+/* Reserve a pair of registers for the logical inport and outport.  A full
+ * 32-bit register each is bigger than we need, but the expression code
+ * doesn't yet support string fields that occupy less than a full OXM. */
+expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
+expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
+
+expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
+expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
+expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
+
+expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
+expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
+expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
+expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
+expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
+expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
+
+expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
+expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
+expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
+
+expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
+expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
+expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
+ "vlan.tci[13..15]");
+expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
+ "vlan.tci[0..11]");
+
+expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
+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.ecn", MFF_IP_ECN, "ip", false);
+expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
+
+expr_symtab_add_field(symtab, "ip4.src", MFF_IPV4_SRC, "ip4", false);
+expr_symtab_add_field(symtab, "ip4.dst", MFF_IPV4_DST, "ip4", false);
+
+expr_symtab_add_predicate(symtab, "icmp4", "ip4 && ip.proto == 1");
+expr_symtab_add_field(symtab, "icmp4.type", MFF_ICMPV4_TYPE, "icmp4",
+  false);
+expr_symtab_add_field(symtab, "icmp4.code", MFF_ICMPV4_CODE, "icmp4",
+  false);
+
+expr_symtab_add_field(symtab, "ip6.src", MFF_IPV6_SRC, "ip6", false);
+expr_symtab_add_field(symtab, "ip6.dst", MFF_IPV6_DST, "ip6", false);
+expr_symtab_add_field(symtab, "ip6.label", MFF_IPV6_LABEL, "ip6", false);
+
+expr_symtab_add_predicate(symtab, "icmp6", "ip6 && ip.proto == 58");
+expr_symtab_add_field(symtab, "icmp6.type", MFF_ICMPV6_TYPE, "icmp6",
+  true);
+expr_symtab_add_field(symtab, "icmp6.code", MFF_ICMPV6_CODE, "icmp6",
+  true);
+
+expr_symtab_add_predicate(symtab, "icmp", "icmp4 || icmp6");
+
+expr_symtab_add_field(symtab, "ip.frag", MFF_IP_FRAG, "ip", false);
+expr_symtab_add_predicate(symtab, "ip.is_frag", "ip.frag[0]");
+expr_symtab_add_predicate(symtab, "ip.later_frag", "ip.frag[1]");
+expr_symtab_add_predicate(symtab, "ip.first_frag", "ip.is_frag && 
!ip.later_frag");
+
+expr_symtab_add_predicate(symtab, "arp", "eth.type == 0x806");
+expr_symtab_add_field(sym

Re: [ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

Here's the v2 version of this (Sorry this is a new email message I was
hoping that this would be grouped together with the previous thread).

This patch adds python3 support and hooks in the testcases with autotest.

I was planning on reusing the existing test inputs that test-ovn parse-expr
uses but I decided against that because I only want parse_match() to return
something if there is an error otherwise just none - test-ovn parse-expr
returns the normalized version of the match which we don't need.

Let me know what you guys think of this patch when you get a chance.

Best,

Aaron



On Thu, Jun 30, 2016 at 2:27 PM, Aaron Rosen  wrote:

> This patch exposes the c function expr_parse_string() to be called via
> python. The motivation for this is so that clients interfacing with
> ovn can call this method in order to validate the data they are writting
> to ovn.
>
> Previously, there were several bugs in the neutron/ovn integration
> that went unnoticed due to the client writing invalid data. This should
> hopefully help catch errors like this earlier as it can now be detected on
> the client side and an error can be raised.
> ---
>  ovn/lib/actions.c   | 105
> 
>  ovn/lib/actions.h   |   3 ++
>  python/automake.mk  |   4 +-
>  python/ovs/_ovn-utils.c | 104
> +++
>  python/setup.py |  31 +-
>  tests/automake.mk   |   4 +-
>  tests/test-ovn-utils.at |  13 ++
>  tests/test-ovn-utils.py |  33 +++
>  tests/test-ovn.c| 104
> ---
>  tests/testsuite.at  |   1 +
>  10 files changed, 295 insertions(+), 107 deletions(-)
>  create mode 100644 python/ovs/_ovn-utils.c
>  create mode 100644 tests/test-ovn-utils.at
>  create mode 100644 tests/test-ovn-utils.py
>
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 52c74e6..06f413d 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -1002,3 +1002,108 @@ actions_parse_string(const char *s, const struct
> action_params *ap,
>
>  return error;
>  }
> +
> +void
> +create_symtab(struct shash *symtab)
> +{
> +shash_init(symtab);
> +
> +/* Reserve a pair of registers for the logical inport and outport.  A
> full
> + * 32-bit register each is bigger than we need, but the expression
> code
> + * doesn't yet support string fields that occupy less than a full
> OXM. */
> +expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
> +expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
> +
> +expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
> +expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
> +expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
> +
> +expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
> +expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
> +expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
> +expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
> +expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
> +expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
> +
> +expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
> +expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
> +expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
> +
> +expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
> +expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
> +expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
> + "vlan.tci[13..15]");
> +expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
> + "vlan.tci[0..11]");
> +
> +expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
> +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.ecn", MFF_IP_ECN, "ip", false);
>

Re: [ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

Yea sorry about that. I'll try and make sure I do that next time around.

Aaron

On Fri, Jul 1, 2016 at 7:39 AM, Ryan Moats  wrote:

> "dev"  wrote on 06/30/2016 04:27:19 PM:
>
> > From: Aaron Rosen 
> > To: dev@openvswitch.org
> > Cc: Aaron Rosen 
> > Date: 06/30/2016 04:29 PM
> > Subject: [ovs-dev] [PATCH] ovn: expose c validator to python
> > Sent by: "dev" 
> >
> > This patch exposes the c function expr_parse_string() to be called via
> > python. The motivation for this is so that clients interfacing with
> > ovn can call this method in order to validate the data they are writting
> > to ovn.
> >
> > Previously, there were several bugs in the neutron/ovn integration
> > that went unnoticed due to the client writing invalid data. This should
> > hopefully help catch errors like this earlier as it can now be detected
> on
> > the client side and an error can be raised.
> > ---
>
> As an FYI, if this is v2 of the patch, please indicate that in the
> subject line by using the -v or --reroll-count option to
> git-format-patch. That makes keeping track of superceded patches in
> the patch queue easier...
>
> Thanks in advance,
> Ryan
>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: expose c validator to python

[Aaron Rosen]

Thanks for the review. Response inline

On Sun, Jul 3, 2016 at 12:04 PM, Ben Pfaff  wrote:

> On Thu, Jun 30, 2016 at 02:27:19PM -0700, Aaron Rosen wrote:
> > This patch exposes the c function expr_parse_string() to be called via
> > python. The motivation for this is so that clients interfacing with
> > ovn can call this method in order to validate the data they are writting
> > to ovn.
> >
> > Previously, there were several bugs in the neutron/ovn integration
> > that went unnoticed due to the client writing invalid data. This should
> > hopefully help catch errors like this earlier as it can now be detected
> on
> > the client side and an error can be raised.
>
> I'm OK with the idea of moving the code to create a symtab for testing
> purposes into the OVN library, but I think that it should go into
> expr.[ch] since that's what implements expressions, and it should have a
> name and comment that makes it clear that it's for testing.
>


Sounds good to me I'll make this update.


>
> I don't yet understand how to use this, though.  When I run a normal
> "make check" I get failures like this:
>
> # -*- compilation -*-
> 2132. test-ovn-utils.at:12: testing test-ovn-utils - Python2 ...
> ../../tests/test-ovn-utils.at:12: $PYTHON $srcdir/test-ovn-utils.py
> stderr:
> Traceback (most recent call last):
>   File "../../../../tests/test-ovn-utils.py", line 17, in 
> from ovs import ovn_utils
> ImportError: cannot import name ovn_utils
> stdout:
> ../../tests/test-ovn-utils.at:12: exit code was 1, expected 0
> 2132. test-ovn-utils.at:12: 2132. test-ovn-utils - Python2 (
> test-ovn-utils.at:12): FAILED (test-ovn-utils.at:12)
>
> Is some extra step required?


Looks like you need to run:

sudo python setup.py install
and
sudo python3 setup.py install (if you want the python3 tests to run).

inside of  ovs/python first.

Is there a good place were we could add a hook to make that occur?  I think
the other issue that we are facing before that is in order to build these C
extensions it currently is relying on the openvswitch-dev package being
installed on the system rather than able to use it from the current source
tree: https://github.com/openvswitch/ovs/blob/master/python/setup.py#L80

I believe Terry Wilson said that he was working on a fix for that though.





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

[ovs-dev] [PATCH v3] ovn: expose c validator to python

[Aaron Rosen]

This patch exposes the c function expr_parse_string() to be called via
python. The motivation for this is so that clients interfacing with
ovn can call this method in order to validate the data they are writting
to ovn.

Previously, there were several bugs in the neutron/ovn integration
that went unnoticed due to the client writing invalid data. This should
hopefully help catch errors like this earlier as it can now be detected on
the client side and an error can be raised.
---
 ovn/lib/expr.c  | 107 +++
 ovn/lib/expr.h  |   1 +
 python/automake.mk  |   4 +-
 python/ovs/_ovn-utils.c | 104 ++
 python/setup.py |  31 +-
 tests/automake.mk   |   4 +-
 tests/test-ovn-utils.at |  15 +++
 tests/test-ovn-utils.py |  33 +++
 tests/test-ovn.c| 108 +---
 tests/testsuite.at  |   1 +
 10 files changed, 299 insertions(+), 109 deletions(-)
 create mode 100644 python/ovs/_ovn-utils.c
 create mode 100644 tests/test-ovn-utils.at
 create mode 100644 tests/test-ovn-utils.py

diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 8c0768d..05036ee 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -3007,3 +3007,110 @@ expr_parse_constant_set(struct lexer *lexer, const 
struct shash *symtab,
 parse_constant_set(&ctx, cs);
 return ctx.error;
 }
+
+/* create_symtab_helper() is used for testing purposes and in the ovn_utils
+ * python package. */
+void
+create_symtab_helper(struct shash *symtab)
+{
+shash_init(symtab);
+
+/* Reserve a pair of registers for the logical inport and outport.  A full
+ * 32-bit register each is bigger than we need, but the expression code
+ * doesn't yet support string fields that occupy less than a full OXM. */
+expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
+expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
+
+expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
+expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
+expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
+
+expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
+expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
+expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
+expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
+expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
+expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
+
+expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
+expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
+expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
+
+expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
+expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
+expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
+ "vlan.tci[13..15]");
+expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
+ "vlan.tci[0..11]");
+
+expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
+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.ecn", MFF_IP_ECN, "ip", false);
+expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
+
+expr_symtab_add_field(symtab, "ip4.src", MFF_IPV4_SRC, "ip4", false);
+expr_symtab_add_field(symtab, "ip4.dst", MFF_IPV4_DST, "ip4", false);
+
+expr_symtab_add_predicate(symtab, "icmp4", "ip4 && ip.proto == 1");
+expr_symtab_add_field(symtab, "icmp4.type", MFF_ICMPV4_TYPE, "icmp4",
+  false);
+expr_symtab_add_field(symtab, "icmp4.code", MFF_ICMPV4_CODE, "icmp4",
+  false);
+
+expr_symtab_add_field(symtab, "ip6.src", MFF_IPV6_SRC, "ip6", false);
+expr_symtab_add_field(symtab, "ip6.dst", MFF_IPV6_DST, "ip6", false);
+expr_symtab_add_field(symtab, "ip6.label", MFF_IPV6_LABEL, "ip6", false);
+
+expr_symtab_add_predicate(symtab, "icmp6", "ip6 && ip.proto == 58");
+expr_symtab_add_field(symtab, "icmp6.type", MFF_ICMPV6_TYPE, "icmp6",
+  true);
+expr_symtab_add_field(symtab, "icmp6.code", MFF_ICMPV6_CODE, "icmp6",
+  true);
+
+expr_symtab_add_predicate(symtab, "icmp", "icmp4 || icmp6");
+
+expr_symtab_add_field(symtab, "ip.frag", MFF_IP_FRAG, "ip", false);
+expr_symtab_add_predicate(symtab, "ip.is_frag", "ip.frag[0]");
+expr_symtab_add_predicate(symtab, "ip.later_frag", "ip.frag[1]");
+expr_symtab_add_predicate(symtab, "ip.first_frag"

Re: [ovs-dev] [PATCH v3] ovn: expose c validator to python

[Aaron Rosen]

Changes for v3:

- Move create_symtab to expr.[ch] and rename create_symtab ->
create_symtab_helper (also added comment above that method is for testing).
- update test-ovn-utils.at to skip the tests if the ovs.ovn_utils libraries
is not installed. I figure this is a good work around until we can get
everything running/install from tree.
- rebase and update code

Thanks,

Aaron





On Tue, Jul 5, 2016 at 1:32 PM, Aaron Rosen  wrote:

> This patch exposes the c function expr_parse_string() to be called via
> python. The motivation for this is so that clients interfacing with
> ovn can call this method in order to validate the data they are writting
> to ovn.
>
> Previously, there were several bugs in the neutron/ovn integration
> that went unnoticed due to the client writing invalid data. This should
> hopefully help catch errors like this earlier as it can now be detected on
> the client side and an error can be raised.
> ---
>  ovn/lib/expr.c  | 107
> +++
>  ovn/lib/expr.h  |   1 +
>  python/automake.mk  |   4 +-
>  python/ovs/_ovn-utils.c | 104
> ++
>  python/setup.py |  31 +-
>  tests/automake.mk   |   4 +-
>  tests/test-ovn-utils.at |  15 +++
>  tests/test-ovn-utils.py |  33 +++
>  tests/test-ovn.c| 108
> +---
>  tests/testsuite.at  |   1 +
>  10 files changed, 299 insertions(+), 109 deletions(-)
>  create mode 100644 python/ovs/_ovn-utils.c
>  create mode 100644 tests/test-ovn-utils.at
>  create mode 100644 tests/test-ovn-utils.py
>
> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> index 8c0768d..05036ee 100644
> --- a/ovn/lib/expr.c
> +++ b/ovn/lib/expr.c
> @@ -3007,3 +3007,110 @@ expr_parse_constant_set(struct lexer *lexer, const
> struct shash *symtab,
>  parse_constant_set(&ctx, cs);
>  return ctx.error;
>  }
> +
> +/* create_symtab_helper() is used for testing purposes and in the
> ovn_utils
> + * python package. */
> +void
> +create_symtab_helper(struct shash *symtab)
> +{
> +shash_init(symtab);
> +
> +/* Reserve a pair of registers for the logical inport and outport.  A
> full
> + * 32-bit register each is bigger than we need, but the expression
> code
> + * doesn't yet support string fields that occupy less than a full
> OXM. */
> +expr_symtab_add_string(symtab, "inport", MFF_REG6, NULL);
> +expr_symtab_add_string(symtab, "outport", MFF_REG7, NULL);
> +
> +expr_symtab_add_field(symtab, "xreg0", MFF_XREG0, NULL, false);
> +expr_symtab_add_field(symtab, "xreg1", MFF_XREG1, NULL, false);
> +expr_symtab_add_field(symtab, "xreg2", MFF_XREG2, NULL, false);
> +
> +expr_symtab_add_subfield(symtab, "reg0", NULL, "xreg0[32..63]");
> +expr_symtab_add_subfield(symtab, "reg1", NULL, "xreg0[0..31]");
> +expr_symtab_add_subfield(symtab, "reg2", NULL, "xreg1[32..63]");
> +expr_symtab_add_subfield(symtab, "reg3", NULL, "xreg1[0..31]");
> +expr_symtab_add_subfield(symtab, "reg4", NULL, "xreg2[32..63]");
> +expr_symtab_add_subfield(symtab, "reg5", NULL, "xreg2[0..31]");
> +
> +expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
> +expr_symtab_add_field(symtab, "eth.dst", MFF_ETH_DST, NULL, false);
> +expr_symtab_add_field(symtab, "eth.type", MFF_ETH_TYPE, NULL, true);
> +
> +expr_symtab_add_field(symtab, "vlan.tci", MFF_VLAN_TCI, NULL, false);
> +expr_symtab_add_predicate(symtab, "vlan.present", "vlan.tci[12]");
> +expr_symtab_add_subfield(symtab, "vlan.pcp", "vlan.present",
> + "vlan.tci[13..15]");
> +expr_symtab_add_subfield(symtab, "vlan.vid", "vlan.present",
> + "vlan.tci[0..11]");
> +
> +expr_symtab_add_predicate(symtab, "ip4", "eth.type == 0x800");
> +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.ecn", MFF_IP_ECN, "ip", false);
> +expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip&

Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

Awesome thanks Ben! I'll update neutron tomorrow morning to work with this.


> On Jun 25, 2015, at 5:35 PM, Ben Pfaff  wrote:
> 
> Until now, the OVN_Northbound schema has been designed to sidestep a
> weakness in the OVSDB protocol when a column has a great deal of data in
> it.  In the current OVSDB protocol, whenever a column changes, the entire
> new value of the column is sent to all of the clients that are monitoring
> that column.  That means that adding or removing a small amount of data,
> say 1 element in a set, requires sending all of the data, which is
> expensive if the column has a lot of data.
> 
> One example of a column with potential to have a lot of data is the set of
> ports within a logical switch, if a logical switch has a large number of
> ports.  Thus, the existing OVN_Northbound schema has each Logical_Port
> point to its containing Logical_Switch instead of the other way around.
> This sidesteps the problem because it does not use any large columns.
> 
> The tradeoff that this forces, however, is that the schema cannot take
> advantage of OVSDB's garbage collection feature, where it automatically
> deletes rows that are unreferenced.  That's a problem for Neutron because
> of Neutron-internal races between deletion of a Logical_Switch and
> creation of new Logical_Ports on the switch being deleted.  When such a
> race happens, OVSDB refuses to delete the Logical_Switch because of
> references to it from the newly created Logical_Port (although Neutron
> does delete the pre-existing logical ports).
> 
> To solve the problem, this commit changes the OVN_Northbound schema to
> use a set of ports within Logical_Switch.  That will lead to large columns
> for large logical switches; I plan to address that (though I don't have
> code written) by enhancing the OVSDB protocol.  With this commit applied,
> the database will automatically cascade deleting a logical switch row to
> delete all of its ports, ACLs, and its router port (if any).
> 
> This commit makes some pretty pervasive changes to ovn-northd, but they
> are mostly beneficial to the code readability because now it becomes
> possible to trivially iterate through the ports that belong to a switch,
> which was difficult before the schema change.
> 
> This commit will break the Neutron integration until that is changed to
> handle the new database schema.
> 
> CC: Aaron Rosen 
> Signed-off-by: Ben Pfaff 
> ---
> ovn/northd/ovn-northd.c | 318 ++--
> ovn/ovn-nb.ovsschema|  39 +++---
> ovn/ovn-nb.xml  |  74 ++-
> ovn/ovn-nbctl.c | 116 ++
> 4 files changed, 282 insertions(+), 265 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index f37df77..c790a90 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -281,139 +281,116 @@ build_pipeline(struct northd_context *ctx)
> }
> 
> /* Table 0: Ingress port security. */
> -const struct nbrec_logical_port *lport;
> -NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> -struct ds match = DS_EMPTY_INITIALIZER;
> -ds_put_cstr(&match, "inport == ");
> -json_string_escape(lport->name, &match);
> -build_port_security("eth.src",
> -lport->port_security, lport->n_port_security,
> -&match);
> -pipeline_add(&pc, lport->lswitch, 0, 50, ds_cstr(&match),
> - lport_is_enabled(lport) ? "next;" : "drop;");
> -ds_destroy(&match);
> -}
> -
> -/* Table 1: Destination lookup, broadcast and multicast handling 
> (priority
> - * 100). */
> NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> -struct ds actions;
> -
> -ds_init(&actions);
> -NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> -if (lport->lswitch == lswitch && lport_is_enabled(lport)) {
> -ds_put_cstr(&actions, "outport = ");
> -json_string_escape(lport->name, &actions);
> -ds_put_cstr(&actions, "; next; ");
> -}
> +for (size_t i = 0; i < lswitch->n_ports; i++) {
> +const struct nbrec_logical_port *lport = lswitch->ports[i];
> +struct ds match = DS_EMPTY_INITIALIZER;
> +ds_put_cstr(&match, "inport == ");
> +json_string_escape(lport->name, &match);
> +build_port_security("eth.src",
> +lport-&

Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

Hi Ben, 

I just tested out this patch and it seems to work as expected for me. I do have 
a few quick questions/thoughts.

- One thing I noticed is if I create a logical_port but don't append the 
port.uuid to  the Logical_Switch.ports column it seems like the port isn't 
showing up in ovsdb.  I'm not sure if this is expected or I'm incorrect.

- Another thing I was thinking about is if requiring the caller to query ovsdb 
for the logical_switch to obtain the switch_ports and then appending the newly 
create logical port could be problematic. For one it seems like we need to lock 
ovsdb for concurrent operations here.  I was wondering if it would be possible 
to keep previous schema where the logical_port stores a reference to it's 
logical_switch. Though, when the logical_switch is deleted we would cascade the 
deletes of the logical_ports on that switch? I also think it feels more natural 
to have a reference from the logical_port to logical_switch because now it 
seems like the schema allows one to create a logical_port without a 
logical_switch? Perhaps ovsdb can't currently work in this way which you were 
explaining in your commit message though I just wanted to make sure.

Thanks, 

Aaron



From: Ben Pfaff 
Sent: Thursday, June 25, 2015 4:57 PM
To: Aaron Rosen
Cc: dev@openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in 
OVN_Northbound schema.

Thanks.  Of course this patch needs review on the OVS side as well; that
might take a few days.

On Thu, Jun 25, 2015 at 11:51:57PM +, Aaron Rosen wrote:
> Awesome thanks Ben! I'll update neutron tomorrow morning to work with this.
>
>
> > On Jun 25, 2015, at 5:35 PM, Ben Pfaff  wrote:
> >
> > Until now, the OVN_Northbound schema has been designed to sidestep a
> > weakness in the OVSDB protocol when a column has a great deal of data in
> > it.  In the current OVSDB protocol, whenever a column changes, the entire
> > new value of the column is sent to all of the clients that are monitoring
> > that column.  That means that adding or removing a small amount of data,
> > say 1 element in a set, requires sending all of the data, which is
> > expensive if the column has a lot of data.
> >
> > One example of a column with potential to have a lot of data is the set of
> > ports within a logical switch, if a logical switch has a large number of
> > ports.  Thus, the existing OVN_Northbound schema has each Logical_Port
> > point to its containing Logical_Switch instead of the other way around.
> > This sidesteps the problem because it does not use any large columns.
> >
> > The tradeoff that this forces, however, is that the schema cannot take
> > advantage of OVSDB's garbage collection feature, where it automatically
> > deletes rows that are unreferenced.  That's a problem for Neutron because
> > of Neutron-internal races between deletion of a Logical_Switch and
> > creation of new Logical_Ports on the switch being deleted.  When such a
> > race happens, OVSDB refuses to delete the Logical_Switch because of
> > references to it from the newly created Logical_Port (although Neutron
> > does delete the pre-existing logical ports).
> >
> > To solve the problem, this commit changes the OVN_Northbound schema to
> > use a set of ports within Logical_Switch.  That will lead to large columns
> > for large logical switches; I plan to address that (though I don't have
> > code written) by enhancing the OVSDB protocol.  With this commit applied,
> > the database will automatically cascade deleting a logical switch row to
> > delete all of its ports, ACLs, and its router port (if any).
> >
> > This commit makes some pretty pervasive changes to ovn-northd, but they
> > are mostly beneficial to the code readability because now it becomes
> > possible to trivially iterate through the ports that belong to a switch,
> > which was difficult before the schema change.
> >
> > This commit will break the Neutron integration until that is changed to
> > handle the new database schema.
> >
> > CC: Aaron Rosen 
> > Signed-off-by: Ben Pfaff 
> > ---
> > ovn/northd/ovn-northd.c | 318 
> > ++--
> > ovn/ovn-nb.ovsschema|  39 +++---
> > ovn/ovn-nb.xml  |  74 ++-
> > ovn/ovn-nbctl.c | 116 ++
> > 4 files changed, 282 insertions(+), 265 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index f37df77..c790a90 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@

Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

Would it be possible to keep the lswitch.uuid value on the port like before, 
but not enforce the integrity there if the network does not exist. Then, have 
the garbage collector here remove the ports on the network for us when the 
network is deleted? This way we don't need to have any retry logic on our end. 
My concern is creating ports on a network is something that occurs usually in a 
very bursty  manor and I think we'll often have to retry.  If not I think 
retrying on our side is probably okay. 

Thanks, 

Aaron




From: Ben Pfaff 
Sent: Friday, June 26, 2015 11:37 AM
To: Aaron Rosen
Cc: dev@openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in 
OVN_Northbound schema.

On Fri, Jun 26, 2015 at 05:20:16PM +0000, Aaron Rosen wrote:
> I just tested out this patch and it seems to work as expected for
> me. I do have a few quick questions/thoughts.
>
> - One thing I noticed is if I create a logical_port but don't append
> the port.uuid to the Logical_Switch.ports column it seems like the
> port isn't showing up in ovsdb.  I'm not sure if this is expected or
> I'm incorrect.

That's a consequence of the garbage collection.  A Logical_Port that
isn't attached to a Logical_Switch gets destroyed.  That's true if it
was previously attached to a Logical_Switch that was just deleted, or if
it was created within the current transaction and never attached to a
Logical_Switch.

> - Another thing I was thinking about is if requiring the caller to
> query ovsdb for the logical_switch to obtain the switch_ports and then
> appending the newly create logical port could be problematic. For one
> it seems like we need to lock ovsdb for concurrent operations here.

OVSDB doesn't support locking, so the equivalent that allows one to
avoid problems with "dirty reads" is to verify that the ports are the
expected set (the set before the transaction started) and make the
transaction abort if it isn't correct.  With the OSVDB Python IDL,
that's just a matter of calling Row.verify() on the column in question
before modifying the column.  (If the transaction aborts, then you just
retry it.)

This is the purpose of the nbrec_logical_switch_verify_ports() calls in
do_lport_add() and remove_lport() in ovn-nbctl.c as modified by this
commit, by the way.

> I was wondering if it would be possible to keep previous schema where
> the logical_port stores a reference to it's logical_switch. Though,
> when the logical_switch is deleted we would cascade the deletes of the
> logical_ports on that switch?

OVSDB only supports cascading with the schema in the form used by this
comit.

> I also think it feels more natural to have a reference from the
> logical_port to logical_switch because now it seems like the schema
> allows one to create a logical_port without a logical_switch? Perhaps
> ovsdb can't currently work in this way which you were explaining in
> your commit message though I just wanted to make sure.

With this schema, an unattached logical_port can exist temporarily
within a transaction but transaction commit will delete it, so that the
unattached logical_port will never be visible to other clients (or in
the database log).
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

That sounds like it would solve this problem for us :) 

In the delete logical_port case we'd just need to delete the port entry and the 
garbage collector will automatically handle updating the logical_switch.ports 
with the current ports right? 

___
From: Ben Pfaff 
Sent: Friday, June 26, 2015 12:51 PM
To: Aaron Rosen
Cc: dev@openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in 
OVN_Northbound schema.

OVSDB can't enforce those exact semantics.

OVSDB can express operations like "add element X to this column
(regardless of what's currently there)".  If this were exposed through
the IDL (currently it's not) then that would solve the problem with
retries due to burstiness.  Does that sound like a good approach?

On Fri, Jun 26, 2015 at 07:11:10PM +, Aaron Rosen wrote:
> Would it be possible to keep the lswitch.uuid value on the port like
> before, but not enforce the integrity there if the network does not
> exist. Then, have the garbage collector here remove the ports on the
> network for us when the network is deleted? This way we don't need to
> have any retry logic on our end. My concern is creating ports on a
> network is something that occurs usually in a very bursty manor and I
> think we'll often have to retry.  If not I think retrying on our side
> is probably okay.
>
> Thanks,
>
> Aaron
>
>
>
> ____
> From: Ben Pfaff 
> Sent: Friday, June 26, 2015 11:37 AM
> To: Aaron Rosen
> Cc: dev@openvswitch.org
> Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in 
> OVN_Northbound schema.
>
> On Fri, Jun 26, 2015 at 05:20:16PM +, Aaron Rosen wrote:
> > I just tested out this patch and it seems to work as expected for
> > me. I do have a few quick questions/thoughts.
> >
> > - One thing I noticed is if I create a logical_port but don't append
> > the port.uuid to the Logical_Switch.ports column it seems like the
> > port isn't showing up in ovsdb.  I'm not sure if this is expected or
> > I'm incorrect.
>
> That's a consequence of the garbage collection.  A Logical_Port that
> isn't attached to a Logical_Switch gets destroyed.  That's true if it
> was previously attached to a Logical_Switch that was just deleted, or if
> it was created within the current transaction and never attached to a
> Logical_Switch.
>
> > - Another thing I was thinking about is if requiring the caller to
> > query ovsdb for the logical_switch to obtain the switch_ports and then
> > appending the newly create logical port could be problematic. For one
> > it seems like we need to lock ovsdb for concurrent operations here.
>
> OVSDB doesn't support locking, so the equivalent that allows one to
> avoid problems with "dirty reads" is to verify that the ports are the
> expected set (the set before the transaction started) and make the
> transaction abort if it isn't correct.  With the OSVDB Python IDL,
> that's just a matter of calling Row.verify() on the column in question
> before modifying the column.  (If the transaction aborts, then you just
> retry it.)
>
> This is the purpose of the nbrec_logical_switch_verify_ports() calls in
> do_lport_add() and remove_lport() in ovn-nbctl.c as modified by this
> commit, by the way.
>
> > I was wondering if it would be possible to keep previous schema where
> > the logical_port stores a reference to it's logical_switch. Though,
> > when the logical_switch is deleted we would cascade the deletes of the
> > logical_ports on that switch?
>
> OVSDB only supports cascading with the schema in the form used by this
> comit.
>
> > I also think it feels more natural to have a reference from the
> > logical_port to logical_switch because now it seems like the schema
> > allows one to create a logical_port without a logical_switch? Perhaps
> > ovsdb can't currently work in this way which you were explaining in
> > your commit message though I just wanted to make sure.
>
> With this schema, an unattached logical_port can exist temporarily
> within a transaction but transaction commit will delete it, so that the
> unattached logical_port will never be visible to other clients (or in
> the database log).
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

inline

From: Ben Pfaff 
Sent: Friday, June 26, 2015 2:12 PM
To: Aaron Rosen
Cc: dev@openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in 
OVN_Northbound schema.

On Fri, Jun 26, 2015 at 08:05:53PM +, Aaron Rosen wrote:
> That sounds like it would solve this problem for us :)

I think so.

> In the delete logical_port case we'd just need to delete the port
> entry and the garbage collector will automatically handle updating the
> logical_switch.ports with the current ports right?

If this proposed patch is applied, then it's the opposite: if you remove
the port from Logical_Switch.ports, then the garbage collector will
automatically delete the Logical_Port object (because there are no
remaining references to it).


arosen - in that case I think we also need something that can ""remove element 
X from this column
(regardless of what's currently there)" otherwise we'll have the same problem 
in the delete case no? 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

Sounds good, thanks Ben!

From: Ben Pfaff 
Sent: Friday, June 26, 2015 2:50 PM
To: Aaron Rosen
Cc: dev@openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in 
OVN_Northbound schema.

On Fri, Jun 26, 2015 at 09:35:04PM +, Aaron Rosen wrote:
> > In the delete logical_port case we'd just need to delete the port
> > entry and the garbage collector will automatically handle updating the
> > logical_switch.ports with the current ports right?
>
> If this proposed patch is applied, then it's the opposite: if you remove
> the port from Logical_Switch.ports, then the garbage collector will
> automatically delete the Logical_Port object (because there are no
> remaining references to it).
>
>
> arosen - in that case I think we also need something that can ""remove 
> element X from this column
> (regardless of what's currently there)" otherwise we'll have the same problem 
> in the delete case no?

Well, yes, I would provide that feature too, it uses the same mechanism.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Allowed Address Pairs - OVN

[Aaron Rosen]

Hi,

The allowed address pair extension was added to the neutron api to allow
protocols like VRRP to work. All it dictates are
mac_address/ip_address(cidrs) that are allowed to pass through a neutron
port.

rest inline

On Wed, Jul 1, 2015 at 10:47 AM, Ben Pfaff  wrote:

> On Wed, Jul 01, 2015 at 12:25:58PM -0500, Kyle Mestery wrote:
> > On Wed, Jul 1, 2015 at 12:11 PM, Ben Pfaff  wrote:
> > > Where's the spec for allowed address pairs?  It's probably pretty easy
> > > to implement in OVN.
> > >
> > The API developer documentation is here [1]. The BP with a link to a
> google
> > doc (this was implemented in 2013) is here [2].
> >
> > [1]
> >
> http://specs.openstack.org/openstack/neutron-specs/specs/api/allowed_address_pairs.html
> > [2] https://blueprints.launchpad.net/neutron/+spec/allowed-address-pairs
>
> It's not as explicit about the meaning as I would like.  Is the
> following correct?
>
> A packet is allowed if one of the following is true:
>
> 1. Its MAC address is 'mac_address' and, if it is an IP packet, its
>IP address is one of those in 'fixed_ips'.
>
> 2. Its MAC address is in 'allowed_address_pairs' and, if it is an IP
>packet, its IP address is in the same 'allowed_address_pairs'
>pair.
>
>
Correct. Also fwiw the difference between fixed_ips and
allowed_address_pairs is that the dhcp-server on the network will hand out
the addresses for a port that matches it's fixed_ips +mac_address (the
allowed-address-pairs are just additional addresses that are allowed to
pass through).


> How is IPv6 handled?  I suppose that 'fixed_ips' and the 'ip_address'
> part of an 'allowed_address_pairs' pair can be an IPv6?
>

Correct it could be an ipv6 address as well.

>
> What happens to an IPv6 packet if 'mac_address' matches but 'fixed_ips'
> only lists IPv4 addresses?  Conversely, what happens to an IPv4 packet
> if 'mac_address' matches but 'fixed_ips' only lists IPv6 addresses?
>

The packet is dropped in both cases if only the mac_address matches but the
ip doesn't match.


>
> Are ARP packets supposed to have their inner IPv4 and MAC addresses
> filtered by these rules?  How about IPv6 ND packets?
>

This is a good question. Today we are only enforcing that the l2/l3 fields
match. That said, I think it probably makes sense for us to filter on this
too.

>
> (All of the possibilities above, in either direction, are implementable
> in OVN, but I didn't know what precedent had been set in Neutron or
> whether that precedent was set for good reason or for convenience, so
> I've only implemented L2 port security so far.)
>
> Thanks a lot; this is a discussion I've been meaning to have for a
> while.
> ___
> 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] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

[Aaron Rosen]

Fwiw the neutron side is ready to merge as well:

https://review.openstack.org/#/c/196132/

we also were able to test your patch in the gate
https://review.openstack.org/#/c/196131/4 with this patchset on top and it
fixes the race we were seeing before.

Aaron


On Mon, Jun 29, 2015 at 5:21 PM, Alex Wang  wrote:

> Looks good to me,
>
> Acked-by: Alex Wang 
>
> On Thu, Jun 25, 2015 at 4:35 PM, Ben Pfaff  wrote:
>
> > Until now, the OVN_Northbound schema has been designed to sidestep a
> > weakness in the OVSDB protocol when a column has a great deal of data in
> > it.  In the current OVSDB protocol, whenever a column changes, the entire
> > new value of the column is sent to all of the clients that are monitoring
> > that column.  That means that adding or removing a small amount of data,
> > say 1 element in a set, requires sending all of the data, which is
> > expensive if the column has a lot of data.
> >
> > One example of a column with potential to have a lot of data is the set
> of
> > ports within a logical switch, if a logical switch has a large number of
> > ports.  Thus, the existing OVN_Northbound schema has each Logical_Port
> > point to its containing Logical_Switch instead of the other way around.
> > This sidesteps the problem because it does not use any large columns.
> >
> > The tradeoff that this forces, however, is that the schema cannot take
> > advantage of OVSDB's garbage collection feature, where it automatically
> > deletes rows that are unreferenced.  That's a problem for Neutron because
> > of Neutron-internal races between deletion of a Logical_Switch and
> > creation of new Logical_Ports on the switch being deleted.  When such a
> > race happens, OVSDB refuses to delete the Logical_Switch because of
> > references to it from the newly created Logical_Port (although Neutron
> > does delete the pre-existing logical ports).
> >
> > To solve the problem, this commit changes the OVN_Northbound schema to
> > use a set of ports within Logical_Switch.  That will lead to large
> columns
> > for large logical switches; I plan to address that (though I don't have
> > code written) by enhancing the OVSDB protocol.  With this commit applied,
> > the database will automatically cascade deleting a logical switch row to
> > delete all of its ports, ACLs, and its router port (if any).
> >
> > This commit makes some pretty pervasive changes to ovn-northd, but they
> > are mostly beneficial to the code readability because now it becomes
> > possible to trivially iterate through the ports that belong to a switch,
> > which was difficult before the schema change.
> >
> > This commit will break the Neutron integration until that is changed to
> > handle the new database schema.
> >
> > CC: Aaron Rosen 
> > Signed-off-by: Ben Pfaff 
> > ---
> >  ovn/northd/ovn-northd.c | 318
> > ++--
> >  ovn/ovn-nb.ovsschema|  39 +++---
> >  ovn/ovn-nb.xml  |  74 ++-
> >  ovn/ovn-nbctl.c | 116 ++
> >  4 files changed, 282 insertions(+), 265 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index f37df77..c790a90 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -281,139 +281,116 @@ build_pipeline(struct northd_context *ctx)
> >  }
> >
> >  /* Table 0: Ingress port security. */
> > -const struct nbrec_logical_port *lport;
> > -NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > -struct ds match = DS_EMPTY_INITIALIZER;
> > -ds_put_cstr(&match, "inport == ");
> > -json_string_escape(lport->name, &match);
> > -build_port_security("eth.src",
> > -lport->port_security,
> lport->n_port_security,
> > -&match);
> > -pipeline_add(&pc, lport->lswitch, 0, 50, ds_cstr(&match),
> > - lport_is_enabled(lport) ? "next;" : "drop;");
> > -ds_destroy(&match);
> > -}
> > -
> > -/* Table 1: Destination lookup, broadcast and multicast handling
> > (priority
> > - * 100). */
> >  NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > -struct ds actions;
> > -
> > -ds_init(&actions);
> > -NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > -if (lpor

Re: [ovs-dev] proposed OVN port security specification (was: Re: Allowed Address Pairs - OVN)

[Aaron Rosen]

Hi Ben,

This looks great to me. One thing I was wondering is if the ACL interface
that ovn will expose would also allow us to implement this? That said, I
think having this port_security column directly on the port will definitely
simplifies the integration work needed in neutron so I'm definitely a fan.

Best,

Aaron



On Thu, Jul 2, 2015 at 12:42 PM, Ben Pfaff  wrote:

> Here's a proposal for an OVN port security specification.  I tried to
> specify it as carefully and completely as possible.  This is not
> implemented yet, only specified.  Comments are welcome!
>
>port_security: set of strings
>   This  column controls the addresses from which the host
> attached
>   to the logical port (``the host’’) is allowed  to  send
> packets
>   and  to  which it is allowed to receive packets.  If this
> column
>   is empty, all addresses are permitted.
>
>   Each element in the  set  must  contain  one  or  more
> Ethernet
>   addresses,  optionally  masked.   An  element that contains
> only
>   Ethernet addresses restricts the host to  sending  packets
> from
>   and receiving packets to those addresses.  It also restricts
> the
>   inner source MAC addresses that the host may  send  in  ARP
> and
>   IPv6 Neighbor Discovery packets.  It does not restrict the
> logi‐
>   cal port to any particular L3 addresses.   The  host  is
> always
>   allowed  to  receive packets to multicast and broadcast
> Ethernet
>   addresses.
>
>   Each element in the set may additionally  contain  one  or
> more
>   IPv4  or  IPv6  addresses  (or both), with optional masks.
> If a
>   mask is given, it must be a  CIDR  mask.   In  addition  to
> the
>   restrictions  described  for  Ethernet  addresses above,
> such an
>   element restricts the IPv4 or IPv6 addresses from the  host
> may
>   send  and  to  which  it may receive to packets to the
> specified
>   addresses.  A masked address, if the host part  is  zero,
> indi‐
>   cates  that the host is allowed to use any addresses in the
> sub‐
>   net; if the host part is nonzero, the mask simply indicates
> the
>   size of the subnet.  In addition:
>
>   *  If  no  IPv4 address is given, the host is not
> allowed to
>  send or receive any IPv4 or ARP traffic.
>
>  If any IPv4 address is given, the host is also
> allowed to
>  receive  packets  to  the  IPv4  local  broadcast
> address
>  255.255.255.255   and   to   IPv4   multicast
>  addresses
>  (224.0.0.0/4).   If an IPv4 address with a mask is
> given,
>  the host is also allowed to receive packets to the
> broad‐
>  cast address in that specified subnet.
>
>  If  any  IPv4  address is given, the host is
> additionally
>  restricted to sending  ARP  packets  with  the
> specified
>  source address.  (RARP is not restricted.)
>
>   *  If  no  IPv6 address is given, the host is not
> allowed to
>  send or receive any IPv6 (including IPv6 Neighbor
> Discov‐
>  ery) traffic.
>
>  If any IPv6 address is given, the host is also
> allowed to
>  receive packets to IPv6 multicast addresses
> (ff00::/8).
>
>  If any IPv6 address is given, the  host  is
> additionally
>  restricted  to  sending IPv6 Neighbor Discovery
> Solicita‐
>  tion or Advertisement packets with the  specified
> source
>  address or, for solicitations, the unspecified
> address.
>
>   Multiple  elements act as a disjunction.  That is, when
> multiple
>   elements exist, any packet that would be permitted by any
> indi‐
>   vidual  element,  as described by the policy above, is
> permitted
>   by the overall policy.
>
>   This column uses the same lexical syntax as the match
> column  in
>   the   OVN   Southbound   database’s  Pipeline  table.
>  Multiple
>   addresses within an element may be space or comma separated.
>
>   Examples:
>
>   80:fa:5b:06:72:b7
>  The host may send traffic from and receive traffic to
> the
>  specified MAC address, and to receive traffic to
> Ethernet
>  multicast and broadcast  addresses,  but  not
> otherwise.
>  The  host  may  not  send  ARP or IPv6 Neighbor
> Discovery
>  packets with inner source Ethernet addresses  other
> than
>  the on

Re: [ovs-dev] OVN - L3 Gap between NB schema and Neutron

[Aaron Rosen]

Hi Gal,

So you're saying that ml2 allows you to configure a topology like this?


VM (10.0.0.2) Logical_Switch(10.0.0.2)LogicalRouter
  |
  |
  +--(10.0.0.3)--Logical-Router--WAN


And then the vm would be responsible for having specific routes to each gw
ip?

I think you're right that this will work with the current L3 agent. That
said, I wondering if it's even worth supporting this topology if it's
complex to implement and there are not many use cases for it (or being
requested by users). I haven't heard anyone asking for this before (and nvp
doesn't implement this either fwiw). As an alternative to accomplishing the
same thing one could use a VM with two ports.

Aaron






On Thu, Jul 30, 2015 at 11:28 AM, Ben Pfaff  wrote:

> [also adding Salvatore]
>
> On Thu, Jul 30, 2015 at 11:27:57AM -0700, Ben Pfaff wrote:
> > If both the router ports point to the same router, then I am not sure
> > why this would need to be two ports.  Maybe the schema is not sufficient
> > to report both IPv4 and IPv6 addresses on a single router port; if so,
> > then I would support enhancing the schema to fix that.
> >
> > I suspect that for connecting to two different routers, it is possible
> > to instead connect one router and then connect that router to others in
> > a way that accomplishes an equivalent goal.  I haven't thought it
> > through though.
> >
> > On Thu, Jul 30, 2015 at 09:12:14PM +0300, Gal Sagie wrote:
> > > Yes, i checked this on my setup.
> > > For example, you can have both IPv6 and IPv4 subnets per the same
> network
> > > (which maps to a logical switch)
> > > and connect both as two different router ports (to the same router)
> > >
> > > You can also connect the same network to two different routers, i am
> not
> > > sure if you need the extra route extension for that or not, i think you
> > > could
> > > configure it as default gateway with out this extension, but with the
> > > extension you
> > > can define routing between the two routers.
> > >
> > >
> > >
> > >
> > >
> > > On Thu, Jul 30, 2015 at 9:03 PM, Ben Pfaff  wrote:
> > >
> > > > [adding Aaron Rosen]
> > > >
> > > > On Wed, Jul 29, 2015 at 12:20:30PM +0300, Gal Sagie wrote:
> > > > > Currently Neutron support defining few subnets (IP cidrs) on a
> network
> > > > > (logical switch)
> > > > > and connecting them to the same router (or different routers).
> > > > > Currently in the NB schema, the logical switch can be connected
> only to
> > > > one
> > > > > logical
> > > > > router port.
> > > > >
> > > > > This needs to be extended so a logical switch can have more then
> one
> > > > > logical router
> > > > > port reference to support the above use case.
> > > >
> > > > Limiting a logical switch to a single router port is an intentional
> > > > design decision.  It means that a packet traverses at most two
> logical
> > > > switches (one at ingress, one at egress), which simplifies some of
> the
> > > > logical switch design, and it prevents loops.
> > > >
> > > > VMware's NVP controller uses the same design, for those reasons and
> > > > others.  The NVP paper from NSDI 2014 (see
> > > > http://benpfaff.org/papers/net-virt.pdf) puts it this way:
> > > >
> > > > As an optimization, we constrain the logical topology such that
> > > > logical L2 destinations can only be present at its edge[6].  This
> > > > restriction means that the OVS flow table of a sending hypervisor
> > > > needs only to have flows for logical datapaths to which its local
> > > > VMs are attached as well as those of the L3 routers of the
> logical
> > > > topology; the receiving hypervisor is determined by the logical
> IP
> > > > destination address, leaving the last logical L2 hop to be
> executed
> > > > at the receiving hypervisor.
> > > >
> > > > [6] We have found little value in supporting logical routers
> > > > interconnected through logical switches without tenant VMs.
> > > >
> > > > Are you sure that Neutron supports multiple router ports per switch?
> > > > Russell Bryant (in IRC) and Aaron Rosen (in a quick in-person chat)
> > > > seemed doubtful.
> > > >
> > >
> > >
> > >
> > > --
> > > Best Regards ,
> > >
> > > The G.
> ___
> 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 RFC] ovn: distributed logical port for VM metadata access

[Aaron Rosen]

I like this idea as well.

The one question I have with is is how we should determine which ip address
to select for the 'distributed' port? In your example above you picked
10.0.0.100. I'm wondering if there is a more flexible way to handle this.
It seems to me like this ip needs to be selected a head of time to be able
to correctly distribute the host route before the instance has configured
it's self with dhcp.   I'm thinking that just picking the first free
ip_address in the subnet and using that might be fine as long as there is a
way to be able to dynamically update this after the fact if the user wants
to use that same address for something else. I'm thinking in the case of
someone deploying a heat template with predefined addresses for everything
how we can avoid this conflict (maybe this doesn't matter much though and
people are okay changing there static assignments or don't do that).

An alternative solution (which has the same problem about picking the ip
address to use) If we want to avoid creating the network name spaces and
running the http proxy on each hypervisor is if we took a similar approach
that openstack uses for handling dhcp/metadata requests.

When a subnet is created we could have the neutron-ovn-plugin notify a
metadata agent which would create a port on the given subnet for the
logical network. Then, to get instances to route its metadata traffic to
this logical port we could have ovn distribute an additional host-route via
dhcp (using option 121).  Similar to what you are proposing.


I.e:  So for example if someone created a network/subnet.

In the ovn plugin we can signal the metadata agent to create a port on that
network.  Then, for every port that is created on this network we would
distribute a hostroute of 169.254.169.254/32 via ; Then,
 we'd have the metadata agent just run there which would answer these meta
data requests and route them back.

One down side to this solution is that this metadata agent would need to be
made HA in some way. In your current solution if the metadata agent crashes
or something the failure is isolated to the hypervisor. That said, this
type of HA seems like it can be implemented in at least an active passive
solution easily enough.

Thoughts?


On Fri, Apr 15, 2016 at 4:49 PM, Ramu Ramamurthy 
wrote:

> >
> > neutron-ovn-metadata would be something we maintain in our OpenStack
> plugin
> > repo (networking-ovn), right?
> >
>
> Russell, Thanks for your review and feedback,
>
> neutron-ovn-metadata would be maintained in networking-ovn repo.
>
> > I like this proposal.  It suggests adding only the minimal amount of
> support
> > needed from OVN itself to enable us to get our OpenStack-specific job
> done.
> > This is much better than anything I had thought of.
> >
> > Thank you!
> >
> > --
> > Russell Bryant
> ___
> 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 RFC] ovn: distributed logical port for VM metadata access

[Aaron Rosen]

Hi Ramu,


On Tue, Apr 19, 2016 at 8:12 AM, Ramu Ramamurthy 
wrote:

> On Mon, Apr 18, 2016 at 2:55 PM, Aaron Rosen 
> wrote:
> > I like this idea as well.
> >
> > The one question I have with is is how we should determine which ip
> address
> > to select for the 'distributed' port?
>
> Aaron, Thanks for your review, and feedback.
>
> We can use the dhcp-port (and its IP address) for the distributed-port.
>
> The current native-dhcp proposal
> (http://openvswitch.org/pipermail/dev/2016-April/069787.html)
> assumes that a dhcp-server ip-address "server_id" is defined on the subnet.
>
> action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1,
> server_id = 10.0.0.2, mtu = 1300, lease_time = 3600,
>
> For openstack, This means that a DHCP port has been created on that
> subnet in neutron.
> In the absence of a dhcp-agent, the neutron-ovn-plugin would have to
> auto-create the
> dhcp-port in neutron upon creation of a subnet, and then use that
> port's IP address as
> the "server_id" when it programs the "dhcp-options"
> column of the Logical_Switch table.
>
> The pros of the distributed-port approach is that a) HA is not needed,
> b) it runs the existing
> neutron-metadata-proxy/neutron-metadata-agent as-is, c) In the future,
> we could remove the
> neutron-metadata-agent also, by (nova-compute) configuring the
> instance-id and tenant-id as external-ids
> of the VM's ovs interface - hence not need to run any neutron-agents at
> all.
> The drawbacks include creation of namespaces and metadata-proxy
> processes on each hypervisor.
>
>
> Cool, makes sense to me.


> > If we want to avoid creating the network name spaces and
> > running the http proxy on each hypervisor is if we took a similar
> approach
> > that openstack uses for handling dhcp/metadata requests.
> > When a subnet is created we could have the neutron-ovn-plugin notify a
> > metadata agent which would create a port on the given subnet for the
> logical
> > network. Then, to get instances to route its metadata traffic to this
> > logical port we could have ovn distribute an additional host-route via
> dhcp
> > (using option 121).  Similar to what you are proposing.
> >
> >
> > I.e:  So for example if someone created a network/subnet.
> >
> > In the ovn plugin we can signal the metadata agent to create a port on
> that
> > network.  Then, for every port that is created on this network we would
> > distribute a hostroute of 169.254.169.254/32 via ; Then,
> > we'd have the metadata agent just run there which would answer these meta
> > data requests and route them back.
> >
> > One down side to this solution is that this metadata agent would need to
> be
> > made HA in some way. In your current solution if the metadata agent
> crashes
> > or something the failure is isolated to the hypervisor. That said, this
> type
> > of HA seems like it can be implemented in at least an active passive
> > solution easily enough.
> >
> > Thoughts?
> >
>
> Your proposal is an alternative solution - which involves changes only to
> the
> neutron components  (and no changes in ovn ?). Would there be only one
> modified neutron-metadata-agent in an active-passive configuration serving
> all
> the VMs ? If there are multiple agents, would you need agent-scheduling to
> assign networks to agents ?
>
> Could you share more details of the approach ?
>


Right, with this approach I don't believe we would need any additional
changes in ovn besides the ability to specify host routes via dhcp.

For this solution we'd need to modify the neutron-metadata-agent to work in
this way. Currently, it has this functionality though it's coupled together
with the dhcp-agent (which we wouldn't want). In order to support HA you
would need to run multiple agents and the HA would use a similar
agent-scheduling method that neutron currently uses to map dhcp-agents to
networks. Also, the HA for this would need to be implemented as
active-passive I believe.

Personally, I do prefer your solution as the HA solution is more elegant as
it runs on each HV. That said, if having the namespaces on the hypervisor
nodes is a deal breaker than this would be an alternative solution to that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] ovn: is it possible to add validation on acl match

[Aaron Rosen]

Hi,

I'm wondering if it would be possible to add any additional validation on
the match column in the ACL table (and potentially other places in the
future)?

For example, we had a silly bug in the ovn plugin where if someone created
a security group rule and specified the protocol number as 6 instead of
tcp,  we forgot to convert the protocol number 6 to tcp and ended up
pushing a rule that looked like this:

  to-lport  1002 (outport == "c48a1ff1-a184-491a-9ffd-3db06ebd18ee" && ip4
&& 6 && *6.dst *== 22) allow-related

ovn-controller does expose this issue in the log:

2016-05-20T03:25:18Z|00061|lflow|WARN|error parsing match "ct.new &&
(outport == "c48a1ff1-a184-491a-9ffd-3db06ebd18ee" && ip4 && 6 && 6.dst ==
22)": Syntax error at `&&' expecting relational operator.

Though it would be nice to be able to detect the issue as an error at the
caller if possible. Currently it looks like one would need to be auditing
the logs on all of their hypervisors to detect this bug so it could go
unnoticed for a while.

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

Re: [ovs-dev] ovn: is it possible to add validation on acl match

[Aaron Rosen]

Makes sense, getting the logging in OpenStack and in northd should
definitely help improve visibility for us to detect this sooner. Even
though we won't be able to completely prevent it from the openstack side I
think this is still a good safe guard.

On Fri, May 20, 2016 at 7:21 AM, Russell Bryant  wrote:

>
>
> On Thu, May 19, 2016 at 11:51 PM, Ben Pfaff  wrote:
>
>> On Thu, May 19, 2016 at 08:42:15PM -0700, Aaron Rosen wrote:
>> > I'm wondering if it would be possible to add any additional validation
>> on
>> > the match column in the ACL table (and potentially other places in the
>> > future)?
>> >
>> > For example, we had a silly bug in the ovn plugin where if someone
>> created
>> > a security group rule and specified the protocol number as 6 instead of
>> > tcp,  we forgot to convert the protocol number 6 to tcp and ended up
>> > pushing a rule that looked like this:
>> >
>> >   to-lport  1002 (outport == "c48a1ff1-a184-491a-9ffd-3db06ebd18ee" &&
>> ip4
>> > && 6 && *6.dst *== 22) allow-related
>>
>> We could validate it in ovn-northd so that it doesn't get pushed down to
>> the southbound database, either just logging it at northd or adding some
>> kind of status or error column to the ACL table so that we could push
>> the problem back up.  Is that the kind of thing you're looking for?
>
>
> Validation in ovn-northd and reporting an error state in the ACL table
> sounds good to me.
>
> We can watch events in our plugin for when ACL rows get updated and check
> to see if the error column was set.  We can at least log an error on the
> OpenStack side in that case.  It would be asynchronous from the OpenStack
> API call, so we wouldn't be able to return an error in the API, though.
>
> --
> Russell Bryant
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] ovn: is it possible to add validation on acl match

[Aaron Rosen]

This would require the client side to integrate with this parser right?
Adding this to the client seems like a good idea as it would stop them from
writing anything invalid.

I still think it would be useful to be able to query for errors as well in
the case that some client isn't using this validator.

I guess ovn-nbctl would also be changed to leverage this too?

On Friday, May 20, 2016, Ben Pfaff  wrote:

> Would it be useful to provide a parser in Python for matches and
> actions?  Then most issues could be found before anything is sent to the
> database.
>
> (At this point I'm brainstorming.)
>
> On Fri, May 20, 2016 at 09:29:28AM -0700, Aaron Rosen wrote:
> > Makes sense, getting the logging in OpenStack and in northd should
> > definitely help improve visibility for us to detect this sooner. Even
> > though we won't be able to completely prevent it from the openstack side
> I
> > think this is still a good safe guard.
> >
> > On Fri, May 20, 2016 at 7:21 AM, Russell Bryant  > wrote:
> >
> > >
> > >
> > > On Thu, May 19, 2016 at 11:51 PM, Ben Pfaff  > wrote:
> > >
> > >> On Thu, May 19, 2016 at 08:42:15PM -0700, Aaron Rosen wrote:
> > >> > I'm wondering if it would be possible to add any additional
> validation
> > >> on
> > >> > the match column in the ACL table (and potentially other places in
> the
> > >> > future)?
> > >> >
> > >> > For example, we had a silly bug in the ovn plugin where if someone
> > >> created
> > >> > a security group rule and specified the protocol number as 6
> instead of
> > >> > tcp,  we forgot to convert the protocol number 6 to tcp and ended up
> > >> > pushing a rule that looked like this:
> > >> >
> > >> >   to-lport  1002 (outport == "c48a1ff1-a184-491a-9ffd-3db06ebd18ee"
> &&
> > >> ip4
> > >> > && 6 && *6.dst *== 22) allow-related
> > >>
> > >> We could validate it in ovn-northd so that it doesn't get pushed down
> to
> > >> the southbound database, either just logging it at northd or adding
> some
> > >> kind of status or error column to the ACL table so that we could push
> > >> the problem back up.  Is that the kind of thing you're looking for?
> > >
> > >
> > > Validation in ovn-northd and reporting an error state in the ACL table
> > > sounds good to me.
> > >
> > > We can watch events in our plugin for when ACL rows get updated and
> check
> > > to see if the error column was set.  We can at least log an error on
> the
> > > OpenStack side in that case.  It would be asynchronous from the
> OpenStack
> > > API call, so we wouldn't be able to return an error in the API, though.
> > >
> > > --
> > > Russell Bryant
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] ovn: is it possible to add validation on acl match

[Aaron Rosen]

Would you mind giving an example of what this would look like?

On Friday, May 20, 2016, Ben Pfaff  wrote:

> Another way to make it harder to send bad matches would be to construct
> them in a structured way rather than as strings.
>
> On Fri, May 20, 2016 at 09:50:43AM -0700, Ben Pfaff wrote:
> > Would it be useful to provide a parser in Python for matches and
> > actions?  Then most issues could be found before anything is sent to the
> > database.
> >
> > (At this point I'm brainstorming.)
> >
> > On Fri, May 20, 2016 at 09:29:28AM -0700, Aaron Rosen wrote:
> > > Makes sense, getting the logging in OpenStack and in northd should
> > > definitely help improve visibility for us to detect this sooner. Even
> > > though we won't be able to completely prevent it from the openstack
> side I
> > > think this is still a good safe guard.
> > >
> > > On Fri, May 20, 2016 at 7:21 AM, Russell Bryant  > wrote:
> > >
> > > >
> > > >
> > > > On Thu, May 19, 2016 at 11:51 PM, Ben Pfaff  > wrote:
> > > >
> > > >> On Thu, May 19, 2016 at 08:42:15PM -0700, Aaron Rosen wrote:
> > > >> > I'm wondering if it would be possible to add any additional
> validation
> > > >> on
> > > >> > the match column in the ACL table (and potentially other places
> in the
> > > >> > future)?
> > > >> >
> > > >> > For example, we had a silly bug in the ovn plugin where if someone
> > > >> created
> > > >> > a security group rule and specified the protocol number as 6
> instead of
> > > >> > tcp,  we forgot to convert the protocol number 6 to tcp and ended
> up
> > > >> > pushing a rule that looked like this:
> > > >> >
> > > >> >   to-lport  1002 (outport ==
> "c48a1ff1-a184-491a-9ffd-3db06ebd18ee" &&
> > > >> ip4
> > > >> > && 6 && *6.dst *== 22) allow-related
> > > >>
> > > >> We could validate it in ovn-northd so that it doesn't get pushed
> down to
> > > >> the southbound database, either just logging it at northd or adding
> some
> > > >> kind of status or error column to the ACL table so that we could
> push
> > > >> the problem back up.  Is that the kind of thing you're looking for?
> > > >
> > > >
> > > > Validation in ovn-northd and reporting an error state in the ACL
> table
> > > > sounds good to me.
> > > >
> > > > We can watch events in our plugin for when ACL rows get updated and
> check
> > > > to see if the error column was set.  We can at least log an error on
> the
> > > > OpenStack side in that case.  It would be asynchronous from the
> OpenStack
> > > > API call, so we wouldn't be able to return an error in the API,
> though.
> > > >
> > > > --
> > > > Russell Bryant
> > > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] ovs complication problem on 3.13.0-83-generic

[Aaron Rosen]

Hi,

The OpenStack infra CI nodes upgraded it's kernels from 3.13.0-79-generic
to 3.13.0-83-generic and now ovs is failing to compile with the following
error:

2016-03-17 20:42:27.078

| make -C /lib/modules/3.13.0-83-generic/build
M=/opt/stack/new/ovs/datapath/linux modules2016-03-17 20:42:27.415

| make[4]: Entering directory
`/usr/src/linux-headers-3.13.0-83-generic'2016-03-17 20:42:27.440

|   CC [M]  /opt/stack/new/ovs/datapath/linux/actions.o2016-03-17
20:42:27.441 

|   CC [M]  /opt/stack/new/ovs/datapath/linux/conntrack.o2016-03-17
20:42:27.443 

|   CC [M]  /opt/stack/new/ovs/datapath/linux/datapath.o2016-03-17
20:42:27.445 

|   CC [M]  /opt/stack/new/ovs/datapath/linux/dp_notify.o2016-03-17
20:42:27.447 

|   CC [M]  /opt/stack/new/ovs/datapath/linux/flow.o2016-03-17
20:42:28.520 

|   CC [M]  /opt/stack/new/ovs/datapath/linux/flow_netlink.o2016-03-17
20:42:28.564 

| /opt/stack/new/ovs/datapath/linux/actions.c: In function
'ovs_fragment':2016-03-17 20:42:28.564

| /opt/stack/new/ovs/datapath/linux/actions.c:726:3: warning: passing
argument 1 of 'v6ops->fragment' from incompatible pointer type
[enabled by default]2016-03-17 20:42:28.564

|v6ops->fragment(skb->sk, skb, ovs_vport_output);2016-03-17
20:42:28.564 

|^2016-03-17 20:42:28.564

| /opt/stack/new/ovs/datapath/linux/actions.c:726:3: note: expected
'struct sk_buff *' but argument is of type 'struct sock *'2016-03-17
20:42:28.564 

| /opt/stack/new/ovs/datapath/linux/actions.c:726:3: warning: passing
argument 2 of 'v6ops->fragment' from incompatible pointer type
[enabled by default]2016-03-17 20:42:28.564

| /opt/stack/new/ovs/datapath/linux/actions.c:726:3: note: expected
'int (*)(struct sk_buff *)' but argument is of type 'struct sk_buff
*'2016-03-17 20:42:28.564

| /opt/stack/new/ovs/datapath/linux/actions.c:726:3: error: too many
arguments to function 'v6ops->fragment'2016-03-17 20:42:28.564

| make[5]: *** [/opt/stack/new/ovs/datapath/linux/actions.o] Error 1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()

[Aaron Rosen]

This patch changes what is being returned from Idl.run() to a tuple
(changed, changes) so one can determine what changes have occurred to
the database without having to read the entire table.

Signed-off-by: Aaron Rosen 
---
 python/ovs/db/idl.py |   16 ++--
 tests/test-ovsdb.py  |4 ++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 55fbcba..095cf4f 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -134,11 +134,13 @@ class Idl:

 def run(self):
 """Processes a batch of messages from the database server.  Returns
-True if the database as seen through the IDL changed, False if it
did
-not change.  The initial fetch of the entire contents of the remote
-database is considered to be one kind of change.  If the IDL has
been
-configured to acquire a database lock (with Idl.set_lock()), then
-successfully acquiring the lock is also considered to be a change.
+a tuple (changed, changes) where changed is True if the database as
+seen through the IDL changed, False if it did not change. The
changes
+variable contain all of the jsonrpc messages that have been
processed.
+The initial fetch of the entire contents of the remote database is
+considered to be one kind of change.  If the IDL has been
configured
+to acquire a database lock (with Idl.set_lock()), then successfully
+acquiring the lock is also considered to be a change.

 This function can return occasional false positives, that is,
report
 that the database changed even though it didn't.  This happens if
the
@@ -153,6 +155,7 @@ class Idl:
 assert not self.txn
 initial_change_seqno = self.change_seqno
 self._session.run()
+changes = []
 i = 0
 while i < 50:
 i += 1
@@ -171,6 +174,7 @@ class Idl:
 msg = self._session.recv()
 if msg is None:
 break
+changes.append(msg)
 if (msg.type == ovs.jsonrpc.Message.T_NOTIFY
 and msg.method == "update"
 and len(msg.params) == 2
@@ -218,7 +222,7 @@ class Idl:
  % (self._session.get_name(),
  ovs.jsonrpc.Message.type_to_string(msg.type)))

-return initial_change_seqno != self.change_seqno
+return (initial_change_seqno != self.change_seqno, changes)

 def wait(self, poller):
 """Arranges for poller.block() to wake up when self.run() has
something
diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index 392ed4b..afc8287 100644
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -366,7 +366,7 @@ def do_idl(schema_file, remote, *commands):
 command = command[1:]
 else:
 # Wait for update.
-while idl.change_seqno == seqno and not idl.run():
+while idl.change_seqno == seqno and not idl.run()[0]:
 rpc.run()

 poller = ovs.poller.Poller()
@@ -415,7 +415,7 @@ def do_idl(schema_file, remote, *commands):

 if rpc:
 rpc.close()
-while idl.change_seqno == seqno and not idl.run():
+while idl.change_seqno == seqno and not idl.run()[0]:
 poller = ovs.poller.Poller()
 idl.wait(poller)
 poller.block()
-- 
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()

[Aaron Rosen]

This patch changes what is being returned from Idl.run() to a tuple
(changed, changes) so one can determine what changes have occurred to
the database without having to read the entire table.

Signed-off-by: Aaron Rosen 
---
 python/ovs/db/idl.py |   16 ++--
 tests/test-ovsdb.py  |4 ++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 55fbcba..1affd54 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -134,11 +134,13 @@ class Idl:

 def run(self):
 """Processes a batch of messages from the database server.  Returns
-True if the database as seen through the IDL changed, False if it
did
-not change.  The initial fetch of the entire contents of the remote
-database is considered to be one kind of change.  If the IDL has
been
-configured to acquire a database lock (with Idl.set_lock()), then
-successfully acquiring the lock is also considered to be a change.
+a tuple (changed, changes) where changed is True if the database as
+seen through the IDL changed, False if it did not change. The
changes
+variable contain all of the jsonrpc messages that have been
processed.
+The initial fetch of the entire contents of the remote database is
+considered to be one kind of change.  If the IDL has been
configured
+to acquire a database lock (with Idl.set_lock()), then successfully
+acquiring the lock is also considered to be a change.

 This function can return occasional false positives, that is,
report
 that the database changed even though it didn't.  This happens if
the
@@ -153,6 +155,7 @@ class Idl:
 assert not self.txn
 initial_change_seqno = self.change_seqno
 self._session.run()
+changes = []
 i = 0
 while i < 50:
 i += 1
@@ -176,6 +179,7 @@ class Idl:
 and len(msg.params) == 2
 and msg.params[0] == None):
 # Database contents changed.
+changes.append(msg)
 self.__parse_update(msg.params[1])
 elif (msg.type == ovs.jsonrpc.Message.T_REPLY
   and self._monitor_request_id is not None
@@ -218,7 +222,7 @@ class Idl:
  % (self._session.get_name(),
  ovs.jsonrpc.Message.type_to_string(msg.type)))

-return initial_change_seqno != self.change_seqno
+return (initial_change_seqno != self.change_seqno, changes)

 def wait(self, poller):
 """Arranges for poller.block() to wake up when self.run() has
something
diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index 392ed4b..afc8287 100644
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -366,7 +366,7 @@ def do_idl(schema_file, remote, *commands):
 command = command[1:]
 else:
 # Wait for update.
-while idl.change_seqno == seqno and not idl.run():
+while idl.change_seqno == seqno and not idl.run()[0]:
 rpc.run()

 poller = ovs.poller.Poller()
@@ -415,7 +415,7 @@ def do_idl(schema_file, remote, *commands):

 if rpc:
 rpc.close()
-while idl.change_seqno == seqno and not idl.run():
+while idl.change_seqno == seqno and not idl.run()[0]:
 poller = ovs.poller.Poller()
 idl.wait(poller)
 poller.block()
-- 
1.7.9.5



On Tue, Aug 6, 2013 at 2:45 PM, Aaron Rosen  wrote:

> This patch changes what is being returned from Idl.run() to a tuple
> (changed, changes) so one can determine what changes have occurred to
> the database without having to read the entire table.
>
> Signed-off-by: Aaron Rosen 
> ---
>  python/ovs/db/idl.py |   16 ++--
>  tests/test-ovsdb.py  |4 ++--
>  2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 55fbcba..095cf4f 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -134,11 +134,13 @@ class Idl:
>
>  def run(self):
>  """Processes a batch of messages from the database server.
>  Returns
> -True if the database as seen through the IDL changed, False if it
> did
> -not change.  The initial fetch of the entire contents of the
> remote
> -database is considered to be one kind of change.  If the IDL has
> been
> -configured to acquire a database lock (with Idl.set_lock()), then
> -successfully acquiring the lock is also considered to be a change.
> +a tuple (changed, changes) where changed is True if the database
> as
> +seen through the IDL changed, False if it did not change. The
> changes
> +variable contain all of the jsonrpc messages that hav

Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()

[Aaron Rosen]

Right, this would break things for anyone checking the return value of
idl.run(). The only alternative I see to that is if we pass an optional arg
to run() (i.e: def run(self, return_changes=False)). Would you prefer this
instead?

Thanks,

Aaron


On Fri, Aug 9, 2013 at 1:02 PM, Ben Pfaff  wrote:

> On Tue, Aug 06, 2013 at 02:45:35PM -0700, Aaron Rosen wrote:
> > This patch changes what is being returned from Idl.run() to a tuple
> > (changed, changes) so one can determine what changes have occurred to
> > the database without having to read the entire table.
> >
> > Signed-off-by: Aaron Rosen 
>
> It seems like a reasonable idea but I suspect it doesn't fix up all
> the users.  Also the patch is wordwrapped so I can't apply it.
>
> Thanks,
>
> Ben.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()

[Aaron Rosen]

On Fri, Aug 9, 2013 at 2:50 PM, Reid Price  wrote:

> Or you could keep the original function behavior the same and expose this
> as a separate function
>
>   def foo(...):
>   
>
>  def run(...):
> return self.foo(...)[0]
>
> where foo is a better function name - update? run_details?
> run_with_changes? run_diff? _run?  No opinion there.
>
>   -Reid
>
>
> On Fri, Aug 9, 2013 at 2:30 PM, Aaron Rosen  wrote:
>
>> Right, this would break things for anyone checking the return value of
>> idl.run(). The only alternative I see to that is if we pass an optional arg
>> to run() (i.e: def run(self, return_changes=False)). Would you prefer this
>> instead?
>>
>> Thanks,
>>
>> Aaron
>>
>>
>> On Fri, Aug 9, 2013 at 1:02 PM, Ben Pfaff  wrote:
>>
>>> On Tue, Aug 06, 2013 at 02:45:35PM -0700, Aaron Rosen wrote:
>>> > This patch changes what is being returned from Idl.run() to a tuple
>>> > (changed, changes) so one can determine what changes have occurred to
>>> > the database without having to read the entire table.
>>> >
>>> > Signed-off-by: Aaron Rosen 
>>>
>>> It seems like a reasonable idea but I suspect it doesn't fix up all
>>> the users.  Also the patch is wordwrapped so I can't apply it.
>>>
>>> Thanks,
>>>
>>> Ben.
>>>
>>
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>>
>


0001-Add-run_with_changes-method-to-Idl.patch
Description: Binary data
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()

[Aaron Rosen]

Hi,

I believe I've addressed the corner case in this patch set by returning a
dict() that represents the current state of the database on connection
reset/initial sync, otherwise a list of changes. This should allow the user
to have a consistent view of the database. I've also changed the response
to be the dict() that contains the changes rather than the full json-rpc
message. Let me know what you guys thing about this change.

Thanks,

Aaron


On Mon, Aug 12, 2013 at 4:00 PM, Reid Price  wrote:

> I am also a bit concerned by issues that might arise
> from a user thinking that this is always accurate, rather
> than hints.  Aaron, I think you had said something
> regarding this when we chatted off-list, but I don't
> recall the details.
>
>   -Reid
>
>
> On Fri, Aug 9, 2013 at 2:50 PM, Reid Price  wrote:
>
>> Or you could keep the original function behavior the same and expose this
>> as a separate function
>>
>>   def foo(...):
>>   
>>
>>  def run(...):
>> return self.foo(...)[0]
>>
>> where foo is a better function name - update? run_details?
>> run_with_changes? run_diff? _run?  No opinion there.
>>
>>   -Reid
>>
>>
>> On Fri, Aug 9, 2013 at 2:30 PM, Aaron Rosen  wrote:
>>
>>> Right, this would break things for anyone checking the return value of
>>> idl.run(). The only alternative I see to that is if we pass an optional arg
>>> to run() (i.e: def run(self, return_changes=False)). Would you prefer this
>>> instead?
>>>
>>> Thanks,
>>>
>>> Aaron
>>>
>>>
>>> On Fri, Aug 9, 2013 at 1:02 PM, Ben Pfaff  wrote:
>>>
>>>> On Tue, Aug 06, 2013 at 02:45:35PM -0700, Aaron Rosen wrote:
>>>> > This patch changes what is being returned from Idl.run() to a tuple
>>>> > (changed, changes) so one can determine what changes have occurred to
>>>> > the database without having to read the entire table.
>>>> >
>>>> > Signed-off-by: Aaron Rosen 
>>>>
>>>> It seems like a reasonable idea but I suspect it doesn't fix up all
>>>> the users.  Also the patch is wordwrapped so I can't apply it.
>>>>
>>>> Thanks,
>>>>
>>>> Ben.
>>>>
>>>
>>>
>>> ___
>>> dev mailing list
>>> dev@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
>>>
>>>
>>
>


0001-Add-run_with_changes-method-to-Idl.patch
Description: Binary data
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] Fix command typo in INSTALL.SSL.md

[Aaron Rosen]

Signed-off-by: Aaron Rosen 
---
 INSTALL.SSL.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/INSTALL.SSL.md b/INSTALL.SSL.md
index 06be303..f294a27 100644
--- a/INSTALL.SSL.md
+++ b/INSTALL.SSL.md
@@ -202,7 +202,7 @@ more secure.
 
 1. Run the following command on the Open vSwitch itself:
 
-   % ovs-pki req sc switch
+   % ovs-pki req+sign sc switch
 
(This command does not require a copy of any of the PKI files
generated by "ovs-pki init", and you should not copy them to the
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] Fix command typo in INSTALL.SSL.md

[Aaron Rosen]

Whoops I see:

  req NAME Create new private key and certificate request
   named NAME-privkey.pem and NAME-req.pem, resp.


$ ovs-pki req sc switch
/usr/local/bin/ovs-pki: req must have exactly one argument; use --help for
help

Maybe the typo then is the option reg does not take a TYPE so 'switch'
should be removed?

Aaron




On Tue, Nov 3, 2015 at 12:40 PM, Ben Pfaff  wrote:

> On Tue, Nov 03, 2015 at 12:29:21PM -0800, Aaron Rosen wrote:
> > Signed-off-by: Aaron Rosen 
> > ---
> >  INSTALL.SSL.md | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/INSTALL.SSL.md b/INSTALL.SSL.md
> > index 06be303..f294a27 100644
> > --- a/INSTALL.SSL.md
> > +++ b/INSTALL.SSL.md
> > @@ -202,7 +202,7 @@ more secure.
> >
> >  1. Run the following command on the Open vSwitch itself:
> >
> > -   % ovs-pki req sc switch
> > +   % ovs-pki req+sign sc switch
> >
> > (This command does not require a copy of any of the PKI files
> > generated by "ovs-pki init", and you should not copy them to the
>
> That's not a typo, this workflow requires taking the certificate request
> to the machine that hosts the PKI for signing.  See the section before
> that one for the req+sign workflow.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Neutron Container integration.

[Aaron Rosen]

I agree, I like Thomas's idea. We actually thought about taking this approach 
as well in neutron though there wasn't a generic way to do this for all the 
plugins since all of them don't use the ovs-agent (so we implemented the 
callback). 

That said, this looks like the most ideal solution to implement for container 
support. 

Best, 

Aaron




From: Gurucharan Shetty 
Sent: Wednesday, April 22, 2015 10:01 AM
To: Thomas Graf
Cc: Russell Bryant; dev; Aaron Rosen; Armando Migliaccio
Subject: Re: [ovs-dev] Neutron Container integration.

> The connectivity test could include reaching a link local address
> on the host bridge or even a remote endpoint.
Listening to all the complications being involved over Neutron, using
a IP address in the same logical network for connectivity test seems
better.


>
> In theory this sounds more scalable and allows to bring up containers
> while the host is temporarily isolated (unless remote endpoints are
> used for the connectivity check).
> ___
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=hwWOe6tnWamV5Up5MRhjgrrG84KhriNQORyVOM1w8AM&m=wZ0uhQiJYH9kVNqn11w1Ijc31-8j3rUpjSRpnfgysig&s=11bfKC3uFRKETiPd10OWIX_1wSJTrp2129qQVGTJMps&e=
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofproto-dpif: Don't output to in_port even if in_port is OFPP_LOCAL.

[Aaron Rosen]

Yep,

Thanks

Aaron

Signed-off-by: Aaron Rosen 

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d19b6f7..8903a7f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4545,11 +4545,9 @@ xlate_output_action__(struct action_xlate_ctx *ctx,
 case OFPP_CONTROLLER:
 execute_controller_action(ctx, max_len, OFPR_ACTION);
 break;
-case OFPP_LOCAL:
-compose_output_action(ctx, OFPP_LOCAL);
-break;
 case OFPP_NONE:
 break;
+case OFPP_LOCAL:
 default:
 if (port != ctx->flow.in_port) {
 compose_output_action(ctx, port);

On Wed, Feb 8, 2012 at 2:01 PM, Ben Pfaff  wrote:
> From: Aaron Rosen 
>
> Test by Ben Pfaff.
>
> Signed-off-by: Ben Pfaff 
> ---
> Aaron, can I get your Signed-off-by on this?
>
> Thanks,
>
> Ben.
>
>  ofproto/ofproto-dpif.c |    4 +---
>  tests/ofproto-dpif.at  |   11 +++
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index d19b6f7..8903a7f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4545,11 +4545,9 @@ xlate_output_action__(struct action_xlate_ctx *ctx,
>     case OFPP_CONTROLLER:
>         execute_controller_action(ctx, max_len, OFPR_ACTION);
>         break;
> -    case OFPP_LOCAL:
> -        compose_output_action(ctx, OFPP_LOCAL);
> -        break;
>     case OFPP_NONE:
>         break;
> +    case OFPP_LOCAL:
>     default:
>         if (port != ctx->flow.in_port) {
>             compose_output_action(ctx, port);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index f5c1358..a21d179 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -151,6 +151,7 @@ OVS_VSWITCHD_START([dnl
>         add-port br0 p7 -- set Interface p7 type=dummy ])
>
>  AT_DATA([flows.txt], [dnl
> +in_port=local actions=local,flood
>  in_port=1 actions=flood
>  in_port=2 actions=all
>  in_port=3 
> actions=output:65534,output:1,output:2,output:3,output:4,output:5,output:6,output:7
> @@ -160,6 +161,16 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  AT_CHECK([ovs-ofctl mod-port br0 5 noforward])
>  AT_CHECK([ovs-ofctl mod-port br0 6 noflood])
>
> +AT_CHECK([ovs-appctl ofproto/trace br0 
> 'in_port(0),eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),eth_type(0x0900)'],
>  [0], [stdout])
> +AT_CHECK([tail -1 stdout \
> +| sed -e 's/Datapath actions: //' | tr ',' '\n' | sort], [0], [dnl
> +1
> +2
> +3
> +4
> +7
> +])
> +
>  AT_CHECK([ovs-appctl ofproto/trace br0 
> 'in_port(1),eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02),eth_type(0x0900)'],
>  [0], [stdout])
>  AT_CHECK([tail -1 stdout \
>  | sed -e 's/Datapath actions: //' | tr ',' '\n' | sort], [0], [dnl
> --
> 1.7.2.5
>



-- 
Aaron O. Rosen
Masters Student - Network Communication
306B Fluor Daniel
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev