Re: [ovs-dev] ovn: Clean up log() action parsing errors.

[0-day Robot]

Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
depbase=`echo ovn/controller/lflow.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I 
./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT 
ovn/controller/lflow.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/lflow.o 
ovn/controller/lflow.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ovn/controller/lport.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I 
./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT 
ovn/controller/lport.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/lport.o 
ovn/controller/lport.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ovn/controller/ofctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I 
./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT 
ovn/controller/ofctrl.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/ofctrl.o 
ovn/controller/ofctrl.c &&\
mv -f $depbase.Tpo $depbase.Po
ovn/controller/ofctrl.c: In function ‘ofctrl_put’:
ovn/controller/ofctrl.c:1086:9: error: missing initializer for field ‘flags’ of 
‘struct ofputil_meter_config’ [-Werror=missing-field-initializers]
 };
 ^
In file included from ovn/controller/ofctrl.c:35:0:
./include/openvswitch/ofp-meter.h:53:14: note: ‘flags’ declared here
 uint16_t flags;
  ^
ovn/controller/ofctrl.c: At top level:
cc1: error: unrecognized command line option "-Wno-null-pointer-arithmetic" 
[-Werror]
cc1: all warnings being treated as errors
make[2]: *** [ovn/controller/ofctrl.o] Error 1
make[2]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.

[Justin Pettit]

> On Jul 30, 2018, at 5:58 PM, Justin Pettit  wrote:
> 
> Thanks for the review!  I've pushed this series to master.

I also just pushed this to branch-2.10.

The rate-limiting is implemented using meters.  Unfortunately, there's a bug in 
the current kernels that prevents meters from working properly.  I've sent a 
patch to upstream Linux, which should solve the problem:

https://marc.info/?l=linux-netdev&m=153281677212826&w=2

Once that is merged, I'll commit it to OVS, and it should work with both Linux 
and userspace datapaths.

--Justin


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

[ovs-dev] [PATCH] ovn: Clean up log() action parsing errors.

[Justin Pettit]

This also add some OVN action parsing tests.

Suggested-by: Ben Pfaff 
Signed-off-by: Justin Pettit 
---
 ovn/lib/actions.c |  9 -
 tests/ovn.at  | 19 +++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 2126fd57551e..bc1a20040cff 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -2071,7 +2071,8 @@ parse_log_arg(struct action_context *ctx, struct 
ovnact_log *log)
 } else if (lexer_match_id(ctx->lexer, "allow")) {
 log->verdict = LOG_VERDICT_ALLOW;
 } else {
-lexer_syntax_error(ctx->lexer, "unknown acl verdict");
+lexer_syntax_error(ctx->lexer, "unknown verdict");
+return;
 }
 } else if (lexer_match_id(ctx->lexer, "name")) {
 if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
@@ -2103,6 +2104,9 @@ parse_log_arg(struct action_context *ctx, struct 
ovnact_log *log)
 log->severity = severity;
 lexer_get(ctx->lexer);
 return;
+} else {
+lexer_syntax_error(ctx->lexer, "unknown severity");
+return;
 }
 }
 lexer_syntax_error(ctx->lexer, "expecting severity");
@@ -2142,6 +2146,9 @@ parse_LOG(struct action_context *ctx)
 lexer_match(ctx->lexer, LEX_T_COMMA);
 }
 }
+if (log->verdict == LOG_VERDICT_UNKNOWN) {
+lexer_syntax_error(ctx->lexer, "expecting verdict");
+}
 }
 
 static void
diff --git a/tests/ovn.at b/tests/ovn.at
index df80b98d64b4..163f5c590516 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1224,6 +1224,25 @@ set_meter(100, 1000, );
 set_meter(4294967295, 4294967295);
 encodes as meter:3
 
+# log
+log(verdict=allow, severity=warning);
+encodes as controller(userdata=00.00.00.07.00.00.00.00.00.04)
+log(name="test1", verdict=drop, severity=info);
+encodes as 
controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31)
+log(verdict=drop, severity=info, meter="meter1");
+encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06,meter_id=4)
+log(name="test1", verdict=drop, severity=info, meter="meter1");
+encodes as 
controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31,meter_id=4)
+log(verdict=drop);
+formats as log(verdict=drop, severity=info);
+encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06)
+log(verdict=bad_verdict, severity=info);
+Syntax error at `bad_verdict' unknown verdict.
+log(verdict=drop, severity=bad_severity);
+Syntax error at `bad_severity' unknown severity.
+log(severity=notice);
+Syntax error at `;' expecting verdict.
+
 # put_nd_ra_opts
 reg1[0] = put_nd_ra_opts(addr_mode = "slaac", mtu = 1500, prefix = aef0::/64, 
slla = ae:01:02:03:04:05);
 encodes as 
controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.00.ff.ff.00.00.00.00.00.00.00.00.05.01.00.00.00.00.05.dc.03.04.40.c0.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.05,pause)
-- 
2.17.1

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

Re: [ovs-dev] [ovs-dev, v1] utilities: check datapath exists before conntrack flush

[0-day Robot]

Bleep bloop.  Greetings Martin Xu, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
depbase=`echo ovn/controller/lflow.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I 
./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT 
ovn/controller/lflow.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/lflow.o 
ovn/controller/lflow.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ovn/controller/lport.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I 
./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT 
ovn/controller/lport.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/lport.o 
ovn/controller/lport.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ovn/controller/ofctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I 
./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT 
ovn/controller/ofctrl.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/ofctrl.o 
ovn/controller/ofctrl.c &&\
mv -f $depbase.Tpo $depbase.Po
ovn/controller/ofctrl.c: In function ‘ofctrl_put’:
ovn/controller/ofctrl.c:1086:9: error: missing initializer for field ‘flags’ of 
‘struct ofputil_meter_config’ [-Werror=missing-field-initializers]
 };
 ^
In file included from ovn/controller/ofctrl.c:35:0:
./include/openvswitch/ofp-meter.h:53:14: note: ‘flags’ declared here
 uint16_t flags;
  ^
ovn/controller/ofctrl.c: At top level:
cc1: error: unrecognized command line option "-Wno-null-pointer-arithmetic" 
[-Werror]
cc1: all warnings being treated as errors
make[2]: *** [ovn/controller/ofctrl.o] Error 1
make[2]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions

[Aravind Prasad]

 > Currently, rule_insert() API does not have return value. There are some
possible
> scenarios where rule insertions can fail at run-time even though the
static
> checks during rule_construct() had passed previously.
> Some possible scenarios for failure of rule insertions:
> **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and
> Normal switch functioning coexist) where the CAM space could get suddenly
> filled up by Normal switch functioning and Openflow gets devoid of
> available space.
> **) Some deployments could have separate independent layers for HW rule
> insertions and application layer to interact with OVS. HW layer
> could face any dynamic issue during rule handling which application could
> not have predicted/captured in rule-construction phase.
> Rule-insert errors for bundles are handled too in this pull-request.
>
> Signed-off-by: Aravind Prasad S 

> Which switches does this help?
> Hi Ben,
> These type of errors are possible in actual Hardware implementations
> of OVS. It is possible that ofproto and netdev providers be
> implemented for an actual HW/NPU. Usually, in such cases, in the rule
> construct phase, all the static checks like verifying the qualifiers/
> actions, CAM full checks could be done and the other related
> verifications.
> But during the rule insert phase, it is possible that
> the rule insertion may get failed in HW (runtime errors,
> HW errors and so on). Also, since HW switches may support Hybrid
> mode (coexistence of Normal and Openflow functioning), the
> possibility of this issue could be even more. Further, when
> rule-insertion fails, it results in a stale state where the
> Controller and Switch Flow-DB differ.
> Hence, we need a way to rollback for rule-insert phase also.
> Kindly let me know your views.


Hi Ben/Aaron/All,

In general, this patch will be very useful for all HW implementations
of OVS since the possibility of dynamic rule insertion failures
are high in such deployments. Static checks during
rule-construct phase are not sufficient and future failure predictions
in construct phase may not be possible. Hence, this patch will make
OVS more flexible to handle such failures.

Kindly review and let me know your views.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1] utilities: check datapath exists before conntrack flush

[Martin Xu]

As part of "force-reload-kmod," conntrack flush command is issued as
'action "ovs-appctl dpctl/flush-conntrack"'. In case no datapath exists
yet when issuing "force-reload-kmod," there is an error message
"ovs-vswitchd: no datapaths exist\ ovs-appctl: ovs-vswitchd: server
returned an error", which is harmless but potentially shows up as "FAILED."
Add an if condition to check whether datapath exists before running the
conntrack flush command.

VMware-BZ: #2170402
Fixes: 265d70310c69 ("utilities: Fix conntrack flush command")

Signed-off-by: Martin Xu 
CC: Greg Rose 
CC: Aaron Conole 
CC: Justin Pettit 
---
 utilities/ovs-lib.in | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
index 92f98ad..d6ef77b 100644
--- a/utilities/ovs-lib.in
+++ b/utilities/ovs-lib.in
@@ -605,7 +605,9 @@ force_reload_kmod () {
 stop_ovsdb
 start_ovsdb || return 1
 
-action "Flush old conntrack entries" ovs-appctl dpctl/flush-conntrack
+if [[ $(ovs-dpctl show) ]]; then
+action "Flush old conntrack entries" ovs-appctl dpctl/flush-conntrack
+fi
 stop_forwarding
 
 if action "Saving interface configuration" save_interfaces; then
-- 
1.8.3.1

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

Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.

[Justin Pettit]

> On Jul 30, 2018, at 11:40 AM, Mark Michelson  wrote:
> 
> Hi Justin,
> 
> I took a look through the patch series, and this is the only one that I had 
> some immediate feedback on.
> 
> First, it would be nice if we could refer to Meters by name when issuing DB 
> commands. For instance `ovn-nbctl add Meter  bands `.

Okay, I'll work on a follow-up patch to add that.  I didn't think it was a 
blocker for this series, though.

> Second, I noticed that the algorithm for computing southbound meter bands can 
> result in a larger number of bands than is in the northbound table. For 
> instance, you could issue the following:
> 
> ovn-nbctl --id=@id create Meter_Band action=drop rate=1000 -- \
> create Meter name=foo unit=kbps band=@id -- \
> create Meter name=bar unit=kbps band=@id
> 
> In the northbound database, you'll have one entry in the meter_band table. In 
> the southbound database, you'll have two entries in the meter_band table. The 
> algorithm does not take into account that multiple northbound meters may 
> refer to the same meter_band. I'm not sure how big an issue this is (or 
> really if it is an issue), but it surprised me a bit when I was playing 
> around with it.

That's interesting, but I don't think it's a problem.  Let me know if you think 
of an issue with it, though.

Thanks for checking out the series, and please let me know if you have any 
other thoughts.

--Justin


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

Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.

[Justin Pettit]

> On Jul 30, 2018, at 12:55 PM, Ben Pfaff  wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:38PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit 
> 
> Besides the comments I gave on patch 6 (oops), I suggest the following
> for more consistent formatting:
> 
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 6d4ed1e2c4ed..4f3cd48ce713 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -2157,7 +2157,7 @@ format_LOG(const struct ovnact_log *log, struct ds *s)
> ds_put_format(s, "severity=%s", log_severity_to_string(log->severity));
> 
> if (log->meter) {
> -ds_put_format(s, ",meter=%s", log->meter);
> +ds_put_format(s, ", meter=%s", log->meter);
> }
> 
> ds_put_cstr(s, ");");

Applied.

> Acked-by: Ben Pfaff 

Thanks for the review!  I've pushed this series to master.

--Justin


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

Re: [ovs-dev] [ACL Meters 6/7] ofproto: Add support for specifying a meter in controller actions.

[Justin Pettit]

> On Jul 30, 2018, at 12:49 PM, Ben Pfaff  wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit 
> 
> In ofproto_check_ofpacts(), the other checks, if they fail, return an
> error and prevent the flow from being added.  The new one doesn't; is
> the difference intentional?

Yes.  My thought is that those controller actions are likely important, so if 
the meter doesn't exist, it's still safer to set up the flow and log a warning.

> Similarly, is there anything that prevents
> a meter from being deleted while still in use and, if that happens, does
> anything particularly bad happen?

Nothing prevents it.  However, if it happens, then the metering just stops 
being enforced, and the other actions are unaffected.

> In ovs-ofctl.8.in, the wording seems a little vague because "associate"
> is such a weak word.  Maybe change
>Associate packets sent to the controller with meter \fIid\fR.
> to
>Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
>that this action sends to the controller.
> or something similarly descriptive

Yes, that is better.

> Do only "drop" actions make sense for controller metering?

Probably.  I added some documentation to that effect to the description of the 
"meter" argument to the log() action in ovn-sb.xml.

> Please add some tests for the new action to the test "ovn -- action
> parsing" in ovn.at.
> 
> Please document the new action in ovn-sb.xml.

Which new action?  I've extended the log() action documentation to add "meter", 
which was missing before.  There was no existing log action parsing test.  I'll 
add that as a separate patch, since I think the existing log() parsing code 
could use a little cleanup in that area, too.

Thanks!

--Justin


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

Re: [ovs-dev] [PATCH v1] rhel: bug fix kmod-openvswitch spec file, rhel6

[Martin Xu]

Thanks, Ben!

On Mon, Jul 30, 2018 at 4:17 PM, Ben Pfaff  wrote:

> OK, I spent some time staring at 89dd5819cf18 and finally got it.  I
> applied this to master and branch-2.10.  I added an appropriate Fixes
> tag to the commit.
>
> On Mon, Jul 30, 2018 at 01:34:19PM -0700, Martin Xu wrote:
> > Hi Ben,
> >
> > I meant to say the previously merged patch. It was 89dd5819cf18 (rhel:
> > support kmod-openvswitch build against multiple kernels, rhel6). This
> patch
> > is meant to fix a bug introduced by 89dd5819cf18.
> >
> > Martin
> >
> > On Mon, Jul 30, 2018 at 1:28 PM, Ben Pfaff  wrote:
> >
> > > On Sun, Jul 29, 2018 at 12:02:57AM -0700, Martin Xu wrote:
> > > > Previous patch removed the if condition for postun script by
> > > > mistake. The weak-update symlinks should be removed only for
> > > > uninstallation not upgrade.
> > > >
> > > > VMware-BZ: #2169383
> > > >
> > > > Signed-off-by: Martin Xu 
> > > > CC: Greg Rose 
> > > > CC: Ben Pfaff 
> > > > CC: Flavio Leitner 
> > >
> > > I looked for the change in the previous patch but it wasn't obvious to
> > > me.  Was this in commit 9207546be025 (rhel: bug fix kmod-openvswitch
> > > spec file, rhel6) or something earlier?
> > >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] rhel: bug fix kmod-openvswitch spec file, rhel6

[Ben Pfaff]

OK, I spent some time staring at 89dd5819cf18 and finally got it.  I
applied this to master and branch-2.10.  I added an appropriate Fixes
tag to the commit.

On Mon, Jul 30, 2018 at 01:34:19PM -0700, Martin Xu wrote:
> Hi Ben,
> 
> I meant to say the previously merged patch. It was 89dd5819cf18 (rhel:
> support kmod-openvswitch build against multiple kernels, rhel6). This patch
> is meant to fix a bug introduced by 89dd5819cf18.
> 
> Martin
> 
> On Mon, Jul 30, 2018 at 1:28 PM, Ben Pfaff  wrote:
> 
> > On Sun, Jul 29, 2018 at 12:02:57AM -0700, Martin Xu wrote:
> > > Previous patch removed the if condition for postun script by
> > > mistake. The weak-update symlinks should be removed only for
> > > uninstallation not upgrade.
> > >
> > > VMware-BZ: #2169383
> > >
> > > Signed-off-by: Martin Xu 
> > > CC: Greg Rose 
> > > CC: Ben Pfaff 
> > > CC: Flavio Leitner 
> >
> > I looked for the change in the previous patch but it wasn't obvious to
> > me.  Was this in commit 9207546be025 (rhel: bug fix kmod-openvswitch
> > spec file, rhel6) or something earlier?
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 5/7] ovn: Support configuring meters through SB Meter table.

[Justin Pettit]

> On Jul 30, 2018, at 11:44 AM, Ben Pfaff  wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:36PM -0700, Justin Pettit wrote:
>> Add the ability to configure meters through the newly introduced Meter
>> table in the Southbound database.  Previously, meters were configured by
>> providing strings to describe the meter in the extended meter table.
>> This patch changes the behavior so that the extended meter table's
>> strings are references to names in the Meter table.  The old behavior is
>> still supported if the extended meter table entry begins with "__string: "
>> 
>> Signed-off-by: Justin Pettit 
> 
> I have the following suggestions.  Neither is important.
> 
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 01981a79480b..d810d413f207 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -907,7 +907,7 @@ ofctrl_can_put(void)
>  *
>  * Replaces the group table and meter table on the switch, if possible,
>  * by the contents of '->desired'.  Regardless of whether the table is
> - * updated, this deletes all the groups ore metersjfrom the '->desired'
> + * updated, this deletes all the groups ore meters from the '->desired'
>  * and frees them. (The hmap itself isn't destroyed.)
>  *
>  * Sends conntrack flush messages to each zone in 'pending_ct_zones' that
> @@ -1080,10 +1080,10 @@ ofctrl_put(struct hmap *flow_table, struct shash 
> *pending_ct_zones,
> struct ovn_extend_table_info *m_installed, *next_meter;
> EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
> /* Delete the meter. */
> -struct ofputil_meter_mod mm;
> -memset(&mm, 0, sizeof mm);
> -mm.command = OFPMC13_DELETE;
> -mm.meter.meter_id = m_installed->table_id;
> +struct ofputil_meter_mod mm = {
> +.command = OFPMC13_DELETE,
> +.meter.meter_id = m_installed->table_id,
> +};
> add_meter_mod(&mm, &msgs);
> 
> ovn_extend_table_remove(meters, m_installed);

That is better.  Thanks.

--Justin


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

Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.

[Justin Pettit]

> On Jul 30, 2018, at 11:26 AM, Ben Pfaff  wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
>> Add support for configuring meters through the Meter and Meter_Band
>> tables in the Northbound database.  This commit also has ovn-northd
>> sync those tables between the Northbound and Southbound databases.
>> 
>> Add support for configuring meters with ovn-nbctl.
>> 
>> Signed-off-by: Justin Pettit 
> 
> Thanks for the patches!
> 
> band_cmp() uses the form "a > b ? -1 : 1" a couple of times.  I believe
> that this will sort 'a' and 'b' into descending order.  Is that what you
> want?
> 
> (Oh, actually I see it doesn't matter, it just needs to be consistent.)

Right, it shouldn't matter, so I'll just leave it.

> In ovn-[ns]b.xml, for consistency with the rest of the tables,
> title="..." in  start tags should not include the word "table"
> and maybe should give better descriptions.

Yes, I copied that from QoS, which was right above it.  I went ahead and 
changed QoS, too.

> The Meter_Band table says the following.  I'm not sure what a "relative
> rate limit" is.  It kind of sounds like it would be an additive one,
> where if you have a first band with a rate of 500 Mbps then the second
> one with a rate of 250 Mbps would effectively be 500 + 250 = 750 Mbps.
> But I don't think you really mean that; I think it's actually an
> absolute rate.  Maybe by relative you just mean that the unit is
> specified by the Meter.
> 
>
>  
>The relative rate limit for this band, in kilobits per second or
>bits per second, depending on whether the parent 
>entry's  column specified
>kbps or pktps.
>  
>

Honestly, I just took it from the ovs-ofctl man page.  I dropped relative, and 
I'll send out a patch to update the ovs-ofctl man page, too.

> The documentation for "meter-add" uses two different names burst_size
> and burst_rate for the same argument.  (Maybe just "burst" would be a
> better name.)

I just went ahead and changed it to "burst".

> The documentation writes "__" for two underscores, but it's a little
> hard to tell in most fonts whether there are one or two underscores, so
> you might add (two underscores) to make it clear.

Good point.  Done.

> The command
>meter-add name unit action rate [burst]
> might be a little more natural if it were more like
>meter-add name action rate [burst]
> where "rate" was to be supplied as, e.g. 1000kbps or 5pktps.
> 
> Possibly the "list" output could be a little more human friendly in the
> same way, e.g.:
> 
>meter1:
>  drop: 10 kbps
>meter3:
>  drop: 100 kbps, 200 kb burst
> 
> but it's not a big deal either way.

Good suggestion.  I cleaned these up a bit.

> If the burst size is truly optional, it might be nice to make it
> optional in the schema, so that it could be omitted instead of set to
> 0.  The schema documentation for Meter_Band doesn't say that it's
> optional; it probably should explain what it means to set it to 0.

I originally had it that way, but it made dealing with the schema a bit more 
complicated without much benefit, since 0 is effectively the same.  I provided 
a description of a 0 value to ovn-nbctl, ovn-nb, and ovn-sb.

> Acked-by: Ben Pfaff 

Thanks!

--Justin


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

[ovs-dev] Project Management

[Ciclo de vida de un proyecto]

Dirigir y gestionar la ejecución del proyecto  

Project Management - Agosto 09 
   

Introducción: 

Este curso va dirigido a todo profesional que participe en el desarrollo de 
proyectos, donde es requerido homologar criterios, métodos y herramientas para 
el debido establecimiento del plan de gestión de tiempo de un proyecto, basado 
en procesos, normas y guías del Project Management Institute (PMI) así como 
incorporar animaciones y transiciones. 
  
Temario: 

Módulo I – Conceptos básicos de Project Management.
1. Que es un proyecto y que es Project Management.
2. Relación entre gestión de proyectos, gestión de operaciones y estrategia 
organizacional.
3. Rol de un Project Manager y del equipo de proyectos.
4. Interesados y gobierno del proyecto.
5. Ciclo de vida de un proyecto 

  Dirigido A: 

Directores, Gerentes, Administradores, Mercadólogos, Jefe de ventas, 
Proyectistas, Ejecutivos de Recursos Humanos, Personas interesadas en la 
gestión de proyectos.  
 
 
Al solicitar información, recibirá de manera gratuita y sin compromiso: 
Temario, costos, reseña del instructor y otros datos de interés. 

Responda por este medio con la frase: "Project" + Empresa + Nombre + Número de 
Teléfono. 

o marcando al: 045 + 5515546630 
 



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

Re: [ovs-dev] [ACL Meters 3/7] ovn-controller: Add "group-table-list" ovs-appctl command.

[Ben Pfaff]

On Mon, Jul 30, 2018 at 01:09:12PM -0700, Justin Pettit wrote:
> 
> > On Jul 30, 2018, at 10:43 AM, Ben Pfaff  wrote:
> > 
> > On Sun, Jul 29, 2018 at 11:46:34PM -0700, Justin Pettit wrote:
> >> Signed-off-by: Justin Pettit 
> > 
> > Thanks for the patches!
> > 
> > I think that group_table_list() is very similar or identical to
> > meter_table_list() in the previous patch.  Is it possible to write both
> > of them in terms of a common helper function?
> 
> Yes, that's true right now.  I have some patches in my local repo that
> will dump information specific to the type of table.  Unfortunately, I
> ran out of time for this release, so it provides just the most basic
> information right now.  I'd prefer to keep them separate since there
> should be a quick follow on that will make them quite different.

That's reasonable, thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] rhel: bug fix kmod-openvswitch spec file, rhel6

[Martin Xu]

Hi Ben,

I meant to say the previously merged patch. It was 89dd5819cf18 (rhel:
support kmod-openvswitch build against multiple kernels, rhel6). This patch
is meant to fix a bug introduced by 89dd5819cf18.

Martin

On Mon, Jul 30, 2018 at 1:28 PM, Ben Pfaff  wrote:

> On Sun, Jul 29, 2018 at 12:02:57AM -0700, Martin Xu wrote:
> > Previous patch removed the if condition for postun script by
> > mistake. The weak-update symlinks should be removed only for
> > uninstallation not upgrade.
> >
> > VMware-BZ: #2169383
> >
> > Signed-off-by: Martin Xu 
> > CC: Greg Rose 
> > CC: Ben Pfaff 
> > CC: Flavio Leitner 
>
> I looked for the change in the previous patch but it wasn't obvious to
> me.  Was this in commit 9207546be025 (rhel: bug fix kmod-openvswitch
> spec file, rhel6) or something earlier?
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] rhel: bug fix kmod-openvswitch spec file, rhel6

[Ben Pfaff]

On Sun, Jul 29, 2018 at 12:02:57AM -0700, Martin Xu wrote:
> Previous patch removed the if condition for postun script by
> mistake. The weak-update symlinks should be removed only for
> uninstallation not upgrade.
> 
> VMware-BZ: #2169383
> 
> Signed-off-by: Martin Xu 
> CC: Greg Rose 
> CC: Ben Pfaff 
> CC: Flavio Leitner 

I looked for the change in the previous patch but it wasn't obvious to
me.  Was this in commit 9207546be025 (rhel: bug fix kmod-openvswitch
spec file, rhel6) or something earlier?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] debian: Add ovn-detrace to ovn-common package.

[Ben Pfaff]

Thanks Han (and aginwala).  Applied to master.

On Mon, Jul 30, 2018 at 11:38:49AM -0700, aginwala wrote:
> Thanks for adding this.
> 
> Acked-by: aginwala 
> 
> On Sun, Jul 29, 2018 at 4:27 PM Han Zhou  wrote:
> 
> > Signed-off-by: Han Zhou 
> > ---
> >  debian/ovn-common.install  | 1 +
> >  debian/ovn-common.manpages | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/debian/ovn-common.install b/debian/ovn-common.install
> > index 99378af..e95ee6c 100644
> > --- a/debian/ovn-common.install
> > +++ b/debian/ovn-common.install
> > @@ -1,5 +1,6 @@
> >  usr/bin/ovn-nbctl
> >  usr/bin/ovn-sbctl
> >  usr/bin/ovn-trace
> > +usr/bin/ovn-detrace
> >  usr/share/openvswitch/scripts/ovn-ctl
> >  usr/share/openvswitch/scripts/ovndb-servers.ocf
> > diff --git a/debian/ovn-common.manpages b/debian/ovn-common.manpages
> > index 44cca92..249349e 100644
> > --- a/debian/ovn-common.manpages
> > +++ b/debian/ovn-common.manpages
> > @@ -5,3 +5,4 @@ ovn/utilities/ovn-ctl.8
> >  ovn/utilities/ovn-nbctl.8
> >  ovn/utilities/ovn-sbctl.8
> >  ovn/utilities/ovn-trace.8
> > +ovn/utilities/ovn-detrace.1
> > --
> > 2.1.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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 3/7] ovn-controller: Add "group-table-list" ovs-appctl command.

[Justin Pettit]

> On Jul 30, 2018, at 10:43 AM, Ben Pfaff  wrote:
> 
> On Sun, Jul 29, 2018 at 11:46:34PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit 
> 
> Thanks for the patches!
> 
> I think that group_table_list() is very similar or identical to
> meter_table_list() in the previous patch.  Is it possible to write both
> of them in terms of a common helper function?

Yes, that's true right now.  I have some patches in my local repo that will 
dump information specific to the type of table.  Unfortunately, I ran out of 
time for this release, so it provides just the most basic information right 
now.  I'd prefer to keep them separate since there should be a quick follow on 
that will make them quite different.

> Acked-by: Ben Pfaff 

Thanks!

--Justin


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

Re: [ovs-dev] [PATCHv2 2/2] dpif-netlink: Add meter support.

[Justin Pettit]

> On Jul 30, 2018, at 12:59 PM, Ben Pfaff  wrote:
> 
> On Fri, Jul 27, 2018 at 02:19:13PM -0700, Justin Pettit wrote:
>> From: Andy Zhou 
>> 
>> To work with kernel datapath that supports meter.
>> 
>> Signed-off-by: Andy Zhou 
>> Co-authored-by: Justin Pettit 
>> Signed-off-by: Justin Pettit 
>> Acked-by: Alin Gabriel Serdean 
>> ---
>> v1->v2: Addressed Ben's comments.
> 
> Acked-by: Ben Pfaff 

Thank you.  I pushed the series.

--Justin


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

[ovs-dev] MPLS performance question

[Carl Baldwin]

I recently tried pushing MPLS labels using OVS in a lab.

Before adding MPLS push to the mix, I had four hosts: two pairs, each
connected to a different set of TOR switches running VRRP. OVS (using
kernel datapath) had a flow to write the VRRP mac address and output to a
bond port. The bond is a Linux LACP bond, not an OVS bond. In this
scenario, the TORs would route the packets through a default route our
gateway routers to egress from the DC. This did fine and I was able to push
something around 42 Gbps using16 iperf2 TCP streams.

ovs-ofctl add-flow -OOpenFlow13 br0 "table=25, ip,
actions=output=${bond_port}"

My next step was to push an MPLS label onto the packet. The above flow
became this:

ovs-ofctl add-flow -OOpenFlow13 br0 "table=25, ip,
actions=push_mpls:0x8847,set_field:1048001->mpls_label,output=${bond_port}"

1048001 is a static label that I configure on the TORs which sends the
packet up to the same gateway using MPLS instead of IP routing. So, the
packets would take the same path out of the network but using an MPLS path.
With this change, things worked well from a functional perspective but the
performance fell drastically to around 30-40 Mbps.

I'm pretty confident in the network fabric because I tried the same
scenario using LInux MPLS and it performed well. From the network fabric's
point of view, it was exactly the same (static label through LACP bond to
VRRP mac).

I found in the faq [1] under "Does Open vSwitch support MPLS?" that "Open
vSwitch version 2.4 can match, push, or pop up to 3 MPLS labels and look
past the MPLS label into the encapsulated packet. It will have kernel
support for MPLS, yielding improved performance." I looked through the git
history and I don't see much evidence of this actually getting done for the
2.4 release. Is this faq accurate?

Carl

[1] http://docs.openvswitch.org/en/latest/faq/openflow/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 1/7] ovn: Use C strings instead of ds for extended tables.

[Ben Pfaff]

On Sun, Jul 29, 2018 at 11:46:32PM -0700, Justin Pettit wrote:
> Dynamic strings are not needed for the most part and are introduing
> additional conversions back and forth with C strings.
> 
> Signed-off-by: Justin Pettit 

For the series, assuming you think my comments are reasonable:

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

Re: [ovs-dev] [PATCHv2 2/2] dpif-netlink: Add meter support.

[Ben Pfaff]

On Fri, Jul 27, 2018 at 02:19:13PM -0700, Justin Pettit wrote:
> From: Andy Zhou 
> 
> To work with kernel datapath that supports meter.
> 
> Signed-off-by: Andy Zhou 
> Co-authored-by: Justin Pettit 
> Signed-off-by: Justin Pettit 
> Acked-by: Alin Gabriel Serdean 
> ---
> v1->v2: Addressed Ben's comments.

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

Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.

[Ben Pfaff]

On Sun, Jul 29, 2018 at 11:46:38PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 

Besides the comments I gave on patch 6 (oops), I suggest the following
for more consistent formatting:

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 6d4ed1e2c4ed..4f3cd48ce713 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -2157,7 +2157,7 @@ format_LOG(const struct ovnact_log *log, struct ds *s)
 ds_put_format(s, "severity=%s", log_severity_to_string(log->severity));
 
 if (log->meter) {
-ds_put_format(s, ",meter=%s", log->meter);
+ds_put_format(s, ", meter=%s", log->meter);
 }
 
 ds_put_cstr(s, ");");

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

Re: [ovs-dev] [PATCH 0/4] Kernel backports from net-next

[Justin Pettit]

> On Jul 16, 2018, at 5:55 PM, Yi-Hung Wei  wrote:
> 
> This patch series backports the following patches from net-next that modify
> ./net/openvswitch/* from the previous backport commit,
> 46e371f0 ("openvswitch: fix vport packet length check.").
> 
> 1) b2d6cee1 Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> 2) 72f17baf openvswitch: Don't swap table in nlattr_set() after 
> OVS_ATTR_NESTED is found
> 3) 2eb0f624 netfilter: add NAT support for shifted portmap ranges
> 4) ec9c7809 ovs: Remove rtnl_lock() from ovs_exit_net()
> 5) f0b07bb1 net: Introduce net_rwsem to protect net_namespace_list
> 6) 2f635cee net: Drop pernet_operations::async
> 7) 03fe2deb Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> 8) ec716650 net: Convert ovs_net_ops
> 9) ddc502df openvswitch: meter: fix the incorrect calculation of max delta_t
> 
> Patch 1 and 7 are merge commit, so they are ignored in the backport.
> Patch 4 and 5 are squashed into one patch since they are logically related.
> Patch 6 and 8 cancel out each other so they do not show up in this series.
> 
> The next patch 11efd5cb ("openvswitch: Support conntrack zone limit") has
> some external dependency on netfilter backend (nf_conncount), I will start
> to backport it that once the nf_conncount backend optimization patch series
> are upstream.

Thank you for the doing these backports.  I pushed these to master and 
branch-2.10.  I also brought the "Don't swap table in nlattr_set() after 
OVS_ATTR_NESTED is found" all the way back to 2.5.

Thanks!

--Justin


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

Re: [ovs-dev] [ACL Meters 6/7] ofproto: Add support for specifying a meter in controller actions.

[Ben Pfaff]

On Mon, Jul 30, 2018 at 12:49:51PM -0700, Ben Pfaff wrote:
> On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
> > Signed-off-by: Justin Pettit 
> 
> In ofproto_check_ofpacts(), the other checks, if they fail, return an
> error and prevent the flow from being added.  The new one doesn't; is
> the difference intentional?  Similarly, is there anything that prevents
> a meter from being deleted while still in use and, if that happens, does
> anything particularly bad happen?
> 
> In ovs-ofctl.8.in, the wording seems a little vague because "associate"
> is such a weak word.  Maybe change
> Associate packets sent to the controller with meter \fIid\fR.
> to
> Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
> that this action sends to the controller.
> or something similarly descriptive
> 
> Do only "drop" actions make sense for controller metering?
> 
> Please add some tests for the new action to the test "ovn -- action
> parsing" in ovn.at.
> 
> Please document the new action in ovn-sb.xml.
> 
> Acked-by: Ben Pfaff 

Oops, some of the above was for patch 6 and some for patch 7.  I think
you can figure it out though.

I'll look at both again to see if I missed anything.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ACL Meters 6/7] ofproto: Add support for specifying a meter in controller actions.

[Ben Pfaff]

On Sun, Jul 29, 2018 at 11:46:37PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 

In ofproto_check_ofpacts(), the other checks, if they fail, return an
error and prevent the flow from being added.  The new one doesn't; is
the difference intentional?  Similarly, is there anything that prevents
a meter from being deleted while still in use and, if that happens, does
anything particularly bad happen?

In ovs-ofctl.8.in, the wording seems a little vague because "associate"
is such a weak word.  Maybe change
Associate packets sent to the controller with meter \fIid\fR.
to
Use meter \fIid\fR to rate-limit the OpenFlow packet-in messages
that this action sends to the controller.
or something similarly descriptive

Do only "drop" actions make sense for controller metering?

Please add some tests for the new action to the test "ovn -- action
parsing" in ovn.at.

Please document the new action in ovn-sb.xml.

Acked-by: Ben Pfaff 

Thanks,

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

Re: [ovs-dev] [PATCH v1] rhel: support kmod build against mulitple kernel versions, fedora

[Flavio Leitner]

On Fri, Jul 27, 2018 at 11:19:58AM -0700, Martin Xu wrote:
> Hi Flavio,
> 
> Thanks for the review. I was thinking of using this to prevent a user to
> directly install openvswitch-kmod rpm built with the fedora spec file when
> the system already has the kmod-openvswitch built from rhel6 spec file.
> Packages are named differently. In that case, it's not a matter of
> versions. They could be built off the identical source tree, but shouldn't
> coexist. Perhaps there's other ways to implement what I intended to do
> here, if you have any suggestions?

If the files are the same, rpm will complain about the file conflicts
when it tries to install. In that case we only need Conflicts and perhaps
it's ok to not add a version.

But for obsoletes, it essentially says that your package should
replace the other one. If that is what you want, that's okay too.
However, we will be unable to roll back that in the future if we
need to revive kmod-openvswitch as this package once built cannot
be changed and it will always replace the other one.

When you add a 'provides', you will create a "virtual package" named
after the provides, with the version and release you want, so RPM can
compare versions and releases as usual. For example:

Provides: kmod-openvswitch = %{version}-%{release}

That means you package is also known to rpm as 
kmod-openvswitch-2.10.0-10.fc29

fbl


> 
> Best,
> Martin
> 
> On Wed, Jul 25, 2018 at 9:10 AM, Flavio Leitner  wrote:
> 
> > On Fri, Jul 20, 2018 at 03:24:53PM -0700, Martin Xu wrote:
> > > This patch ports changes from kmod rhel6 spec file to fedora spec file,
> > > to support packaging kernel modules built against multiple versions of
> > > kernel sources.
> > >
> > > RHEL 7.4 introduced backward incompatible changes in the kernel. As
> > > a result, prebuilt PRM packages against kernels newer than 693.17.1
> > > will cannot be used on systems with older kernels, vice versa.
> > >
> > > Intended to work only on RHEL 7.4 (kernel version 3.10.0-693.yy.zz).
> > > This patch allows multiple kernel version numbers delimited by
> > > whitespace to be passed as variable "kversion". The result RPM packages
> > > the kernel module .ko files from all specified kernel versions. For
> > > example,
> > >
> > > make rpm-fedora-kmod \
> > > RPMBUILD_OPT='-D "kversion 3.10.0-693.1.1.el7.x86_64 \
> > > 3.10.0-693.17.1.el7.x86_64"'
> > >
> > > By default, make tries to build against the current running kernel.
> > >
> > > This patch also includes a script to update the weak-update symlinks
> > > if the system kernel version is upgraded or downgraded after
> > > openvswitch-kmod is installed.
> > >
> > > Signed-off-by: Martin Xu 
> > > CC: Greg Rose 
> > > CC: Flavio Leitner 
> > > ---
> > >  rhel/openvswitch-kmod-fedora.spec.in | 86
> > +++-
> > >  1 file changed, 55 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/rhel/openvswitch-kmod-fedora.spec.in b/rhel/
> > openvswitch-kmod-fedora.spec.in
> > > index c0cd298..24f8290 100644
> > > --- a/rhel/openvswitch-kmod-fedora.spec.in
> > > +++ b/rhel/openvswitch-kmod-fedora.spec.in
> > > @@ -1,6 +1,6 @@
> > >  # Spec file for Open vSwitch.
> > >
> > > -# Copyright (C) 2009, 2010, 2015 Nicira Networks, Inc.
> > > +# Copyright (C) 2009, 2010, 2015, 2018 Nicira Networks, Inc.
> > >  #
> > >  # Copying and distribution of this file, with or without modification,
> > >  # are permitted in any medium without royalty provided the copyright
> > > @@ -26,6 +26,9 @@ Release: 1%{?dist}
> > >  Source: openvswitch-%{version}.tar.gz
> > >  #Source1: openvswitch-init
> > >  Buildroot: /tmp/openvswitch-xen-rpm
> > > +Provides: kmod-openvswitch
> > > +Conflicts: kmod-openvswitch
> > > +Obsoletes: kmod-openvswitch
> >
> > Usually the above is versioned to avoid future issues.
> > e.g.: Conflicts: kmod-openvswitch < %{version}-%{release}
> >
> > I didn't spot anything else other than the above, thanks!
> > fbl
> >
> > >
> > >  %description
> > >  Open vSwitch provides standard network bridging functions augmented with
> > > @@ -36,55 +39,76 @@ traffic. This package contains the kernel modules.
> > >  %setup -q -n openvswitch-%{version}
> > >
> > >  %build
> > > -%configure --with-linux=/lib/modules/%{kernel}/build --enable-ssl
> > > -make %{_smp_mflags} -C datapath/linux
> > > +for kv in %{kversion}; do
> > > +mkdir -p _$kv
> > > +(cd _$kv && /bin/cp -f ../configure . && %configure --srcdir=.. \
> > > +--with-linux=/usr/src/kernels/${kv}/ --enable-ssl)
> > > +make %{_smp_mflags} -C _$kv/datapath/linux
> > > +done
> > >
> > >  %install
> > > +export INSTALL_MOD_DIR=extra/openvswitch
> > >  rm -rf $RPM_BUILD_ROOT
> > > -make INSTALL_MOD_PATH=$RPM_BUILD_ROOT -C datapath/linux modules_install
> > > +for kv in %{kversion}; do
> > > +make INSTALL_MOD_PATH=$RPM_BUILD_ROOT -C _$kv/datapath/linux
> > modules_install
> > > +done
> > >  mkdir -p $RPM_BUILD_ROOT/etc/depmod.d
> > > -for module in $RPM_BUILD_ROOT/lib/mo

Re: [ovs-dev] [PATCH] dpif-netdev: Fix zero length keys insertion to EMC.

[Stokes, Ian]

> 'key.len' should be calculated before inserting to EMC, otherwise
> resulting entry will match with any packet with the same hash.
> 
> CC: Yipeng Wang 
> Fixes: 60d8ccae135f ("dpif-netdev: Add SMC cache after EMC cache")
> Signed-off-by: Ilya Maximets 

Thanks Ilya,

I'll apply this to dpdk merge, will be part of the next pull request.

Thanks
Ian

> ---
>  lib/dpif-netdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 13a20f0..0a833dc
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -5810,9 +5810,9 @@ smc_lookup_batch(struct dp_netdev_pmd_thread *pmd,
>  if (OVS_LIKELY(dpcls_rule_matches_key(&flow->cr,
> &keys[i]) &&
>  flow->flow.in_port.odp_port == packet-
> >md.in_port.odp_port)) {
>  /* SMC hit and emc miss, we insert into EMC */
> -emc_probabilistic_insert(pmd, &keys[i], flow);
>  keys[i].len =
> 
> netdev_flow_key_size(miniflow_n_values(&keys[i].mf));
> +emc_probabilistic_insert(pmd, &keys[i], flow);
>  dp_netdev_queue_batches(packet, flow,
>  miniflow_get_tcp_flags(&keys[i].mf), batches,
> n_batches);
>  n_smc_hit++;
> --
> 2.7.4

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

Re: [ovs-dev] [PATCHv2] netdev-dpdk: Fix failure to configure flow control at netdev-init.

[Stokes, Ian]

> Configuring flow control at ixgbe netdev-init is throwing error in port
> start.
> 
> For eg: without this fix, user cannot configure flow control on ixgbe dpdk
> port as below,
> 
> "
> ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
> options:dpdk-devargs=:05:00.1 options:rx-flow-ctrl=true "
> 
> Instead,  it must be configured as two different commands,
> 
> "
> ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
>options:dpdk-devargs=:05:00.1
> ovs-vsctl set Interface dpdk0 options:rx-flow-ctrl=true "
> 
> The DPDK ixgbe driver is now validating all the 'rte_eth_fc_conf' fields
> before trying to configuring the dpdk ethdev. Hence OVS can no longer set
> the 'dont care' fields to just '0' as before. This commit make sure all
> the 'rte_eth_fc_conf' fields are populated with default values before the
> dev init.
> 

Thanks for the patch Sugesh, I think this is a better approach, I've validated 
with various SRIOV VFs as well as i350,ixgbe and i40e drivers, all were ok. 
Some minor comments below.

> Signed-off-by: Sugesh Chandran 
> ---
> V1 -> V2
> Read DPDK port flow-control parameters only when reconfiguration is
> required. This will avoid flow control read error on unsupported ports.

I think it's worth mentioning this in the commit message as rather than just in 
the version change.

> 
> ---
>  lib/netdev-dpdk.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index bb4d60f..11eebe3
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1065,14 +1065,6 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> 
>  mbp_priv = rte_mempool_get_priv(dev->dpdk_mp->mp);
>  dev->buf_size = mbp_priv->mbuf_data_room_size - RTE_PKTMBUF_HEADROOM;
> -
> -/* Get the Flow control configuration for DPDK-ETH */
> -diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
> -if (diag) {
> -VLOG_DBG("cannot get flow control parameters on port
> "DPDK_PORT_ID_FMT
> - ", err=%d", dev->port_id, diag);
> -}
> -
>  return 0;
>  }
> 
> @@ -1776,6 +1768,12 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args,
>  if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg)
> {
>  dev->fc_conf.mode = fc_mode;
>  dev->fc_conf.autoneg = autoneg;
> +/* Get the Flow control configuration for DPDK-ETH */
> +err = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
> +if (err) {
> +VLOG_INFO("cannot get flow control parameters on port "
> +DPDK_PORT_ID_FMT", err=%d", dev->port_id, err);

This should probably be an error or at least a warning instead of info as in 
this case someone has attempted to configure flow control but an error 
occurred. Also minor nit but the First word of the message should be 
capitalized. i.e. 'cannot' -> 'Cannot'.

Thanks
Ian
> +}
>  dpdk_eth_flow_ctrl_setup(dev);
>  }
> 
> --
> 2.7.4

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

Re: [ovs-dev] [ACL Meters 5/7] ovn: Support configuring meters through SB Meter table.

[Ben Pfaff]

On Sun, Jul 29, 2018 at 11:46:36PM -0700, Justin Pettit wrote:
> Add the ability to configure meters through the newly introduced Meter
> table in the Southbound database.  Previously, meters were configured by
> providing strings to describe the meter in the extended meter table.
> This patch changes the behavior so that the extended meter table's
> strings are references to names in the Meter table.  The old behavior is
> still supported if the extended meter table entry begins with "__string: "
> 
> Signed-off-by: Justin Pettit 

I have the following suggestions.  Neither is important.

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 01981a79480b..d810d413f207 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -907,7 +907,7 @@ ofctrl_can_put(void)
  *
  * Replaces the group table and meter table on the switch, if possible,
  * by the contents of '->desired'.  Regardless of whether the table is
- * updated, this deletes all the groups ore metersjfrom the '->desired'
+ * updated, this deletes all the groups ore meters from the '->desired'
  * and frees them. (The hmap itself isn't destroyed.)
  *
  * Sends conntrack flush messages to each zone in 'pending_ct_zones' that
@@ -1080,10 +1080,10 @@ ofctrl_put(struct hmap *flow_table, struct shash 
*pending_ct_zones,
 struct ovn_extend_table_info *m_installed, *next_meter;
 EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
 /* Delete the meter. */
-struct ofputil_meter_mod mm;
-memset(&mm, 0, sizeof mm);
-mm.command = OFPMC13_DELETE;
-mm.meter.meter_id = m_installed->table_id;
+struct ofputil_meter_mod mm = {
+.command = OFPMC13_DELETE,
+.meter.meter_id = m_installed->table_id,
+};
 add_meter_mod(&mm, &msgs);
 
 ovn_extend_table_remove(meters, m_installed);

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

Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.

[Mark Michelson]

Hi Justin,

I took a look through the patch series, and this is the only one that I 
had some immediate feedback on.


First, it would be nice if we could refer to Meters by name when issuing 
DB commands. For instance `ovn-nbctl add Meter  bands UUID>`.


Second, I noticed that the algorithm for computing southbound meter 
bands can result in a larger number of bands than is in the northbound 
table. For instance, you could issue the following:


ovn-nbctl --id=@id create Meter_Band action=drop rate=1000 -- \
create Meter name=foo unit=kbps band=@id -- \
create Meter name=bar unit=kbps band=@id

In the northbound database, you'll have one entry in the meter_band 
table. In the southbound database, you'll have two entries in the 
meter_band table. The algorithm does not take into account that multiple 
northbound meters may refer to the same meter_band. I'm not sure how big 
an issue this is (or really if it is an issue), but it surprised me a 
bit when I was playing around with it.



On 07/30/2018 02:46 AM, Justin Pettit wrote:

Add support for configuring meters through the Meter and Meter_Band
tables in the Northbound database.  This commit also has ovn-northd
sync those tables between the Northbound and Southbound databases.

Add support for configuring meters with ovn-nbctl.

Signed-off-by: Justin Pettit 
---
  ovn/northd/ovn-northd.c   | 145 ++
  ovn/ovn-nb.ovsschema  |  33 +++-
  ovn/ovn-nb.xml|  80 +++
  ovn/ovn-sb.ovsschema  |  27 ++-
  ovn/ovn-sb.xml|  72 +
  ovn/utilities/ovn-nbctl.8.xml |  50 
  ovn/utilities/ovn-nbctl.c | 143 +
  tests/ovn-nbctl.at|  58 ++
  8 files changed, 604 insertions(+), 4 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 04a072ba8de7..45557170edc8 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -6606,6 +6606,140 @@ sync_port_groups(struct northd_context *ctx)
  shash_destroy(&sb_port_groups);
  }
  
+struct band_entry {

+int64_t rate;
+int64_t burst_size;
+const char *action;
+};
+
+static int
+band_cmp(const void *band1_, const void *band2_)
+{
+const struct band_entry *band1p = band1_;
+const struct band_entry *band2p = band2_;
+
+if (band1p->rate != band2p->rate) {
+return band1p->rate > band2p->rate ? -1 : 1;
+} else if (band1p->burst_size != band2p->burst_size) {
+return band1p->burst_size > band2p->burst_size ? -1 : 1;
+} else {
+return strcmp(band1p->action, band2p->action);
+}
+}
+
+static bool
+bands_need_update(const struct nbrec_meter *nb_meter,
+  const struct sbrec_meter *sb_meter)
+{
+if (nb_meter->n_bands != sb_meter->n_bands) {
+return true;
+}
+
+/* A single band is the most common scenario, so speed up that
+ * check. */
+if (nb_meter->n_bands == 1) {
+struct nbrec_meter_band *nb_band = nb_meter->bands[0];
+struct sbrec_meter_band *sb_band = sb_meter->bands[0];
+
+return !(nb_band->rate == sb_band->rate
+ && nb_band->burst_size == sb_band->burst_size
+ && !strcmp(sb_band->action, nb_band->action));
+}
+
+/* Place the Northbound entries in sorted order. */
+struct band_entry *nb_bands;
+nb_bands = xmalloc(sizeof *nb_bands * nb_meter->n_bands);
+for (size_t i = 0; i < nb_meter->n_bands; i++) {
+struct nbrec_meter_band *nb_band = nb_meter->bands[i];
+
+nb_bands[i].rate = nb_band->rate;
+nb_bands[i].burst_size = nb_band->burst_size;
+nb_bands[i].action = nb_band->action;
+}
+qsort(nb_bands, nb_meter->n_bands, sizeof *nb_bands, band_cmp);
+
+/* Place the Southbound entries in sorted order. */
+struct band_entry *sb_bands;
+sb_bands = xmalloc(sizeof *sb_bands * sb_meter->n_bands);
+for (size_t i = 0; i < sb_meter->n_bands; i++) {
+struct sbrec_meter_band *sb_band = sb_meter->bands[i];
+
+sb_bands[i].rate = sb_band->rate;
+sb_bands[i].burst_size = sb_band->burst_size;
+sb_bands[i].action = sb_band->action;
+}
+qsort(sb_bands, sb_meter->n_bands, sizeof *sb_bands, band_cmp);
+
+bool need_update = false;
+for (size_t i = 0; i < nb_meter->n_bands; i++) {
+if (nb_bands[i].rate != sb_bands[i].rate
+|| nb_bands[i].burst_size != sb_bands[i].burst_size
+|| strcmp(nb_bands[i].action, nb_bands[i].action)) {
+need_update = true;
+goto done;
+}
+}
+
+done:
+free(nb_bands);
+free(sb_bands);
+
+return need_update;
+}
+
+/* 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.
+ */
+static void
+sync_meters(struct northd_context *ctx)
+{
+struct shash sb_meters 

Re: [ovs-dev] [PATCH] debian: Add ovn-detrace to ovn-common package.

[aginwala]

Thanks for adding this.

Acked-by: aginwala 

On Sun, Jul 29, 2018 at 4:27 PM Han Zhou  wrote:

> Signed-off-by: Han Zhou 
> ---
>  debian/ovn-common.install  | 1 +
>  debian/ovn-common.manpages | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/debian/ovn-common.install b/debian/ovn-common.install
> index 99378af..e95ee6c 100644
> --- a/debian/ovn-common.install
> +++ b/debian/ovn-common.install
> @@ -1,5 +1,6 @@
>  usr/bin/ovn-nbctl
>  usr/bin/ovn-sbctl
>  usr/bin/ovn-trace
> +usr/bin/ovn-detrace
>  usr/share/openvswitch/scripts/ovn-ctl
>  usr/share/openvswitch/scripts/ovndb-servers.ocf
> diff --git a/debian/ovn-common.manpages b/debian/ovn-common.manpages
> index 44cca92..249349e 100644
> --- a/debian/ovn-common.manpages
> +++ b/debian/ovn-common.manpages
> @@ -5,3 +5,4 @@ ovn/utilities/ovn-ctl.8
>  ovn/utilities/ovn-nbctl.8
>  ovn/utilities/ovn-sbctl.8
>  ovn/utilities/ovn-trace.8
> +ovn/utilities/ovn-detrace.1
> --
> 2.1.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] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.

[Ben Pfaff]

On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
> Add support for configuring meters through the Meter and Meter_Band
> tables in the Northbound database.  This commit also has ovn-northd
> sync those tables between the Northbound and Southbound databases.
> 
> Add support for configuring meters with ovn-nbctl.
> 
> Signed-off-by: Justin Pettit 

Thanks for the patches!

band_cmp() uses the form "a > b ? -1 : 1" a couple of times.  I believe
that this will sort 'a' and 'b' into descending order.  Is that what you
want?

(Oh, actually I see it doesn't matter, it just needs to be consistent.)

In ovn-[ns]b.xml, for consistency with the rest of the tables,
title="..." in  start tags should not include the word "table"
and maybe should give better descriptions.

The Meter_Band table says the following.  I'm not sure what a "relative
rate limit" is.  It kind of sounds like it would be an additive one,
where if you have a first band with a rate of 500 Mbps then the second
one with a rate of 250 Mbps would effectively be 500 + 250 = 750 Mbps.
But I don't think you really mean that; I think it's actually an
absolute rate.  Maybe by relative you just mean that the unit is
specified by the Meter.


  
The relative rate limit for this band, in kilobits per second or
bits per second, depending on whether the parent 
entry's  column specified
kbps or pktps.
  


The documentation for "meter-add" uses two different names burst_size
and burst_rate for the same argument.  (Maybe just "burst" would be a
better name.)

The documentation writes "__" for two underscores, but it's a little
hard to tell in most fonts whether there are one or two underscores, so
you might add (two underscores) to make it clear.

The command
meter-add name unit action rate [burst]
might be a little more natural if it were more like
meter-add name action rate [burst]
where "rate" was to be supplied as, e.g. 1000kbps or 5pktps.

Possibly the "list" output could be a little more human friendly in the
same way, e.g.:

meter1:
  drop: 10 kbps
meter3:
  drop: 100 kbps, 200 kb burst

but it's not a big deal either way.

If the burst size is truly optional, it might be nice to make it
optional in the schema, so that it could be omitted instead of set to
0.  The schema documentation for Meter_Band doesn't say that it's
optional; it probably should explain what it means to set it to 0.

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

Re: [ovs-dev] [ACL Meters 3/7] ovn-controller: Add "group-table-list" ovs-appctl command.

[Ben Pfaff]

On Sun, Jul 29, 2018 at 11:46:34PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 

Thanks for the patches!

I think that group_table_list() is very similar or identical to
meter_table_list() in the previous patch.  Is it possible to write both
of them in terms of a common helper function?

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

Re: [ovs-dev] [PATCH v3 1/6] datapath: add transport ports in route lookup to enable IPsec policy match.

[Qiuyu Xiao]

Greg gave me Reviewed-by and Tested-by tags. I will add those to
commit messages.

Thanks,
Qiuyu

On Mon, Jul 30, 2018 at 9:49 AM, Ben Pfaff  wrote:
> On Fri, Jul 27, 2018 at 01:44:29PM -0700, Qiuyu Xiao wrote:
>> This patch adds transport ports information for route lookup so that IPsec
>> can select tunnel traffic (geneve, stt, vxlan) to do encryption.
>>
>> The patch was tested for geneve, stt, and vxlan tunnel and the results
>> show that IPsec policy can be set to only match the corresponding tunnel
>> traffic.
>>
>> Signed-off-by: Qiuyu Xiao 
>
> I think that this patch should probably be broken up into three:
>
> 1. Geneve changes.  These changes need to go to upstream Linux before
>we commit them to the OVS repo.
>
> 2. VXLAN changes.  As I understand it, similar changes are already
>upstream, so we can put them into OVS right away.
>
> 3. STT changes.  STT is not in upstream Linux, so we can put these into
>OVS right away too.
>
> I think that Greg has already positively reviewed this.  Did he give you
> an Acked-by tag?  If he did, then you should add it to the commit
> message.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 1/6] datapath: add transport ports in route lookup to enable IPsec policy match.

[Ben Pfaff]

On Fri, Jul 27, 2018 at 01:44:29PM -0700, Qiuyu Xiao wrote:
> This patch adds transport ports information for route lookup so that IPsec
> can select tunnel traffic (geneve, stt, vxlan) to do encryption.
> 
> The patch was tested for geneve, stt, and vxlan tunnel and the results
> show that IPsec policy can be set to only match the corresponding tunnel
> traffic.
> 
> Signed-off-by: Qiuyu Xiao 

I think that this patch should probably be broken up into three:

1. Geneve changes.  These changes need to go to upstream Linux before
   we commit them to the OVS repo.

2. VXLAN changes.  As I understand it, similar changes are already
   upstream, so we can put them into OVS right away.

3. STT changes.  STT is not in upstream Linux, so we can put these into
   OVS right away too.

I think that Greg has already positively reviewed this.  Did he give you
an Acked-by tag?  If he did, then you should add it to the commit
message.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Introduce ovs-appctl command to monitor HVs sb connection status

[Mark Michelson]

Looks good to me. Ship it!

Acked-by: Mark Michelson 

On 07/30/2018 11:10 AM, Lorenzo Bianconi wrote:

Add 'connection-status' command to ovs-appctl utility in order to check
if a given chassis is currently connected to SB db

Co-authored-by: aginwala 
Signed-off-by: aginwala 
Signed-off-by: Lorenzo Bianconi 
---
Changes since v1:
- add unit tests for ovs-appctl -t ovn-controller connection-status
   command
- use jsonrpc_session_is_connected() in ovsdb_idl_is_connected routine
---
  lib/ovsdb-idl.c |  6 +
  lib/ovsdb-idl.h |  1 +
  ovn/controller/ovn-controller.8.xml |  5 +
  ovn/controller/ovn-controller.c | 17 +++
  tests/ovn-controller.at | 34 +
  5 files changed, 63 insertions(+)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index ae0a55c3a..f181ae831 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -922,6 +922,12 @@ ovsdb_idl_is_alive(const struct ovsdb_idl *idl)
 idl->state != IDL_S_ERROR;
  }
  
+bool

+ovsdb_idl_is_connected(const struct ovsdb_idl *idl)
+{
+return jsonrpc_session_is_connected(idl->session);
+}
+
  /* Returns the last error reported on a connection by 'idl'.  The return value
   * is 0 only if no connection made by 'idl' has ever encountered an error and
   * a negative response to a schema request has never been received. See
diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index ea18b22f5..5f933b628 100644
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -80,6 +80,7 @@ void ovsdb_idl_force_reconnect(struct ovsdb_idl *);
  void ovsdb_idl_verify_write_only(struct ovsdb_idl *);
  
  bool ovsdb_idl_is_alive(const struct ovsdb_idl *);

+bool ovsdb_idl_is_connected(const struct ovsdb_idl *idl);
  int ovsdb_idl_get_last_error(const struct ovsdb_idl *);
  
  void ovsdb_idl_set_probe_interval(const struct ovsdb_idl *, int probe_interval);

diff --git a/ovn/controller/ovn-controller.8.xml 
b/ovn/controller/ovn-controller.8.xml
index 0eff2113f..0861edbc4 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -388,6 +388,11 @@
  0x800.


+
+  connection-status
+  
+Show OVN SBDB connection status for the chassis.
+  

  
  
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c

index 6ee72a9fa..3338d3318 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -66,6 +66,7 @@ VLOG_DEFINE_THIS_MODULE(main);
  static unixctl_cb_func ovn_controller_exit;
  static unixctl_cb_func ct_zone_list;
  static unixctl_cb_func inject_pkt;
+static unixctl_cb_func ovn_controller_conn_show;
  
  #define DEFAULT_BRIDGE_NAME "br-int"

  #define DEFAULT_PROBE_INTERVAL_MSEC 5000
@@ -588,6 +589,9 @@ main(int argc, char *argv[])
  ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
  ovsdb_idl_set_leader_only(ovnsb_idl_loop.idl, false);
  
+unixctl_command_register("connection-status", "", 0, 0,

+ ovn_controller_conn_show, ovnsb_idl_loop.idl);
+
  struct ovsdb_idl_index *sbrec_chassis_by_name
  = chassis_index_create(ovnsb_idl_loop.idl);
  struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
@@ -1061,3 +1065,16 @@ update_probe_interval(const struct 
ovsrec_open_vswitch_table *ovs_table,
  
  ovsdb_idl_set_probe_interval(ovnsb_idl, interval);

  }
+
+static void
+ovn_controller_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED, void *idl_)
+{
+char *result = "not connected";
+struct ovsdb_idl *idl = idl_;
+
+if (idl && ovsdb_idl_is_connected(idl)) {
+   result = "connected";
+}
+unixctl_command_reply(conn, result);
+}
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index ecc6a50da..13e88a0cc 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -228,3 +228,37 @@ as ovn-sb
  OVS_APP_EXIT_AND_WAIT([ovsdb-server])
  
  AT_CLEANUP

+
+# Check ovn-controller connection status to Southbound database
+AT_SETUP([ovn-controller - check sbdb connection])
+AT_KEYWORDS([ovn])
+ovn_init_db ovn-sb
+
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl \
+-- add-br br-phys \
+-- add-br br-eth0 \
+-- add-br br-eth1 \
+-- add-br br-eth2
+ovn_attach n1 br-phys 192.168.0.1
+
+check_sbdb_connection () {
+test "$(ovs-appctl -t ovn-controller connection-status)" = "$1"
+}
+
+OVS_WAIT_UNTIL([check_sbdb_connection connected])
+
+ovs-vsctl set open . external_ids:ovn-remote=tcp:192.168.0.10:6642
+OVS_WAIT_UNTIL([check_sbdb_connection 'not connected'])
+
+# reset the remote for clean-up
+ovs-vsctl set open . external_ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock
+# Gracefully terminate daemons
+OVN_CLEANUP_SBOX([hv])
+OVN_CLEANUP_VSWITCH([main])
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+AT_CLEANUP



___

[ovs-dev] [PATCH v2] Introduce ovs-appctl command to monitor HVs sb connection status

[Lorenzo Bianconi]

Add 'connection-status' command to ovs-appctl utility in order to check
if a given chassis is currently connected to SB db

Co-authored-by: aginwala 
Signed-off-by: aginwala 
Signed-off-by: Lorenzo Bianconi 
---
Changes since v1:
- add unit tests for ovs-appctl -t ovn-controller connection-status
  command
- use jsonrpc_session_is_connected() in ovsdb_idl_is_connected routine
---
 lib/ovsdb-idl.c |  6 +
 lib/ovsdb-idl.h |  1 +
 ovn/controller/ovn-controller.8.xml |  5 +
 ovn/controller/ovn-controller.c | 17 +++
 tests/ovn-controller.at | 34 +
 5 files changed, 63 insertions(+)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index ae0a55c3a..f181ae831 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -922,6 +922,12 @@ ovsdb_idl_is_alive(const struct ovsdb_idl *idl)
idl->state != IDL_S_ERROR;
 }
 
+bool
+ovsdb_idl_is_connected(const struct ovsdb_idl *idl)
+{
+return jsonrpc_session_is_connected(idl->session);
+}
+
 /* Returns the last error reported on a connection by 'idl'.  The return value
  * is 0 only if no connection made by 'idl' has ever encountered an error and
  * a negative response to a schema request has never been received. See
diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index ea18b22f5..5f933b628 100644
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -80,6 +80,7 @@ void ovsdb_idl_force_reconnect(struct ovsdb_idl *);
 void ovsdb_idl_verify_write_only(struct ovsdb_idl *);
 
 bool ovsdb_idl_is_alive(const struct ovsdb_idl *);
+bool ovsdb_idl_is_connected(const struct ovsdb_idl *idl);
 int ovsdb_idl_get_last_error(const struct ovsdb_idl *);
 
 void ovsdb_idl_set_probe_interval(const struct ovsdb_idl *, int 
probe_interval);
diff --git a/ovn/controller/ovn-controller.8.xml 
b/ovn/controller/ovn-controller.8.xml
index 0eff2113f..0861edbc4 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -388,6 +388,11 @@
 0x800.
   
   
+
+  connection-status
+  
+Show OVN SBDB connection status for the chassis.
+  
   
 
 
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 6ee72a9fa..3338d3318 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -66,6 +66,7 @@ VLOG_DEFINE_THIS_MODULE(main);
 static unixctl_cb_func ovn_controller_exit;
 static unixctl_cb_func ct_zone_list;
 static unixctl_cb_func inject_pkt;
+static unixctl_cb_func ovn_controller_conn_show;
 
 #define DEFAULT_BRIDGE_NAME "br-int"
 #define DEFAULT_PROBE_INTERVAL_MSEC 5000
@@ -588,6 +589,9 @@ main(int argc, char *argv[])
 ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
 ovsdb_idl_set_leader_only(ovnsb_idl_loop.idl, false);
 
+unixctl_command_register("connection-status", "", 0, 0,
+ ovn_controller_conn_show, ovnsb_idl_loop.idl);
+
 struct ovsdb_idl_index *sbrec_chassis_by_name
 = chassis_index_create(ovnsb_idl_loop.idl);
 struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
@@ -1061,3 +1065,16 @@ update_probe_interval(const struct 
ovsrec_open_vswitch_table *ovs_table,
 
 ovsdb_idl_set_probe_interval(ovnsb_idl, interval);
 }
+
+static void
+ovn_controller_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED, void *idl_)
+{
+char *result = "not connected";
+struct ovsdb_idl *idl = idl_;
+
+if (idl && ovsdb_idl_is_connected(idl)) {
+   result = "connected";
+}
+unixctl_command_reply(conn, result);
+}
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index ecc6a50da..13e88a0cc 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -228,3 +228,37 @@ as ovn-sb
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
 AT_CLEANUP
+
+# Check ovn-controller connection status to Southbound database
+AT_SETUP([ovn-controller - check sbdb connection])
+AT_KEYWORDS([ovn])
+ovn_init_db ovn-sb
+
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl \
+-- add-br br-phys \
+-- add-br br-eth0 \
+-- add-br br-eth1 \
+-- add-br br-eth2
+ovn_attach n1 br-phys 192.168.0.1
+
+check_sbdb_connection () {
+test "$(ovs-appctl -t ovn-controller connection-status)" = "$1"
+}
+
+OVS_WAIT_UNTIL([check_sbdb_connection connected])
+
+ovs-vsctl set open . external_ids:ovn-remote=tcp:192.168.0.10:6642
+OVS_WAIT_UNTIL([check_sbdb_connection 'not connected'])
+
+# reset the remote for clean-up
+ovs-vsctl set open . external_ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock
+# Gracefully terminate daemons
+OVN_CLEANUP_SBOX([hv])
+OVN_CLEANUP_VSWITCH([main])
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+AT_CLEANUP
-- 
2.17.1

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

Re: [ovs-dev] [PATCH v2 0/3] Port Group fixes

[Mark Michelson]

Wow bad timing on my part for my review of v1.

Acked-by: Mark Michelson 

On 07/30/2018 10:37 AM, Jakub Sitnicki wrote:

This series addresses two issues noticed with Port Groups:

(1) Port Group name cannot be used to refer to a row in NB DB.
(2) LSP dynamic addresses don't get copied to port group address sets.

The series depends on recent fixes for IPAM from Mark Michelson [1].

Thanks,
Jakub

[1] https://patchwork.ozlabs.org/patch/947155/

v1 -> v2:
- Check if LSP addresses are static before treating them as such to avoid a
   warning. Reported by Han Zhou.


Jakub Sitnicki (3):
   ovn-nbctl: Allow referring to port groups by name.
   ovn-northd: Make use of svec for storing lists of addresses.
   ovn-northd: Propagate dynamic addresses to port group address sets.

  ovn/northd/ovn-northd.c   |  77 
  ovn/utilities/ovn-nbctl.c |   3 ++
  tests/ovn-nbctl.at|  14 ++
  tests/ovn.at  | 109 ++
  4 files changed, 164 insertions(+), 39 deletions(-)

--
2.14.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 0/3] Port Group fixes

[Mark Michelson]

For the series (aside from Han's finding):

Acked-by: Mark Michelson 

On 07/26/2018 06:51 AM, Jakub Sitnicki wrote:

This series addresses two issues noticed with Port Groups:

(1) Port Group name cannot be used to refer to a row in NB DB.
(2) LSP dynamic addresses don't get copied to port group address sets.

The series depends on recent fixes for IPAM from Mark Michelson [1].

Thanks,
Jakub

[1] https://patchwork.ozlabs.org/patch/947155/


Jakub Sitnicki (3):
   ovn-nbctl: Allow referring to port groups by name.
   ovn-northd: Make use of svec for storing lists of addresses.
   ovn-northd: Propagate dynamic addresses to port group address sets.

  ovn/northd/ovn-northd.c   |  77 
  ovn/utilities/ovn-nbctl.c |   3 ++
  tests/ovn-nbctl.at|  14 ++
  tests/ovn.at  | 109 ++
  4 files changed, 163 insertions(+), 40 deletions(-)

--
2.14.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 3/3] ovn-northd: Propagate dynamic addresses to port group address sets.

[Jakub Sitnicki]

On Sat, 28 Jul 2018 23:54:32 -0700
Han Zhou  wrote:

> On Thu, Jul 26, 2018 at 3:51 AM, Jakub Sitnicki  wrote:
> >
> > If a logical switch port belongs to a port group and has dynamic
> > addresses assigned, propagate the addresses to the auto-generated
> > address sets for the port group.
> >
> > Signed-off-by: Jakub Sitnicki 
> > ---

(...)

> Thanks for the fix! Just a minor problem:
> 
> It would be better to check if the address is dynamic using
> is_dynamic_lsp_address() before calling split_addresses(). Otherwise, there
> seems to be annoying INFO log: "invalid syntax '%s' in address" printed if
> the address is "dynamic".
> 
> Acked-by: Han Zhou 

Thanks a lot for the review, Han. I've posted v2 with the suggested fix
and kept your acks:

https://patchwork.ozlabs.org/project/openvswitch/list/?series=58323

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

[ovs-dev] [PATCH v2 2/3] ovn-northd: Make use of svec for storing lists of addresses.

[Jakub Sitnicki]

Get rid of what is, esentially, an open-coded version of svec.

Signed-off-by: Jakub Sitnicki 
Acked-by: Han Zhou 
---
 ovn/northd/ovn-northd.c | 47 +++---
 tests/ovn.at| 50 +
 2 files changed, 65 insertions(+), 32 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 572022897..52f47c5ed 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -39,6 +39,7 @@
 #include "openvswitch/poll-loop.h"
 #include "smap.h"
 #include "sset.h"
+#include "svec.h"
 #include "stream.h"
 #include "stream-ssl.h"
 #include "unixctl.h"
@@ -6675,54 +6676,36 @@ sync_address_sets(struct northd_context *ctx)
 /* sync port group generated address sets first */
 const struct nbrec_port_group *nb_port_group;
 NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) {
-char **ipv4_addrs = xcalloc(1, sizeof *ipv4_addrs);
-size_t n_ipv4_addrs = 0;
-size_t n_ipv4_addrs_buf = 1;
-char **ipv6_addrs = xcalloc(1, sizeof *ipv6_addrs);
-size_t n_ipv6_addrs = 0;
-size_t n_ipv6_addrs_buf = 1;
+struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER;
+struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER;
 for (size_t i = 0; i < nb_port_group->n_ports; i++) {
 for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) {
 struct lport_addresses laddrs;
 extract_lsp_addresses(nb_port_group->ports[i]->addresses[j],
  &laddrs);
-while (n_ipv4_addrs_buf < n_ipv4_addrs + laddrs.n_ipv4_addrs) {
-n_ipv4_addrs_buf *= 2;
-ipv4_addrs = xrealloc(ipv4_addrs,
-n_ipv4_addrs_buf * sizeof *ipv4_addrs);
-}
 for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) {
-ipv4_addrs[n_ipv4_addrs++] =
-xstrdup(laddrs.ipv4_addrs[k].addr_s);
-}
-while (n_ipv6_addrs_buf < n_ipv6_addrs + laddrs.n_ipv6_addrs) {
-n_ipv6_addrs_buf *= 2;
-ipv6_addrs = xrealloc(ipv6_addrs,
-n_ipv6_addrs_buf * sizeof *ipv6_addrs);
+svec_add(&ipv4_addrs, laddrs.ipv4_addrs[k].addr_s);
 }
 for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) {
-ipv6_addrs[n_ipv6_addrs++] =
-xstrdup(laddrs.ipv6_addrs[k].addr_s);
+svec_add(&ipv6_addrs, laddrs.ipv6_addrs[k].addr_s);
 }
 destroy_lport_addresses(&laddrs);
 }
 }
 char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name);
 char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name);
-sync_address_set(ctx, ipv4_addrs_name, (const char **)ipv4_addrs,
- n_ipv4_addrs, &sb_address_sets);
-sync_address_set(ctx, ipv6_addrs_name, (const char **)ipv6_addrs,
- n_ipv6_addrs, &sb_address_sets);
+sync_address_set(ctx, ipv4_addrs_name,
+ /* "char **" is not compatible with "const char **" */
+ (const char **)ipv4_addrs.names,
+ ipv4_addrs.n, &sb_address_sets);
+sync_address_set(ctx, ipv6_addrs_name,
+ /* "char **" is not compatible with "const char **" */
+ (const char **)ipv6_addrs.names,
+ ipv6_addrs.n, &sb_address_sets);
 free(ipv4_addrs_name);
 free(ipv6_addrs_name);
-for (size_t i = 0; i < n_ipv4_addrs; i++) {
-free(ipv4_addrs[i]);
-}
-free(ipv4_addrs);
-for (size_t i = 0; i < n_ipv6_addrs; i++) {
-free(ipv6_addrs[i]);
-}
-free(ipv6_addrs);
+svec_destroy(&ipv4_addrs);
+svec_destroy(&ipv6_addrs);
 }
 
 /* sync user defined address sets, which may overwrite port group
diff --git a/tests/ovn.at b/tests/ovn.at
index e80c965a3..e7bdfe250 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10217,6 +10217,56 @@ done
 OVN_CLEANUP([hv1], [hv2], [hv3])
 AT_CLEANUP
 
+AT_SETUP([ovn -- Address Set generation from Port Groups (static addressing)])
+ovn_start
+
+ovn-nbctl ls-add ls1
+
+ovn-nbctl lsp-add ls1 lp1
+ovn-nbctl lsp-add ls1 lp2
+ovn-nbctl lsp-add ls1 lp3
+
+ovn-nbctl lsp-set-addresses lp1 "02:00:00:00:00:01 10.0.0.1 2001:db8::1"
+ovn-nbctl lsp-set-addresses lp2 "02:00:00:00:00:02 10.0.0.2 2001:db8::2"
+ovn-nbctl lsp-set-addresses lp3 "02:00:00:00:00:03 10.0.0.3 2001:db8::3"
+
+ovn-nbctl create Port_Group name=pg1
+ovn-nbctl create Port_Group name=pg2
+
+ovn-nbctl --id=@p get Logical_Switch_Port lp1 -- add Port_Group pg1 ports @p
+ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg1 ports @p
+ovn-n

[ovs-dev] [PATCH v2 3/3] ovn-northd: Propagate dynamic addresses to port group address sets.

[Jakub Sitnicki]

If a logical switch port belongs to a port group and has dynamic
addresses assigned, propagate the addresses to the auto-generated
address sets for the port group.

Signed-off-by: Jakub Sitnicki 
Acked-by: Han Zhou 
---
 ovn/northd/ovn-northd.c | 34 
 tests/ovn.at| 59 +
 2 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 52f47c5ed..d744836d3 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -6653,6 +6653,24 @@ sync_address_set(struct northd_context *ctx, const char 
*name,
 addrs, n_addrs);
 }
 
+/* Go through 'addresses' and add found IPv4 addresses to 'ipv4_addrs' and IPv6
+ * addresses to 'ipv6_addrs'.
+ */
+static void
+split_addresses(const char *addresses, struct svec *ipv4_addrs,
+struct svec *ipv6_addrs)
+{
+struct lport_addresses laddrs;
+extract_lsp_addresses(addresses, &laddrs);
+for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) {
+svec_add(ipv4_addrs, laddrs.ipv4_addrs[k].addr_s);
+}
+for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) {
+svec_add(ipv6_addrs, laddrs.ipv6_addrs[k].addr_s);
+}
+destroy_lport_addresses(&laddrs);
+}
+
 /* OVN_Southbound Address_Set table contains same records as in north
  * bound, plus the records generated from Port_Group table in north bound.
  *
@@ -6680,16 +6698,14 @@ sync_address_sets(struct northd_context *ctx)
 struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER;
 for (size_t i = 0; i < nb_port_group->n_ports; i++) {
 for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) {
-struct lport_addresses laddrs;
-extract_lsp_addresses(nb_port_group->ports[i]->addresses[j],
- &laddrs);
-for (size_t k = 0; k < laddrs.n_ipv4_addrs; k++) {
-svec_add(&ipv4_addrs, laddrs.ipv4_addrs[k].addr_s);
-}
-for (size_t k = 0; k < laddrs.n_ipv6_addrs; k++) {
-svec_add(&ipv6_addrs, laddrs.ipv6_addrs[k].addr_s);
+const char *addrs = nb_port_group->ports[i]->addresses[j];
+if (!is_dynamic_lsp_address(addrs)) {
+split_addresses(addrs, &ipv4_addrs, &ipv6_addrs);
 }
-destroy_lport_addresses(&laddrs);
+}
+if (nb_port_group->ports[i]->dynamic_addresses) {
+split_addresses(nb_port_group->ports[i]->dynamic_addresses,
+&ipv4_addrs, &ipv6_addrs);
 }
 }
 char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name);
diff --git a/tests/ovn.at b/tests/ovn.at
index e7bdfe250..32e6c1738 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10267,6 +10267,65 @@ AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses],
 
 AT_CLEANUP
 
+AT_SETUP([ovn -- Address Set generation from Port Groups (dynamic addressing)])
+ovn_start
+
+ovn-nbctl ls-add ls1
+ovn-nbctl ls-add ls2
+ovn-nbctl ls-add ls3
+
+ovn-nbctl set Logical_Switch ls1 \
+other_config:subnet=10.1.0.0/24 other_config:ipv6_prefix="2001:db8:1::"
+ovn-nbctl set Logical_Switch ls2 \
+other_config:subnet=10.2.0.0/24 other_config:ipv6_prefix="2001:db8:2::"
+ovn-nbctl set Logical_Switch ls3 \
+other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::"
+
+ovn-nbctl lsp-add ls1 lp1
+ovn-nbctl lsp-add ls2 lp2
+ovn-nbctl lsp-add ls3 lp3
+
+ovn-nbctl lsp-set-addresses lp1 "02:00:00:00:00:01 dynamic"
+ovn-nbctl lsp-set-addresses lp2 "02:00:00:00:00:02 dynamic"
+ovn-nbctl lsp-set-addresses lp3 "02:00:00:00:00:03 dynamic"
+
+ovn-nbctl create Port_Group name=pg1
+ovn-nbctl create Port_Group name=pg2
+
+ovn-nbctl --id=@p get Logical_Switch_Port lp1 -- add Port_Group pg1 ports @p
+ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg1 ports @p
+ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg2 ports @p
+ovn-nbctl --id=@p get Logical_Switch_Port lp3 -- add Port_Group pg2 ports @p
+
+ovn-nbctl --wait=sb sync
+
+dnl Check if port group address sets were populated with ports' addresses
+AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses],
+ [0], [[["10.1.0.2", "10.2.0.2"]]
+])
+AT_CHECK([ovn-sbctl get Address_Set pg2_ip4 addresses],
+ [0], [[["10.2.0.2", "10.3.0.2"]]
+])
+AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses],
+ [0], [[["2001:db8:1::ff:fe00:1", "2001:db8:2::ff:fe00:2"]]
+])
+AT_CHECK([ovn-sbctl get Address_Set pg2_ip6 addresses],
+ [0], [[["2001:db8:2::ff:fe00:2", "2001:db8:3::ff:fe00:3"]]
+])
+
+ovn-nbctl set Logical_Switch ls1 \
+other_config:subnet=10.11.0.0/24 other_config:ipv6_prefix="2001:db8:11::"
+
+dnl Check if updated address got propagated to the port group address sets
+AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 a

[ovs-dev] [PATCH v2 1/3] ovn-nbctl: Allow referring to port groups by name.

[Jakub Sitnicki]

Be user-friendly and allow using port group's name as its identifier in
database commands.

Signed-off-by: Jakub Sitnicki 
Acked-by: Han Zhou 
---
 ovn/utilities/ovn-nbctl.c |  3 +++
 tests/ovn-nbctl.at| 14 ++
 2 files changed, 17 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 3c3e582cb..f99b81bc0 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -4369,6 +4369,9 @@ static const struct ctl_table_class 
tables[NBREC_N_TABLES] = {
 [NBREC_TABLE_ADDRESS_SET].row_ids[0]
 = {&nbrec_address_set_col_name, NULL, NULL},
 
+[NBREC_TABLE_PORT_GROUP].row_ids[0]
+= {&nbrec_port_group_col_name, NULL, NULL},
+
 [NBREC_TABLE_ACL].row_ids[0] = {&nbrec_acl_col_name, NULL, NULL},
 };
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 64e217654..069b7b5b6 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -1505,3 +1505,17 @@ AT_CHECK([grep 'command takes at most .* arguments' 
stderr], [0], [ignore])
 
 OVN_NBCTL_TEST_STOP
 AT_CLEANUP
+
+dnl -
+
+AT_SETUP([ovn-nbctl - Port Groups])
+OVN_NBCTL_TEST_START
+
+dnl Check that port group can be looked up by name
+AT_CHECK([ovn-nbctl create Port_Group name=pg0], [0], [ignore])
+AT_CHECK([ovn-nbctl get Port_Group pg0 name], [0], [dnl
+"pg0"
+])
+
+OVN_NBCTL_TEST_STOP
+AT_CLEANUP
-- 
2.14.4

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

[ovs-dev] [PATCH v2 0/3] Port Group fixes

[Jakub Sitnicki]

This series addresses two issues noticed with Port Groups:

(1) Port Group name cannot be used to refer to a row in NB DB.
(2) LSP dynamic addresses don't get copied to port group address sets.

The series depends on recent fixes for IPAM from Mark Michelson [1].

Thanks,
Jakub

[1] https://patchwork.ozlabs.org/patch/947155/

v1 -> v2:
- Check if LSP addresses are static before treating them as such to avoid a
  warning. Reported by Han Zhou.


Jakub Sitnicki (3):
  ovn-nbctl: Allow referring to port groups by name.
  ovn-northd: Make use of svec for storing lists of addresses.
  ovn-northd: Propagate dynamic addresses to port group address sets.

 ovn/northd/ovn-northd.c   |  77 
 ovn/utilities/ovn-nbctl.c |   3 ++
 tests/ovn-nbctl.at|  14 ++
 tests/ovn.at  | 109 ++
 4 files changed, 164 insertions(+), 39 deletions(-)

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

[ovs-dev] [PATCH v2 1/2] ovn: Add '--restart' flag to ovn-controller exit.

[Mark Michelson]

When "--restart" is passed to ovn-controller's exit command, then
database entries are not removed for this hypervisor. This means that
* Encaps
* Chassis
* OVS ports
are not removed.

The reasoning is that if the intent is to restart ovn-controller, this
will allow for tunnels to remain up and allow for traffic not to be
interrupted during the restart. When ovn-controller is started again, it
picks back up from where it was.

Signed-off-by: Mark Michelson 
---
 ovn/controller/ovn-controller.c |  92 +++-
 tests/ovn.at| 186 
 2 files changed, 238 insertions(+), 40 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 6ee72a9fa..bd8175af5 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -541,11 +541,18 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 physical_register_ovs_idl(ovs_idl);
 }
 
+struct ovn_controller_exit_args {
+bool *exiting;
+bool *restart;
+};
+
 int
 main(int argc, char *argv[])
 {
 struct unixctl_server *unixctl;
 bool exiting;
+bool restart;
+struct ovn_controller_exit_args exit_args = {&exiting, &restart};
 int retval;
 
 ovs_cmdl_proctitle_init(argc, argv);
@@ -560,7 +567,8 @@ main(int argc, char *argv[])
 if (retval) {
 exit(EXIT_FAILURE);
 }
-unixctl_command_register("exit", "", 0, 0, ovn_controller_exit, &exiting);
+unixctl_command_register("exit", "", 0, 1, ovn_controller_exit,
+ &exit_args);
 
 /* Initialize group ids for loadbalancing. */
 struct ovn_extend_table group_table;
@@ -631,6 +639,7 @@ main(int argc, char *argv[])
 stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS);
 /* Main loop. */
 exiting = false;
+restart = false;
 while (!exiting) {
 /* Check OVN SB database. */
 char *new_ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
@@ -848,42 +857,45 @@ main(int argc, char *argv[])
 }
 }
 
-/* It's time to exit.  Clean up the databases. */
-bool done = false;
-while (!done) {
-struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(&ovs_idl_loop);
-struct ovsdb_idl_txn *ovnsb_idl_txn
-= ovsdb_idl_loop_run(&ovnsb_idl_loop);
+/* It's time to exit.  Clean up the databases if we are not restarting */
+if (!restart) {
+bool done = false;
+while (!done) {
+struct ovsdb_idl_txn *ovs_idl_txn
+= ovsdb_idl_loop_run(&ovs_idl_loop);
+struct ovsdb_idl_txn *ovnsb_idl_txn
+= ovsdb_idl_loop_run(&ovnsb_idl_loop);
+
+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_port_binding_table *port_binding_table
+= sbrec_port_binding_table_get(ovnsb_idl_loop.idl);
+
+const struct ovsrec_bridge *br_int = get_br_int(ovs_idl_txn,
+bridge_table,
+ovs_table);
+const char *chassis_id = get_chassis_id(ovs_table);
+const struct sbrec_chassis *chassis
+= (chassis_id
+   ? chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id)
+   : NULL);
+
+/* Run all of the cleanup functions, even if one of them returns
+ * false. We're done if all of them return true. */
+done = binding_cleanup(ovnsb_idl_txn, port_binding_table, chassis);
+done = chassis_cleanup(ovnsb_idl_txn, chassis) && done;
+done = encaps_cleanup(ovs_idl_txn, br_int) && done;
+if (done) {
+poll_immediate_wake();
+}
 
-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_port_binding_table *port_binding_table
-= sbrec_port_binding_table_get(ovnsb_idl_loop.idl);
-
-const struct ovsrec_bridge *br_int = get_br_int(ovs_idl_txn,
-bridge_table,
-ovs_table);
-const char *chassis_id = get_chassis_id(ovs_table);
-const struct sbrec_chassis *chassis
-= (chassis_id
-   ? chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id)
-   : NULL);
-
-/* Run all of the cleanup functions, even if one of them returns false.
- * We're done if all of them return true. */
-done = binding_clean

[ovs-dev] [PATCH v2 0/2] Allow for smoother restarting of ovn-controller

[Mark Michelson]

When ovn-controller is restarted, the ovn-controller process is stopped
and then started again. When the process is stopped, the process cleans
itself up by removing all traces of itself from the central southbound
database and the OVS database on the hypervisor. The issue with this is
that this removes tunnels, meaning that traffic from other hypervisors
in the cluster is unable to reach the local hypervisor. When restarting,
it would be better to not clean up, thus allowing for uninterrupted
traffic flow.

This patchset allows for ovn-controller to be stopped without cleaning
itself up. This way, during a restart, traffic can still flow freely
while ovn-controller is down.
---
v1 -> v2: Modified patch 2 to pass $@ instead of $1 to stop_daemon when
restarting ovn_controller.

Mark Michelson (2):
  ovn: Add '--restart' flag to ovn-controller exit.
  ovn: Modify restart_controller in ovn-ctl to use --restart

 ovn/controller/ovn-controller.c |  92 +++-
 ovn/utilities/ovn-ctl   |   4 +-
 tests/ovn.at| 186 
 utilities/ovs-lib.in|   2 +-
 4 files changed, 241 insertions(+), 43 deletions(-)

-- 
2.14.4

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

[ovs-dev] [PATCH v2 2/2] ovn: Modify restart_controller in ovn-ctl to use --restart

[Mark Michelson]

The --restart flag allows for uninterrupted packet flowage when exiting
ovn-controller. This patch modifies the restart_controller argument to
ovn-ctl to use --restart.

Signed-off-by: Mark Michelson 
---
 ovn/utilities/ovn-ctl | 4 ++--
 utilities/ovs-lib.in  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
index 2fce47714..05feed2b6 100755
--- a/ovn/utilities/ovn-ctl
+++ b/ovn/utilities/ovn-ctl
@@ -350,7 +350,7 @@ stop_northd () {
 }
 
 stop_controller () {
-OVS_RUNDIR=${OVN_RUNDIR} stop_daemon ovn-controller
+OVS_RUNDIR=${OVN_RUNDIR} stop_daemon ovn-controller $@
 }
 
 stop_controller_vtep () {
@@ -367,7 +367,7 @@ restart_northd () {
 }
 
 restart_controller () {
-stop_controller
+stop_controller --restart
 start_controller
 }
 
diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
index 92f98ad92..9f62dfd25 100644
--- a/utilities/ovs-lib.in
+++ b/utilities/ovs-lib.in
@@ -258,7 +258,7 @@ stop_daemon () {
 case $action in
 EXIT)
 action "Exiting $1 ($pid)" \
-${bindir}/ovs-appctl -T 1 -t $rundir/$1.$pid.ctl 
exit
+${bindir}/ovs-appctl -T 1 -t $rundir/$1.$pid.ctl 
exit $2
 ;;
 TERM)
 action "Killing $1 ($pid)" kill $pid
-- 
2.14.4

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

Re: [ovs-dev] [ovs-dev, v3, 2 of 6] ipsec: reintroduce IPsec support for tunneling

[Aaron Conole]

0-day Robot  writes:

> Bleep bloop.  Greetings Qiuyu Xiao, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> ERROR: Too many signoffs; are you missing Co-authored-by lines?

My understanding is that there should be a 'Signed-off-by' tag from the
principal or submitter, and a Signed-off-by: and Co-authored-by from
each co-author.

Maybe it would be better match the tags and ignore these kinds of cases
(where each 'co-author' adds both tags)?

> WARNING: Line has trailing whitespace
> #522 FILE: ipsec/ovs-monitor-ipsec:456:
> self.secrets_file.write('%s %s : PSK "%s"\n' % 
>
> WARNING: Line is 80 characters long (recommended limit is 79)
> WARNING: Line has trailing whitespace
> #523 FILE: ipsec/ovs-monitor-ipsec:457:
> (tunnel.conf["local_ip"], 
> tunnel.conf["remote_ip"], 
>
> Lines checked: 1228, Warnings: 3, Errors: 1
>
>
> Please check this out.  If you feel there has been an error, please email 
> acon...@bytheb.org
>
> Thanks,
> 0-day Robot
> ___
> 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 v6 2/3] Send gateway port ARP through router internal ports

[Anil Venkata]

Thanks Mark for the review. Please see my reply inline.


On Thu, Jul 19, 2018 at 7:04 PM, Mark Michelson  wrote:

> This patch had some compilation errors. It may be due to changes in master
> since this patchset was posted:
>
> ovn/controller/pinctrl.c: In function ‘send_garp_run’:
> ovn/controller/pinctrl.c:2303:48: error: passing argument 8 of
> ‘get_nat_addresses_and_keys’ discards ‘const’ qualifier from pointer target
> type [-Werror=discarded-qualifiers]
> &nat_addresses, local_datapaths);
> ^~~
> ovn/controller/pinctrl.c:2182:1: note: expected ‘struct hmap *’ but
> argument is of type ‘const struct hmap *’
>  get_nat_addresses_and_keys(struct ovsdb_idl_index *sbrec_chassis_by_name,
>  ^~
>
>
Thanks, it is a warning and I will fix it.


>
>
> On 06/25/2018 03:53 PM, vkomm...@redhat.com wrote:
>
>> From: venkata anil 
>>
>> External switches should learn the distributed gateway port MAC address
>> as they have to forward the packet tagged with tenant vlan network but
>> with this MAC as destination MAC address. So router has to send ARP
>> reply and gARP for this MAC address through router internal patch ports
>> connecting tenant vlan networks.
>>
>> Signed-off-by: Venkata Anil 
>> ---
>>
>> v5->v6:
>> * Rebased
>>
>> v4->v5:
>> * No changes in this patch
>>
>>   ovn/controller/pinctrl.c | 57 ++
>> +-
>>   ovn/northd/ovn-northd.c  | 57 ++
>> ++
>>   tests/ovn.at |  6 +
>>   3 files changed, 114 insertions(+), 6 deletions(-)
>>
>> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>> index a0bf602..37157aa 100644
>> --- a/ovn/controller/pinctrl.c
>> +++ b/ovn/controller/pinctrl.c
>> @@ -2185,8 +2185,47 @@ get_nat_addresses_and_keys(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>>  struct sset *local_l3gw_ports,
>>  const struct sbrec_chassis *chassis,
>>  const struct sset *active_tunnels,
>> -   struct shash *nat_addresses)
>> +   struct shash *nat_addresses,
>> +   struct hmap *local_datapaths)
>>   {
>> +/* When a router has tenant vlan networks, gARP for distributed
>> gateway
>> + * router port has to be sent through internal tenant vlan network's
>> + * localnet port, so that external switches can learn this MAC and
>> forward
>> + * tenant vlan network traffic with distributed gateway router port
>> MAC
>> + * as destination MAC address */
>> +
>> +struct local_datapath *ldp;
>> +struct shash router_vlan_ports;
>> +
>> +shash_init(&router_vlan_ports);
>> +HMAP_FOR_EACH (ldp, hmap_node, local_datapaths) {
>> +const struct sbrec_port_binding *crp;
>> +crp = ldp->chassisredirect_port;
>> +/* check if it a router with chassis redirect port,
>> + * get corresponding distributed port */
>> +if (crp && crp->chassis &&
>> +!strcmp(crp->chassis->name, chassis->name)) {
>> +const struct sbrec_port_binding *dp = NULL;
>> +for (int i = 0; i < ldp->n_peer_dps; i++) {
>> +if (!strcmp(ldp->peer_dps[i]->patch->logical_port,
>> +smap_get(&crp->options,
>> "distributed-port"))) {
>> +dp = ldp->peer_dps[i]->peer;
>> +break;
>> +}
>> +}
>> +
>>
>
> It seems possible to reach this point and have dp be NULL. You probably
> should continue the hmap traversal if dp is NULL.


No need. As I said in my previous patch, chassisredirect port is created
from distributed router port and doesn't exist without distributed port.


>
>
> +/* Save router internal port (patch port on tenant vlan
>> network)
>> + * along with distributed port. */
>> +for (int i = 0; i < ldp->n_peer_dps; i++) {
>> +if (strcmp(ldp->peer_dps[i]->patch->logical_port,
>> +   smap_get(&crp->options, "distributed-port")))
>> {
>> +shash_add(&router_vlan_ports,
>> +  ldp->peer_dps[i]->peer->logical_port, dp);
>> +}
>> +}
>> +}
>> +}
>> +
>>   const char *gw_port;
>>   SSET_FOR_EACH(gw_port, local_l3gw_ports) {
>>   const struct sbrec_port_binding *pb;
>> @@ -2196,11 +2235,16 @@ get_nat_addresses_and_keys(struct
>> ovsdb_idl_index *sbrec_chassis_by_name,
>>   continue;
>>   }
>>   -if (pb->n_nat_addresses) {
>> -for (int i = 0; i < pb->n_nat_addresses; i++) {
>> +/* Router internatl ports should send gARP for distributed port
>>
>
> s/internatl/internal/
>
>
>
sure.



> + * NAT addresses */
>> +const struct sb

Re: [ovs-dev] [PATCH 0/2] Allow for smoother restarting of ovn-controller

[Jakub Sitnicki]

On Tue, 24 Jul 2018 16:30:04 -0400
Mark Michelson  wrote:

> When ovn-controller is restarted, the ovn-controller process is stopped
> and then started again. When the process is stopped, the process cleans
> itself up by removing all traces of itself from the central southbound
> database and the OVS database on the hypervisor. The issue with this is
> that this removes tunnels, meaning that traffic from other hypervisors
> in the cluster is unable to reach the local hypervisor. When restarting,
> it would be better to not clean up, thus allowing for uninterrupted
> traffic flow.
> 
> This patchset allows for ovn-controller to be stopped without cleaning
> itself up. This way, during a restart, traffic can still flow freely
> while ovn-controller is down.
> 
> Mark Michelson (2):
>   ovn: Add '--restart' flag to ovn-controller exit.
>   ovn: Modify restart_controller in ovn-ctl to use --restart
> 
>  ovn/controller/ovn-controller.c |  92 +++-
>  ovn/utilities/ovn-ctl   |   4 +-
>  tests/ovn.at| 186 
> 
>  utilities/ovs-lib.in|   2 +-
>  4 files changed, 241 insertions(+), 43 deletions(-)
> 

I had one minor comment. Otherwise LGTM:

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

Re: [ovs-dev] [PATCH 2/2] ovn: Modify restart_controller in ovn-ctl to use --restart

[Jakub Sitnicki]

On Tue, 24 Jul 2018 16:30:06 -0400
Mark Michelson  wrote:

> The --restart flag allows for uninterrupted packet flowage when exiting
> ovn-controller. This patch modifies the restart_controller argument to
> ovn-ctl to use --restart.
> 
> Signed-off-by: Mark Michelson 
> ---
>  ovn/utilities/ovn-ctl | 4 ++--
>  utilities/ovs-lib.in  | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
> index 2fce47714..f76248e84 100755
> --- a/ovn/utilities/ovn-ctl
> +++ b/ovn/utilities/ovn-ctl
> @@ -350,7 +350,7 @@ stop_northd () {
>  }
>  
>  stop_controller () {
> -OVS_RUNDIR=${OVN_RUNDIR} stop_daemon ovn-controller
> +OVS_RUNDIR=${OVN_RUNDIR} stop_daemon ovn-controller $1
>  }

Might be less surprising if the wrapper passed all arguments
to the target command, but this works too.

>  
>  stop_controller_vtep () {
> @@ -367,7 +367,7 @@ restart_northd () {
>  }
>  
>  restart_controller () {
> -stop_controller
> +stop_controller --restart
>  start_controller
>  }
>  
> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> index 92f98ad92..9f62dfd25 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -258,7 +258,7 @@ stop_daemon () {
>  case $action in
>  EXIT)
>  action "Exiting $1 ($pid)" \
> -${bindir}/ovs-appctl -T 1 -t $rundir/$1.$pid.ctl 
> exit
> +${bindir}/ovs-appctl -T 1 -t $rundir/$1.$pid.ctl 
> exit $2
>  ;;
>  TERM)
>  action "Killing $1 ($pid)" kill $pid

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

Re: [ovs-dev] [PATCH] oss-fuzz: Move oss-fuzz test harnesses and fuzzer configs to ovs source repo

[Bhargava Shastry]

Any updates on the proposed patch? :-)

On 07/16/2018 02:07 PM, Bhargava Shastry wrote:
> Update: I fixed these errors in the attached patch that supersedes the
> patch here (https://patchwork.ozlabs.org/patch/942118/)
> 
> The major change is that I add the following line for each fuzz target
> binary in the tests/oss-fuzz/automake.mk file:
> 
> e.g.,
> tests_oss_fuzz_flow_extract_target_LDFLAGS = $(LIB_FUZZING_ENGINE) \
> -lc++
> 
> Regards,
> Bhargava
> 
> On 07/16/2018 11:45 AM, Bhargava Shastry wrote:
>> Oops, here's the link failure log:
>>
>> ```
>> /usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../x86_64-linux-gnu/crt1.o: In
>> function `_start':
>> (.text+0x20): undefined reference to `main'
>> clang-7: error: linker command failed with exit code 1 (use -v to see
>> invocation)
>> Makefile:4159: recipe for target 'tests/oss-fuzz/flow_extract_target' failed
>> make: *** [tests/oss-fuzz/flow_extract_target] Error 1
>> fuzzers build failed.
>> ```
>>
>> The main symbol is provided by libfuzzer (clang++ -lFuzzingEngine)
>>
>> Regards,
>> Bhargava
>>
>> On 07/16/2018 11:36 AM, Bhargava Shastry wrote:
>>> Hi Ben,
>>>
 Never mind that one, I failed to check in some of that.

 I sent it formally:
 https://patchwork.ozlabs.org/patch/942118/
>>>
>>> Thanks for the patch. This fixes the previous error. Now, there are some
>>> new errors during the compilation/linking process. I think most of this
>>> can be fixed if I figure out how automake works. In a nutshell, here's
>>> the problem:
>>>
>>> - oss-fuzz provides compilation flags that can be plugged in like so
>>> ```
>>> CC=clang
>>> CXX=clang++
>>> CFLAGS=-O1 -fno-omit-frame-pointer -gline-tables-only
>>> -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address
>>> -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link
>>> CXXFLAGS=-O1 -fno-omit-frame-pointer -gline-tables-only
>>> -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address
>>> -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -stdlib=libc++
>>> ```
>>>
>>> And here's what I used to do before
>>>
>>> - Use clang and CFLAGS above plus some additional includes to compile
>>> each of the fuzzer tests
>>> - Use clang++ and additional linker flags to link these into a fuzzer binary
>>>
>>> Now, I see that the compilation works
>>> ```
>>> depbase=`echo tests/oss-fuzz/flow_extract_target.o | sed
>>> 's|[^/]*$|.deps/&|;s|\.o$||'`;\
>>> clang -DHAVE_CONFIG_H -I.-I ./include -I ./include -I ./lib -I ./lib
>>> -I/usr/include   -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
>>> -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
>>> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
>>> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
>>> -Wthread-safety -fno-strict-aliasing -Wswitch-bool
>>> -Wlogical-not-parentheses -Wsizeof-array-argument -Wshift-negative-value
>>> -Qunused-arguments -Wshadow -Wno-null-pointer-arithmetic-O1
>>> -fno-omit-frame-pointer -gline-tables-only
>>> -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address
>>> -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -MT
>>> tests/oss-fuzz/flow_extract_target.o -MD -MP -MF $depbase.Tpo -c -o
>>> tests/oss-fuzz/flow_extract_target.o
>>> tests/oss-fuzz/flow_extract_target.c &&\
>>> mv -f $depbase.Tpo $depbase.Po
>>> ```
>>>
>>> However, the linking fails
>>> ```
>>> libtool: link: clang -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
>>> -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
>>> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
>>> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
>>> -Wthread-safety -fno-strict-aliasing -Wswitch-bool
>>> -Wlogical-not-parentheses -Wsizeof-array-argument -Wshift-negative-value
>>> -Qunused-arguments -Wshadow -Wno-null-pointer-arithmetic -O1
>>> -fno-omit-frame-pointer -gline-tables-only
>>> -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address
>>> -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -o
>>> tests/oss-fuzz/flow_extract_target tests/oss-fuzz/flow_extract_target.o
>>> -L/usr/lib lib/.libs/libopenvswitch.a -lssl -lcrypto -latomic
>>> ```
>>>
>>> I think adding -lFuzzingEngine should fix this but another variable
>>> between my build script and automake is the use of clang++ for linking.
>>>
>>> Do you know how I can experiment with different linker flags and
>>> compiler/linker in automake?
>>>
>>> Regards,
>>> Bhargava
>>>
>>>
>>
> 

-- 
Bhargava Shastry 
Security in Telecommunications
TU Berlin / Telekom Innovation Laboratories
Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany
phone: +49 30 8353 58235
Keybase: https://keybase.io/bshastry
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] Revert "dpctl: Expand the flow dump type filter"

[Roi Dayan]

On 27/07/2018 14:00, Simon Horman wrote:
> Hi Gavi,
> 
> On 26 July 2018 at 17:36, Justin Pettit  > wrote:
> 
> 
> > On Jul 26, 2018, at 7:29 AM, Gavi Teitz  > wrote:
> > 
> > From: Justin Pettit, sent: Thursday, July 26, 2018 12:02 AM:
> >> Commit ab15e70eb587 ("dpctl: Expand the flow dump type filter") had a 
> number of issues with style, build breakage, and failing unit tests.
> >> The patch is being reverted so that they can addressed.
> > 
> > I acknowledge the build breakage issue, could you elaborate regarding 
> the style issues?
> 
> The main style issue was that lines shouldn't be over 79 characters longs.
> 
> > As for the failing unit tests, this commit provides the means to fix 
> unit tests that lost their relevance due to the changes introduced in commit 
> d63ca5329ff9 ("dpctl: Properly reflect a rule's offloaded to HW state"). Are 
> there other unit tests that are broken due to this commit?
> 
> I saw about a dozen unit tests failing when I ran "make check".
> 
> 
> I believe this gives an overview of the failing tests:
> 
> https://travis-ci.org/horms2/ovs/jobs/408446254 
> 


sorry all about this. we missed it internally.
next time we'll make sure we do all needed testing before submitting.
we usually do submit to travis to run all internal checks.
we probably missed it with this patch.

as for what Gavi mean about the needing the commit to fix the tests
it's for the offload tests only.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev, ACL, Meters, 7 of 7] ovn: Add rate-limiting for ACL logs.

[0-day Robot]

Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 372 characters long (recommended limit is 79)
#239 FILE: ovn/utilities/ovn-nbctl.8.xml:88:
  [--type={switch | port-group}] 
[--log] [--meter=meter] 
[--severity=severity] 
[--name=name] [--may-exist] 
acl-add entity direction priority 
match verdict

Lines checked: 386, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev, ACL, Meters, 4 of 7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.

[0-day Robot]

Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#217 FILE: ovn/ovn-nb.ovsschema:202:
  "enum": ["set", ["kbps", "pktps"]]}}},

WARNING: Line is 80 characters long (recommended limit is 79)
#358 FILE: ovn/ovn-sb.ovsschema:105:
  "enum": ["set", ["kbps", "pktps"]]}}},

WARNING: Line is 129 characters long (recommended limit is 79)
#473 FILE: ovn/utilities/ovn-nbctl.8.xml:177:
meter-add name unit 
action rate [burst_size]

WARNING: Line lacks whitespace around operator
#533 FILE: ovn/utilities/ovn-nbctl.c:500:
  meter-add NAME UNIT ACTION RATE [BURST_SIZE]\n\

WARNING: Line lacks whitespace around operator
#535 FILE: ovn/utilities/ovn-nbctl.c:502:
  meter-del [NAME]  remove meters\n\

WARNING: Line lacks whitespace around operator
#536 FILE: ovn/utilities/ovn-nbctl.c:503:
  meter-listprint meters\n\

Lines checked: 764, Warnings: 6, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev