from:"Flavio Fernandes"

Re: [ovs-dev] [PATCH v2] python: idl: Avoid pre-allocating column defaults

2021-11-23 Thread Flavio Fernandes

LGTM!

Acked-by: Flavio Fernandes 




> -- Forwarded message -
> From: Terry Wilson mailto:twil...@redhat.com>>
> Date: Fri, Nov 5, 2021 at 10:14 AM
> Subject: [ovs-dev] [PATCH v2] python: idl: Avoid pre-allocating column 
> defaults
> To: mailto:d...@openvswitch.org>>
> 
> 
> Many python implementations pre-allocate space for multiple
> objects in empty dicts and lists. Using a custom dict-like object
> that only generates these objects when they are accessed can save
> memory.
> 
> On a fairly pathological case where the DB has 1000 networks each
> with 100 ports, with only 'name' fields set, this saves around
> 300MB of memory.
> 
> One could argue that if values are not going to change from their
> defaults, then users should not be monitoring those columns, but
> it's also probably good to not waste memory even if user code is
> sub-optimal.
> 
> Signed-off-by: Terry Wilson mailto:twil...@redhat.com>>
> ---
>  python/ovs/db/idl.py | 39 +--
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index ecae5e143..402aadabc 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -45,6 +45,36 @@ Notice = collections.namedtuple('Notice', ('event', 'row', 
> 'updates'))
>  Notice.__new__.__defaults__ = (None,)  # default updates=None
> 
> 
> +class ColumnDefaultDict(dict):
> +"""A column dictionary with on-demand generated default values
> +
> +This object acts like the Row._data column dictionary, but without the
> +necessity of populating column default values. These values are generated
> +on-demand and therefore only use memory once they are accessed.
> +"""
> +__slots__ = ('_table', )
> +
> +def __init__(self, table):
> +self._table = table
> +super().__init__()
> +
> +def __missing__(self, column):
> +column = self._table.columns[column]
> +return ovs.db.data.Datum.default(column.type)
> +
> +def keys(self):
> +return self._table.columns.keys()
> +
> +def values(self):
> +return iter(self[k] for k in self)
> +
> +def __iter__(self):
> +return iter(self.keys())
> +
> +def __contains__(self, item):
> +return item in self.keys()
> +
> +
>  class Idl(object):
>  """Open vSwitch Database Interface Definition Language (OVSDB IDL).
> 
> @@ -908,10 +938,7 @@ class Idl(object):
>  return changed
> 
>  def __create_row(self, table, uuid):
> -data = {}
> -for column in table.columns.values():
> -data[column.name <http://column.name/>] = 
> ovs.db.data.Datum.default(column.type)
> -return Row(self, table, uuid, data)
> +return Row(self, table, uuid, ColumnDefaultDict(table))
> 
>  def __error(self):
>  self._session.force_reconnect()
> @@ -1249,7 +1276,7 @@ class Row(object):
>  A transaction must be in progress."""
>  assert self._idl.txn
>  assert self._changes is not None
> -if not self._data or column_name in self._changes:
> +if self._data is None or column_name in self._changes:
>  return
> 
>  self._prereqs[column_name] = None
> @@ -1777,7 +1804,7 @@ class Transaction(object):
>  # transaction only does writes of existing values, without making any
>  # real changes, we will drop the whole transaction later in
>  # ovsdb_idl_txn_commit().)
> -if (not column.alert and row._data and
> +if (not column.alert and row._data is not None and
>  row._data.get(column.name <http://column.name/>) == datum):
>  new_value = row._changes.get(column.name <http://column.name/>)
>  if new_value is None or new_value == datum:
> -- 
> 2.31.1
> 
> ___
> dev mailing list
> d...@openvswitch.org <mailto:d...@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev 
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 2/2] ovn-controller: Access OVS Datapath table only if supported.

2021-09-01 Thread Flavio Fernandes




> On Sep 1, 2021, at 3:13 PM, Dumitru Ceara  wrote:
> 
> Use the newly added IDL API, ovsdb_idl_server_has_table(), through the
> auto generated ovsrec_server_has_datapath_table() to determine if it's
> OK to read and potentially create a Datapath record in the local OVS DB.
> 
> In order to do that we also need to bump the OVS submodule to include
> 7502849e9581 ("ovsdb-idl: Add APIs to query if a table and a column is
> present.").
> 
> Fixes: 56e2cd3a2f06 ("ovn-controller: Detect OVS datapath capabilities.")
> Reported-at: https://bugzilla.redhat.com/1992705
> Signed-off-by: Dumitru Ceara 
> ---
> controller/ovn-controller.c |   32 +++-
> lib/features.c  |5 +
> ovs |2 +-
> 3 files changed, 25 insertions(+), 14 deletions(-)


Tested-by: Flavio Fernandes 


> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 739048cf8..0031a1035 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -439,12 +439,11 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>const struct ovsrec_bridge_table *bridge_table,
>const struct ovsrec_open_vswitch_table *ovs_table,
>const struct ovsrec_bridge **br_int_,
> -   const struct ovsrec_datapath **br_int_dp_)
> +   const struct ovsrec_datapath **br_int_dp)
> {
> const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
> -const struct ovsrec_datapath *br_int_dp = NULL;
> 
> -ovs_assert(br_int_ && br_int_dp_);
> +ovs_assert(br_int_);
> if (ovs_idl_txn) {
> if (!br_int) {
> br_int = create_br_int(ovs_idl_txn, ovs_table);
> @@ -476,15 +475,16 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> ovsrec_bridge_set_fail_mode(br_int, "secure");
> VLOG_WARN("Integration bridge fail-mode changed to 
> 'secure'.");
> }
> -br_int_dp = get_br_datapath(cfg, datapath_type);
> -if (!br_int_dp) {
> -br_int_dp = create_br_datapath(ovs_idl_txn, cfg,
> -   datapath_type);
> +if (br_int_dp) {
> +*br_int_dp = get_br_datapath(cfg, datapath_type);
> +if (!(*br_int_dp)) {
> +*br_int_dp = create_br_datapath(ovs_idl_txn, cfg,
> +datapath_type);
> +}
> }
> }
> }
> *br_int_ = br_int;
> -*br_int_dp_ = br_int_dp;
> }
> 
> static const char *
> @@ -3519,8 +3519,10 @@ main(int argc, char *argv[])
> ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> const struct ovsrec_bridge *br_int = NULL;
> const struct ovsrec_datapath *br_int_dp = NULL;
> -process_br_int(ovs_idl_txn, bridge_table, ovs_table,
> -   _int, _int_dp);
> +process_br_int(ovs_idl_txn, bridge_table, ovs_table, _int,
> +   ovsrec_server_has_datapath_table(ovs_idl_loop.idl)
> +   ? _int_dp
> +   : NULL);
> 
> /* Enable ACL matching for double tagged traffic. */
> if (ovs_idl_txn) {
> @@ -3563,9 +3565,13 @@ main(int argc, char *argv[])
>   _private);
> }
> 
> -/* If any OVS feature support changed, force a full recompute. */
> -if (br_int_dp
> -&& ovs_feature_support_update(_int_dp->capabilities)) 
> {
> +/* If any OVS feature support changed, force a full recompute.
> + * 'br_int_dp' is valid only if an OVS transaction is possible.
> + */
> +if (ovs_idl_txn
> +&& ovs_feature_support_update(br_int_dp
> +  ? _int_dp->capabilities
> +  : NULL)) {
> VLOG_INFO("OVS feature set changed, force recompute.");
> engine_set_force_recompute(true);
> }
> diff --git a/lib/features.c b/lib/features.c
> index 87d04ee3f..fddf4c450 100644
> --- a/lib/features.c
> +++ b/lib/features.c
> @@ -62,8 +62,13 @@ ovs_feature_is_supported(enum ovs_feature_value feature)
> bool
> ovs_feature_support_update(const struct smap *ovs_capabilities)
> {
> +static struct smap empty_caps = SMAP_INITIALIZER(_caps);
> bool updated = false;
> 
> +if (!ovs_capabilities) {
> +ovs_capabiliti

Re: [ovs-dev] [PATCH ovn 1/2] Revert "Revert features detection and zero-snat patches."

2021-09-01 Thread Flavio Fernandes




> On Sep 1, 2021, at 3:12 PM, Dumitru Ceara  wrote:
> 
> This reverts commit 82e241542cbf19a144257a72324bc3278f76ee99.
> 
> Turns out the dataplane performance issue was not caused by the
> zero-snat feature:
> https://bugzilla.redhat.com/show_bug.cgi?id=1992012#c24
> 
> The upgrade problem will be addressed as a separate patch.
> 
> Signed-off-by: Dumitru Ceara 


 Acked-by: Flavio Fernandes 



> ---
> controller/ovn-controller.c   |  115 +++--
> include/ovn/actions.h |1
> include/ovn/features.h|   18 
> lib/actions.c |   31 +++
> lib/automake.mk   |1
> lib/features.c|   84 ++
> lib/test-ovn-features.c   |   56 
> tests/automake.mk |3 +
> tests/ovn-controller.at   |   11 +-
> tests/ovn-features.at |8 ++
> tests/system-common-macros.at |4 +
> tests/system-ovn.at   |  190 +
> tests/testsuite.at|1
> 13 files changed, 490 insertions(+), 33 deletions(-)
> create mode 100644 lib/features.c
> create mode 100644 lib/test-ovn-features.c
> create mode 100644 tests/ovn-features.at


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] Revert features detection and zero-snat patches.

2021-08-27 Thread Flavio Fernandes




> On Aug 27, 2021, at 4:11 PM, Mark Michelson  wrote:
> 
> This commit reverts the following two commits:
> 56e2cd3a (ovn-controller: Detect OVS datapath capabilities.)
> 58683a42 (ovn-controller: Handle DNAT/no-NAT conntrack tuple
> collisions.)
> 
> The former commit causes an incompatibility with OVS databases that do
> not have the datapath table present. This specifically is making
> OpenStack upgrades from OSP 13 to OSP 16.2 fail.
> 
> The latter commit causes significantly degraded dataplane performance
> when testing a sense OpenShift cluster. This was pinpointed to be due to
> an extra ct(nat(src)) that this commit added.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992705
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992012
> 
> Signed-off-by: Mark Michelson 
> 
> ---
> v1 -> v2:
>  * Switched from reverting just one commit to reverting two, thereby
>eliminating two issues.
> ---
> 
> Signed-off-by: Mark Michelson 
> ---


Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> controller/ovn-controller.c   | 115 +---
> include/ovn/actions.h |   1 -
> include/ovn/features.h|  18 
> lib/actions.c |  31 --
> lib/automake.mk   |   1 -
> lib/features.c|  84 ---
> lib/test-ovn-features.c   |  56 --
> tests/automake.mk |   3 -
> tests/ovn-controller.at   |  11 +-
> tests/ovn-features.at |   8 --
> tests/ovn.at  |   2 +-
> tests/system-common-macros.at |   4 -
> tests/system-ovn.at   | 190 --
> tests/testsuite.at|   1 -
> 14 files changed, 34 insertions(+), 491 deletions(-)
> delete mode 100644 lib/features.c
> delete mode 100644 lib/test-ovn-features.c
> delete mode 100644 tests/ovn-features.at

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] Revert "ovn-controller: Handle DNAT/no-NAT conntrack tuple collisions."

2021-08-27 Thread Flavio Fernandes



This revert does not include a 1 liner change needed done later in
https://github.com/ovn-org/ovn/commit/4e6c498068dc4fa9546d3661f78f0a42e99c74bb 
<https://github.com/ovn-org/ovn/commit/4e6c498068dc4fa9546d3661f78f0a42e99c74bb>
That is not an issue, since regex change there is backwards compatible, agree?


Acked-by: Flavio Fernandes 


> On Aug 27, 2021, at 11:29 AM, Mark Michelson  wrote:
> 
> This commit resulted in significant decreased dataplane performance when
> testing a dense OpenShift cluster. This was pinpointed to be due to an
> extra ct(nat(src)) that this commit added.
> 
> For now, revert this commit.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992012
> Signed-off-by: Mark Michelson 
> ---
> include/ovn/actions.h |   1 -
> lib/actions.c |  31 --
> tests/ovn.at  |   2 +-
> tests/system-common-macros.at |   4 -
> tests/system-ovn.at   | 190 --
> 5 files changed, 1 insertion(+), 227 deletions(-)
> 
...


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2 1/1] ovn-controller: Ensure br-int is using secure fail-mode

2021-05-07 Thread Flavio Fernandes

By default, OVS bridges use standalone fail-mode, which means it is
configured with a single row with the NORMAL action as its OpenFlow table.
Upon system reboot, an integration bridge with many ports and such a table
could create broadcast storms and duplicate packets. That is why
ovn-controller creates the integration bridge with secure fail-mode.
Under that mode, the OpenFlow table remains empty until the controller
populates it, which could happen many seconds after the bridge is
operational. Unfortunately, the fail-mode setting was not being
done if the bridge was already created by the time ovn-controller
starts. This change fixes that and logs a warning should the fail-mode
ever needed to be corrected.

Reported-at: https://bugzilla.redhat.com/1957025
Signed-off-by: Flavio Fernandes 
---
 v1->v2: Changes from code review. Thanks, Frode!

 controller/ovn-controller.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 6106a9661..d925522e1 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -413,6 +413,10 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
 if (datapath_type && strcmp(br_int->datapath_type, datapath_type)) {
 ovsrec_bridge_set_datapath_type(br_int, datapath_type);
 }
+if (!br_int->fail_mode || strcmp(br_int->fail_mode, "secure")) {
+ovsrec_bridge_set_fail_mode(br_int, "secure");
+VLOG_WARN("Integration bridge fail-mode changed to 'secure'.");
+}
 }
 return br_int;
 }
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v1 1/1] ovn-controller: Ensure br-int is using secure fail-mode

2021-05-07 Thread Flavio Fernandes




> On May 7, 2021, at 3:02 AM, Frode Nordahl  wrote:
> 
> 
> 
> fre. 7. mai 2021, 03:22 skrev Flavio Fernandes  <mailto:fla...@flaviof.com>>:
> By default, OVS bridges use standalone fail-mode, which means it is
> configured with a single row with the NORMAL action as its OpenFlow table.
> Upon system reboot, an integration bridge with many ports and such a table
> could create broadcast storms and duplicate packets. That is why
> ovn-controller creates the integration bridge with secure fail-mode.
> Under that mode, the OpenFlow table remains empty until the controller
> populates it, which could happen many seconds after the bridge is
> operational. Unfortunately, the fail-mode setting was not being
> done if the bridge was already created by the time ovn-controller
> starts. This change fixes that and logs a warning should the fail-mode
> ever needed to be corrected.
> 
> It would indeed be good to liberate the deployment tooling from the 
> responsibility of maintaining the fail-mode, so I welcome this change.

Cool beans!

> 
> Reported-at: https://bugzilla.redhat.com/1957025 
> <https://bugzilla.redhat.com/1957025>
> Signed-off-by: Flavio Fernandes  <mailto:fla...@flaviof.com>>
> ---
>  controller/ovn-controller.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 6106a9661..e4cbf3583 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>  ovs_table);
>  if (!br_int) {
>  br_int = create_br_int(ovs_idl_txn, ovs_table);
> +} else if (ovs_idl_txn) {
> 
> Would it make sense to put this code into the branch below instead? It 
> appears to me it already executes on the condition you want.

Yeah, we could do that. Right now, the creation of the bridge in 
"create_br_int" is already setting the fail-mode and
by moving this below would make that potentially redundant. But the cost of an 
additional strcmp() for sake of simplicity
may be a valid trade off? So the new code would look more like:

if (br_int && ovs_idl_txn) {
  const struct ovsrec_open_vswitch *cfg;
...
if (datapath_type && strcmp(br_int->datapath_type, datapath_type)) {
ovsrec_bridge_set_datapath_type(br_int, datapath_type);
}

+const char *fail_mode = br_int->fail_mode;
+if (!fail_mode || strcmp(fail_mode, "secure")) {
+ovsrec_bridge_set_fail_mode(br_int, "secure");
+VLOG_WARN("Integration bridge fail-mode needed to be set as 
secure.");}
+   }
 }
 return br_int;

Sounds reasonable?

> 
> +const char *fail_mode = br_int->fail_mode;
> +if (!fail_mode || strcmp(fail_mode, "secure")) {
> +ovsrec_bridge_set_fail_mode(br_int, "secure");
> +VLOG_WARN("Integration bridge fail-mode set to secure.");
> 
> While I agree this action should be logged, I wonder if the text could be 
> rephrased. As it stands it could read as having br-int in secure fail-mode is 
> a undesirable situation, whereas the opposite is true.

heh English troubles! ;) I can see what you mean now.

Do you think this is more appropriate:

 Integration bridge fail-mode needed to be set as secure.

? Chime in with any tweaks, please.

-- flaviof

> 
> +}
>  }
>  if (br_int && ovs_idl_txn) {
>  const struct ovsrec_open_vswitch *cfg;
> --
> 2.25.1
> 
> --
> Frode Nordahl
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org <mailto:d...@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev 
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v1 1/1] ovn-controller: Ensure br-int is using secure fail-mode

2021-05-06 Thread Flavio Fernandes

By default, OVS bridges use standalone fail-mode, which means it is
configured with a single row with the NORMAL action as its OpenFlow table.
Upon system reboot, an integration bridge with many ports and such a table
could create broadcast storms and duplicate packets. That is why
ovn-controller creates the integration bridge with secure fail-mode.
Under that mode, the OpenFlow table remains empty until the controller
populates it, which could happen many seconds after the bridge is
operational. Unfortunately, the fail-mode setting was not being
done if the bridge was already created by the time ovn-controller
starts. This change fixes that and logs a warning should the fail-mode
ever needed to be corrected.

Reported-at: https://bugzilla.redhat.com/1957025
Signed-off-by: Flavio Fernandes 
---
 controller/ovn-controller.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 6106a9661..e4cbf3583 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
 ovs_table);
 if (!br_int) {
 br_int = create_br_int(ovs_idl_txn, ovs_table);
+} else if (ovs_idl_txn) {
+const char *fail_mode = br_int->fail_mode;
+if (!fail_mode || strcmp(fail_mode, "secure")) {
+ovsrec_bridge_set_fail_mode(br_int, "secure");
+VLOG_WARN("Integration bridge fail-mode set to secure.");
+}
 }
 if (br_int && ovs_idl_txn) {
 const struct ovsrec_open_vswitch *cfg;
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH V3 0/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-17 Thread Flavio Fernandes

Dear OVS community:

I hit a segfault exception while attempting to remove all meters from a bridge.
Upon looking a little closer, I can see that the fact that an invalid n_bands 
value
in the meter structure caused the issue. More details below.

I humbly propose the changes in this patchset as a way to address this issue.

Thank you,

-- flaviof

gdb --args ovs-ofctl -OOpenFlow15 del-meters br-int
(gdb) r
Starting program: /usr/local/bin/ovs-ofctl -OOpenFlow15 del-meters br-int

Program received signal SIGSEGV, Segmentation fault.
ofputil_put_bands (n_bands=, mb=0x0, 
msg=msg@entry=0x557849c0) at 
/usr/include/x86_64-linux-gnu/bits/byteswap.h:37
37 return __builtin_bswap16 (__bsx);
(gdb) bt
#0 ofputil_put_bands (n_bands=, mb=0x0, 
msg=msg@entry=0x557849c0) at 
/usr/include/x86_64-linux-gnu/bits/byteswap.h:37
#1 0x555e1d2d in ofputil_encode_meter_mod (ofp_version=, 
mm=0x7fffe380) at lib/ofp-meter.c:557
#2 0x555817a0 in ofctl_meter_mod__ (bridge=0x7fffe7f4 "br-int", 
str=, command=) at utilities/ovs-ofctl.c:4038
#3 0x5558a01e in ovs_cmdl_run_command__ (ctx=0x7fffe420, 
commands=, read_only=) at lib/command-line.c:247
#4 0x55578dc0 in main (argc=, argv=) at 
utilities/ovs-ofctl.c:5108
(gdb) down
#1 0x555e1d2d in ofputil_encode_meter_mod (ofp_version=, 
mm=0x7fffea10) at lib/ofp-meter.c:557
(gdb) p *mm
$4 = {command = 2, meter = {meter_id = 4294967295, flags = 65535, n_bands = 
65535, bands = 0x0}}

Note how n_bands should be set to 0. ^^ For sake of consistency, I'm also 
proposing that flags attribute be set to 0 as well.

Flavio Fernandes (1):
  ofp-parse: Fix segfault due to bad meter n_bands

 tests/dpif-netdev.at  | 2 ++
 utilities/ovs-ofctl.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH V3 1/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-17 Thread Flavio Fernandes

Meter commands internally use ofctl_meter_mod__ and ofctl_meter_request__ 
functions,
which have an optional parameter called str. When str is NULL, these 2 
functions initialize
a struct with meter bands set as NULL. It also needs to set meter n_bands to 0.

Once del-meters change in test dpif-netdev.at is added, the valgrind report on 
test
'992: dpif-netdev - meters' shows this issue:

   Conditional jump or move depends on uninitialised value(s)
  at 0x473534: ofputil_put_bands (ofp-meter.c:207)
  by 0x473534: ofputil_encode_meter_mod (ofp-meter.c:557)
  by 0x40FBA2: ofctl_meter_mod__ (ovs-ofctl.c:4038)
  by 0x417BD3: ovs_cmdl_run_command__ (command-line.c:247)
  by 0x4078BA: main (ovs-ofctl.c:179)
Uninitialised value was created by a stack allocation
  at 0x409350: ofctl_del_meters (ovs-ofctl.c:4088)

Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.")
Signed-off-by: Flavio Fernandes 
---
v3:
 - Nit: Fix commit message

v2:
 - Use memset to initialize struct instead of setting individial members
 - Invoke del-meters in existing tests/dpif-netdev.at test
---
 tests/dpif-netdev.at  | 2 ++
 utilities/ovs-ofctl.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 3e6222557..e223446d4 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -370,6 +370,8 @@ 
recirc_id(0),in_port(7),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), a
 recirc_id(0),in_port(8),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), 
actions:2
 ])
 
+AT_CHECK([ovs-ofctl -O OpenFlow13 del-meters br0])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3601890f4..62059e962 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -4020,6 +4020,7 @@ ofctl_meter_mod__(const char *bridge, const char *str, 
int command)
 enum ofputil_protocol usable_protocols;
 enum ofp_version version;
 
+memset(, 0, sizeof mm);
 if (str) {
 char *error;
 error = parse_ofp_meter_mod_str(, str, command, _protocols);
@@ -4030,7 +4031,6 @@ ofctl_meter_mod__(const char *bridge, const char *str, 
int command)
 usable_protocols = OFPUTIL_P_OF13_UP;
 mm.command = command;
 mm.meter.meter_id = OFPM13_ALL;
-mm.meter.bands = NULL;
 }
 
 protocol = open_vconn_for_flow_mod(bridge, , usable_protocols);
@@ -4050,6 +4050,7 @@ ofctl_meter_request__(const char *bridge, const char *str,
 enum ofputil_protocol protocol;
 enum ofp_version version;
 
+memset(, 0, sizeof mm);
 if (str) {
 char *error;
 error = parse_ofp_meter_mod_str(, str, -1, _protocols);
@@ -4059,7 +4060,6 @@ ofctl_meter_request__(const char *bridge, const char *str,
 } else {
 usable_protocols = OFPUTIL_P_OF13_UP;
 mm.meter.meter_id = OFPM13_ALL;
-mm.meter.bands = NULL;
 }
 
 protocol = open_vconn_for_flow_mod(bridge, , usable_protocols);
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH V2 1/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-17 Thread Flavio Fernandes

Meter commands internally use ofctl_meter_mod__ and ofctl_meter_request__ 
functions,
which have an optional parameter called str. When str is NULL, these 2 functions
initialize a struct with meter bands set as NULL. It also needs to set meter
n_bands to 0.

Once del-meters change is added to test dpif-netdev.at is added, the valgrind 
report on
test '992: dpif-netdev - meters' shows this issue:

   Conditional jump or move depends on uninitialised value(s)
  at 0x473534: ofputil_put_bands (ofp-meter.c:207)
  by 0x473534: ofputil_encode_meter_mod (ofp-meter.c:557)
  by 0x40FBA2: ofctl_meter_mod__ (ovs-ofctl.c:4038)
  by 0x417BD3: ovs_cmdl_run_command__ (command-line.c:247)
  by 0x4078BA: main (ovs-ofctl.c:179)
Uninitialised value was created by a stack allocation
  at 0x409350: ofctl_del_meters (ovs-ofctl.c:4088)

Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.")
Signed-off-by: Flavio Fernandes 
---
v2:
- Use memset to initialize struct instead of setting individial members
- Invoke del-meters in existing tests/dpif-netdev.at test
---
 tests/dpif-netdev.at  | 2 ++
 utilities/ovs-ofctl.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 3e6222557..e223446d4 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -370,6 +370,8 @@ 
recirc_id(0),in_port(7),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), a
 recirc_id(0),in_port(8),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), 
actions:2
 ])
 
+AT_CHECK([ovs-ofctl -O OpenFlow13 del-meters br0])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3601890f4..62059e962 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -4020,6 +4020,7 @@ ofctl_meter_mod__(const char *bridge, const char *str, 
int command)
 enum ofputil_protocol usable_protocols;
 enum ofp_version version;
 
+memset(, 0, sizeof mm);
 if (str) {
 char *error;
 error = parse_ofp_meter_mod_str(, str, command, _protocols);
@@ -4030,7 +4031,6 @@ ofctl_meter_mod__(const char *bridge, const char *str, 
int command)
 usable_protocols = OFPUTIL_P_OF13_UP;
 mm.command = command;
 mm.meter.meter_id = OFPM13_ALL;
-mm.meter.bands = NULL;
 }
 
 protocol = open_vconn_for_flow_mod(bridge, , usable_protocols);
@@ -4050,6 +4050,7 @@ ofctl_meter_request__(const char *bridge, const char *str,
 enum ofputil_protocol protocol;
 enum ofp_version version;
 
+memset(, 0, sizeof mm);
 if (str) {
 char *error;
 error = parse_ofp_meter_mod_str(, str, -1, _protocols);
@@ -4059,7 +4060,6 @@ ofctl_meter_request__(const char *bridge, const char *str,
 } else {
 usable_protocols = OFPUTIL_P_OF13_UP;
 mm.meter.meter_id = OFPM13_ALL;
-mm.meter.bands = NULL;
 }
 
 protocol = open_vconn_for_flow_mod(bridge, , usable_protocols);
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH V2 0/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-17 Thread Flavio Fernandes

Dear OVS community:

I hit a segfault exception while attempting to remove all meters from a bridge.
Upon looking a little closer, I can see that the fact that an invalid n_bands 
value
in the meter structure caused the issue. More details below.

I humbly propose the changes in this patchset as a way to address this issue.

Thank you,

-- flaviof

gdb --args ovs-ofctl -OOpenFlow15 del-meters br-int
(gdb) r
Starting program: /usr/local/bin/ovs-ofctl -OOpenFlow15 del-meters br-int

Program received signal SIGSEGV, Segmentation fault.
ofputil_put_bands (n_bands=, mb=0x0, 
msg=msg@entry=0x557849c0) at 
/usr/include/x86_64-linux-gnu/bits/byteswap.h:37
37 return __builtin_bswap16 (__bsx);
(gdb) bt
#0 ofputil_put_bands (n_bands=, mb=0x0, 
msg=msg@entry=0x557849c0) at 
/usr/include/x86_64-linux-gnu/bits/byteswap.h:37
#1 0x555e1d2d in ofputil_encode_meter_mod (ofp_version=, 
mm=0x7fffe380) at lib/ofp-meter.c:557
#2 0x555817a0 in ofctl_meter_mod__ (bridge=0x7fffe7f4 "br-int", 
str=, command=) at utilities/ovs-ofctl.c:4038
#3 0x5558a01e in ovs_cmdl_run_command__ (ctx=0x7fffe420, 
commands=, read_only=) at lib/command-line.c:247
#4 0x55578dc0 in main (argc=, argv=) at 
utilities/ovs-ofctl.c:5108
(gdb) down
#1 0x555e1d2d in ofputil_encode_meter_mod (ofp_version=, 
mm=0x7fffea10) at lib/ofp-meter.c:557
(gdb) p *mm
$4 = {command = 2, meter = {meter_id = 4294967295, flags = 65535, n_bands = 
65535, bands = 0x0}}

Note how n_bands should be set to 0. ^^ For sake of consistency, I'm also 
proposing that flags attribute be set to 0 as well.

Flavio Fernandes (1):
  ofp-parse: Fix segfault due to bad meter n_bands

 tests/dpif-netdev.at  | 2 ++
 utilities/ovs-ofctl.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-17 Thread Flavio Fernandes




> On Mar 16, 2021, at 5:28 PM, Ilya Maximets  wrote:
> 
> On 3/16/21 10:27 PM, Ilya Maximets wrote:
>> On 3/16/21 10:21 PM, Ilya Maximets wrote:
>>> On 3/16/21 9:24 PM, Flavio Fernandes wrote:
>>>> Meter commands internally use ofctl_meter_mod__ and ofctl_meter_request__ 
>>>> functions,
>>>> which have an optional parameter called str. When str is NULL, these 2 
>>>> functions initialize
>>>> a struct with meter bands set as NULL. It also needs to set meter n_bands 
>>>> to 0.
>>> 
>>> Good catch!  Indeed, I can see the issue from the valgrind report
>>> on test '992: dpif-netdev - meters':
>>> 
>>>   Conditional jump or move depends on uninitialised value(s)
>>>  at 0x473534: ofputil_put_bands (ofp-meter.c:207)
>>>  by 0x473534: ofputil_encode_meter_mod (ofp-meter.c:557)
>>>  by 0x40FBA2: ofctl_meter_mod__ (ovs-ofctl.c:4038)
>>>  by 0x417BD3: ovs_cmdl_run_command__ (command-line.c:247)
>>>  by 0x4078BA: main (ovs-ofctl.c:179)
>>>Uninitialised value was created by a stack allocation
>>>  at 0x409350: ofctl_del_meters (ovs-ofctl.c:4088)
>> 
>> Oops, I see that report because I modified the test like this:
>> 
>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
>> index 3e6222557..244b10ac7 100644
>> --- a/tests/dpif-netdev.at
>> +++ b/tests/dpif-netdev.at
>> @@ -370,6 +370,8 @@ 
>> recirc_id(0),in_port(7),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
>>  a
>> recirc_id(0),in_port(8),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
>>  actions:2
>> ])
>> 
>> +dnl AT_CHECK([ovs-ofctl -O OpenFlow13 del-meters br0])
> 
> s/dnl //
> Sorry.  I need some rest, apparently. :)
> 

ha. no worries, and yes to your review feedbacks! will do.

-- flaviof


>> +
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>> 
>> ---
>> 
>> It doesn't happen on master.
>> Flavio, could you, please, also add above test modification
>> to the patch?
>> 
>>> 
>>> 
>>> For the implementation, I think we need to just memset(, 0, sizeof mm);
>>> the whole structure to zero before the 'if' condition and not initialize
>>> anything that doesn't have a value, i.e. remove setting of
>>> mm.meter.bands to NULL.
>>> 
>>> Could you update the patch this way?  It might be also good if you can
>>> include above valgrind log into commit message.
>>> 
>>> Best regards, Ilya Maximets.
>>> 
>>>> 
>>>> Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.")
>>>> Signed-off-by: Flavio Fernandes 
>>>> ---
>>>> utilities/ovs-ofctl.c | 4 
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
>>>> index 3601890f4..bd1ba9222 100644
>>>> --- a/utilities/ovs-ofctl.c
>>>> +++ b/utilities/ovs-ofctl.c
>>>> @@ -4030,6 +4030,8 @@ ofctl_meter_mod__(const char *bridge, const char 
>>>> *str, int command)
>>>> usable_protocols = OFPUTIL_P_OF13_UP;
>>>> mm.command = command;
>>>> mm.meter.meter_id = OFPM13_ALL;
>>>> +mm.meter.flags = 0;
>>>> +mm.meter.n_bands = 0;
>>>> mm.meter.bands = NULL;
>>>> }
>>>> 
>>>> @@ -4059,6 +4061,8 @@ ofctl_meter_request__(const char *bridge, const char 
>>>> *str,
>>>> } else {
>>>> usable_protocols = OFPUTIL_P_OF13_UP;
>>>> mm.meter.meter_id = OFPM13_ALL;
>>>> +mm.meter.flags = 0;
>>>> +mm.meter.n_bands = 0;
>>>> mm.meter.bands = NULL;
>>>> }
>>>> 
>>>> 
>>> 
>> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-16 Thread Flavio Fernandes

Meter commands internally use ofctl_meter_mod__ and ofctl_meter_request__ 
functions,
which have an optional parameter called str. When str is NULL, these 2 
functions initialize
a struct with meter bands set as NULL. It also needs to set meter n_bands to 0.

Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.")
Signed-off-by: Flavio Fernandes 
---
 utilities/ovs-ofctl.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3601890f4..bd1ba9222 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -4030,6 +4030,8 @@ ofctl_meter_mod__(const char *bridge, const char *str, 
int command)
 usable_protocols = OFPUTIL_P_OF13_UP;
 mm.command = command;
 mm.meter.meter_id = OFPM13_ALL;
+mm.meter.flags = 0;
+mm.meter.n_bands = 0;
 mm.meter.bands = NULL;
 }
 
@@ -4059,6 +4061,8 @@ ofctl_meter_request__(const char *bridge, const char *str,
 } else {
 usable_protocols = OFPUTIL_P_OF13_UP;
 mm.meter.meter_id = OFPM13_ALL;
+mm.meter.flags = 0;
+mm.meter.n_bands = 0;
 mm.meter.bands = NULL;
 }
 
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 0/1] ofp-parse: Fix segfault due to bad meter n_bands

2021-03-16 Thread Flavio Fernandes

Dear OVS community:

I hit a segfault exception while attempting to remove all meters from a bridge.
Upon looking a little closer, I can see that the fact that an invalid n_bands 
value
in the meter structure caused the issue. More details below.

I humbly propose the changes in this patchset as a way to address this issue.

Thank you,

-- flaviof

gdb --args ovs-ofctl -OOpenFlow15 del-meters br-int
(gdb) r
Starting program: /usr/local/bin/ovs-ofctl -OOpenFlow15 del-meters br-int

Program received signal SIGSEGV, Segmentation fault.
ofputil_put_bands (n_bands=, mb=0x0, 
msg=msg@entry=0x557849c0) at 
/usr/include/x86_64-linux-gnu/bits/byteswap.h:37
37 return __builtin_bswap16 (__bsx);
(gdb) bt
#0 ofputil_put_bands (n_bands=, mb=0x0, 
msg=msg@entry=0x557849c0) at 
/usr/include/x86_64-linux-gnu/bits/byteswap.h:37
#1 0x555e1d2d in ofputil_encode_meter_mod (ofp_version=, 
mm=0x7fffe380) at lib/ofp-meter.c:557
#2 0x555817a0 in ofctl_meter_mod__ (bridge=0x7fffe7f4 "br-int", 
str=, command=) at utilities/ovs-ofctl.c:4038
#3 0x5558a01e in ovs_cmdl_run_command__ (ctx=0x7fffe420, 
commands=, read_only=) at lib/command-line.c:247
#4 0x55578dc0 in main (argc=, argv=) at 
utilities/ovs-ofctl.c:5108
(gdb) down
#1 0x555e1d2d in ofputil_encode_meter_mod (ofp_version=, 
mm=0x7fffea10) at lib/ofp-meter.c:557
(gdb) p *mm
$4 = {command = 2, meter = {meter_id = 4294967295, flags = 65535, n_bands = 
65535, bands = 0x0}}

Note how n_bands should be set to 0. ^^ For sake of consistency, I'm also 
proposing that flags attribute beset to 0 as well.


Flavio Fernandes (1):
  ofp-parse: Fix segfault due to bad meter n_bands

 utilities/ovs-ofctl.c | 4 
 1 file changed, 4 insertions(+)

-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] python: Send notifications after the transaction ends

2021-03-12 Thread Flavio Fernandes

Tested-by: Flavio Fernandes 

Used this change against Openstack's OVN Zuul :

 https://review.opendev.org/c/openstack/neutron/+/778595 
<https://review.opendev.org/c/openstack/neutron/+/778595>




> On Mar 4, 2021, at 11:18 AM, Terry Wilson  wrote:
> 
> The Python IDL notification mechanism was sending a notification
> for each processed update in a transaction as it was processed.
> This causes issues with multi-row changes that contain references
> to each other.
> 
> For example, if a Logical_Router_Port is created along with a
> Gateway_Chassis, and the LRP.gateway_chassis set to that GC, then
> when the notify() passes the CREATE event for the LRP, the GC will
> not yet have been processed, so __getattr__ when _uuid_to_row fails
> to find the GC, will return the default value for LRP.gateway_chassis
> which is [].
> 
> This patch has the process_update methods return the notifications
> that would be produced when a row changes, so they can be queued
> and sent after all rows have been processed.
> 
> Fixes: d7d417fcddf9 ("Allow subclasses of Idl to define a notification hook")
> Signed-off-by: Terry Wilson 
> ---
> python/ovs/db/idl.py | 39 ---
> tests/ovsdb-idl.at   | 22 ++
> tests/test-ovsdb.py  |  7 +--
> 3 files changed, 51 insertions(+), 17 deletions(-)
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] python: Send notifications after the transaction ends

2021-03-04 Thread Flavio Fernandes

Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>

> On Mar 4, 2021, at 11:18 AM, Terry Wilson  wrote:
> 
> The Python IDL notification mechanism was sending a notification
> for each processed update in a transaction as it was processed.
> This causes issues with multi-row changes that contain references
> to each other.
> 
> For example, if a Logical_Router_Port is created along with a
> Gateway_Chassis, and the LRP.gateway_chassis set to that GC, then
> when the notify() passes the CREATE event for the LRP, the GC will
> not yet have been processed, so __getattr__ when _uuid_to_row fails
> to find the GC, will return the default value for LRP.gateway_chassis
> which is [].
> 
> This patch has the process_update methods return the notifications
> that would be produced when a row changes, so they can be queued
> and sent after all rows have been processed.
> 
> Fixes: d7d417fcddf9 ("Allow subclasses of Idl to define a notification hook")
> Signed-off-by: Terry Wilson 
> ---
> python/ovs/db/idl.py | 39 ---
> tests/ovsdb-idl.at   | 22 ++
> tests/test-ovsdb.py  |  7 +--
> 3 files changed, 51 insertions(+), 17 deletions(-)
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] python: Send notifications after the transaction ends

2021-03-03 Thread Flavio Fernandes



Acked-by: Flavio Fernandes 


LGTM! Only one nit in the commit message.
Testing changes in Openstack Zuul: 
https://review.opendev.org/c/openstack/neutron/+/778595

> On Mar 3, 2021, at 4:49 PM, Terry Wilson  wrote:
> 
> The Python IDL notification mechanism was sending a notification
> for each processed update in a transaction as it was processed.
> This causes isseues with multi-row change that contain references
> to each other.

nit. typo: issues

> 
> For example, if a Logical_Router_Port is created along with a
> Gateway_Chassis, and the LRP.gateway_chassis set to that GC, then
> when the notify() passes the CREATE event for the LRP, the GC will
> not yet have been processed, so __getattr__ when _uuid_to_row fails
> to find the GC, will return the default value for LRP.gateway_chassis
> which is [].
> 
> This patch has the process_update methods return the notifications
> that would be produced when a row changes, so they can be queued
> and sent after all rows have been processed.
> 
> Fixes: d7d417fcddf9 ("Allow subclasses of Idl to define a notification hook")

Apr 25 2015. wow is it me or is time flying by? :)

> Signed-off-by: Terry Wilson 
> ---
> python/ovs/db/idl.py | 39 ---
> tests/ovsdb-idl.at   | 22 ++
> tests/test-ovsdb.py  |  7 +--
> 3 files changed, 51 insertions(+), 17 deletions(-)
> 
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 5850ac7ab..4226d1cb2 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -12,6 +12,7 @@
> # See the License for the specific language governing permissions and
> # limitations under the License.
> 
> +import collections
> import functools
> import uuid
> 
> @@ -39,6 +40,10 @@ OVSDB_UPDATE2 = 1
> CLUSTERED = "clustered"
> 
> 
> +Notice = collections.namedtuple('Notice', ('event', 'row', 'updates'))
> +Notice.__new__.__defaults__ = (None,)  # default updates=None
> +
> +
> class Idl(object):
> """Open vSwitch Database Interface Definition Language (OVSDB IDL).
> 
> @@ -614,6 +619,7 @@ class Idl(object):
> raise error.Error(" is not an object",
>   table_updates)
> 
> +notices = []
> for table_name, table_update in table_updates.items():
> table = tables.get(table_name)
> if not table:
> @@ -639,7 +645,9 @@ class Idl(object):
>   % (table_name, uuid_string))
> 
> if version == OVSDB_UPDATE2:
> -if self.__process_update2(table, uuid, row_update):
> +changes = self.__process_update2(table, uuid, row_update)
> +if changes:
> +notices.append(changes)
> self.change_seqno += 1
> continue
> 
> @@ -652,17 +660,20 @@ class Idl(object):
> raise error.Error(' missing "old" and '
>   '"new" members', row_update)
> 
> -if self.__process_update(table, uuid, old, new):
> +changes = self.__process_update(table, uuid, old, new)
> +if changes:
> +notices.append(changes)
> self.change_seqno += 1
> +for notice in notices:
> +self.notify(*notice)
> 
> def __process_update2(self, table, uuid, row_update):
> +"""Returns Notice if a column changed, False otherwise."""
> row = table.rows.get(uuid)
> -changed = False
> if "delete" in row_update:
> if row:
> del table.rows[uuid]
> -self.notify(ROW_DELETE, row)
> -changed = True
> +return Notice(ROW_DELETE, row)
> else:
> # XXX rate-limit
> vlog.warn("cannot delete missing row %s from table"
> @@ -681,29 +692,27 @@ class Idl(object):
> changed = self.__row_update(table, row, row_update)
> table.rows[uuid] = row
> if changed:
> -self.notify(ROW_CREATE, row)
> +return Notice(ROW_CREATE, row)
> elif "modify" in row_update:
> if not row:
> raise error.Error('Modify non-existing row')
> 
> old_row = self.__apply_diff(table, row, row_update['modify'])
> -self.notify(ROW_UPDATE, row, Row(self, table, uuid, old_row))
> -changed = True
> +return Notice(ROW_UPDATE, row, Row(self, t

[ovs-dev] [PATCH ovn 1/1] DNM: please ignore.. testing new server email send

2021-02-21 Thread Flavio Fernandes

Signed-off-by: Flavio Fernandes 
---
 utilities/ovn-nbctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 2c77f4ba7..85298770f 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -2717,7 +2717,7 @@ nbctl_meter_list(struct ctl_context *ctx)
 ds_put_format(>output, "%s:", meter->name);
 if (meter->fair) {
 ds_put_format(>output, " (%s)",
-  *meter->fair ? "fair" : "shared");
+  *meter->fair ? "fairy" : "shared");
 }
 ds_put_format(>output, " bands:\n");
 
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] northd: Fix ACL fair log meters for Port_Group ACLs.

2021-01-15 Thread Flavio Fernandes

Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> On Jan 15, 2021, at 8:41 AM, Dumitru Ceara  wrote:
> 
> Commit 880dca99eaf7 added support for fair meters but didn't cover the
> case when an ACL is configured on a port group instead of a logical
> switch.
> 
> Iterate over PG ACLs too when syncing fair meters to the Southbound
> database.  Due to the fact that a meter might be used for ACLs that are
> applied on multiple logical datapaths (through port groups) we also need
> to change the logic of deleting stale SB Meter records.
> 
> Fixes: 880dca99eaf7 ("northd: Enhance the implementation of ACL log meters 
> (pre-ddlog merge).")
> Reported-by: Dmitry Yusupov 
> Signed-off-by: Dumitru Ceara 
> ---
> northd/ovn-northd.c | 61 -
> tests/ovn-northd.at | 42 
> 2 files changed, 74 insertions(+), 29 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 969fbe1..27df6a3 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -12042,17 +12042,20 @@ static void
> sync_meters_iterate_nb_meter(struct northd_context *ctx,
>  const char *meter_name,
>  const struct nbrec_meter *nb_meter,
> - struct shash *sb_meters)
> + struct shash *sb_meters,
> + struct sset *used_sb_meters)
> {
> +const struct sbrec_meter *sb_meter;
> bool new_sb_meter = false;
> 
> -const struct sbrec_meter *sb_meter = shash_find_and_delete(sb_meters,
> -   meter_name);
> +sb_meter = shash_find_data(sb_meters, meter_name);
> if (!sb_meter) {
> sb_meter = sbrec_meter_insert(ctx->ovnsb_txn);
> sbrec_meter_set_name(sb_meter, meter_name);
> +shash_add(sb_meters, sb_meter->name, sb_meter);
> new_sb_meter = true;
> }
> +sset_add(used_sb_meters, meter_name);
> 
> if (new_sb_meter || bands_need_update(nb_meter, sb_meter)) {
> struct sbrec_meter_band **sb_bands;
> @@ -12074,6 +12077,24 @@ sync_meters_iterate_nb_meter(struct northd_context 
> *ctx,
> sbrec_meter_set_unit(sb_meter, nb_meter->unit);
> }
> 
> +static void
> +sync_acl_fair_meter(struct northd_context *ctx, struct shash *meter_groups,
> +const struct nbrec_acl *acl, struct shash *sb_meters,
> +struct sset *used_sb_meters)
> +{
> +const struct nbrec_meter *nb_meter =
> +fair_meter_lookup_by_name(meter_groups, acl->meter);
> +
> +if (!nb_meter) {
> +return;
> +}
> +
> +char *meter_name = alloc_acl_log_unique_meter_name(acl);
> +sync_meters_iterate_nb_meter(ctx, meter_name, nb_meter, sb_meters,
> + used_sb_meters);
> +free(meter_name);
> +}
> +
> /* Each entry in the Meter and Meter_Band tables in OVN_Northbound have
>  * a corresponding entries in the Meter and Meter_Band tables in
>  * OVN_Southbound. Additionally, ACL logs that use fair meters have
> @@ -12081,9 +12102,10 @@ sync_meters_iterate_nb_meter(struct northd_context 
> *ctx,
>  */
> static void
> sync_meters(struct northd_context *ctx, struct hmap *datapaths,
> -struct shash *meter_groups)
> +struct shash *meter_groups, struct hmap *port_groups)
> {
> struct shash sb_meters = SHASH_INITIALIZER(_meters);
> +struct sset used_sb_meters = SSET_INITIALIZER(_sb_meters);
> 
> const struct sbrec_meter *sb_meter;
> SBREC_METER_FOR_EACH (sb_meter, ctx->ovnsb_idl) {
> @@ -12093,7 +12115,7 @@ sync_meters(struct northd_context *ctx, struct hmap 
> *datapaths,
> const struct nbrec_meter *nb_meter;
> NBREC_METER_FOR_EACH (nb_meter, ctx->ovnnb_idl) {
> sync_meters_iterate_nb_meter(ctx, nb_meter->name, nb_meter,
> - _meters);
> + _meters, _sb_meters);
> }
> 
> /*
> @@ -12107,19 +12129,28 @@ sync_meters(struct northd_context *ctx, struct hmap 
> *datapaths,
> continue;
> }
> for (size_t i = 0; i < od->nbs->n_acls; i++) {
> -struct nbrec_acl *acl = od->nbs->acls[i];
> -nb_meter = fair_meter_lookup_by_name(meter_groups, acl->meter);
> -if (!nb_meter) {
> -continue;
> +sync_acl_fair_meter(ctx, meter_groups, od->nbs->acls[i],
> +_meters, _sb_meters);
>

Re: [ovs-dev] [PATCH ovn] Fix OVN update issue when ovn-controller is updated first from 20.06 to 20.09.

2020-11-22 Thread Flavio Fernandes

Acked-by: Flavio Fernandes 


> On Nov 20, 2020, at 5:52 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> The commit in the Fixes tag changed the ct_commit signature from
> ct_commit(..) to ct_commit{ ..}.  When ovn-controllers are updated
> to a vesion which has this change and if ovn-northd is still not
> udated to this version, then the logical flow with the action
> ct_commit(..) will be rejected by ovn-controllers resulting in
> datapath disruptions.  OVN recommends ovn-controller updates before
> ovn-northd, but it is broken now.  This patch fixes this issue by
> adding back the support for the old ct_commit(..).  We now support
> both the versions of ct_commit.
> 
> Fixes: 6cfb44a76c61("Used nested actions in ct_commit)
> CC: Mark Michelson 
> Signed-off-by: Numan Siddique 
> ---
> include/ovn/actions.h |  10 ++-
> lib/actions.c | 137 --
> tests/ovn.at  |  44 ++
> utilities/ovn-trace.c |   3 +-
> 4 files changed, 186 insertions(+), 8 deletions(-)
> 
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 630bbe79e..7bec6a6f8 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -57,7 +57,8 @@ struct ovn_extend_table;
> OVNACT(EXCHANGE,  ovnact_move)\
> OVNACT(DEC_TTL,   ovnact_null)\
> OVNACT(CT_NEXT,   ovnact_ct_next) \
> -OVNACT(CT_COMMIT, ovnact_nest)\
> +OVNACT(CT_COMMIT_V1,  ovnact_ct_commit_v1)\
> +OVNACT(CT_COMMIT_V2,  ovnact_nest)\
> OVNACT(CT_DNAT,   ovnact_ct_nat)  \
> OVNACT(CT_SNAT,   ovnact_ct_nat)  \
> OVNACT(CT_LB, ovnact_ct_lb)   \
> @@ -226,6 +227,13 @@ struct ovnact_ct_next {
> uint8_t ltable;/* Logical table ID of next table. */
> };
> 
> +/* OVNACT_CT_COMMIT_V1. */
> +struct ovnact_ct_commit_v1 {
> +struct ovnact ovnact;
> +uint32_t ct_mark, ct_mark_mask;
> +ovs_be128 ct_label, ct_label_mask;
> +};
> +
> /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
> struct ovnact_ct_nat {
> struct ovnact ovnact;
> diff --git a/lib/actions.c b/lib/actions.c
> index 82b35ccf9..36c8078b0 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -627,16 +627,75 @@ ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED)
> {
> }
> 
> +static void
> +parse_ct_commit_v1_arg(struct action_context *ctx,
> +   struct ovnact_ct_commit_v1 *cc)
> +{
> +if (lexer_match_id(ctx->lexer, "ct_mark")) {
> +if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +return;
> +}
> +if (ctx->lexer->token.type == LEX_T_INTEGER) {
> +cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
> +cc->ct_mark_mask = UINT32_MAX;
> +} else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
> +cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
> +cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);
> +} else {
> +lexer_syntax_error(ctx->lexer, "expecting integer");
> +return;
> +}
> +lexer_get(ctx->lexer);
> +} else if (lexer_match_id(ctx->lexer, "ct_label")) {
> +if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +return;
> +}
> +if (ctx->lexer->token.type == LEX_T_INTEGER) {
> +cc->ct_label = ctx->lexer->token.value.be128_int;
> +cc->ct_label_mask = OVS_BE128_MAX;
> +} else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
> +cc->ct_label = ctx->lexer->token.value.be128_int;
> +cc->ct_label_mask = ctx->lexer->token.mask.be128_int;
> +} else {
> +lexer_syntax_error(ctx->lexer, "expecting integer");
> +return;
> +}
> +lexer_get(ctx->lexer);
> +} else {
> +lexer_syntax_error(ctx->lexer, NULL);
> +}
> +}
> +
> +static void
> +parse_CT_COMMIT_V1(struct action_context *ctx)
> +{
> +add_prerequisite(ctx, "ip");
> +
> +struct ovnact_ct_commit_v1 *ct_commit =
> +ovnact_put_CT_COMMIT_V1(ctx->ovnacts);
> +if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> +while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> +parse_ct_commit_v1_arg(ctx, ct_commit);
> +if (ctx->lexer->error) {
> +return;
> +}
> +

Re: [ovs-dev] [PATCH ovn] controller: Allow pinctrl thread to handle packet-ins when version mismatch with northd.

2020-11-22 Thread Flavio Fernandes

Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>
Tested-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> On Nov 22, 2020, at 9:44 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> The commit 1dd27ea7aea4 added the support to pin ovn-controller to a
> specific version of ovn-northd.  This patch did not handle the
> scenario the packet-in scenario when ovn-controller is started and it
> detects version mismatch with ovn-northd.  In this case, pinctrl
> thread is not configured with the proper integration bridge name,
> because of which it cannot handle any packet-ins.
> 
> This patch addresses this scenario.  This is required so that any
> existing VMs do not loose DHCP address during renewal.
> 
> Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd 
> to a specific version")
> Signed-off-by: Numan Siddique 
> ---
> controller/ovn-controller.c | 27 -
> controller/pinctrl.c| 34 ++---
> controller/pinctrl.h|  2 +-
> tests/ovn.at| 59 -
> 4 files changed, 102 insertions(+), 20 deletions(-)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 41e2678cf4..61de0af8d9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2648,25 +2648,29 @@ main(int argc, char *argv[])
> 
> engine_set_context(_ctx);
> 
> -if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> +bool northd_version_match =
> check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
> - ovn_version)) {
> + ovn_version);
> +
> +const struct ovsrec_bridge_table *bridge_table =
> +ovsrec_bridge_table_get(ovs_idl_loop.idl);
> +const struct ovsrec_open_vswitch_table *ovs_table =
> +ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> +const struct ovsrec_bridge *br_int =
> +process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> +
> +if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> +northd_version_match) {
> /* Contains the transport zones that this Chassis belongs to */
> struct sset transport_zones = SSET_INITIALIZER(_zones);
> sset_from_delimited_string(_zones,
> get_transport_zones(ovsrec_open_vswitch_table_get(
> ovs_idl_loop.idl)), ",");
> 
> -const struct ovsrec_bridge_table *bridge_table =
> -ovsrec_bridge_table_get(ovs_idl_loop.idl);
> -const struct ovsrec_open_vswitch_table *ovs_table =
> -ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> const struct sbrec_chassis_table *chassis_table =
> sbrec_chassis_table_get(ovnsb_idl_loop.idl);
> const struct sbrec_chassis_private_table *chassis_pvt_table =
> sbrec_chassis_private_table_get(ovnsb_idl_loop.idl);
> -const struct ovsrec_bridge *br_int =
> -process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> const char *chassis_id = get_ovs_chassis_id(ovs_table);
> const struct sbrec_chassis *chassis = NULL;
> const struct sbrec_chassis_private *chassis_private = NULL;
> @@ -2836,6 +2840,13 @@ main(int argc, char *argv[])
> }
> }
> 
> +if (!northd_version_match && br_int) {
> +/* Set the integration bridge name to pinctrl so that the pinctrl
> + * thread can handle any packet-ins when we are not processing
> + * any DB updates due to version mismatch. */
> +pinctrl_set_br_int_name(br_int->name);
> +}
> +
> unixctl_server_run(unixctl);
> 
> unixctl_server_wait(unixctl);
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index a5236564b4..c9dd5ea301 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_)
> return NULL;
> }
> 
> +static void
> +pinctrl_set_br_int_name_(char *br_int_name)
> +{
> +if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name,
> +   br_int_name))) {
> +if (pinctrl.br_int_name) {
> +free(pinctrl.br_int_name);
> +}
> +pinctrl.br_int_name = xstrdup(br_int_name);
> +/* Notify pinctrl_handler that integration bridge is
> + * set/changed. */
> +notify_pinctrl_hand

Re: [ovs-dev] [PATCH ovn] controller: Allow pinctrl thread to handle packet-ins when version mismatch with northd.

2020-11-22 Thread Flavio Fernandes

Tested-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> On Nov 22, 2020, at 9:44 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> The commit 1dd27ea7aea4 added the support to pin ovn-controller to a
> specific version of ovn-northd.  This patch did not handle the
> scenario the packet-in scenario when ovn-controller is started and it
> detects version mismatch with ovn-northd.  In this case, pinctrl
> thread is not configured with the proper integration bridge name,
> because of which it cannot handle any packet-ins.
> 
> This patch addresses this scenario.  This is required so that any
> existing VMs do not loose DHCP address during renewal.
> 
> Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd 
> to a specific version")
> Signed-off-by: Numan Siddique 
> ---
> controller/ovn-controller.c | 27 -
> controller/pinctrl.c| 34 ++---
> controller/pinctrl.h|  2 +-
> tests/ovn.at| 59 -
> 4 files changed, 102 insertions(+), 20 deletions(-)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 41e2678cf4..61de0af8d9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2648,25 +2648,29 @@ main(int argc, char *argv[])
> 
> engine_set_context(_ctx);
> 
> -if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> +bool northd_version_match =
> check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
> - ovn_version)) {
> + ovn_version);
> +
> +const struct ovsrec_bridge_table *bridge_table =
> +ovsrec_bridge_table_get(ovs_idl_loop.idl);
> +const struct ovsrec_open_vswitch_table *ovs_table =
> +ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> +const struct ovsrec_bridge *br_int =
> +process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> +
> +if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> +northd_version_match) {
> /* Contains the transport zones that this Chassis belongs to */
> struct sset transport_zones = SSET_INITIALIZER(_zones);
> sset_from_delimited_string(_zones,
> get_transport_zones(ovsrec_open_vswitch_table_get(
> ovs_idl_loop.idl)), ",");
> 
> -const struct ovsrec_bridge_table *bridge_table =
> -ovsrec_bridge_table_get(ovs_idl_loop.idl);
> -const struct ovsrec_open_vswitch_table *ovs_table =
> -ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> const struct sbrec_chassis_table *chassis_table =
> sbrec_chassis_table_get(ovnsb_idl_loop.idl);
> const struct sbrec_chassis_private_table *chassis_pvt_table =
> sbrec_chassis_private_table_get(ovnsb_idl_loop.idl);
> -const struct ovsrec_bridge *br_int =
> -process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> const char *chassis_id = get_ovs_chassis_id(ovs_table);
> const struct sbrec_chassis *chassis = NULL;
> const struct sbrec_chassis_private *chassis_private = NULL;
> @@ -2836,6 +2840,13 @@ main(int argc, char *argv[])
> }
> }
> 
> +if (!northd_version_match && br_int) {
> +/* Set the integration bridge name to pinctrl so that the pinctrl
> + * thread can handle any packet-ins when we are not processing
> + * any DB updates due to version mismatch. */
> +pinctrl_set_br_int_name(br_int->name);
> +}
> +
> unixctl_server_run(unixctl);
> 
> unixctl_server_wait(unixctl);
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index a5236564b4..c9dd5ea301 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_)
> return NULL;
> }
> 
> +static void
> +pinctrl_set_br_int_name_(char *br_int_name)
> +{
> +if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name,
> +   br_int_name))) {
> +if (pinctrl.br_int_name) {
> +free(pinctrl.br_int_name);
> +}
> +pinctrl.br_int_name = xstrdup(br_int_name);
> +/* Notify pinctrl_handler that integration bridge is
> + * set/changed. */
> +notify_pinctrl_handler();
> +}
> +}
> +
> +void
> +pinc

Re: [ovs-dev] [PATCH ovn] controller: Allow pinctrl thread to handle packet-ins when version mismatch with northd.

2020-11-22 Thread Flavio Fernandes

Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> On Nov 22, 2020, at 9:44 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> The commit 1dd27ea7aea4 added the support to pin ovn-controller to a
> specific version of ovn-northd.  This patch did not handle the
> scenario the packet-in scenario when ovn-controller is started and it
> detects version mismatch with ovn-northd.  In this case, pinctrl
> thread is not configured with the proper integration bridge name,
> because of which it cannot handle any packet-ins.
> 
> This patch addresses this scenario.  This is required so that any
> existing VMs do not loose DHCP address during renewal.
> 
> Fixes: 1dd27ea7aea4("Provide the option to pin ovn-controller and ovn-northd 
> to a specific version")
> Signed-off-by: Numan Siddique 
> ---
> controller/ovn-controller.c | 27 -
> controller/pinctrl.c| 34 ++---
> controller/pinctrl.h|  2 +-
> tests/ovn.at| 59 -
> 4 files changed, 102 insertions(+), 20 deletions(-)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 41e2678cf4..61de0af8d9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2648,25 +2648,29 @@ main(int argc, char *argv[])
> 
> engine_set_context(_ctx);
> 
> -if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> +bool northd_version_match =
> check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
> - ovn_version)) {
> + ovn_version);
> +
> +const struct ovsrec_bridge_table *bridge_table =
> +ovsrec_bridge_table_get(ovs_idl_loop.idl);
> +const struct ovsrec_open_vswitch_table *ovs_table =
> +ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> +const struct ovsrec_bridge *br_int =
> +process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> +
> +if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> +northd_version_match) {
> /* Contains the transport zones that this Chassis belongs to */
> struct sset transport_zones = SSET_INITIALIZER(_zones);
> sset_from_delimited_string(_zones,
> get_transport_zones(ovsrec_open_vswitch_table_get(
> ovs_idl_loop.idl)), ",");
> 
> -const struct ovsrec_bridge_table *bridge_table =
> -ovsrec_bridge_table_get(ovs_idl_loop.idl);
> -const struct ovsrec_open_vswitch_table *ovs_table =
> -ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
> const struct sbrec_chassis_table *chassis_table =
> sbrec_chassis_table_get(ovnsb_idl_loop.idl);
> const struct sbrec_chassis_private_table *chassis_pvt_table =
> sbrec_chassis_private_table_get(ovnsb_idl_loop.idl);
> -const struct ovsrec_bridge *br_int =
> -process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> const char *chassis_id = get_ovs_chassis_id(ovs_table);
> const struct sbrec_chassis *chassis = NULL;
> const struct sbrec_chassis_private *chassis_private = NULL;
> @@ -2836,6 +2840,13 @@ main(int argc, char *argv[])
> }
> }
> 
> +if (!northd_version_match && br_int) {
> +/* Set the integration bridge name to pinctrl so that the pinctrl
> + * thread can handle any packet-ins when we are not processing
> + * any DB updates due to version mismatch. */
> +pinctrl_set_br_int_name(br_int->name);
> +}
> +
> unixctl_server_run(unixctl);
> 
> unixctl_server_wait(unixctl);
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index a5236564b4..c9dd5ea301 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3114,6 +3114,29 @@ pinctrl_handler(void *arg_)
> return NULL;
> }
> 
> +static void
> +pinctrl_set_br_int_name_(char *br_int_name)
> +{
> +if (br_int_name && (!pinctrl.br_int_name || strcmp(pinctrl.br_int_name,
> +   br_int_name))) {
> +if (pinctrl.br_int_name) {
> +free(pinctrl.br_int_name);
> +}
> +pinctrl.br_int_name = xstrdup(br_int_name);
> +/* Notify pinctrl_handler that integration bridge is
> + * set/changed. */
> +notify_pinctrl_handler();
> +}
> +}
> +
> +void
> +pinctrl_set_br_int_name(char *

Re: [ovs-dev] [PATCH v5 ovn 1/2] northd: Fair ACL log meters.

2020-11-20 Thread Flavio Fernandes




> On Nov 19, 2020, at 4:01 AM, Numan Siddique  wrote:
> 
> On Mon, Nov 16, 2020 at 9:20 PM Flavio Fernandes  <mailto:fla...@flaviof.com>> wrote:
>> 
>> When multiple ACL rows use the same Meter for log rate-limiting, the
>> 'noisiest' ACL matches may completely consume the Meter and shadow other
>> ACL logs. This patch introduces a column in northbound's Meter table
>> called fair that allows for a Meter to rate-limit each ACL log separately.
>> 
>> The change is backward compatible. Based on the fair column of a Meter row,
>> northd will turn a single Meter in the NB into multiple Meters in the SB
>> by appending the ACL uuid to its name. It will also make action in logical
>> flow use the unique Meter name for each affected ACL log. In order to
>> make the Meter behave as described, add Meter with this option:
>> 
>>  ovn-nbctl --fair meter-add  ${meter_name} drop ${rate} ${unit}
>> 
>> Example:
>> 
>>  ovn-nbctl --fair meter-add meter_me drop 1 pktps
>> 
>> Reported-by: Flavio Fernandes 
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
>> Suggested-by: Dumitru Ceara 
>> Suggested-by: Ben Pfaff 
>> Signed-off-by: Flavio Fernandes 
>> ---
>> 
>> This change can be tracked in the following github clone/branch:
>> 
>>https://github.com/flavio-fernandes/ovn/commits/acl-meters.v5.ddlog
>> 
>> v4 -> v5:
>> * Rebase https://github.com/blp/ovs-reviews/ddlog10 (f56dafa83).
> 
> Hi Flavio,
> 
> Thanks for v5. This patch doesn't apply cleanly on the latest master.
> 
> I'd suggest to post the patch 1 was an independent patch rebased on on
> top of latest master
> 
> I think you can submit the patch 2 once the patch 1 is accepted ,
> either as part of ddlog patch series
> or after the ddlog patches are merged.

Okay! ;)

I submitted v6 as a standalone, based on master with changes from your
comments below:

https://patchwork.ozlabs.org/project/ovn/patch/20201120182211.2646-2-fla...@flaviof.com/


> 
> Few comments below.
> 
> Thanks
> Numan
> 
> 
>> v3 -> v4:
>> * Rename 'shared meters' to 'fair meters' to keep it less confusing.
>> * NB schema change: Add column in Meter to track which meters are 'fair'.
>> * Add optional '--fair' flag to ovn-nbctl meter-add command.
>> v2 -> v3:
>> * Use recently introduced testing helpers (4afe409e9).
>> v1 -> v2:
>> * Rebase master b38e10f4b.
>> 
>> NEWS  |   2 +
>> northd/ovn-northd.c   | 184 ++
>> ovn-nb.ovsschema  |   5 +-
>> ovn-nb.xml|  16 +++-
>> tests/ovn-nbctl.at|   6 +-
>> tests/ovn-northd.at   |  97 ++
>> utilities/ovn-nbctl.c |  16 +++-
>> 7 files changed, 268 insertions(+), 58 deletions(-)
>> 
>> diff --git a/NEWS b/NEWS
>> index 04b75e68c..0965dff4f 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -14,6 +14,8 @@ Post-v20.09.0
>>  server.
>>- Support other_config:vlan-passthru=true to allow VLAN tagged incoming
>>  traffic.
>> +   - Add "fair" column in Meter table to allow multiple ACL logs to use the
>> + same Meter while being rate-limited independently.
>> 
>> OVN v20.09.0 - 28 Sep 2020
>> --
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 4d4190cb9..cee8abce9 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -5353,8 +5353,45 @@ build_acl_hints(struct ovn_datapath *od, struct hmap 
>> *lflows)
>> }
>> }
>> 
>> +static bool
>> +is_a_fair_meter(const struct shash *meter_groups, const char *meter_name)
>> +{
>> +const struct nbrec_meter *nb_meter =
>> +shash_find_data(meter_groups, meter_name);
>> +if (nb_meter && nb_meter->fair) {
>> +return *nb_meter->fair;
>> +}
> 
> I would suggest to change the above 'if' as
> 
> if (nb_meter) {
>return nb_meter->n_fair && *nb_meter->fair;
> }

Ack. Turns out I needed some further mods to this function to return the 
nb_meter,
so I renamed it to "fair_meter_lookup_by_name" and made the statement change
there.



>> static void
>> -sync_meters(struct northd_context *ctx)
>> +sync_meters(struct northd_context *ctx, struct hmap *datapaths)
>> {
>> struct shash sb_meters = SHASH_INITIALIZER(_meters);
>> 
>> @@ -11649,33 +11752,14 @@ sync_meters(struct northd_context *ctx)
>> 
>&g

Re: [ovs-dev] [PATCH v5 ovn 1/2] northd: Fair ACL log meters.

2020-11-20 Thread Flavio Fernandes




> On Nov 19, 2020, at 12:37 PM, Ben Pfaff  wrote:
> 
> On Thu, Nov 19, 2020 at 02:31:04PM +0530, Numan Siddique wrote:
>> Thanks for v5. This patch doesn't apply cleanly on the latest master.
>> 
>> I'd suggest to post the patch 1 was an independent patch rebased on on
>> top of latest master
>> 
>> I think you can submit the patch 2 once the patch 1 is accepted ,
>> either as part of ddlog patch series
>> or after the ddlog patches are merged.
> 
> Yeah, I wouldn't worry about the ddlog patch.  I'm happy to help
> shepherd that in after the initial patch goes in.
> 

Sounds fair. ;) Thank you, Ben!

-- flaviof


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v6 ovn 0/1] northd: Enhance the implementation of ACL log meters (pre-ddlog merge).

2020-11-20 Thread Flavio Fernandes

Using meters is a great way to keep the ovn-controllers from getting
overwhelmed with ACL log events. Since multiple ACL rows with logging
enabled can refer to the same meter name, I ran a little experiment
to better understand how that behaves [1].

>From that experiment, we see that a 'noisy' ACL match could consume
all the events allowed by the meter, shadowing logs for other ACLs
that also use the same meter. The thought of maintaining a meter row
per ACL at the NB side is a solution, but it could easily become a
management burden for the CMS. A much better approach would be to
leverage northd to take care of this on behalf of the ACLs.

As northd populates SB meter table from NB meter table, a new logic
checks if the meter is configured as 'fair'. Such config is kept
as a new column in the Meter table. Fair meters result in additional
rows in the SB that have the same attributes of the original (aka
template) meter except for its name has the ACL UUID appended to
it.

Last but not least, northd takes care of using the corresponding
meter name as the logical flow action for the logging of the ACL.

This change can be tracked in the following github clone/branch:
https://github.com/flavio-fernandes/ovn/commits/acl-meters.v6

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

v5 -> v6:
* Rebase master (4da6881b6).
* Implement changes from Numan review.
* Keep ddlog portion as a follow up after it merges.
v4 -> v5:
* Rebase master (e14b52a9f).
v3 -> v4:
* Rename 'shared meters' to 'fair meters' to keep it less confusing.
* NB schema change: Add column in Meter to track which meters are 'fair'.
* Add optional '--fair' flag to ovn-nbctl meter-add command.
v2 -> v3:
* Use recently introduced testing helpers (4afe409e9).
v1 -> v2:
* Rebase master b38e10f4b.

Flavio Fernandes (1):
northd: Enhance the implementation of ACL log meters.

--
2.18.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] Provide the option to pin ovn-controller and ovn-northd to a specific version.

2020-11-17 Thread Flavio Fernandes

Hi Numan!

A few nits I thought of mentioning [inline]

> On Nov 17, 2020, at 6:57 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> OVN recommends updating/upgrading ovn-controllers first and then
> ovn-northd and OVN DB ovsdb-servers.  This is to ensure that any
> new functionality specified by the database or logical flows created
> by ovn-northd is understood by ovn-controller.
> 
> However certain deployments may upgrade ovn-northd services first and
> then ovn-controllers.  In a large scal deployment, this can result in
> downtime during upgrades as old ovn-controllers may not understand
> new logical flows or new actions added by ovn-northd.
> 
> Even with upgrading ovn-controllers first, can result in ovn-controllers
> rejecting some of the logical flows if an existing OVN action is
> changed.  One such example is ct_commit action which recently was updated
> to take new arguments.
> 
> To avoid such downtimes during upgrades, this patch adds the
> functionality of pinning ovn-controller and ovn-northd to a specific
> version. An internal OVN version is generated and this version is stored
> by ovn-northd in the Southbound SB_Global table's
> options:northd_internal_version.
> 
> When ovn-controller notices that the internal version has changed, it
> stops handling the database changes - both Southbound and OVS. All the
> existing OF flows are preserved.  When ovn-controller is upgraded to the
> same version as ovn-northd services, it will process the database
> changes.
> 
> This feature is made optional and disabled by default. Any CMS can
> enable it by configuring the OVS local database with the option -
> ovn-pin-to-northd=false.

^^=false or =true^^ ?

> 
> Signed-off-by: Numan Siddique 
> ---
> 
> v1 -> v2
> 
>  * Added test cases and missing documentation.
>  * Renamed the ovsdb option from ignore_northd_verison
>to ovn-pin-to-northd.
>  * Added NEWS item.
> 
> NEWS|   3 +
> controller/ovn-controller.8.xml |  11 +++
> controller/ovn-controller.c |  58 -
> lib/ovn-util.c  |  17 
> lib/ovn-util.h  |   4 +
> northd/ovn-northd.c |  18 +++-
> tests/ovn.at| 147 
> 7 files changed, 251 insertions(+), 7 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 6010230679..bb95724b36 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,9 @@ Post-v20.09.0
>  server.
>- Support other_config:vlan-passthru=true to allow VLAN tagged incoming
>  traffic.
> +   - Support version pinning between ovn-northd and ovn-controller as an
> + option. If the option is enabled and the versions don't match,
> + ovn-controller will not process any DB changes.
> 
> OVN v20.09.0 - 28 Sep 2020
> --
> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> index 16bc47b205..357edd1f5c 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -233,6 +233,17 @@
> The boolean flag indicates if the chassis is used as an
> interconnection gateway.
>   
> +
> +  external_ids:ovn-pin-to-northd
> +  
> +The boolean flag indicates if ovn-controller needs to
> +be pinned to the exact ovn-northd version. If this
> +flag is set to true and the ovn-northd's version 
> (reported
> +in the Southbound database) doesn't match with the
> +ovn-controller's internal version, then it will stop
> +processing the Southbound and local Open vSwitch database changes.
> +The default value is considered false if this option is not defined.
> +  
> 
> 
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index a06cae3ccb..a7344ea9c4 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2136,6 +2136,37 @@ struct ovn_controller_exit_args {
> bool *restart;
> };
> 
> +static bool
> +pin_to_northd(struct ovsdb_idl *ovs_idl)
> +{
> +const struct ovsrec_open_vswitch *cfg = 
> ovsrec_open_vswitch_first(ovs_idl);
> +return !cfg ? false : smap_get_bool(>external_ids,
> +"ovn-pin-to-northd", false);
> +}
> +
> +/* Returns false if the northd internal version and ovn-controller internal
> + * version doesn't match.
> + */
> +static bool
> +check_northd_version(const struct sbrec_sb_global *sb, const char 
> *my_version)
> +{
> +if (!sb) {
> +return false;
> +}
> +
> +const char *northd_version =
> +smap_get_def(>options, "northd_internal_version", "");
> +
> +bool mismatch = strcmp(northd_version, my_version);
> +if (mismatch) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +VLOG_WARN_RL(, "controller version - %s mismatch with northd "
> + "version - %s", my_version, northd_version);
> +}
> +
> +return mismatch;
>

[ovs-dev] [PATCH v5.master ovn 0/2] northd: Fair ACL log meters (pre-ddlog merge).

2020-11-16 Thread Flavio Fernandes

Last but not least, northd takes care of using the corresponding
meter name as the logical flow action for the logging of the ACL.

This change can be tracked in the following github clone/branch:
https://github.com/flavio-fernandes/ovn/commits/acl-meters.v5.ddlog

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (1):
northd: Enhance the implementation of ACL log meters.

--
2.18.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v5 ovn 2/2] northd: Fair ACL log meters.

2020-11-16 Thread Flavio Fernandes

ddlog for ACL log meters.

Implement fair meters for acl logs in ovn_northd.dl.
Enable fair Meter test to also exercise ovn-northd-ddlog.

This is a small variation of blp's implementation of
ddlog for ACL log meters, now using the northbound schema changes [1].
The changes address overall design issues mentioned in that link.

[1]: 
https://patchwork.ozlabs.org/project/ovn/patch/20201107213913.gc2753...@ovn.org/

Co-authored-by: Ben Pfaff 
Signed-off-by: Flavio Fernandes 
---
 northd/ovn_northd.dl | 281 ---
 tests/ovn-northd.at  |   2 +
 2 files changed, 161 insertions(+), 122 deletions(-)

diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 3fbe67b31..46489e4cb 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -27,6 +27,23 @@ output relation Warning[string]
 
 index Logical_Flow_Index() on sb::Out_Logical_Flow()
 
+/* Single-row relation that contains the set of meters marked as fair. */
+relation AclFairLogMeters[Set]
+AclFairLogMeters[meters] :- AclFairLogMeters0[meters].
+AclFairLogMeters[set_empty()] :-
+Unit(),
+not AclFairLogMeters0[_].
+
+relation AclFairLogMeters0[Set]
+AclFairLogMeters0[meters] :-
+meter in nb::Meter(.fair = Some{fair}),
+fair,
+var meters = {
+var meters = set_empty();
+set_insert(meters, meter.name);
+meters
+}.
+
 /* Meter_Band table */
 for (mb in nb::Meter_Band) {
 sb::Out_Meter_Band(._uuid = mb._uuid,
@@ -42,6 +59,14 @@ for (meter in nb::Meter) {
  .unit = meter.unit,
  .bands = meter.bands)
 }
+sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
+  .name = "${name}__${uuid2str(acl_uuid)}",
+  .unit = unit,
+  .bands = bands) :-
+AclFairLogMeters[names],
+var name = FlatMap(names),
+nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
+nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
 
 /* Proxy table for Out_Datapath_Binding: contains all Datapath_Binding fields,
  * except tunnel id, which is allocated separately (see TunKeyAllocation). */
@@ -2017,7 +2042,7 @@ for ((.ls = ls)) {
  .external_ids = map_empty())
 }
 
-function build_acl_log(acl: nb::ACL): string =
+function build_acl_log(acl: nb::ACL, fair_meters: Set): string =
 {
 if (not acl.log) {
 ""
@@ -2049,7 +2074,14 @@ function build_acl_log(acl: nb::ACL): string =
 };
 match (acl.meter) {
 None -> (),
-Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}")
+Some{meter} -> {
+var s = if (fair_meters.contains(meter)) {
+"${meter}__${uuid2str(acl._uuid)}"
+} else {
+meter
+};
+vec_push(strs, "meter=${json_string_escape(s)}")
+}
 };
 "log(${string_join(strs, \", \")}); "
 }
@@ -2067,31 +2099,33 @@ function oVN_ACL_PRI_OFFSET(): integer = 1000
 relation Reject(lsuuid: uuid, pipeline: string, stage: Stage, acl: nb::ACL, 
extra_match: string, extra_actions: string)
 
 /* build_reject_acl_rules() */
-for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
-var extra_match = match (extra_match_) {
-"" -> "",
-s -> "(${s}) && "
-} in
-var extra_actions = match (extra_actions_) {
-"" -> "",
-s -> "${s} "
-} in
-var next = match (pipeline == "ingress") {
-true  -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, 
QOS_MARK)).0})",
-false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, 
L2_LKUP)).0})"
-} in
-var acl_log = build_acl_log(acl) in {
-var __match = extra_match ++ acl.__match in
-var actions = acl_log ++ extra_actions ++ "reg0 = 0; "
-  "reject { "
-  "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. 
*/ "
-  "outport <-> inport; ${next}; };" in
-Flow(.logical_datapath = lsuuid,
- .stage= stage,
- .priority = acl.priority + oVN_ACL_PRI_OFFSET(),
- .__match  = __match,
- .actions  = actions,
- .external_ids = stage_hint(acl._uuid))
+for (AclFairLogMeters[fair_meters]) {
+for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
+var extra_match = match (extra_match_) {
+"" -> "",
+s -> "(${s}) && "
+} in
+var extra_actions = match (extra_actions_) {
+"" -> "",
+s -&g

[ovs-dev] [PATCH v5 ovn 0/2] northd: Fair ACL log meters.

2020-11-16 Thread Flavio Fernandes

Last but not least, northd takes care of using the corresponding
meter name as the logical flow action for the logging of the ACL.

This change can be tracked in the following github clone/branch:
https://github.com/flavio-fernandes/ovn/commits/acl-meters.v5.ddlog

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (2):
northd: Enhance the implementation of ACL log meters.
ddlog for ACL log meters

--
2.18.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 ovn 2/2] northd: Fair ACL log meters.

2020-11-13 Thread Flavio Fernandes

ddlog for ACL log meters.

Implement fair meters for acl logs in ovn_northd.dl.
Enable fair Meter test to also exercise ovn-northd-ddlog.

This is a small variation of blp's implementation of
ddlog for ACL log meters, now using the northbound schema changes [1].
The changes address overall design issues mentioned in that link.

[1]: 
https://patchwork.ozlabs.org/project/ovn/patch/20201107213913.gc2753...@ovn.org/

Co-authored-by: Ben Pfaff 
Signed-off-by: Flavio Fernandes 
---
 northd/ovn_northd.dl | 281 ---
 tests/ovn-northd.at  |   2 +
 2 files changed, 161 insertions(+), 122 deletions(-)

diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 3fbe67b31..46489e4cb 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -27,6 +27,23 @@ output relation Warning[string]
 
 index Logical_Flow_Index() on sb::Out_Logical_Flow()
 
+/* Single-row relation that contains the set of meters marked as fair. */
+relation AclFairLogMeters[Set]
+AclFairLogMeters[meters] :- AclFairLogMeters0[meters].
+AclFairLogMeters[set_empty()] :-
+Unit(),
+not AclFairLogMeters0[_].
+
+relation AclFairLogMeters0[Set]
+AclFairLogMeters0[meters] :-
+meter in nb::Meter(.fair = Some{fair}),
+fair,
+var meters = {
+var meters = set_empty();
+set_insert(meters, meter.name);
+meters
+}.
+
 /* Meter_Band table */
 for (mb in nb::Meter_Band) {
 sb::Out_Meter_Band(._uuid = mb._uuid,
@@ -42,6 +59,14 @@ for (meter in nb::Meter) {
  .unit = meter.unit,
  .bands = meter.bands)
 }
+sb::Out_Meter(._uuid = hash128(meter_uuid ++ acl_uuid),
+  .name = "${name}__${uuid2str(acl_uuid)}",
+  .unit = unit,
+  .bands = bands) :-
+AclFairLogMeters[names],
+var name = FlatMap(names),
+nb::Meter(._uuid = meter_uuid, .name = name, .unit = unit, .bands = bands),
+nb::ACL(._uuid = acl_uuid, .meter = Some{name}).
 
 /* Proxy table for Out_Datapath_Binding: contains all Datapath_Binding fields,
  * except tunnel id, which is allocated separately (see TunKeyAllocation). */
@@ -2017,7 +2042,7 @@ for ((.ls = ls)) {
  .external_ids = map_empty())
 }
 
-function build_acl_log(acl: nb::ACL): string =
+function build_acl_log(acl: nb::ACL, fair_meters: Set): string =
 {
 if (not acl.log) {
 ""
@@ -2049,7 +2074,14 @@ function build_acl_log(acl: nb::ACL): string =
 };
 match (acl.meter) {
 None -> (),
-Some{meter} -> vec_push(strs, "meter=${json_string_escape(meter)}")
+Some{meter} -> {
+var s = if (fair_meters.contains(meter)) {
+"${meter}__${uuid2str(acl._uuid)}"
+} else {
+meter
+};
+vec_push(strs, "meter=${json_string_escape(s)}")
+}
 };
 "log(${string_join(strs, \", \")}); "
 }
@@ -2067,31 +2099,33 @@ function oVN_ACL_PRI_OFFSET(): integer = 1000
 relation Reject(lsuuid: uuid, pipeline: string, stage: Stage, acl: nb::ACL, 
extra_match: string, extra_actions: string)
 
 /* build_reject_acl_rules() */
-for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
-var extra_match = match (extra_match_) {
-"" -> "",
-s -> "(${s}) && "
-} in
-var extra_actions = match (extra_actions_) {
-"" -> "",
-s -> "${s} "
-} in
-var next = match (pipeline == "ingress") {
-true  -> "next(pipeline=egress,table=${stage_id(switch_stage(OUT, 
QOS_MARK)).0})",
-false -> "next(pipeline=ingress,table=${stage_id(switch_stage(IN, 
L2_LKUP)).0})"
-} in
-var acl_log = build_acl_log(acl) in {
-var __match = extra_match ++ acl.__match in
-var actions = acl_log ++ extra_actions ++ "reg0 = 0; "
-  "reject { "
-  "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. 
*/ "
-  "outport <-> inport; ${next}; };" in
-Flow(.logical_datapath = lsuuid,
- .stage= stage,
- .priority = acl.priority + oVN_ACL_PRI_OFFSET(),
- .__match  = __match,
- .actions  = actions,
- .external_ids = stage_hint(acl._uuid))
+for (AclFairLogMeters[fair_meters]) {
+for (Reject(lsuuid, pipeline, stage, acl, extra_match_, extra_actions_)) {
+var extra_match = match (extra_match_) {
+"" -> "",
+s -> "(${s}) && "
+} in
+var extra_actions = match (extra_actions_) {
+"" -> "",
+s -&g

[ovs-dev] [PATCH v4 ovn 0/2] northd: Fair ACL log meters.

2020-11-13 Thread Flavio Fernandes

Last but not least, northd takes care of using the corresponding
meter name as the logical flow action for the logging of the ACL.

This change can be tracked in the following github clone/branch:
https://github.com/flavio-fernandes/ovn/commits/acl-meters.v4.merge

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (2):
northd: Fair ACL log meters.
ddlog for ACL log meters.

--
2.18.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v5 8/8] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.

2020-11-12 Thread Flavio Fernandes

Hm..

Could it be that you inadvertently remove this change in V5 ?


diff --git a/northd/automake.mk b/northd/automake.mk
index 2717f59c5f3a..157b5d0df487 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -22,8 +22,7 @@ bin_PROGRAMS += northd/ovn-northd-ddlog
northd_ovn_northd_ddlog_SOURCES = \
northd/ovn-northd-ddlog.c \
northd/ovn-northd-ddlog-sb.inc \
-   northd/ovn-northd-ddlog-nb.inc \
-   northd/ovn_northd_ddlog/ddlog.h
+   northd/ovn-northd-ddlog-nb.inc
northd_ovn_northd_ddlog_LDADD = \
northd/ovn_northd_ddlog/target/release/libovn_northd_ddlog.la 
 \
lib/libovn.la  \
@@ -46,6 +45,7 @@ BUILT_SOURCES += \
northd/ovn-northd-ddlog-sb.inc \
northd/ovn-northd-ddlog-nb.inc

+northd/ovn-northd-ddlog.$(OBJEXT): northd/ovn_northd_ddlog/ddlog.h
northd/ovn_northd_ddlog/ddlog.h: northd/ddlog.stamp

CARGO_VERBOSE = $(cargo_verbose_$(V))


-- flaviof



> On Nov 11, 2020, at 8:45 PM, Ben Pfaff  wrote:
> 
> From: Leonid Ryzhyk 
> 
> This implementation is incremental, meaning that it only recalculates
> what is needed for the southbound database when northbound changes
> occur.  It is expected to scale better than the C implementation,
> for large deployments.  (This may take testing and tuning to be
> effective.)
> 
> There are three tests that I'm having mysterious trouble getting
> to work with DDlog.  For now, I've marked the testsuite to skip
> them unless RUN_ANYWAY=yes is set in the environment.
> 
> Signed-off-by: Leonid Ryzhyk 
> Co-authored-by: Justin Pettit 
> Signed-off-by: Justin Pettit 
> Co-authored-by: Ben Pfaff 
> Signed-off-by: Ben Pfaff 
> ---

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ddlog for ACL log meters (was: [PATCH v3 ovn 1/1] northd: Enhance the implementation of ACL log meters.)

2020-11-10 Thread Flavio Fernandes

[cc Dimitru, Numan, MarkM]

> On Nov 10, 2020, at 2:15 PM, Ben Pfaff  wrote:
> 
> On Mon, Nov 09, 2020 at 09:53:16AM -0500, Flavio Fernandes wrote:
>>> On Nov 7, 2020, at 4:39 PM, Ben Pfaff  wrote:
>>> On Tue, Nov 03, 2020 at 10:18:34PM +, Flavio Fernandes wrote:
>>>> When multiple ACL rows use the same Meter for log rate-limiting, the
>>>> 'noisiest' ACL matches may completely consume the Meter and shadow other
>>>> ACL logs. This patch introduces an option in NB_Global that allows for a
>>>> Meter to rate-limit each ACL log separately.
>>> 
>>> I'm not sure I like the overall design here.  This option isn't going to
>>> scale very well to many of these meters, since they'd all need to be
>>> shoved into a single comma-separated list.  Another way might be to add
>>> a column to the ACL table to mark a meter as shared or private.  Or
>>> there could be a name convention, e.g. prefix with "shared_" to make it
>>> shared.
>> 
>> Understood. I think I tried "too hard" to avoid making changes to the
>> northbound schema, but you bring a valid concern about
>> scalability. Adding a "bool" to the ACL row, or to the Meter row would
>> definitely make this more scalable. I would like to ask for opinion
>> from you on the following choices (not listed in any order). You ==
>> Ben and the rest of the ovn core developers and users.
>> 
>> A) add a bool column to ACL row called "fair_log_meter" (please help me with 
>> the name);
>> B) add a bool column to Meter row called ^same as above^;
>> C) change parsing of the value of the global to allow for up to one Meter 
>> name
>> D) change parsing of the value of the global to allow for up to a constant 
>> Meter names
>> E) have an implict behavior based on the name of the Meter "shared_", so 
>> that multiple meters are created in the SB as needed.
>> F) same as 'E', but use a different prefix str
>> G) any other good approach?
> 
> I think that A and B are reasonable approaches.  I don't understand the
> issue well enough to know whether this property has more to do with the
> ACL or with the Meter, so I'm not sure where it better to put it.

Let's pursue the schema change, then! Funny thing is that my very initial idea 
[1] was 'A'.
I can see how 'B' could benefit non-ACL-log related functionality in the NB 
that refers to
Meters, but I think that is overkill. I ended up hesitant in changing the 
schema after talking
with my OVN guru Dumitru. I will follow up with him on this idea to see how 
strongly he feels
about it.

> I don't think C or D makes sense, because I don't know a reason why
> there would be only one of these meter or only a small number of these
> meters.

Option C (not the language) is likely too restrictive and D would still feel 
like a leash.
A small number of 'fair' Meters would be all that ACL logs needed. If many ACL 
logs end up having
different Meter needs, then having a 1:1 mapping between ACLs and Meter rows 
would be
the answer. And that is what we already have the ability to do today. So, I 
cannot imagine a usage
case where we would have more than a couple of values for the NB_global. The 
potential for scaling
troubles does not give me a good felling, so I'm back to 'A'. ;)

> 
> I think that E or F would only make sense if there was some reason A or
> B would not work.

++

> 
> OK, let's step back here and consider G.  Why do we need a new
> southbound Meter row for each instance?  That's a bit of a waste, isn't
> it?  Why can't we just add an indicator to the southbound Meter that
> says "each reference to me is unique"?  Or a new argument to the
> southbound log() action that distinguishes references to a given meter,
> so that identical values get the same one and different values get
> distinct ones?

Hmm That sounds really good, actually. We would still need 'A' as a way
to propagate that desire from the CMS' perspective, agree?

I must confess that the wasteful approach of rows in the SB comes from my
limited knowledge on how to efficiently use the log action to do what you
described. Also, because I was hoping to solve the whole problem within
northd.

If I understand you correctly, option 'G' you mention here would require
changes in the SB schema as well as in ovn-controller? I will definitely
need some pointers to make that happen. Wanna be my partner in
crime? :) No pressure.

> 
>>> I don't really understand the vocabulary here, either.  I would have
>>> guessed that "shared" would mean that all the ACLs with a given meter
>>> name share a single southbound Meter, but it's the opposite: a "shared"
>

Re: [ovs-dev] ddlog for ACL log meters (was: [PATCH v3 ovn 1/1] northd: Enhance the implementation of ACL log meters.)

2020-11-09 Thread Flavio Fernandes

[inline]

> On Nov 7, 2020, at 4:39 PM, Ben Pfaff  wrote:
> 
> On Tue, Nov 03, 2020 at 10:18:34PM +0000, Flavio Fernandes wrote:
>> When multiple ACL rows use the same Meter for log rate-limiting, the
>> 'noisiest' ACL matches may completely consume the Meter and shadow other
>> ACL logs. This patch introduces an option in NB_Global that allows for a
>> Meter to rate-limit each ACL log separately.
>> 
>> The change is backward compatible. Based on a well known option, northd
>> will turn a single Meter in the NB into multiple Meters in the SB by
>> appending the ACL uuid to its name. It will also make action in logical
>> flow use the unique Meter name for each affected ACL log. In order to
>> make the Meter behave as described, add this option:
>> 
>>  ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"
>> 
>> If more than one Meter is wanted, simply use a comma to separate each name.
>> 
>> Reported-by: Flavio Fernandes 
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
>> Signed-off-by: Flavio Fernandes 
> 
> I'm not sure I like the overall design here.  This option isn't going to
> scale very well to many of these meters, since they'd all need to be
> shoved into a single comma-separated list.  Another way might be to add
> a column to the ACL table to mark a meter as shared or private.  Or
> there could be a name convention, e.g. prefix with "shared_" to make it
> shared.

Understood. I think I tried "too hard" to avoid making changes to the 
northbound schema, but
you bring a valid concern about scalability. Adding a "bool" to the ACL row, or 
to the Meter row
would definitely make this more scalable. I would like to ask for opinion from 
you on the following
choices (not listed in any order). You == Ben and the rest of the ovn core 
developers and users.

A) add a bool column to ACL row called "fair_log_meter" (please help me with 
the name);
B) add a bool column to Meter row called ^same as above^;
C) change parsing of the value of the global to allow for up to one Meter name
D) change parsing of the value of the global to allow for up to a constant 
Meter names
E) have an implict behavior based on the name of the Meter "shared_", so that 
multiple meters are created in the SB as needed.
F) same as 'E', but use a different prefix str
G) any other good approach?

> 
> I don't really understand the vocabulary here, either.  I would have
> guessed that "shared" would mean that all the ACLs with a given meter
> name share a single southbound Meter, but it's the opposite: a "shared"
> meter has a different southbound Meter for each ACL.

Yes, good point. I was seeing this change mostly from the Northbound 
perspective,
but you are right that this is a confusing name looking at what happens at the 
SB.
How about we use the word "fair" instead? So, something like 
"acl_fair_log_meters".
Any suggestions on a better name are very welcome.

> 
> OK, all that is quibbling.  You asked me to help with the ddlog part.  I
> did that, and let me explain it.

I am super grateful for the help below. I will defer going over that in this
message to focus on the topics above. Regardless, this is super useful for
folks [like me] to ramp up on ddlog-northd.

Best,

-- flaviof

> 
> I'll start with a way that the ddlog implementation I built differs from
> the C implementation.  That's in the southbound Meter_Band table.  The C
> implementation creates Meter_Band rows per meter instance; if a "shared"
> meter has 3 instances, it'll create 3 copies of the Meter_Band rows.
> There's nothing valuable about the extra instances, since they are all
> the same, and ovn-controller only looks at the values in the rows and
> doesn't care that they're different rows.  It's slightly easier not to
> create the extra instances in the ddlog implementation, so I just made
> it create one.  So this existing code in ovn_northd.dl can stay:
> 
>/* Meter_Band table */
>for (mb in nb::Meter_Band) {
>sb::Out_Meter_Band(._uuid = mb._uuid,
>  .action = mb.action,
>  .rate = mb.rate,
>  .burst_size = mb.burst_size)
>}
> 
> The new test does check that there are three (identical) Meter_Band
> rows.  I updated the test to be more flexible, so that it expects 1 or 3
> rows all with the correct rate:
> 
># Make sure that every Meter_Band has the right rate.  (ovn-northd
># creates 3 identical Meter_Band rows, all identical; ovn-northd-ddlog
># creates just 1.  It doesn't matter, they work just as well.)
>n_meter_band

Re: [ovs-dev] [PATCH ovn] northd: fix lb_action when there are no active backends for lb health_check

2020-11-04 Thread Flavio Fernandes

Tested-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> On Nov 3, 2020, at 11:43 AM, Lorenzo Bianconi  
> wrote:
> 
> Fix the following warning reported by ovn-controller when there are no
> active backends for lb health_check and selection_fields have been
> configured for hash computation
> 
> flow|WARN|error parsing actions "drop; 
> hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");":
> Syntax error at `hash_fields' expecting end of input.
> 
> Fixes: 5af304e747 ("Support selection fields in load balancer.")
> Signed-off-by: Lorenzo Bianconi 
> ---
> northd/ovn-northd.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 8800a3d3c..88d3e3ed2 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3598,6 +3598,8 @@ static void build_lb_vip_ct_lb_actions(struct lb_vip 
> *lb_vip,
>struct ds *action,
>char *selection_fields)
> {
> +bool skip_hash_fields = false;
> +
> if (lb_vip->health_check) {
> ds_put_cstr(action, "ct_lb(backends=");
> 
> @@ -3616,6 +3618,7 @@ static void build_lb_vip_ct_lb_actions(struct lb_vip 
> *lb_vip,
> }
> 
> if (!n_active_backends) {
> +skip_hash_fields = true;
> ds_clear(action);
> ds_put_cstr(action, "drop;");
> } else {
> @@ -3626,7 +3629,7 @@ static void build_lb_vip_ct_lb_actions(struct lb_vip 
> *lb_vip,
> ds_put_format(action, "ct_lb(backends=%s);", lb_vip->backend_ips);
> }
> 
> -if (selection_fields && selection_fields[0]) {
> +if (!skip_hash_fields && selection_fields && selection_fields[0]) {
> ds_chomp(action, ';');
> ds_chomp(action, ')');
> ds_put_format(action, "; hash_fields=\"%s\");", selection_fields);
> --
> 2.26.2
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 ovn 0/1] northd: Enhance the implementation of ACL log meters.

2020-11-03 Thread Flavio Fernandes

As northd populates SB meter table from NB meter table, a new logic
checks if the meter is configured as 'shared'. Such config is kept
as a new option in nb_global. Shared meters result in additional
rows in the SB that have the same attributes of the original (aka
template) meter except for its name has the ACL UUID appended to
it.

Last but not least, northd takes care of using the corresponding
meter name as the action in the logging of the ACL.

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (1):
northd: Enhance the implementation of ACL log meters.

northd/ovn-northd.c | 201
ovn-nb.xml | 14 +++
tests/ovn-northd.at | 92
3 files changed, 255 insertions(+), 52 deletions(-)

--
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 ovn 1/1] northd: Enhance the implementation of ACL log meters.

2020-11-03 Thread Flavio Fernandes

When multiple ACL rows use the same meter for log rate-limiting, the
'noisiest' ACL matches may completely consume the meter and shadow other
ACL logs. This patch introduces an option in NB_Global that allows for a
meter to rate-limit each ACL log separately.

The change is backward compatible. Based on a well known option, northd
will turn a single meter in the NB into multiple meters in the SB by
appending the ACL uuid to its name. It will also make action in logical
flow use the unique meter name for each affected ACL log. In order to
make the meter behave as described, add this option:

  ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"

If more than one meter is wanted, simply use a comma to separate each name.

Reported-by: Flavio Fernandes 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
Signed-off-by: Flavio Fernandes 
---
v1 -> v2:
 * rebase master b38e10f4b

TODO: I do not yet know the implications that this change have on the new
  ddlog based implementation of northd. Will read up and contact folks on 
that.

 northd/ovn-northd.c | 201 
 ovn-nb.xml  |  14 +++
 tests/ovn-northd.at |  99 ++
 3 files changed, 262 insertions(+), 52 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8800a3d3c..721e7be97 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5378,8 +5378,53 @@ build_acl_hints(struct ovn_datapath *od, struct hmap 
*lflows)
 }
 }
 
+static bool
+is_a_shared_meter(const struct smap *nb_options, const char *meter_name)
+{
+bool is_shared_meter = false;
+const char *meters = smap_get(nb_options, "acl_shared_log_meters");
+if (meters && meters[0]) {
+char *cur, *next, *start;
+next = start = xstrdup(meters);
+while ((cur = strsep(, ",")) && *cur) {
+if (!strcmp(meter_name, cur)) {
+is_shared_meter = true;
+break;
+}
+}
+free(start);
+}
+return is_shared_meter;
+}
+
+static char*
+alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl)
+{
+return xasprintf("%s__" UUID_FMT,
+ acl->meter, UUID_ARGS(>header_.uuid));
+}
+
+static void
+build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl,
+const struct smap *nb_options)
+{
+if (!acl->meter) {
+return;
+}
+
+/* If ACL log meter uses a shared meter, use unique Meter name. */
+if (is_a_shared_meter(nb_options, acl->meter)) {
+char *meter_name = alloc_acl_log_unique_meter_name(acl);
+ds_put_format(actions, "meter=\"%s\", ", meter_name);
+free(meter_name);
+} else {
+ds_put_format(actions, "meter=\"%s\", ", acl->meter);
+}
+}
+
 static void
-build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
+build_acl_log(struct ds *actions, const struct nbrec_acl *acl,
+  const struct smap *nb_options)
 {
 if (!acl->log) {
 return;
@@ -5407,9 +5452,7 @@ build_acl_log(struct ds *actions, const struct nbrec_acl 
*acl)
 ds_put_cstr(actions, "verdict=allow, ");
 }
 
-if (acl->meter) {
-ds_put_format(actions, "meter=\"%s\", ", acl->meter);
-}
+build_acl_log_meter(actions, acl, nb_options);
 
 ds_chomp(actions, ' ');
 ds_chomp(actions, ',');
@@ -5420,7 +5463,8 @@ static void
 build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
enum ovn_stage stage, struct nbrec_acl *acl,
struct ds *extra_match, struct ds *extra_actions,
-   const struct ovsdb_idl_row *stage_hint)
+   const struct ovsdb_idl_row *stage_hint,
+   const struct smap *nb_options)
 {
 struct ds match = DS_EMPTY_INITIALIZER;
 struct ds actions = DS_EMPTY_INITIALIZER;
@@ -5432,7 +5476,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct 
hmap *lflows,
   ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
   : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP));
 
-build_acl_log(, acl);
+build_acl_log(, acl, nb_options);
 if (extra_match->length > 0) {
 ds_put_format(, "(%s) && ", extra_match->string);
 }
@@ -5457,7 +5501,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct 
hmap *lflows,
 
 static void
 consider_acl(struct hmap *lflows, struct ovn_datapath *od,
- struct nbrec_acl *acl, bool has_stateful)
+ struct nbrec_acl *acl, bool has_stateful,
+ const struct smap *nb_options)
 {
 bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
 enum ovn_stage stage =

[ovs-dev] [PATCH v2 ovn 0/1] northd: Enhance the implementation of ACL log meters.

2020-11-03 Thread Flavio Fernandes

Last but not least, northd takes care of using the corresponding
meter name as the action in the logging of the ACL.

[1]:
https://github.com/flavio-fernandes/ovsdbapp_playground/blob/acl_meter_issue/scripts/acl_meter.sh

Flavio Fernandes (1):
northd: Enhance the implementation of ACL log meters.

northd/ovn-northd.c | 201
ovn-nb.xml | 14 +++
tests/ovn-northd.at | 99 ++
3 files changed, 262 insertions(+), 52 deletions(-)

--
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1 ovn] northd: Enhance the implementation of ACL log meters.

2020-11-02 Thread Flavio Fernandes

When multiple ACL rows use the same Meter for log rate-limiting, the
'noisiest' ACL matches may completely consume the Meter and shadow other
ACL logs. This patch introduces an option in NB_Global that allows for a
Meter to rate-limit each ACL log separately.

The change is backward compatible. Based on a well known option, northd
will turn a single Meter in the NB into multiple Meters in the SB by
appending the ACL uuid to its name. It will also make action in logical
flow use the unique Meter name for each affected ACL log. In order to
make the Meter behave as described, add this option:

  ovn-nbctl set nb_global . options:acl_shared_log_meters="${meter_name}"

If more than one Meter is wanted, simply use a comma to separate each name.

Reported-by: Flavio Fernandes 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-October/050769.html
Signed-off-by: Flavio Fernandes 
---

TODO: I do not yet know the implications that this change have on the new
  ddlog based implementation of northd. Will read up and contact folks on 
that.

 northd/ovn-northd.c | 201 
 ovn-nb.xml  |  14 +++
 tests/ovn-northd.at |  99 ++
 3 files changed, 262 insertions(+), 52 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8800a3d3c..721e7be97 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5378,8 +5378,53 @@ build_acl_hints(struct ovn_datapath *od, struct hmap 
*lflows)
 }
 }
 
+static bool
+is_a_shared_meter(const struct smap *nb_options, const char *meter_name)
+{
+bool is_shared_meter = false;
+const char *meters = smap_get(nb_options, "acl_shared_log_meters");
+if (meters && meters[0]) {
+char *cur, *next, *start;
+next = start = xstrdup(meters);
+while ((cur = strsep(, ",")) && *cur) {
+if (!strcmp(meter_name, cur)) {
+is_shared_meter = true;
+break;
+}
+}
+free(start);
+}
+return is_shared_meter;
+}
+
+static char*
+alloc_acl_log_unique_meter_name(const struct nbrec_acl *acl)
+{
+return xasprintf("%s__" UUID_FMT,
+ acl->meter, UUID_ARGS(>header_.uuid));
+}
+
+static void
+build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl,
+const struct smap *nb_options)
+{
+if (!acl->meter) {
+return;
+}
+
+/* If ACL log meter uses a shared meter, use unique Meter name. */
+if (is_a_shared_meter(nb_options, acl->meter)) {
+char *meter_name = alloc_acl_log_unique_meter_name(acl);
+ds_put_format(actions, "meter=\"%s\", ", meter_name);
+free(meter_name);
+} else {
+ds_put_format(actions, "meter=\"%s\", ", acl->meter);
+}
+}
+
 static void
-build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
+build_acl_log(struct ds *actions, const struct nbrec_acl *acl,
+  const struct smap *nb_options)
 {
 if (!acl->log) {
 return;
@@ -5407,9 +5452,7 @@ build_acl_log(struct ds *actions, const struct nbrec_acl 
*acl)
 ds_put_cstr(actions, "verdict=allow, ");
 }
 
-if (acl->meter) {
-ds_put_format(actions, "meter=\"%s\", ", acl->meter);
-}
+build_acl_log_meter(actions, acl, nb_options);
 
 ds_chomp(actions, ' ');
 ds_chomp(actions, ',');
@@ -5420,7 +5463,8 @@ static void
 build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
enum ovn_stage stage, struct nbrec_acl *acl,
struct ds *extra_match, struct ds *extra_actions,
-   const struct ovsdb_idl_row *stage_hint)
+   const struct ovsdb_idl_row *stage_hint,
+   const struct smap *nb_options)
 {
 struct ds match = DS_EMPTY_INITIALIZER;
 struct ds actions = DS_EMPTY_INITIALIZER;
@@ -5432,7 +5476,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct 
hmap *lflows,
   ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
   : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP));
 
-build_acl_log(, acl);
+build_acl_log(, acl, nb_options);
 if (extra_match->length > 0) {
 ds_put_format(, "(%s) && ", extra_match->string);
 }
@@ -5457,7 +5501,8 @@ build_reject_acl_rules(struct ovn_datapath *od, struct 
hmap *lflows,
 
 static void
 consider_acl(struct hmap *lflows, struct ovn_datapath *od,
- struct nbrec_acl *acl, bool has_stateful)
+ struct nbrec_acl *acl, bool has_stateful,
+ const struct smap *nb_options)
 {
 bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
 enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL;
@@ -5471,7

Re: [ovs-dev] [PATCH ovn] Add missing curly braces to bare ct_commits

2020-08-06 Thread Flavio Fernandes



Verified this fix on top of commit acd38429638c01afe1b2a1d15404e4724232ec1d .

Tested-by: Flavio Fernandes 



> On Aug 6, 2020, at 9:38 AM, Mark Michelson  wrote:
> 
> In the fixes commit below, ct_commit was changed to use nested actions.
> This requires that curly braces be present for all ct_commits. When
> adjusting ovn-northd, some ct_commits were not updated to have them.
> This commit fixes the issue.
> 
> Fixes: 6cfb44a76c61("Used nested actions in ct_commit")
> Signed-off-by: Mark Michelson 
> ---
> northd/ovn-northd.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 03c62bafa..d2312f020 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5590,7 +5590,7 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
> if (server_id && server_mac && lease_time) {
> struct ds match = DS_EMPTY_INITIALIZER;
> const char *actions =
> -has_stateful ? "ct_commit; next;" : "next;";
> +has_stateful ? "ct_commit { }; next;" : "next;";
> ds_put_format(, "outport == \"%s\" && eth.src == %s "
>   "&& ip4.src == %s && udp && udp.src == 67 "
>   "&& udp.dst == 68", od->nbs->ports[i]->name,
> @@ -5616,7 +5616,7 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
> ipv6_string_mapped(server_ip, );
> 
> struct ds match = DS_EMPTY_INITIALIZER;
> -const char *actions = has_stateful ? "ct_commit; next;" :
> +const char *actions = has_stateful ? "ct_commit { }; next;" :
> "next;";
> ds_put_format(, "outport == \"%s\" && eth.src == %s "
>   "&& ip6.src == %s && udp && udp.src == 547 "
> @@ -5634,7 +5634,7 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
>  * if the CMS has configured DNS records for the datapath.
>  */
> if (ls_has_dns_records(od->nbs)) {
> -const char *actions = has_stateful ? "ct_commit; next;" : "next;";
> +const char *actions = has_stateful ? "ct_commit { }; next;" : 
> "next;";
> ovn_lflow_add(
> lflows, od, S_SWITCH_OUT_ACL, 34000, "udp.src == 53",
> actions);
> --
> 2.25.4
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovs-ctl: Don't set hostname as external-id

2020-05-20 Thread Flavio Fernandes



Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>> 


> On May 20, 2020, at 11:52 AM, Daniel Alvarez  wrote:
> 
> ovs-ctl started to add the hostname as external-id [0] at some point.
> 
> However, this can be problematic as if it's already set by an external
> entity it will get overwritten. In RHEL systems, systemd will invoke
> ovs-ctl to start OVS and that will overwrite it to the hostname of the
> machine.
> 
> For OVN this can have a big impact because if, for whatever reason the
> hostname changes and the host gets restarted, ovn-controller won't
> claim the ports back leaving the workloads unaccessible.
> 
> Also, it makes sense to leave this untouched as 1) it's an external_id,
> so it will actually let external entities to configure it (unlike now),
> and 2) it's optional. In the case of OVN, if the external-id doesn't
> exist, it'll default to its hostname so nothing should get broken by
> this change.
> 
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2016-March/312054.html
> 
> Signed-off-by: Daniel Alvarez 
> ---
> utilities/ovs-ctl.in | 12 
> 1 file changed, 12 deletions(-)
> 
> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> index 8c5cd7032..87fc4934f 100644
> --- a/utilities/ovs-ctl.in
> +++ b/utilities/ovs-ctl.in
> @@ -35,17 +35,6 @@ insert_mod_if_required () {
> ovs_kmod_ctl insert
> }
> 
> -set_hostname () {
> -# 'hostname -f' needs network connectivity to work.  So we should
> -# call this only after ovs-vswitchd is running.
> -if test X$FULL_HOSTNAME = Xyes; then
> -hn="$(hostname -f)" || hn="$(uname -n)"
> -else
> -hn="$(uname -n)"
> -fi
> -ovs_vsctl set Open_vSwitch . external-ids:hostname="$hn"
> -}
> -
> set_system_ids () {
> set ovs_vsctl set Open_vSwitch .
> 
> @@ -225,7 +214,6 @@ start_forwarding () {
> if test X"$OVS_VSWITCHD" = Xyes; then
> do_start_forwarding || return 1
> fi
> -set_hostname &
> return 0
> }
> 
> --
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] Add support for DHCP options 35 and 38

2020-04-23 Thread Flavio Fernandes

Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>


> On Apr 23, 2020, at 10:32 AM, lmart...@redhat.com wrote:
> 
> From: Lucas Alvares Gomes 
> 
> This patch is adding support for two new DHCP options:
> 
> * Option 35, ARP timeout:

One nit: call it "ARP Cache Timeout" as stated in rfc1232

I would add 'cache' throughout, to be consistent with rfc.

>  http://www.networksorcery.com/enp/protocol/bootp/option035.htm
> * Option 38, TCP Keepalive interval:
>  http://www.networksorcery.com/enp/protocol/bootp/option038.htm
> 
> I noticed that these two options were missing in OVN while working in
> DHCP related code for OpenStack.
> 
> Signed-off-by: Lucas Alvares Gomes 
> ---
> lib/ovn-l7.h|  4 
> northd/ovn-northd.c |  2 ++
> ovn-nb.xml  | 14 ++
> tests/ovn.at|  6 +++---
> tests/test-ovn.c|  2 ++
> 5 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
> index cbea2a0c8..10569264a 100644
> --- a/lib/ovn-l7.h
> +++ b/lib/ovn-l7.h
> @@ -82,6 +82,10 @@ struct gen_opts_map {
> #define DHCP_OPT_TFTP_SERVER_ADDRESS \
> DHCP_OPTION("tftp_server_address", 150, "ipv4")
> 
> +#define DHCP_OPT_ARP_TIMEOUT DHCP_OPTION("arp_timeout", 35, "uint32")
> +#define DHCP_OPT_TCP_KEEPALIVE_INTERVAL \
> +DHCP_OPTION("tcp_keepalive_interval", 38, "uint32")
> +
> static inline uint32_t
> gen_opt_hash(char *opt_name)
> {
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index c4675bd68..d790f9152 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -11300,6 +11300,8 @@ static struct gen_opts_map supported_dhcp_opts[] = {
> DHCP_OPT_PATH_PREFIX,
> DHCP_OPT_TFTP_SERVER_ADDRESS,
> DHCP_OPT_DOMAIN_NAME,
> +DHCP_OPT_ARP_TIMEOUT,

DHCP_OPT_ARP_CACHE_TIMEOUT 

> +DHCP_OPT_TCP_KEEPALIVE_INTERVAL,
> };
> 
> static struct gen_opts_map supported_dhcpv6_opts[] = {
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 163dd12ee..6bf54b1f5 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2852,6 +2852,19 @@
>   client begins trying to rebind its address.  The DHCPv4 option code
>   for this option is 59.
> 
> +
> + +type='{"type": "integer", "minInteger": 0, "maxInteger": 
> 255}'>
> +  The DHCPv4 option code for this option is 35. This option
> +  specifies the timeout in seconds for ARP cache entries.
> +
> +
> + +type='{"type": "integer", "minInteger": 0, "maxInteger": 
> 255}'>
> +  The DHCPv4 option code for this option is 38. This option
> +  specifies the interval that the client TCP should wait before
> +  sending a keepalive message on a TCP connection.
> +
>   
> 
>   
> @@ -2898,6 +2911,7 @@
> resolving hostnames via the Domain Name System.
>   
> 
> +
>   
> 
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 2a7ee7528..6e1436f97 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1188,9 +1188,9 @@ reg1[0] = put_dhcp_opts(offerip = 1.2.3.4, router = 
> 10.0.0.1);
> reg2[5] = 
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="ovn.org",wpad="https://example.org",bootfile_name="https://127.0.0.1/boot.ipxe",path_prefix="/tftpboot;);
> formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router = 10.0.0.1, 
> netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org", wpad = 
> "https://example.org;, bootfile_name = "https://127.0.0.1/boot.ipxe;, 
> path_prefix = "/tftpboot");
> encodes as 
> controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.fe.00.1a.02.05.78.0f.07.6f.76.6e.2e.6f.72.67.fc.13.68.74.74.70.73.3a.2f.2f.65.78.61.6d.70.6c.65.2e.6f.72.67.43.1b.68.74.74.70.73.3a.2f.2f.31.32.37.2e.30.2e.30.2e.31.2f.62.6f.6f.74.2e.69.70.78.65.d2.09.2f.74.66.74.70.62.6f.6f.74,pause)
> -reg0[15] = 
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},classless_static_route={30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,0.0.0.0/0,10.0.0.1},ethernet_encap=1,router_discovery=0,tftp_server_address={10.0.0.4,10.0.0.5});
> -formats as reg0[15] = put_dhcp_opts(offerip = 10.0.0.4, router = 
> 10.0.0.1, netmask = 255.255.255.0, mtu = 1400, ip_forward_enable = 1, 
> default_ttl = 121, dns_server = {8.8.8.8, 7.7.7.7}, class

[ovs-dev] [PATCH ovn v1] tests: Fix failures in 4 HV, 1 LS, 1 LR, packet test with HA dist rtr gw prt

2020-04-23 Thread Flavio Fernandes

The test case "76: ovn -- 4 HV, 1 LS, 1 LR, packet test with HA
distributed router gateway port" fails sometimes. This patch fixes
this by leveraging the command ovn-nbctl sync. It also adds a
delay before sending a packet between foo1 and outside1, so it does
not get out of order with the gratuitous arp broadcast.

The failed test would look like this:
  
  checking packets in ext1/vif1-tx.pcap against ext1-vif1.expected:
  ovn.at:12: waiting until $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" 
$rcv_pcap > $rcv_text
rcv_n=`wc -l < "$rcv_text"`
echo "rcv_n=$rcv_n exp_n=$exp_n"
test $rcv_n -ge $exp_n...
  
  ovn.at:12: wait succeeded after 2 seconds
  ./ovn.at:8865: sort $rcv_text
  expout2020-04-22 15:24:29.948703642 -0400
  /home/ffernand/work/ovn/tests/testsuite.dir/at-groups/76/stdout   
2020-04-22 15:24:29.948703642 -0400
 -1,2 +1,2
  
-f0010204020102030800451c3f110100c0a80102ac10010300350008
  
+020102030806000108000604000102010203ac100101ac100101
   
020102030806000108000604000102010203ac100101ac100101
  76. ovn.at:8695: 76. ovn -- 4 HV, 1 LS, 1 LR, packet test with HA distributed 
router gateway port (ovn.at:8695): FAILED (ovn.at:8865)

Reported-by: Dumitru Ceara 
Signed-off-by: Flavio Fernandes 
---
 tests/ovn.at | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 2a7ee7528..149345101 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8818,8 +8818,7 @@ as ext1 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 AT_CHECK([ovn-nbctl --timeout=3 --wait=sb sync], [0], [ignore])
 
 # Allow some time for ovn-northd and ovn-controller to catch up.
-# XXX This should be more systematic.
-sleep 2
+ovn-nbctl --wait=hv sync
 
 ip_to_hex() {
 printf "%02x%02x%02x%02x" "$@"
@@ -8886,6 +8885,8 @@ grep actions=mod_dl_dst:f0:00:00:01:02:04 | wc -l` -eq 1
 fi
 as ext1 reset_pcap_file ext1-vif1 ext1/vif1
 
+sleep 1
+
 # Resend packet from foo1 to outside1
 as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
 
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v4] tests: Fix failures in 1 LR with distributed router gateway port

2020-04-22 Thread Flavio Fernandes

The test case "78: ovn -- 1 LR with distributed router gateway port"
fails sometimes. This patch fixes this by leveraging the command
ovn-nbctl sync

The failed test would look like this:
   
   grep "Port patch-br-int-to-ln-alice" | wc -l`...
   ovn.at:9356: wait succeeded immediately

  checking packets in hv3/vif1-tx.pcap against hv3-vif1.expected:
  ovn.at:12: waiting until $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" 
$rcv_pcap > $rcv_text
  rcv_n=`wc -l < "$rcv_text"`
  echo "rcv_n=$rcv_n exp_n=$exp_n"
  test $rcv_n -ge $exp_n...
  rcv_n=0 exp_n=2
  rcv_n=0 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=2 exp_n=2
  ovn.at:12: wait succeeded after 5 seconds
  ./ovn.at:9367: sort $rcv_text
  expout  2020-04-17 20:17:08.255531551 +
  /home/vagrant/ovn/tests/testsuite.dir/at-groups/78/stdout   2020-04-17 
20:17:08.258531517 +
 -1,2 +1,2
  
-f0010204020102030806000108000604000202010203ac100101f0010204ac100103
  
+020102030806000108000604000102010203ac100101ac100101
   
020102030806000108000604000102010203ac100101ac100101
  78. ovn.at:9139: 78. ovn -- 1 LR with distributed router gateway port 
(ovn.at:9139): FAILED (ovn.at:9367)

Signed-off-by: Flavio Fernandes 
---
V4:
- Address Dumitru's comments:
 - Use test name instead of number because number may change
 - "--wait=hv" should be enough because it implies "--wait=sb".
 - skip the "--timeout=3" because OVS_CTL_TIMEOUT is set when
running tests and its value will be used as default.
V3:
- Nits
V2:
- Nits
---
 tests/ovn.at | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 2a7ee7528..e5222a1d6 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9268,9 +9268,8 @@ as hv1 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
 
 
-# Allow some time for ovn-northd and ovn-controller to catch up.
-# XXX This should be more systematic.
-sleep 2
+dnl Allow some time for ovn-northd and ovn-controller to catch up.
+ovn-nbctl --wait=hv sync
 
 echo "-NB dump-"
 ovn-nbctl show
@@ -9385,6 +9384,9 @@ as hv2 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
 grep "Port patch-br-int-to-ln-alice" | wc -l`])
 
+dnl Allow some time for ovn-controller to catch up.
+ovn-nbctl --wait=hv sync
+
 # ARP for router IP address from outside1
 test_arp 3 1 f0010204 $outside_ip $rtr_ip 02010203
 
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v1] NAT: port range cannot be stateless

2020-04-22 Thread Flavio Fernandes

Minor change to ovn-nbctl to prevent users from attempting
to use port range and stateless together. That is so
because port range uses conntrack to set the source port.

Signed-off-by: Flavio Fernandes 
---
 tests/ovn-nbctl.at| 4 
 utilities/ovn-nbctl.c | 6 ++
 2 files changed, 10 insertions(+)

diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 66fbcc748..637d88fcd 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -652,6 +652,10 @@ AT_CHECK([ovn-nbctl --portrange lr-nat-add lr0 
dnat_and_snat 40.0.0.6 192.168.1.
 [ovn-nbctl: invalid port range 0-10.
 ])
 
+AT_CHECK([ovn-nbctl --stateless --portrange lr-nat-add lr0 dnat_and_snat 
40.0.0.5 192.168.1.8 6], [1], [],
+[ovn-nbctl: --stateless and --portrange may not be used together
+])
+
 AT_CHECK([ovn-nbctl show lr0 | grep -c 'external port(s): "1-3000"'], [0], [dnl
 3
 ])
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index cb46d3aa5..95eb54bf3 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -4167,6 +4167,12 @@ nbctl_lr_nat_add(struct ctl_context *ctx)
 ctl_error(ctx, "stateless is not applicable to dnat or snat types");
 return;
 }
+/* Port range needs conntrack, so it can't be stateless. */
+if (stateless && is_portrange) {
+ctl_error(ctx, "--stateless and --portrange may not be used "
+  "together");
+return;
+}
 
 int is_snat = !strcmp("snat", nat_type);
 for (size_t i = 0; i < lr->n_nat; i++) {
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v3] tests: Fix occasional failures for test 78

2020-04-17 Thread Flavio Fernandes

The test case "78: ovn -- 1 LR with distributed router gateway port"
fails sometimes. This patch fixes this by leveraging the command
ovn-nbctl sync

The failed test would look like this:
   
   grep "Port patch-br-int-to-ln-alice" | wc -l`...
   ovn.at:9356: wait succeeded immediately

  checking packets in hv3/vif1-tx.pcap against hv3-vif1.expected:
  ovn.at:12: waiting until $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" 
$rcv_pcap > $rcv_text
  rcv_n=`wc -l < "$rcv_text"`
  echo "rcv_n=$rcv_n exp_n=$exp_n"
  test $rcv_n -ge $exp_n...
  rcv_n=0 exp_n=2
  
  rcv_n=2 exp_n=2
  ovn.at:12: wait succeeded after 5 seconds
  ./ovn.at:9367: sort $rcv_text
  expout  2020-04-17 20:17:08.255531551 +
  /home/vagrant/ovn/tests/testsuite.dir/at-groups/78/stdout   2020-04-17 
20:17:08.258531517 +
 -1,2 +1,2
  
-f0010204020102030806000108000604000202010203ac100101f0010204ac100103
  
+020102030806000108000604000102010203ac100101ac100101
   
020102030806000108000604000102010203ac100101ac100101
  78. ovn.at:9139: 78. ovn -- 1 LR with distributed router gateway port 
(ovn.at:9139): FAILED (ovn.at:9367)

Signed-off-by: Flavio Fernandes 
---
 tests/ovn.at | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index f83d3f536..e73772300 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9239,9 +9239,9 @@ as hv1 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
 
 
-# Allow some time for ovn-northd and ovn-controller to catch up.
-# XXX This should be more systematic.
-sleep 2
+dnl Allow some time for ovn-northd and ovn-controller to catch up.
+ovn-nbctl --wait=sb --timeout=3 sync
+ovn-nbctl --wait=hv --timeout=3 sync
 
 echo "-NB dump-"
 ovn-nbctl show
@@ -9356,6 +9356,10 @@ as hv2 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
 grep "Port patch-br-int-to-ln-alice" | wc -l`])
 
+dnl Allow some time for ovn-controller to catch up.
+ovn-nbctl --wait=hv --timeout=3 sync
+ovn-nbctl --wait=sb --timeout=3 sync
+
 # ARP for router IP address from outside1
 test_arp 3 1 f0010204 $outside_ip $rtr_ip 02010203
 
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2] tests: Fix occasional failures for test 78

2020-04-17 Thread Flavio Fernandes

The test case "78: ovn -- 1 LR with distributed router gateway port"
fails sometimes. This patch fixes this by leveraging the command
ovn-nbctl sync

The failed test would look like this:
   
   grep "Port patch-br-int-to-ln-alice" | wc -l`...
   ovn.at:9356: wait succeeded immediately

  checking packets in hv3/vif1-tx.pcap against hv3-vif1.expected:
  ovn.at:12: waiting until $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" 
$rcv_pcap > $rcv_text
  rcv_n=`wc -l < "$rcv_text"`
  echo "rcv_n=$rcv_n exp_n=$exp_n"
  test $rcv_n -ge $exp_n...
  rcv_n=0 exp_n=2
  rcv_n=0 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=1 exp_n=2
  rcv_n=2 exp_n=2
  ovn.at:12: wait succeeded after 5 seconds
  ./ovn.at:9367: sort $rcv_text
  expout  2020-04-17 20:17:08.255531551 +
  /home/vagrant/ovn/tests/testsuite.dir/at-groups/78/stdout   2020-04-17 
20:17:08.258531517 +
 -1,2 +1,2   
  
-f0010204020102030806000108000604000202010203ac100101f0010204ac100103
  
+020102030806000108000604000102010203ac100101ac100101
   
020102030806000108000604000102010203ac100101ac100101
  78. ovn.at:9139: 78. ovn -- 1 LR with distributed router gateway port 
(ovn.at:9139): FAILED (ovn.at:9367)


Signed-off-by: Flavio Fernandes 
---
 tests/ovn.at | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index f83d3f536..f95a7ba57 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9239,9 +9239,9 @@ as hv1 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
 
 
-# Allow some time for ovn-northd and ovn-controller to catch up.
-# XXX This should be more systematic.
-sleep 2
+dnl Allow some time for ovn-northd and ovn-controller to catch up.
+ovn-nbctl --wait=sb --timeout=3 sync
+ovn-nbctl --wait=hv --timeout=3 sync
 
 echo "-NB dump-"
 ovn-nbctl show
@@ -9356,6 +9356,9 @@ as hv2 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
 grep "Port patch-br-int-to-ln-alice" | wc -l`])
 
+dnl Allow some time for ovn-controller to catch up.
+ovn-nbctl --wait=sb --timeout=3 sync
+
 # ARP for router IP address from outside1
 test_arp 3 1 f0010204 $outside_ip $rtr_ip 02010203
 
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] tests: Fix occasional failures for test 78

2020-04-17 Thread Flavio Fernandes

The test case "78: ovn -- 1 LR with distributed router gateway port"
fails sometimes. This patch fixes this by leveraging the command
ovn-nbctl sync

The failed test would look like this:

grep "Port patch-br-int-to-ln-alice" | wc -l`...
ovn.at:9356: wait succeeded immediately

checking packets in hv3/vif1-tx.pcap against hv3-vif1.expected:
ovn.at:12: waiting until $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" $rcv_pcap 
> $rcv_text
rcv_n=`wc -l < "$rcv_text"`
echo "rcv_n=$rcv_n exp_n=$exp_n"
test $rcv_n -ge $exp_n...
rcv_n=0 exp_n=2
rcv_n=0 exp_n=2
rcv_n=1 exp_n=2
rcv_n=1 exp_n=2
rcv_n=1 exp_n=2
rcv_n=1 exp_n=2
rcv_n=2 exp_n=2
ovn.at:12: wait succeeded after 5 seconds
./ovn.at:9367: sort $rcv_text
--- expout  2020-04-17 20:17:08.255531551 +
+++ /home/vagrant/ovn/tests/testsuite.dir/at-groups/78/stdout   2020-04-17 
20:17:08.258531517 +
@@ -1,2 +1,2 @@
-f0010204020102030806000108000604000202010203ac100101f0010204ac100103
+020102030806000108000604000102010203ac100101ac100101
 
020102030806000108000604000102010203ac100101ac100101
78. ovn.at:9139: 78. ovn -- 1 LR with distributed router gateway port 
(ovn.at:9139): FAILED (ovn.at:9367)

Signed-off-by: Flavio Fernandes 

Signed-off-by: Flavio Fernandes 
---
 tests/ovn.at | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 040f7cff5..1861de14d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9239,9 +9239,9 @@ as hv1 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 as hv3 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
 
 
-# Allow some time for ovn-northd and ovn-controller to catch up.
-# XXX This should be more systematic.
-sleep 2
+dnl Allow some time for ovn-northd and ovn-controller to catch up.
+ovn-nbctl --wait=sb --timeout=3 sync
+ovn-nbctl --wait=hv --timeout=3 sync
 
 echo "-NB dump-"
 ovn-nbctl show
@@ -9356,6 +9356,9 @@ as hv2 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
 OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
 grep "Port patch-br-int-to-ln-alice" | wc -l`])
 
+dnl Allow some time for ovn-controller to catch up.
+ovn-nbctl --wait=sb --timeout=3 sync
+
 # ARP for router IP address from outside1
 test_arp 3 1 f0010204 $outside_ip $rtr_ip 02010203
 
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] tests: Fix occasional failures for test 85.

2020-04-17 Thread Flavio Fernandes

Tested-by: Flavio Fernandes 



> On Apr 17, 2020, at 10:57 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> The test case "85: ovn -- send gratuitous ARP for NAT rules on HA distributed 
> router"
> fails occaionally. On faster systems, chances of failure are higher.
> 
> This patch fixes this.
> 
> Signed-off-by: Numan Siddique 
> ---
> tests/ovn.at | 21 +++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f83d3f536..040f7cff5 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11035,6 +11035,12 @@ only_broadcast_from_lrp1() {
> garp="f00108060001080006040001f001c0a80064c0a80064"
> echo $garp > expout
> 
> +OVS_WAIT_UNTIL(
> +[$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > 
> rcv_text
> + exp_rcvd=$(cat rcv_text | grep $garp | wc -l)
> + echo "expected received = $exp_rcvd"
> + test $exp_rcvd -ge 1])
> +
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros 
> | only_broadcast_from_lrp1 | uniq > hv1_snoop_tx
> echo "packets on hv1-snoopvif:"
> cat hv1_snoop_tx
> @@ -11063,12 +11069,17 @@ as hv1 reset_pcap_file snoopvif hv1/snoopvif
> as hv2 reset_pcap_file br-phys_n1 hv2/br-phys_n1
> as hv3 reset_pcap_file br-phys_n1 hv3/br-phys_n1
> 
> -# Wait for packets to be received.
> -OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 100])
> trim_zeros() {
> sed 's/\(00\)\{1,\}$//'
> }
> 
> +# Wait for packets to be received.
> +OVS_WAIT_UNTIL(
> +[$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > 
> rcv_text
> + exp_rcvd=$(cat rcv_text | grep $garp | wc -l)
> + echo "expected received = $exp_rcvd"
> + test $exp_rcvd -ge 1])
> +
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros 
> | only_broadcast_from_lrp1 | uniq >  hv1_snoopvif_tx
> AT_CHECK([sort hv1_snoopvif_tx], [0], [expout])
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv3/br-phys_n1-tx.pcap | 
> trim_zeros | only_broadcast_from_lrp1 | uniq > hv3_br_phys_tx
> @@ -4,6 +11125,12 @@ trim_zeros() {
> garp="f001810007de08060001080006040001f001c0a80064c0a80064"
> echo $garp > expout
> 
> +OVS_WAIT_UNTIL(
> +[$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > 
> rcv_text
> + exp_rcvd=$(cat rcv_text | grep $garp | wc -l)
> + echo "expected received = $exp_rcvd"
> + test $exp_rcvd -ge 1])
> +
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros 
> | only_broadcast_from_lrp1 | uniq >  hv1_snoopvif_tx
> AT_CHECK([sort hv1_snoopvif_tx], [0], [expout])
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv3/br-phys_n1-tx.pcap | 
> trim_zeros | only_broadcast_from_lrp1 | uniq > hv3_br_phys_tx
> -- 
> 2.25.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] tests: Fix occasional failures for test 85.

2020-04-17 Thread Flavio Fernandes

Tested-by: Flavio Fernandes mailto:fla...@flaviof.com>>



> On Apr 17, 2020, at 10:57 AM, num...@ovn.org wrote:
> 
> From: Numan Siddique 
> 
> The test case "85: ovn -- send gratuitous ARP for NAT rules on HA distributed 
> router"
> fails occaionally. On faster systems, chances of failure are higher.
> 
> This patch fixes this.
> 
> Signed-off-by: Numan Siddique 
> ---
> tests/ovn.at | 21 +++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f83d3f536..040f7cff5 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11035,6 +11035,12 @@ only_broadcast_from_lrp1() {
> garp="f00108060001080006040001f001c0a80064c0a80064"
> echo $garp > expout
> 
> +OVS_WAIT_UNTIL(
> +[$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > 
> rcv_text
> + exp_rcvd=$(cat rcv_text | grep $garp | wc -l)
> + echo "expected received = $exp_rcvd"
> + test $exp_rcvd -ge 1])
> +
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros 
> | only_broadcast_from_lrp1 | uniq > hv1_snoop_tx
> echo "packets on hv1-snoopvif:"
> cat hv1_snoop_tx
> @@ -11063,12 +11069,17 @@ as hv1 reset_pcap_file snoopvif hv1/snoopvif
> as hv2 reset_pcap_file br-phys_n1 hv2/br-phys_n1
> as hv3 reset_pcap_file br-phys_n1 hv3/br-phys_n1
> 
> -# Wait for packets to be received.
> -OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 100])
> trim_zeros() {
> sed 's/\(00\)\{1,\}$//'
> }
> 
> +# Wait for packets to be received.
> +OVS_WAIT_UNTIL(
> +[$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > 
> rcv_text
> + exp_rcvd=$(cat rcv_text | grep $garp | wc -l)
> + echo "expected received = $exp_rcvd"
> + test $exp_rcvd -ge 1])
> +
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros 
> | only_broadcast_from_lrp1 | uniq >  hv1_snoopvif_tx
> AT_CHECK([sort hv1_snoopvif_tx], [0], [expout])
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv3/br-phys_n1-tx.pcap | 
> trim_zeros | only_broadcast_from_lrp1 | uniq > hv3_br_phys_tx
> @@ -4,6 +11125,12 @@ trim_zeros() {
> garp="f001810007de08060001080006040001f001c0a80064c0a80064"
> echo $garp > expout
> 
> +OVS_WAIT_UNTIL(
> +[$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > 
> rcv_text
> + exp_rcvd=$(cat rcv_text | grep $garp | wc -l)
> + echo "expected received = $exp_rcvd"
> + test $exp_rcvd -ge 1])
> +
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap | trim_zeros 
> | only_broadcast_from_lrp1 | uniq >  hv1_snoopvif_tx
> AT_CHECK([sort hv1_snoopvif_tx], [0], [expout])
> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv3/br-phys_n1-tx.pcap | 
> trim_zeros | only_broadcast_from_lrp1 | uniq > hv3_br_phys_tx
> -- 
> 2.25.1
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v1 0/1] NAT: Enhancements to external port range support

2020-04-15 Thread Flavio Fernandes

Hi folks,

It took me long enough, but I finally got to sit down and look at the nice
changes that Ankur Sharma did to make OVN capable of providing a port range
in the OVN NB schema. Trouble is, the changes are already merged, so I
decided to submit this follow up a patch with the changes that would
otherwise be my review comments. I hope it is not too late. :^)

Among the comments, I really thought it would be important to provide show
output of the configuration for the external port range. That turned out to
be the bulk of the changes being proposed here.

Examples of how the output of NAT with external port range looks like:

[root@ovn-central /]# sudo ovn-nbctl show lr0
router 1d409abe-751f-4251-a6af-5a6858a10d7c (lr0)

nat 2618d410-1d73-4ff3-8164-744ff1709e3d
external ip: "172.16.0.100"
logical ip: "20.0.0.0/24"
type: "snat"
nat 598b6495-928a-4053-b2ae-f2e8d4ae31f5
external ip: "172.16.0.110"
external port(s): "8080" <=== NEW
logical ip: "10.0.0.3"
type: "dnat_and_snat"
nat fa9c5f8c-1c42-44a0-b9c5-3a4fb799c75d
external ip: "172.16.0.100"
external port(s): "123-456"  <=== NEW
logical ip: "10.0.0.0/24"
type: "snat"

[root@ovn-central /]# ovn-nbctl lr-nat-list lr0
TYPE EXTERNAL_IPEXTERNAL_PORTLOGICAL_IP
EXTERNAL_MAC LOGICAL_PORT
dnat_and_snat172.16.0.110   8080 10.0.0.3  
30:54:00:00:00:03sw0-port1
snat 172.16.0.100   123-456  10.0.0.0/24
snat 172.16.0.10020.0.0.0/24



Flavio Fernandes (1):
  NAT: Enhancements to external port range support

 NEWS  |   1 +
 lib/actions.c |   5 +-
 northd/ovn-northd.c   |   8 +--
 ovn-nb.xml|   2 +-
 tests/ovn-nbctl.at| 130 ++
 tests/ovn.at  |   4 +-
 utilities/ovn-nbctl.c |  27 ++---
 7 files changed, 112 insertions(+), 65 deletions(-)

-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v1 1/1] NAT: Enhancements to external port range support

2020-04-15 Thread Flavio Fernandes

This change is a mix of minor fixes to the external port range
support recently merged to OVN [1], as well as some enhancements.

- Added external port range column to ovn-nbctl lr-nat-list output;
- Added external port range to NAT section of "ovn-nbctl show" (if needed);
- Minor fix to is_valid_port_range() checker, to catch cases when string
  with multiple '-' tokens are provided. An example would be "12-34-56";
- Minor fix to actions parser, to ensure that value "0" is not allowed;
- Updated tests as needed to account for the changes mentioned above;
- Added line in NEWS to mention about this update to the NB schema;
- Minor typo in description of external port range;
- Instead of using "if (strlen(str))", use "if (str[0])" code convention.

[1]: https://patchwork.ozlabs.org/project/openvswitch/list/?series=169084

Signed-off-by: Flavio Fernandes 
---
 NEWS  |   1 +
 lib/actions.c |   5 +-
 northd/ovn-northd.c   |   8 +--
 ovn-nb.xml|   2 +-
 tests/ovn-nbctl.at| 130 ++
 tests/ovn.at  |   4 +-
 utilities/ovn-nbctl.c |  27 ++---
 7 files changed, 112 insertions(+), 65 deletions(-)

diff --git a/NEWS b/NEWS
index 623c8810e..765b79f1c 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
 Post-v20.03.0
 --
+   - Added support for external_port_range in NAT table.
 
 
 OVN v20.03.0 - xx xxx 
diff --git a/lib/actions.c b/lib/actions.c
index b3bf98106..c19813de0 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -779,6 +779,9 @@ parse_ct_nat(struct action_context *ctx, const char *name,
}
 
cn->port_range.port_lo = ntohll(ctx->lexer->token.value.integer);
+   if (cn->port_range.port_lo == 0) {
+   lexer_syntax_error(ctx->lexer, "range can't be 0");
+   }
lexer_get(ctx->lexer);
 
if (lexer_match(ctx->lexer, LEX_T_HYPHEN)) {
@@ -792,7 +795,7 @@ parse_ct_nat(struct action_context *ctx, const char *name,
 
if (cn->port_range.port_hi <= cn->port_range.port_lo) {
lexer_syntax_error(ctx->lexer, "range high should be "
-  "greater than range lo");
+  "greater than range low");
}
lexer_get(ctx->lexer);
} else {
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 198028c50..25fca20e1 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -8917,7 +8917,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
 ds_put_format(, "flags.loopback = 1; "
   "ct_dnat(%s", nat->logical_ip);
 
-if (strlen(nat->external_port_range)) {
+if (nat->external_port_range[0]) {
 ds_put_format(, ",%s",
   nat->external_port_range);
 }
@@ -8950,7 +8950,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
   is_v6 ? "6" : "4", nat->logical_ip);
 } else {
 ds_put_format(, "ct_dnat(%s", nat->logical_ip);
-if (strlen(nat->external_port_range)) {
+if (nat->external_port_range[0]) {
 ds_put_format(, ",%s",
   nat->external_port_range);
 }
@@ -9061,7 +9061,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
 ds_put_format(, "ct_snat(%s",
   nat->external_ip);
 
-if (strlen(nat->external_port_range)) {
+if (nat->external_port_range[0]) {
 ds_put_format(, ",%s",
   nat->external_port_range);
 }
@@ -9104,7 +9104,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
 } else {
 ds_put_format(, "ct_snat(%s",
   nat->external_ip);
-if (strlen(nat->external_port_range)) {
+if (nat->external_port_range[0]) {
 ds_put_format(, ",%s",
   nat->external_port_range);
 }
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 4dfdffdd7..163dd12ee 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2579,7 +2579,7 @@
   
 
   
-Range of port, from

[ovs-dev] [PATCH ovn v1] vagrant: Increase memory sizes

2020-01-30 Thread Flavio Fernandes

Using default memory sizes is not enough to run some of the tests
invoked by make check. Being so, This change bumps the default
memory size of boxes on Virtualbox and Libvirt providers
to 1Gb and 4 CPUs. This value can be changed by setting the
environment variables VM_MEMORY and VM_CPUS, respectively.

   debian-10: OVN components
   debian-10:
   debian-10:  15: ovn -- 4-term numeric expression normalization
   debian-10:  FAILED (ovn.at:497)
   debian-10:  18: ovn -- 5-term numeric expression normalization
   debian-10:  FAILED (ovn.at:515)
   debian-10:  19: ovn -- 5-term string expression normalization
   debian-10:  FAILED (ovn.at:521)
   debian-10:
   debian-10: OVN end-to-end tests
   debian-10:
   debian-10: 126: ovn -- interconnection
   debian-10:  FAILED (ovn.at:18133)

dmesg:

[ 2303.348732] Out of memory: Kill process 10068 (ovstest) score 821 or 
sacrifice child
[ 2303.350508] Killed process 10068 (ovstest) total-vm:419844kB, 
anon-rss:405848kB, file-rss:0kB, shmem-rss:0kB
[ 2303.369933] oom_reaper: reaped process 10068 (ovstest), now anon-rss:0kB, 
file-rss:0kB, shmem-rss:0kB
root@debian-10:/vagrant/ovn/tests/testsuite.dir/015#

Signed-off-by: Flavio Fernandes 
---
 Vagrantfile | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/Vagrantfile b/Vagrantfile
index f0526ec48..6a3f32010 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -102,6 +102,17 @@ make check RECHECK=yes || {
 SCRIPT
 
 Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
+  vm_memory = ENV['VM_MEMORY'] || '1024'
+  vm_cpus = ENV['VM_CPUS'] || '4'
+  config.vm.provider 'libvirt' do |lb|
+lb.memory = vm_memory
+lb.cpus = vm_cpus
+  end
+  config.vm.provider "virtualbox" do |vb|
+vb.memory = vm_memory
+vb.cpus = vm_cpus
+  end
+
   config.vm.define "debian-10" do |debian|
debian.vm.hostname = "debian-10"
debian.vm.box = "debian/buster64"
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v1] ovn-openstack.rst: Account for networking-ovn-migration

2020-01-29 Thread Flavio Fernandes

The networking-ovn repo has been migrated into Neutron [0]
as of Ussuri release. This change implements the necessary
updates to the OVN OpenStack tutorial.

Other minor changes here include commands needed to make
Devstack work with Centos 7, as well as the removal of
workarounds that are no longer needed.

[0]: 
https://review.opendev.org/#/c/658414/19/specs/ussuri/ml2ovs-ovn-convergence.rst

Signed-off-by: Flavio Fernandes 
---
 Documentation/tutorials/ovn-openstack.rst | 51 ---
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/Documentation/tutorials/ovn-openstack.rst 
b/Documentation/tutorials/ovn-openstack.rst
index 3ef052396..2e4f63404 100644
--- a/Documentation/tutorials/ovn-openstack.rst
+++ b/Documentation/tutorials/ovn-openstack.rst
@@ -43,7 +43,7 @@ potential users to understand how OVN works and how to debug 
and
 troubleshoot it.
 
 In addition to new material, this tutorial incorporates content from
-``testing.rst`` in OpenStack networking-ovn, by Russell Bryant and
+``ovn_devstack.rst`` in OpenStack neutron, by Russell Bryant and
 others.  Without that example, this tutorial could not have been
 written.
 
@@ -60,7 +60,7 @@ packaging for developers, in a way that allows you to follow 
along
 with the tutorial in full.
 
 Unless you have a spare computer laying about, it's easiest to install
-DevStacck in a virtual machine.  This tutorial was built using a VM
+DevStack in a virtual machine.  This tutorial was built using a VM
 implemented by KVM and managed by virt-manager.  I recommend
 configuring the VM configured for the x86-64 architecture, 6 GB RAM, 2
 VCPUs, and a 20 GB virtual disk.
@@ -102,7 +102,7 @@ Here are step-by-step instructions to get started:
 
 1. Install a VM.
 
-   I tested these instructions with Centos 7.3.  Download the "minimal
+   I tested these instructions with Centos 7.6.  Download the "minimal
install" ISO and booted it.  The install is straightforward.  Be
sure to enable networking, and set a host name, such as
"ovn-devstack-1".  Add a regular (non-root) user, and check the box
@@ -160,6 +160,13 @@ Here are step-by-step instructions to get started:
 
.. note::
 
+  Support for `Centos 7 in Devstack 
<https://review.opendev.org/#/c/688799/>`_
+  is going away, but you can still use it. Especially while Centos 8 
support
+  is not finished. The one important caveat for making Centos 7 work with 
Devstack
+  is that you will explicitly have to install these packages as well::
+
+   $ sudo yum install python3 python3-devel
+
   If you installed a 32-bit i386 guest (against the advice above),
   install a non-PAE kernel and reboot into it at this point::
 
@@ -169,12 +176,12 @@ Here are step-by-step instructions to get started:
   Be sure to select the non-PAE kernel from the list at boot.
   Without this step, DevStack will fail to install properly later.
 
-3. Get copies of DevStack and OVN and set them up::
+3. Get copies of DevStack and Neutron and set them up::
 
- $ git clone http://git.openstack.org/openstack-dev/devstack.git
- $ git clone http://git.openstack.org/openstack/networking-ovn.git
+ $ git clone https://git.openstack.org/openstack-dev/devstack.git
+ $ git clone https://git.openstack.org/openstack/neutron.git
  $ cd devstack
- $ cp ../networking-ovn/devstack/local.conf.sample local.conf
+ $ cp ../neutron/devstack/ovn-local.conf.sample local.conf
 
.. note::
 
@@ -219,16 +226,7 @@ Here are step-by-step instructions to get started:
the alternative command-line interfaces because they are easier to
explain and to cut and paste.
 
-5. As of this writing, you need to run the following to fix a problem
-   with using VM consoles from the OpenStack web instance::
-
- $ (cd /opt/stack/noVNC && git checkout v0.6.0)
-
-   See
-   
https://serenity-networks.com/how-to-fix-setkeycodes-00-and-unknown-key-pressed-console-errors-on-openstack/
-   for more details.
-
-6. The firewall in the VM by default allows SSH access but not HTTP.
+5. The firewall in the VM by default allows SSH access but not HTTP.
You will probably want HTTP access to use the OpenStack web
interface.  The following command enables that.  (It also enables
every other kind of network access, so if you're concerned about
@@ -240,7 +238,7 @@ Here are step-by-step instructions to get started:
 
(You need to re-run this if you reboot the VM.)
 
-7. To use OpenStack command line utilities in the tutorial, run::
+6. To use OpenStack command line utilities in the tutorial, run::
 
  $ . ~/devstack/openrc admin
 
@@ -1331,20 +1329,6 @@ with an IP address from the "private" network, then we 
create a
 floating IP address on the "public" network, then we associate the
 port with the floating IP address.
 
-As of this writing, you may need to run the following to fix a
-problem with asso

Re: [ovs-dev] [PATCH 2/3] ovn-controller.c: Move the position of handling OVN-SB related settings.

2020-01-15 Thread Flavio Fernandes




> On Jan 14, 2020, at 5:34 PM, Han Zhou  wrote:
> 
> On Tue, Jan 14, 2020 at 1:24 PM Mark Michelson  wrote:
>> 
>> The commit message doesn't make much sense to me. The external-ids are
>> set outside of ovn-controller, so the concept of them being handled in
>> "the same iteration" or "the next one" only works if ovn-controller is
>> setting them at some point in the loop.
>> 
> Maybe the commit message is not clear enough. Let me explain with more
> details.
> In each iteration, the OVS IDL's data is updated AFTER the line:
>struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(_idl_loop);
> 
> Without this patch, it (the lines that are moved) applies the settings
> before reading the new settings. So if a change is made to external-ids,
> e.g. ovn-remote-db, the loop will be waked up because of the OVSDB RPC
> messages, but it won't apply any of the new settings. The new settings will
> be applied only if there is another event coming to wake the loop, e.g.
> probe interval timeout. In my testing I saw the change took effect after 5
> seconds when probe interval timed out. If probe was disabled, it would
> never got applied unless a new change is made. I suspect the problem
> reported here [0] may due to the same reason.
> 
> [0]
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-January/049695.html
> 
>> Couldn't this have a negative effect on the first iteration of the loop?
>> If we don't set SSL parameters or the sb remote first, then we will have
>> errors when attempting to connect to the southbound database.
>> 
> 
> At the first iteration, it just make sure the OVS IDL data is refreshed
> before setting the SSL parameters. We are still setting the parameters. The
> patch doesn't skip anything.
> 
>> On 1/13/20 5:52 PM, Han Zhou wrote:
>>> Move the logic of handling OVN-SB related setting in external-ids
>>> after the ovs_idl_loop run, so that any change in the external-ids
>>> settings can take effect in the same iteration, without waiting for
>>> the next one.
>>> 
>>> Signed-off-by: Han Zhou 
>>> ---
>>>  controller/ovn-controller.c | 8 
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>> index 3d843f7..abb1b4c 100644
>>> --- a/controller/ovn-controller.c
>>> +++ b/controller/ovn-controller.c
>>> @@ -2012,10 +2012,6 @@ main(int argc, char *argv[])
>>>  while (!exiting) {
>>>  engine_init_run();
>>> 
>>> -update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
>>> -update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
>>> -
> ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
>>> -
>>>  struct ovsdb_idl_txn *ovs_idl_txn =
> ovsdb_idl_loop_run(_idl_loop);
>>>  unsigned int new_ovs_cond_seqno
>>>  = ovsdb_idl_get_condition_seqno(ovs_idl_loop.idl);
>>> @@ -2027,6 +2023,10 @@ main(int argc, char *argv[])
>>>  ovs_cond_seqno = new_ovs_cond_seqno;
>>>  }
>>> 
>>> +update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
>>> +update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
>>> +
> ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
>>> +
>>>  struct ovsdb_idl_txn *ovnsb_idl_txn
>>>  = ovsdb_idl_loop_run(_idl_loop);
>>>  unsigned int new_ovnsb_cond_seqno
>>> 
>> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


This change addresses the issue of setting the tunnel as Lars discovered.
I needs to be back merged to 2.12 as well.


Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-January/049695.html 
<https://mail.openvswitch.org/pipermail/ovs-discuss/2020-January/049695.html>
Tested-by: Flavio Fernandes 



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] [RFC] ovsdb-server: Allow OVSDB clients to specify the UUID for inserted rows.

2020-01-09 Thread Flavio Fernandes




> On Aug 26, 2019, at 5:00 PM, Ben Pfaff  wrote:
> 
> Requested-by: Leonid Ryzhyk 
> Signed-off-by: Ben Pfaff 
> ---
> Documentation/ref/ovsdb-server.7.rst |  9 +
> NEWS |  3 ++-
> ovsdb/execution.c| 26 ++
> ovsdb/transaction.c  | 22 +-
> ovsdb/transaction.h  |  5 -
> tests/ovsdb-execution.at | 15 +++
> tests/uuidfilt.py| 18 --
> 7 files changed, 89 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/ref/ovsdb-server.7.rst 
> b/Documentation/ref/ovsdb-server.7.rst
> index bc533611cb4a..967761bdfb84 100644
> --- a/Documentation/ref/ovsdb-server.7.rst
> +++ b/Documentation/ref/ovsdb-server.7.rst
> @@ -546,6 +546,15 @@ condition can be either a 3-element JSON array as 
> described in the RFC or a
> boolean value. In case of an empty array an implicit true boolean value will 
> be
> considered.
> 
> +5.2.1 Insert
> +
> +
> +As an extension, Open vSwitch 2.13 and later allow an optional ``uuid`` 
> member
> +to specify the UUID for the new row.  The specified UUID must be unique 
> within
> +the table when it is inserted and not the UUID of a row previously deleted
> +within the transaction.  If the UUID violates these rules, then the operation
> +fails with a ``duplicate uuid`` error.
> +
> 5.2.6 Wait, 5.2.7 Commit, 5.2.9 Comment
> ---
> 
> diff --git a/NEWS b/NEWS
> index c5caa13d6374..7cda1e91a138 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,6 +1,7 @@
> Post-v2.12.0
> -
> -
> +   - ovsdb-server: New OVSDB extension to allow clients to specify row UUIDs.
> + 
> 
> v2.12.0 - xx xxx 
> -
> diff --git a/ovsdb/execution.c b/ovsdb/execution.c
> index c55a0b771032..a8083da126dd 100644
> --- a/ovsdb/execution.c
> +++ b/ovsdb/execution.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2017 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2017, 2019 Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -329,11 +329,12 @@ ovsdb_execute_insert(struct ovsdb_execution *x, struct 
> ovsdb_parser *parser,
> {
> struct ovsdb_table *table;
> struct ovsdb_row *row = NULL;
> -const struct json *uuid_name, *row_json;
> +const struct json *uuid_json, *uuid_name, *row_json;
> struct ovsdb_error *error;
> struct uuid row_uuid;
> 
> table = parse_table(x, parser, "table");
> +uuid_json = ovsdb_parser_member(parser, "uuid", OP_STRING | OP_OPTIONAL);
> uuid_name = ovsdb_parser_member(parser, "uuid-name", OP_ID | OP_OPTIONAL);
> row_json = ovsdb_parser_member(parser, "row", OP_OBJECT);
> error = ovsdb_parser_get_error(parser);
> @@ -341,6 +342,19 @@ ovsdb_execute_insert(struct ovsdb_execution *x, struct 
> ovsdb_parser *parser,
> return error;
> }
> 
> +if (uuid_json) {
> +if (!uuid_from_string(_uuid, json_string(uuid_json))) {
> +return ovsdb_syntax_error(uuid_json, NULL, "bad uuid");
> +}
> +
> +if (!ovsdb_txn_may_create_row(table, _uuid)) {
> +return ovsdb_syntax_error(uuid_json, "duplicate uuid",
> +  "This UUID would duplicate a UUID "
> +  "already present within the table or "
> +  "deleted within the same 
> transaction.");
> +}
> +}
> +
> if (uuid_name) {
> struct ovsdb_symbol *symbol;
> 
> @@ -350,9 +364,13 @@ ovsdb_execute_insert(struct ovsdb_execution *x, struct 
> ovsdb_parser *parser,
>   "This \"uuid-name\" appeared on an "
>   "earlier \"insert\" operation.");
> }
> -row_uuid = symbol->uuid;
> +if (uuid_json) {
> +symbol->uuid = row_uuid;
> +} else {
> +row_uuid = symbol->uuid;
> +}
> symbol->created = true;
> -} else {
> +} else if (!uuid_json) {
> uuid_generate(_uuid);
> }
> 
> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
> index 866fabe8df57..369436bffbf5 100644
> --- a/ovsdb/transaction.c
> +++ b/ovsdb/transaction.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2017 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2017, 2019 
> Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -1287,6 +1287,26 @@ ovsdb_txn_row_delete(struct ovsdb_txn *txn, const 
> struct ovsdb_row *row_)
> }
> }
> 
> +/* Returns true if 'row_uuid' may be used as the UUID for a newly created row
> + * in 'table' (that is, that it is

[ovs-dev] [PATCH ovn v4] vagrant: Use centos-8 box from official location

2019-11-17 Thread Flavio Fernandes

From: Flavio Fernandes 

Prefer the usage of "vagrant box update" instead of
"dnf update" to save provisioning time.

Also, as part of provisioning centos-8, use pip to install
packages twisted, zope-interface and six.

Signed-off-by: Flavio Fernandes 
---
v2: Nit: Make dnf update line consistent with other images.
v3: Oops.
v4: Make 0-day robot happy: Fix signed-off.
---
 Vagrantfile | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index 88c981c71..f0526ec48 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -37,7 +37,7 @@ apt-get -y install build-essential fakeroot graphviz autoconf 
automake bzip2 \
 SCRIPT
 
 $bootstrap_ovs_centos7 = <

[ovs-dev] [PATCH ovn v3] vagrant: Use centos-8 box from official location

2019-11-17 Thread Flavio Fernandes

From: Flavio Fernandes 

Prefer the usage of "vagrant box update" instead of
"dnf update" to save provisioning time.

Also, as part of provisioning centos-8, use pip to install
packages twisted, zope-interface and six.

Signed-off-by: Flavio Fernandes 
---
 Vagrantfile | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index 88c981c71..f0526ec48 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -37,7 +37,7 @@ apt-get -y install build-essential fakeroot graphviz autoconf 
automake bzip2 \
 SCRIPT
 
 $bootstrap_ovs_centos7 = <

[ovs-dev] [PATCH ovn v2] vagrant: Use centos-8 box from official location

2019-11-17 Thread Flavio Fernandes

From: Flavio Fernandes 

Prefer the usage of "vagrant box update" instead of
"dnf update" to save provisioning time.

Also, as part of provisioning centos-8, use pip to install
packages twisted, zope-interface and six.

Signed-off-by: Flavio Fernandes 
---
v2: Nit: Make dnf update line consistent with other images.
---
 Vagrantfile | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index 88c981c71..f0526ec48 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -37,7 +37,7 @@ apt-get -y install build-essential fakeroot graphviz autoconf 
automake bzip2 \
 SCRIPT
 
 $bootstrap_ovs_centos7 = <

[ovs-dev] [PATCH ovn] vagrant: Use centos-8 box from official location

2019-11-17 Thread Flavio Fernandes

From: Flavio Fernandes 

Prefer the usage of "vagrant box update" instead of
"dnf update" to save provisioning time.

Also, as part of provisioning centos-8, use pip to install
packages twisted, zope-interface and six.

Signed-off-by: Flavio Fernandes 
---
 Vagrantfile | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index 88c981c71..321a1693f 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -37,7 +37,7 @@ apt-get -y install build-essential fakeroot graphviz autoconf 
automake bzip2 \
 SCRIPT
 
 $bootstrap_ovs_centos7 = <

[ovs-dev] [PATCH ovn v1] vagrant: Use python3 and newer linux distros

2019-11-03 Thread Flavio Fernandes

Stop usage of python2 as ovs+ovn no longer support it.
Update vagrant boxes to the following revisions:

- Debian: buster (from jessie)
- Fedora: v31 (from v29)
- Centos: 8 (from 7, kinda**)

Centos 7 may still be used, but only if explicitly
provided in command: 'vagrant up centos-7'

Fedora-31's dnf is not reliable. This patch
uses a max retry loop to improve the odds of success.

Not all tests are passing when doing 'make check' and
that is not related to this change [1].
While provisioning will invoke 'make check', failures will
be ignored based on this variable: exit_rc_when_failed.

The provisioning does not yet include building rpms+deb
packages.

[1]: https://github.com/ovn-org/ovn/issues/23

Signed-off-by: Flavio Fernandes 
---
Vagrantfile | 96 +
 1 file changed, 68 insertions(+), 28 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index 07ed0b0e0..88c981c71 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -6,41 +6,58 @@ VAGRANTFILE_API_VERSION = "2"
 Vagrant.require_version ">=1.7.0"
 
 $bootstrap_ovs_fedora = <

[ovs-dev] [PATCH ovn v2] Utilities: add ovn-sim

2019-10-10 Thread Flavio Fernandes

Adding a wrapper to ovs-sim called ovn-sim, which re-introduces the
primitives for OVN in the simulator that were lost as part of
the ovn-org/ovn split.

Also resurrected the man page used to document ovn-sim usage.

Lastly, there were some leftover ovs-sim references that were
not proper. This patch renames these to ovn-sim.

All in all, this change is nothing but porting over the
OVN sections of ovs-sim from OVS version 2.12 (pre-split).

Reported-by: Flavio Fernandes 
Reported-at: https://github.com/ovn-org/ovn/issues/21 (Fixes #21)
Signed-off-by: Flavio Fernandes 
---
v1 -> v2:
* Fix real issues found by 0-day Robot
---

 Documentation/automake.mk   |   2 +-
 Documentation/conf.py   |   4 +-
 Documentation/ref/index.rst |   2 +-
 Documentation/ref/ovn-sim.1.rst | 126 +
 Documentation/ref/ovs-sim.1.rst | 244 -
 Makefile.am |   1 +
 utilities/.gitignore|   1 +
 utilities/automake.mk   |   6 +-
 utilities/ovn-sim.in| 308 
 9 files changed, 445 insertions(+), 249 deletions(-)
 create mode 100644 Documentation/ref/ovn-sim.1.rst
 delete mode 100644 Documentation/ref/ovs-sim.1.rst
 create mode 100755 utilities/ovn-sim.in

diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index 5968d6941..bf2166349 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -106,7 +106,7 @@ RST_MANPAGES = \
 # rST formatted manpages that we don't want to install because they
 # document stuff that only works with a build tree, not with an
 # installed OVS.
-RST_MANPAGES_NOINST = ovs-sim.1.rst
+RST_MANPAGES_NOINST = ovn-sim.1.rst
 
 # The GNU standards say that these variables should control
 # installation directories for manpages in each section.  Automake
diff --git a/Documentation/conf.py b/Documentation/conf.py
index 6bf528bde..2c85dcf4d 100644
--- a/Documentation/conf.py
+++ b/Documentation/conf.py
@@ -114,8 +114,8 @@ html_static_path = ['_static']
 # One entry per manual page. List of tuples
 # (source start file, name, description, authors, manual section).
 _man_pages = [
-('ovs-sim.1',
- u'Open vSwitch simulator environment'),
+('ovn-sim.1',
+ u'Open Virtual Network simulator environment'),
 ('ovsdb-server.7',
  u'Open vSwitch Database Server Protocol'),
 ('ovsdb.5',
diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
index 530d639d6..7ff9edd09 100644
--- a/Documentation/ref/index.rst
+++ b/Documentation/ref/index.rst
@@ -39,7 +39,7 @@ time:
 .. toctree::
:maxdepth: 3
 
-   ovs-sim.1
+   ovn-sim.1
ovsdb-server.7
ovsdb.5
ovsdb.7
diff --git a/Documentation/ref/ovn-sim.1.rst b/Documentation/ref/ovn-sim.1.rst
new file mode 100644
index 0..35abd39b1
--- /dev/null
+++ b/Documentation/ref/ovn-sim.1.rst
@@ -0,0 +1,126 @@
+===
+ovn-sim
+===
+
+Synopsis
+
+
+``ovn-sim`` [*option*]... [*script*]...
+
+Description
+===
+
+``ovn-sim`` is a wrapper script that adds ovn related commands on
+top of ``ovs-sim``.
+
+``ovs-sim`` provides a convenient environment for running one or more Open
+vSwitch instances and related software in a sandboxed simulation environment.
+
+To use ``ovn-sim``, first build Open vSwitch, then invoke it directly from the
+build directory, e.g.::
+
+git clone https://github.com/openvswitch/ovs.git
+cd ovs
+./boot.sh && ./configure && make
+cd ..
+git clone https://github.com/ovn-org/ovn.git
+cd ovn
+./boot.sh && ./configure --with-ovs-source=${PWD}/../ovs
+make
+utilities/ovn-sim
+
+See documentation on ``ovs-sim`` for info on simulator, including the
+parameters you can use.
+
+OVN Commands
+
+
+These commands interact with OVN, the Open Virtual Network.
+
+``ovn_start`` [*options*]
+Creates and initializes the central OVN databases (both
+``ovn-sb(5)`` and ``ovn-nb(5)``) and starts an instance of
+``ovsdb-server`` for each one.  Also starts an instance of
+``ovn-northd``.
+
+The following options are available:
+
+   ``--nbdb-model`` *model*
+   Uses the given database model for the northbound database.
+   The *model* may be ``standalone`` (the default), ``backup``,
+   or ``clustered``.
+
+   ``--nbdb-servers`` *n*
+   For a clustered northbound database, the number of servers in
+   the cluster.  The default is 3.
+
+   ``--sbdb-model`` *model*
+   Uses the given database model for the southbound database.
+   The *model* may be ``standalone`` (the default), ``backup``,
+   or ``clustered``.
+
+   ``--sbdb-servers`` *n*
+   For a clustered southbound database, the number of servers in
+   the cluster.  The default is 3.
+
+``ovn_attach`` *network* *bridge* *ip* [*masklen*]
+First, this command attaches bridge to interconnection network
+network, just l

[ovs-dev] [PATCH ovn v1] utilities: add ovn-sim

2019-10-09 Thread Flavio Fernandes

Adding a wrapper to ovs-sim called ovn-sim, which re-introduces the
primitives for OVN in the simulator that were lost as part of
the ovn-org/ovn split.

Also resurrected the man page used to document ovn-sim usage.

Lastly, there were some leftover ovs-sim references that were
not proper. This patch renames these to ovn-sim.

All in all, this change is nothing but porting over the
OVN sections of ovs-sim from OVS version 2.12 (pre-split).
Fixes #21

Reported-by: Flavio Fernandes 
Reported-at: https://github.com/ovn-org/ovn/issues/21
Signed-off-by: Flavio Fernandes 
---
 Documentation/automake.mk   |   2 +-
 Documentation/conf.py   |   4 +-
 Documentation/ref/index.rst |   2 +-
 Documentation/ref/ovn-sim.1.rst | 122 +
 Documentation/ref/ovs-sim.1.rst | 244 -
 Makefile.am |   1 +
 utilities/.gitignore|   1 +
 utilities/automake.mk   |   6 +-
 utilities/ovn-sim.in| 308 
 9 files changed, 441 insertions(+), 249 deletions(-)
 create mode 100644 Documentation/ref/ovn-sim.1.rst
 delete mode 100644 Documentation/ref/ovs-sim.1.rst
 create mode 100755 utilities/ovn-sim.in

diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index 5968d6941..bf2166349 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -106,7 +106,7 @@ RST_MANPAGES = \
 # rST formatted manpages that we don't want to install because they
 # document stuff that only works with a build tree, not with an
 # installed OVS.
-RST_MANPAGES_NOINST = ovs-sim.1.rst
+RST_MANPAGES_NOINST = ovn-sim.1.rst
 
 # The GNU standards say that these variables should control
 # installation directories for manpages in each section.  Automake
diff --git a/Documentation/conf.py b/Documentation/conf.py
index 6bf528bde..2c85dcf4d 100644
--- a/Documentation/conf.py
+++ b/Documentation/conf.py
@@ -114,8 +114,8 @@ html_static_path = ['_static']
 # One entry per manual page. List of tuples
 # (source start file, name, description, authors, manual section).
 _man_pages = [
-('ovs-sim.1',
- u'Open vSwitch simulator environment'),
+('ovn-sim.1',
+ u'Open Virtual Network simulator environment'),
 ('ovsdb-server.7',
  u'Open vSwitch Database Server Protocol'),
 ('ovsdb.5',
diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
index 530d639d6..7ff9edd09 100644
--- a/Documentation/ref/index.rst
+++ b/Documentation/ref/index.rst
@@ -39,7 +39,7 @@ time:
 .. toctree::
:maxdepth: 3
 
-   ovs-sim.1
+   ovn-sim.1
ovsdb-server.7
ovsdb.5
ovsdb.7
diff --git a/Documentation/ref/ovn-sim.1.rst b/Documentation/ref/ovn-sim.1.rst
new file mode 100644
index 0..b906740f0
--- /dev/null
+++ b/Documentation/ref/ovn-sim.1.rst
@@ -0,0 +1,122 @@
+===
+ovn-sim
+===
+
+Synopsis
+
+
+``ovn-sim`` [*option*]... [*script*]...
+
+Description
+===
+
+``ovn-sim`` is a wrapper script that adds ovn related commands on
+top of ``ovs-sim``.
+
+``ovs-sim`` provides a convenient environment for running one or more Open
+vSwitch instances and related software in a sandboxed simulation environment.
+
+To use ``ovn-sim``, first build Open vSwitch, then invoke it directly from the
+build directory, e.g.::
+
+git clone https://github.com/openvswitch/ovs.git
+cd ovs
+./boot.sh && ./configure && make
+cd ..
+git clone https://github.com/ovn-org/ovn.git
+cd ovn
+./boot.sh && ./configure --with-ovs-source=${PWD}/../ovs
+make
+utilities/ovn-sim
+
+See documentation on ``ovs-sim`` for info on simulator, including the
+parameters you can use.
+
+OVN Commands
+
+
+These commands interact with OVN, the Open Virtual Network.
+
+``ovn_start`` [*options*]
+Creates and initializes the central OVN databases (both
+``ovn-sb(5)`` and ``ovn-nb(5)``) and starts an instance of
+``ovsdb-server`` for each one.  Also starts an instance of
+``ovn-northd``.
+
+The following options are available:
+
+   ``--nbdb-model`` *model*
+   Uses the given database model for the northbound database.
+   The *model* may be ``standalone`` (the default), ``backup``,
+   or ``clustered``.
+
+   ``--nbdb-servers`` *n*
+   For a clustered northbound database, the number of servers in
+   the cluster.  The default is 3.
+
+   ``--sbdb-model`` *model*
+   Uses the given database model for the southbound database.
+   The *model* may be ``standalone`` (the default), ``backup``,
+   or ``clustered``.
+
+   ``--sbdb-servers`` *n*
+   For a clustered southbound database, the number of servers in
+   the cluster.  The default is 3.
+
+``ovn_attach`` *network* *bridge* *ip* [*masklen*]
+First, this command attaches bridge to interconnection network
+network, just like ``net_attach`` *network* *bridge*.  Second, it
+c

[ovs-dev] [PATCH ovn v1] Documentation cleanup: sandbox

2019-10-01 Thread Flavio Fernandes

Fix ovn-sandbox documentation with regards to SANDBOXFLAGS. That
is, the sandbox in this repo will actually fail if we attempt to
start it with the --ovn flag.

Signed-off-by: Flavio Fernandes 
---
 Documentation/tutorials/ovn-sandbox.rst | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/Documentation/tutorials/ovn-sandbox.rst 
b/Documentation/tutorials/ovn-sandbox.rst
index 4c2781afd..a150109d8 100644
--- a/Documentation/tutorials/ovn-sandbox.rst
+++ b/Documentation/tutorials/ovn-sandbox.rst
@@ -36,14 +36,12 @@ Getting Started
 For some general information about ``ovs-sandbox``, see the Open vSwitch
 documentaion on ``ovs-sandbox``.
 
-``ovs-sandbox`` does not include OVN support by default.  To enable OVN, you
-must pass the ``--ovn`` flag.  For example, if running it straight from the OVS
-git tree you would run::
+``ovs-sandbox`` in the OVN repo includes OVN support by default.  To start it,
+you would simply need to run::
 
-$ make sandbox SANDBOXFLAGS="--ovn"
+$ make sandbox
 
-Running the sandbox with OVN enabled does the following additional steps to the
-environment:
+Running the sandbox does the following steps to the environment:
 
 1. Creates the ``OVN_Northbound`` and ``OVN_Southbound`` databases as 
described in
`ovn-nb(5)`_ and `ovn-sb(5)`_.
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v1] vagrant: Port Vagrantfile to build and test OVN

2019-09-26 Thread Flavio Fernandes

Revisit Vagrantfile for OVN, so these distros can be used
to build OVN and run its tests:
  - centos7
  - debian-8
  - fedora-29

Vagrant box fedora/23-cloud-base is not working anymore, so it
got bumped to fedora-29.

This patch assumes that OVS source is located on same directory
where this OVN source is located (i.e.: ../ovs).

Provisioning makes no attempt to build and install openvswitch
kernel modules, but it will build ovs on each VM in order to
support OVN.

Signed-off-by: Flavio Fernandes 
---
Items looking for feedback:

- There is an OVN test failing, which needs a closer look:
  77: ovn -- 1 LR with distributed router gateway port FAILED (ovn.at:9175)

- Is building + using the OVS kernel important for OVN's sake?

- Maybe we should use a different verion of the Vagrant boxes (e.g. Centos 8)

- The provisioning is not building rpms+deb packages. It is possible that is
  a show stopper before we merge this patch.


Vagrantfile | 141 +++-
 1 file changed, 62 insertions(+), 79 deletions(-)

diff --git a/Vagrantfile b/Vagrantfile
index fbd772a1b..07ed0b0e0 100644
--- a/Vagrantfile
+++ b/Vagrantfile
@@ -5,7 +5,7 @@
 VAGRANTFILE_API_VERSION = "2"
 Vagrant.require_version ">=1.7.0"
 
-$bootstrap_fedora = <

[ovs-dev] [PATCH ovn] ovn-openstack.rst: Add check for Gateway_Chassis table

2019-09-05 Thread Flavio Fernandes

This is related to the section called "Adding a Gateway".

Added workaround command for a known issue in networking-ovn.
Even after the issue is resolved, it may be useful to have this
in the tutorial, so folks have a feel for how OVN keeps track
of chassis gateway.

Also, removed redundant route command when assigning an address
to the br-ex interface.

Signed-off-by: Flavio Fernandes 
---
 Documentation/tutorials/ovn-openstack.rst | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/tutorials/ovn-openstack.rst 
b/Documentation/tutorials/ovn-openstack.rst
index 353ef209e..ed30e3044 100644
--- a/Documentation/tutorials/ovn-openstack.rst
+++ b/Documentation/tutorials/ovn-openstack.rst
@@ -1331,6 +1331,20 @@ with an IP address from the "private" network, then we 
create a
 floating IP address on the "public" network, then we associate the
 port with the floating IP address.
 
+As of this writing, you may need to run the following to fix a
+problem with associating a logical port of router with the external
+gateway::
+
+  $ CHASSIS=$(ovn-nbctl --bare --columns="_uuid" find gateway_chassis) ; \
+[ -z "${CHASSIS}" ] && PORT_NAME='' || \
+PORT_NAME=$(ovn-nbctl --bare --columns=name \
+find logical_router_port gateway_chassis="${CHASSIS}")
+
+  $ [ -z "${PORT_NAME}" ] && {
+  openstack router unset --external-gateway router1 && \
+  openstack router set --external-gateway public router1
+} || echo logical port \"${PORT_NAME}\" in chassis \"${CHASSIS}\"
+
 Let's add a new VM ``d`` with a floating IP::
 
   $ openstack server create --nic net-id=private --flavor m1.nano --image 
$IMAGE_ID --key-name demo d
@@ -1347,7 +1361,6 @@ It's also necessary to configure the "public" network 
because DevStack
 does not do it automatically::
 
   $ sudo ip link set br-ex up
-  $ sudo ip route add 172.24.4.0/24 dev br-ex
   $ sudo ip addr add 172.24.4.1/24 dev br-ex
 
 Now you should be able to "ping" VM ``d`` from the OpenStack host::
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3] ovn-openstack.rst: Miscelaneous fixes.

2019-08-07 Thread Flavio Fernandes

Added a few clarifications to improve the tutorial:

  - VM needs to be at least 6 Gb on latest openstack (ouch!);
  - VM must support nesting;
  - local.conf may require HOST_IP depending on device name;
  - Omit --project from network create to be consistent with OS_PROJECT_NAME;
  - Use a temporary file to ensure datapath flow for recirc(0) is obtained;
  - Reword what to expect when tracing packet before logical router creation;
  - Fix minor mistake on the usage of netcat command;
  - Use MAC of logical router's IPv4 interface to make ACL example more 
realistic.

Signed-off-by: Flavio Fernandes 
---
v1->v2: Fix [most] issues found by 0-day robot. Bleep bloop. :)
The one lingering issue I don't know how to fix, bc I rather not break 
up url.
v2->v3: Fix backslash mistake introduced on v2.

Open issues that I will look into in parallel:
  - With OVN moving to its own repo, this patch may need to move as well.
  - I was not able to see NAT entries in a logical flow when following the 
Gateway section.
---
 Documentation/tutorials/ovn-openstack.rst | 64 +++
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/Documentation/tutorials/ovn-openstack.rst 
b/Documentation/tutorials/ovn-openstack.rst
index c6dff5e55..353ef209e 100644
--- a/Documentation/tutorials/ovn-openstack.rst
+++ b/Documentation/tutorials/ovn-openstack.rst
@@ -62,12 +62,16 @@ with the tutorial in full.
 Unless you have a spare computer laying about, it's easiest to install
 DevStacck in a virtual machine.  This tutorial was built using a VM
 implemented by KVM and managed by virt-manager.  I recommend
-configuring the VM configured for the x86-64 architecture, 4 GB RAM, 2
+configuring the VM configured for the x86-64 architecture, 6 GB RAM, 2
 VCPUs, and a 20 GB virtual disk.
 
 .. note::
+   Since we will be creating VMs inside this VM, it is important to have
+   nesting configured properly. See
+   
https://github.com/openstack/devstack/blob/master/doc/source/guides/devstack-with-nested-kvm.rst
+   for details on that.
 
-   If you happen to run your Linux-based host with 32-bit userspace,
+   Also, if you happen to run your Linux-based host with 32-bit userspace,
then you will have some special issues, even if you use a 64-bit
kernel:
 
@@ -174,6 +178,12 @@ Here are step-by-step instructions to get started:
 
.. note::
 
+  Depending on the name of the network device used by the VM, devstack
+  may be unable to automatically obtain its IP address. If that happens,
+  edit ``local.conf`` and explicitly provide it (X marks the spot)::
+
+HOST_IP=X
+
   If you installed a 32-bit i386 guest (against the advice above),
   at this point edit ``local.conf`` to add the following line::
 
@@ -430,7 +440,7 @@ Creating network ``n1``
 
 Let's start by creating the network::
 
-  $ openstack network create --project admin --provider-network-type geneve n1
+  $ openstack network create --provider-network-type geneve n1
 
 OpenStack needs to know the subnets that a network serves.  We inform
 it by creating subnet objects.  To keep it simple, let's give our
@@ -997,6 +1007,22 @@ this::
 
   
recirc_id(0),in_port(3),eth(src=fa:16:3e:f5:2a:90),eth_type(0x0800),ipv4(src=10.1.1.5,frag=no),
 packets:388, bytes:38024, used:0.977s, actions:ct(zone=8),recirc(0x18)
 
+.. note::
+
+  Flows in the datapath can expire quickly and the ``watch`` command
+  mentioned above may be too slow to catch it. If that is your
+  case, stop the ``ping 10.1.1.6`` session and re-start it a few
+  seconds after this command::
+
+$ sudo conntrack -F ; rm -f /tmp/flows.txt ; \
+ for _ in $(seq 100) ; do \
+ sudo ovs-dpctl dump-flows >> /tmp/flows.txt ; \
+ sleep 0.1 ; done
+
+  Then, look for ``recirc_id(0)`` in flows.txt after ping command was issued::
+
+$ sort --uniq /tmp/flows.txt | grep zone
+
 We can hand the first part of this (everything up to the first space)
 to ``ofproto/trace``, and it will tell us what happens::
 
@@ -1064,7 +1090,7 @@ tracing with ``recirc_id(0xc)``::
   ...
   Datapath actions: 4
 
-It was took multiple hops, but we finally came to the end of the line
+It took multiple hops, but we finally came to the end of the line
 where the packet was output to ``b`` after passing through both
 firewalls.  The port number here is a datapath port number, which is
 usually different from an OpenFlow port number.  To check that it is
@@ -1114,15 +1140,16 @@ works in OVN.
 There's nothing really new for the network and the VM so let's just go
 ahead and create them::
 
-  $ openstack network create --project admin --provider-network-type geneve n2
+  $ openstack network create --provider-network-type geneve n2
   $ openstack subnet create --subnet-range 10.1.2.0/24 --network n2 n2subnet
   $ openstack server create --nic net-id=n2,v4-fixed-ip=10.1.2.7 --flavor 
m1.nano --image $IMAGE_ID --key-name demo c
   $ openstack port set --name cp

[ovs-dev] [PATCH v2] ovn-openstack.rst: Miscelaneous fixes.

2019-08-07 Thread Flavio Fernandes

Added a few clarifications to improve the tutorial:

  - VM needs to be at least 6 Gb on latest openstack (ouch!);
  - VM must support nesting;
  - local.conf may require HOST_IP depending on device name;
  - Omit --project from network create to be consistent with OS_PROJECT_NAME;
  - Use a temporary file to ensure datapath flow for recirc(0) is obtained;
  - Reword what to expect when tracing packet before logical router creation;
  - Fix minor mistake on the usage of netcat command;
  - Use MAC of logical router's IPv4 interface to make ACL example more 
realistic.

Signed-off-by: Flavio Fernandes 
---
v1->v2: Fix [most] issues found by 0-day robot. Bleep bloop. :)
The one lingering issue I don't know how to fix, bc I rather not break 
up url

Open issues that I will look into in parallel:
  - With OVN moving to its own repo, this patch may need to move as well.
  - I was not able to see NAT entries in a logical flow when following the 
Gateway section.


Documentation/tutorials/ovn-openstack.rst | 64 +++
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/Documentation/tutorials/ovn-openstack.rst 
b/Documentation/tutorials/ovn-openstack.rst
index c6dff5e55..06715af71 100644
--- a/Documentation/tutorials/ovn-openstack.rst
+++ b/Documentation/tutorials/ovn-openstack.rst
@@ -62,12 +62,16 @@ with the tutorial in full.
 Unless you have a spare computer laying about, it's easiest to install
 DevStacck in a virtual machine.  This tutorial was built using a VM
 implemented by KVM and managed by virt-manager.  I recommend
-configuring the VM configured for the x86-64 architecture, 4 GB RAM, 2
+configuring the VM configured for the x86-64 architecture, 6 GB RAM, 2
 VCPUs, and a 20 GB virtual disk.
 
 .. note::
+   Since we will be creating VMs inside this VM, it is important to have
+   nesting configured properly. See
+   
https://github.com/openstack/devstack/blob/master/doc/source/guides/devstack-with-nested-kvm.rst
+   for details on that.
 
-   If you happen to run your Linux-based host with 32-bit userspace,
+   Also, if you happen to run your Linux-based host with 32-bit userspace,
then you will have some special issues, even if you use a 64-bit
kernel:
 
@@ -174,6 +178,12 @@ Here are step-by-step instructions to get started:
 
.. note::
 
+  Depending on the name of the network device used by the VM, devstack
+  may be unable to automatically obtain its IP address. If that happens,
+  edit ``local.conf`` and explicitly provide it (X marks the spot)::
+
+HOST_IP=X
+
   If you installed a 32-bit i386 guest (against the advice above),
   at this point edit ``local.conf`` to add the following line::
 
@@ -430,7 +440,7 @@ Creating network ``n1``
 
 Let's start by creating the network::
 
-  $ openstack network create --project admin --provider-network-type geneve n1
+  $ openstack network create --provider-network-type geneve n1
 
 OpenStack needs to know the subnets that a network serves.  We inform
 it by creating subnet objects.  To keep it simple, let's give our
@@ -997,6 +1007,22 @@ this::
 
   
recirc_id(0),in_port(3),eth(src=fa:16:3e:f5:2a:90),eth_type(0x0800),ipv4(src=10.1.1.5,frag=no),
 packets:388, bytes:38024, used:0.977s, actions:ct(zone=8),recirc(0x18)
 
+.. note::
+
+  Flows in the datapath can expire quickly and the ``watch`` command
+  mentioned above may be too slow to catch it. If that is your
+  case, stop the ``ping 10.1.1.6`` session and re-start it a few
+  seconds after this command::
+
+$ sudo conntrack -F ; rm -f /tmp/flows.txt ; \
+ for _ in $(seq 100) ; do \
+ sudo ovs-dpctl dump-flows >> /tmp/flows.txt ; \
+ sleep 0.1 ; done
+
+  Then, look for ``recirc_id(0)`` in flows.txt after ping command was issued::
+
+$ sort --uniq /tmp/flows.txt | grep zone
+
 We can hand the first part of this (everything up to the first space)
 to ``ofproto/trace``, and it will tell us what happens::
 
@@ -1064,7 +1090,7 @@ tracing with ``recirc_id(0xc)``::
   ...
   Datapath actions: 4
 
-It was took multiple hops, but we finally came to the end of the line
+It took multiple hops, but we finally came to the end of the line
 where the packet was output to ``b`` after passing through both
 firewalls.  The port number here is a datapath port number, which is
 usually different from an OpenFlow port number.  To check that it is
@@ -1114,15 +1140,16 @@ works in OVN.
 There's nothing really new for the network and the VM so let's just go
 ahead and create them::
 
-  $ openstack network create --project admin --provider-network-type geneve n2
+  $ openstack network create --provider-network-type geneve n2
   $ openstack subnet create --subnet-range 10.1.2.0/24 --network n2 n2subnet
   $ openstack server create --nic net-id=n2,v4-fixed-ip=10.1.2.7 --flavor 
m1.nano --image $IMAGE_ID --key-name demo c
   $ openstack port set --name cp $(openstack port list --server c -f value -c 
ID)

[ovs-dev] [PATCH 1/1] ovn-openstack.rst: Miscelaneous fixes.

2019-08-07 Thread Flavio Fernandes

Added a few clarifications to improve the tutorial:

  - VM needs to be at least 6 Gb on latest openstack (ouch!);
  - VM must support nesting;
  - local.conf may require HOST_IP depending on device name;
  - Omit --project from network create to be consistent with OS_PROJECT_NAME;
  - Use a temporary file to ensure datapath flow for recirc(0) is obtained;
  - Reword what to expect when tracing packet before logical router creation;
  - Fix minor mistake on the usage of netcat command;
  - Use MAC of logical router's IPv4 interface to make ACL example more 
realistic.

Signed-off-by: Flavio Fernandes 
---
Open issues that I will look into in parallel:
  - With OVN moving to its own repo, this patch may need to move as well.
  - I was not able to see NAT entries in a logical flow when following the 
Gateway section.


 Documentation/tutorials/ovn-openstack.rst | 54 ++-
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/Documentation/tutorials/ovn-openstack.rst 
b/Documentation/tutorials/ovn-openstack.rst
index c6dff5e55..b5d776dd8 100644
--- a/Documentation/tutorials/ovn-openstack.rst
+++ b/Documentation/tutorials/ovn-openstack.rst
@@ -62,12 +62,16 @@ with the tutorial in full.
 Unless you have a spare computer laying about, it's easiest to install
 DevStacck in a virtual machine.  This tutorial was built using a VM
 implemented by KVM and managed by virt-manager.  I recommend
-configuring the VM configured for the x86-64 architecture, 4 GB RAM, 2
+configuring the VM configured for the x86-64 architecture, 6 GB RAM, 2
 VCPUs, and a 20 GB virtual disk.
 
 .. note::
+   Since we will be creating VMs inside this VM, it is important to have
+   nesting configured properly. See
+   
https://github.com/openstack/devstack/blob/master/doc/source/guides/devstack-with-nested-kvm.rst
+   for details on that.
 
-   If you happen to run your Linux-based host with 32-bit userspace,
+   Also, if you happen to run your Linux-based host with 32-bit userspace,
then you will have some special issues, even if you use a 64-bit
kernel:
 
@@ -174,6 +178,12 @@ Here are step-by-step instructions to get started:
 
.. note::
 
+  Depending on the name of the network device used by the VM, devstack
+  may be unable to automatically obtain its IP address. If that happens
+  to you, edit ``local.conf`` and explicitly provide it (X marks the 
spot)::
+
+HOST_IP=X
+
   If you installed a 32-bit i386 guest (against the advice above),
   at this point edit ``local.conf`` to add the following line::
 
@@ -430,7 +440,7 @@ Creating network ``n1``
 
 Let's start by creating the network::
 
-  $ openstack network create --project admin --provider-network-type geneve n1
+  $ openstack network create --provider-network-type geneve n1
 
 OpenStack needs to know the subnets that a network serves.  We inform
 it by creating subnet objects.  To keep it simple, let's give our
@@ -997,6 +1007,22 @@ this::
 
   
recirc_id(0),in_port(3),eth(src=fa:16:3e:f5:2a:90),eth_type(0x0800),ipv4(src=10.1.1.5,frag=no),
 packets:388, bytes:38024, used:0.977s, actions:ct(zone=8),recirc(0x18)
 
+.. note::
+
+  Flows in the datapath can expire quickly and the ``watch`` command
+  mentioned above may be too slow to catch it. If that is your
+  case, stop the ``ping 10.1.1.6`` session and re-start it a few
+  seconds after this command::
+
+$ sudo conntrack -F ; rm -f /tmp/flows.txt ; \
+ for _ in $(seq 100) ; do \
+sudo ovs-dpctl dump-flows >> /tmp/flows.txt ; \
+sleep 0.1 ; done
+
+  Then, look for ``recirc_id(0)`` in flows.txt after ping command was issued::
+
+$ sort --uniq /tmp/flows.txt | grep zone
+
 We can hand the first part of this (everything up to the first space)
 to ``ofproto/trace``, and it will tell us what happens::
 
@@ -1064,7 +1090,7 @@ tracing with ``recirc_id(0xc)``::
   ...
   Datapath actions: 4
 
-It was took multiple hops, but we finally came to the end of the line
+It took multiple hops, but we finally came to the end of the line
 where the packet was output to ``b`` after passing through both
 firewalls.  The port number here is a datapath port number, which is
 usually different from an OpenFlow port number.  To check that it is
@@ -1114,15 +1140,16 @@ works in OVN.
 There's nothing really new for the network and the VM so let's just go
 ahead and create them::
 
-  $ openstack network create --project admin --provider-network-type geneve n2
+  $ openstack network create --provider-network-type geneve n2
   $ openstack subnet create --subnet-range 10.1.2.0/24 --network n2 n2subnet
   $ openstack server create --nic net-id=n2,v4-fixed-ip=10.1.2.7 --flavor 
m1.nano --image $IMAGE_ID --key-name demo c
   $ openstack port set --name cp $(openstack port list --server c -f value -c 
ID)
   $ CP_MAC=$(openstack port show -f value -c mac_address cp)
 
 The new network ``n2`` is not yet connected to ``n1`` in any way.  You
-can try t

[ovs-dev] [PATCH v1 2/2] ovs-vsctl: Fix segfault when attempting to del-port from parent bridge.

2018-05-01 Thread Flavio Fernandes

The error message in the check for improper bridge param is de-referencing
parent from the wrong bridge. Also, the message itself had the parent and
child bridges reversed, so that got a small tweak as well.

Signed-off-by: Flavio Fernandes <fla...@flaviof.com>
---
 utilities/ovs-vsctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index c69e89e..188a390 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1748,9 +1748,9 @@ cmd_del_port(struct ctl_context *ctx)
 if (port->bridge != bridge) {
 if (port->bridge->parent == bridge) {
 ctl_fatal("bridge %s does not have a port %s (although "
-"its parent bridge %s does)",
+"its child bridge %s does)",
 ctx->argv[1], ctx->argv[2],
-bridge->parent->name);
+port->bridge->name);
 } else {
 ctl_fatal("bridge %s does not have a port %s",
 ctx->argv[1], ctx->argv[2]);
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1 1/2] ovs-vsctl.at: deleting a port from fake bridge

2018-05-01 Thread Flavio Fernandes

This test will exercise the code path in ovs-vsctl where a del-port
is attempted using the parent of a fake-bridge. It expects a message
saying that user provided the wrong bridge. Not a segfault.

Signed-off-by: Flavio Fernandes <fla...@flaviof.com>
---
 tests/ovs-vsctl.at | 17 +
 1 file changed, 17 insertions(+)

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index 3189a9b..f9e7f3b 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -605,6 +605,23 @@ CHECK_PORTS([xapi1], [eth0.$1])
 CHECK_IFACES([xapi1], [eth0.$1])
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
+
+AT_SETUP([simple fake bridge + del-port from parent (VLAN $1)])
+AT_KEYWORDS([ovs-vsctl fake-bridge del-port])
+OVS_VSCTL_SETUP
+OVS_VSCTL_SETUP_SIMPLE_FAKE_CONF([$1])
+AT_CHECK([RUN_OVS_VSCTL([del-port xenbr0 eth0.$1])], [1], [],
+ [ovs-vsctl: bridge xenbr0 does not have a port eth0.$1 (although its child 
bridge xapi1 does)
+])
+CHECK_PORTS([xenbr0], [eth0])
+CHECK_IFACES([xenbr0], [eth0])
+CHECK_PORTS([xapi1], [eth0.$1])
+CHECK_IFACES([xapi1], [eth0.$1])
+AT_CHECK([RUN_OVS_VSCTL([del-port xapi1 eth0.$1])])
+CHECK_PORTS([xenbr0], [eth0])
+CHECK_IFACES([xenbr0], [eth0])
+OVS_VSCTL_CLEANUP
+AT_CLEANUP
 ]) # OVS_VSCTL_FAKE_BRIDGE_TESTS
 
 OVS_VSCTL_FAKE_BRIDGE_TESTS([9])
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1 0/2] ovs-vsctl segfaults on del-port of fake-bridge when parent is provided

2018-05-01 Thread Flavio Fernandes

Greetings!

I was lucky enough to spend some time having fun with OVS recently and 
encountered
a bug that may be worth sharing with you.

The code path in the error case when bridge's del-port is attempted for the
parent (instead of the fake-bridge) segfaults. Here are some simple steps for
reproducing this issue:

./boot.sh ; ./configure ; make -j4 ; make sandbox

PARENT_BRIDGE=br0 ; FAKE_BRIDGE=br0c ; VLAN_TAG=666
ovs-vsctl add-br ${PARENT_BRIDGE}
ovs-vsctl add-br $FAKE_BRIDGE $PARENT_BRIDGE $VLAN_TAG

# Add a port to parent bridge, which happens to have same tag as the fake_bridge
# Note: The port could be have been added directly to the fake bridge too, of 
course.
# The end result of the add-port is the same.
ovs-vsctl add-port $PARENT_BRIDGE p1 -- set port p1 tag=${VLAN_TAG}
# ovs-vsctl add-port $FAKE_BRIDGE p1

# removing p1 will make a segfault
ovs-vsctl del-port $PARENT_BRIDGE p1  ; # sad panda moment

# Here are 3 ways of working around this segfault

# workaround 1: remove tag before removing port from parent
ovs-vsctl remove port p1 tag $VLAN_TAG && \
ovs-vsctl del-port $PARENT_BRIDGE p1  && echo ok
# workaround 2: remove port as if it belongs to fake bridge
ovs-vsctl del-port $FAKE_BRIDGE p1  && echo ok
# workaround 3: remove port w/out specifying a bridge
ovs-vsctl del-port p1  && echo ok


This issue appears to exist since commit 7c79588e , which dates back to 
Feb/2010.
I see it in OVS 2.3.2.

-- flaviof



More details about the segfault:

|main: Ubuntu ~/ovs.git/_ffbuilddir/utilities/sandbox on devel
$ sudo ovs-vsctl show
fa096c6f-8f5f-49ae-92b4-e94ce58aceec
Bridge "br0"
Port "br0"
Interface "br0"
type: internal
Port "p1"
tag: 666
Interface "p1"
Port "br0c"
tag: 666
Interface "br0c"
type: internal
|main: Ubuntu ~/ovs.git/_ffbuilddir/utilities/sandbox on devel
$ gdb /home/ff/ovs.git/_ffbuilddir/utilities/ovs-vsctl
...
Reading symbols from /home/ff/ovs.git/_ffbuilddir/utilities/ovs-vsctl...done.
(gdb) run del-port br0 p1
Starting program: /home/ff/ovs.git/_ffbuilddir/utilities/ovs-vsctl del-port br0 
p1
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x0040a945 in cmd_del_port (ctx=0x7fffc5d0) at 
../utilities/ovs-vsctl.c:1750
1750ctl_fatal("bridge %s does not have a port %s 
(although "
(gdb) bt
#0  0x0040a945 in cmd_del_port (ctx=0x7fffc5d0) at 
../utilities/ovs-vsctl.c:1750
#1  0x004075dd in do_vsctl (args=0xc8f7f0 
"/home/ff/ovs.git/_ffbuilddir/utilities/ovs-vsctl del-port br0 p1", 
commands=0xc8fd50, n_commands=1, idl=0xc8fdb0)
at ../utilities/ovs-vsctl.c:2623
#2  0x00405e7e in main (argc=4, argv=0x7fffc868) at 
../utilities/ovs-vsctl.c:184
(gdb) list
1745struct vsctl_bridge *bridge;
1746
1747bridge = find_bridge(vsctl_ctx, ctx->argv[1], true);
1748if (port->bridge != bridge) {
1749if (port->bridge->parent == bridge) {
1750ctl_fatal("bridge %s does not have a port %s 
(although "
1751"its parent bridge %s does)",
1752ctx->argv[1], ctx->argv[2],
1753bridge->parent->name);
1754} else {
(gdb) p ctx->argv[1]
$1 = 0x7fffcba5 "br0"
(gdb) p ctx->argv[2]
$2 = 0x7fffcba9 "p1"
(gdb) p port->bridge->name
$3 = 0xc9a290 "br0c"
(gdb) p bridge
$4 = (struct vsctl_bridge *) 0xcc9d60
(gdb) p bridge->parent
$5 = (struct vsctl_bridge *) 0x0
(gdb) p port->bridge
$6 = (struct vsctl_bridge *) 0xccb440
(gdb) p bridge->name
$7 = 0xcc8da0 "br0"
(gdb)


Flavio Fernandes (2):
  ovs-vsctl.at: deleting a port from fake bridge
  ovs-vsctl: Fix segfault when attempting to del-port from parent
bridge.

 tests/ovs-vsctl.at| 17 +
 utilities/ovs-vsctl.c |  4 ++--
 2 files changed, 19 insertions(+), 2 deletions(-)

-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/5] ovn: add sfc tables to northbound schema

2016-11-08 Thread Flavio Fernandes

Reported-at: http://openvswitch.org/pipermail/discuss/2016-March/020628.html
Reported-at:  
http://openvswitch.org/pipermail/discuss/2016-May/thread.html#21201
Co-authored-by: John McDowall <jmcdow...@paloaltonetworks.com>
Signed-off-by: Flavio Fernandes <fla...@flaviof.com>
---
 ovn/ovn-architecture.7.xml | 185 +
 ovn/ovn-nb.ovsschema   |  56 +-
 ovn/ovn-nb.xml | 119 +
 3 files changed, 359 insertions(+), 1 deletion(-)

diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index 95cba98..681d0c3 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -381,6 +381,13 @@
   switch.  Logical switches and routers are both implemented as logical
   datapaths.
 
+
+
+  Logical services are logical references to virtual network 
functions
+  (VNF). Adding a logical service requires adding steering rules to the 
OVN Northbound
+  database. These are the only rules necessary to implement traffic 
steering for VNFs.
+  The section below "Life Cycle of an inserted VNF" provides 
more details.
+
   
 
   Life Cycle of a VIF
@@ -536,6 +543,184 @@
 
   
 
+ Life Cycle of an inserted Virtual Network Function (VNF)
+
+ 
+   OVN provides an abstraction to enable the insertion of an arbitrary virtual 
network
+   function (VNF) into the path of traffic to and from an application. A VNF 
is different
+   from an application VM in that it acts on traffic between applications, and 
im most
+   cases does not terminiate a flow. Proxy functions are an exception as they 
terminate the
+   flow from the src and create a new flow to the dst. Examples of VNF's are 
security functions,
+   load balancing, and traffic enhancement services, this is not a complete 
list.
+ 
+ 
+   The requirements on the VNF to be inserted are minimal, it must
+   act as a bump in the wire (BITW) and can have one or two 
virtual network
+   ports for traffic. If it has two network ports traffic is directed into one 
and out the other,
+   if it has only one port, then traffic is bi-directional. The requirement 
for the VNF to act as
+   a BITW removes the need for the VNF to participate in L3/L2 networking 
which provides
+   improved agility and reduces the coupling between OVN and the VNF.
+ 
+ 
+   The service insertion is implemented by adding 4 new flow rules into the 
ovn-nb database for
+   each VNF inserted. The rules are added into the last table in the ingress 
path (L2_LKUP).
+   The service insertion rules have a higher priority than the standard 
forwarding rules.
+   This means that they override the existing forwarding rules. There are four
+   new rules added for each insertion. Two ingress and two egress, The first 
ingress
+   rule sends all traffic destined for the application into the VNF ingress 
port and the
+   second rule takes all traffic destined to the application from the VNF 
egress port
+   to the application, the priorities are such that the second rule is always 
checked
+   first. In the egress direction the rules are similar if the traffic is from 
the
+   application it is sent to the VNF egress port and if if is from the 
application
+   and is from the VNF ingress port it is delivered to the destination.
+   
+ 
+ 
+   The steps in this example refer to the details of the OVN Northbound 
database schema.
+   There is a new table in the OVN Northbound database to support service 
insertion
+   called Services this contains the required information for 
each new
+   service inserted. The same service can be used for multiple applications, as
+   there is typically a n:1 relationship between applications and VNFs. A
+   single VNF may be part of several service insertions, but each one is 
logically
+   separate.
+ 
+ 
+   The steps in this example refer often to details of the OVN and OVN
+   Northbound database schemas.  Please see ovn-sb(5) and
+   ovn-nb(5), respectively, for the full story on these
+   databases. The ovn-nb database has specific schema enhancements for the 
service
+   insertion function. The ovn-sb database has no schema changes for the
+   service insertion function.
+ 
+ 
+   The following steps are an overview to inserting a new VNF into the traffic 
path.
+   The sections below go into each step in more detail.
+ 
+ 
+   
+ The service insertion lifecycle begins when a CMS administrator creates a 
new
+ virtual network function (VNF) using the CMS user
+ interface or API. The CMS administrator creates the logical ports 
(ingress and egress)
+ for the VNF. If the CMS is Openstack this will create a reusable 
port-pair defining the
+ interface to the VNF. There is also typically a seperate management port 
for the VNF,
+ but that is not relevant to the service insertion workflow. A single VNF 
can
+ participate with several applications, either as a security VM

75 matches

Mail list logo