Re: [ovs-dev] [PATCH ovn v2] Support vlan-passthru for tag=0 logical switch ports
On Wed, Mar 24, 2021 at 9:22 PM Ben Pfaff wrote: > > On Tue, Mar 23, 2021 at 09:22:43PM -0400, Ihar Hrachyshka wrote: > > When vlan-passthru is true for LS, for untagged localnet ports, tagged > > VLAN traffic originating from VIFs should be passed through intact since > > the VLAN header belongs to VIF user, not the localnet port fabric. > > > > This patch adds multinode test cases to cover scenarios where packets > > traverse through fabric layer, for both tagged and untagged (tag=0) > > localnet ports. > > > > Signed-off-by: Ihar Hrachyshka > > Thanks for adding ddlog support. It looks correct to me. Hi Ihar, The patch LGTM. I just have one comment. I noticed that there is slight difference in the way ovn-northd and ovn-northd-ddlog sets the vlan-passthru in the options column of Port_Binding. If the logical switch's other_config:vlan-passthru is set to false i.e ovn-nbctl set logical_switch ls other_config:vlan-passthru=false then, ovn-northd doesn't set "vlan-passthru=false" in the options column of port binding. It will clear the option if it was set earlier. Whereas ovn-northd-ddlog, sets "vlan-passthru=false". Technically this will not have any effect on the functionality. Although I'd suggest making it consistent. Either make ovn-northd to set "vlan-passthru=false" or make ovn-northd-ddlog to clear vlan-passthru. I'm fine either way. Thanks Numan > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.
On Fri, Mar 26, 2021 at 9:16 AM Ben Pfaff wrote:
>
> On Mon, Mar 22, 2021 at 04:29:11PM +0530, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > Presently we add 65535 priority lflows in the stages -
> > 'ls_in_acl' and 'ls_out_acl' to drop packets which
> > match on 'ct.inv'.
> >
> > This patch provides an option to not use this field in the
> > logical flow matches for the following
> > reasons:
> >
> > • Some of the smart NICs which support offloading datapath
> >flows may not support this field. In such cases, almost
> >all the datapath flows cannot be offloaded if ACLs/load balancers
> >are configured on the logical switch datapath.
> >
> > • A recent commit in kernel ovs datapath sets the committed
> >connection tracking entry to be liberal for out-of-window
> >tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP
> >packets will not be marked as invalid.
> >
> > There are still certain scenarios where a packet can be marked
> > invalid. Like
> > • If the tcp flags are not correct.
> >
> > • If there are checksum errors.
> >
> > In such cases, the packet will be delivered to the destination
> > port. So CMS should evaluate their usecases before enabling
> > this option.
> >
> > Signed-off-by: Numan Siddique
>
> Thanks for writing dldog code! I have a few comments.
>
> This looks correct:
>
> function can_use_ct_inv_match(options: Map): bool {
> map_get_bool_def(options, "use_ct_inv_match", true)
> }
>
> relation UseCtInvMatch(use_ct_inv_match: bool)
> UseCtInvMatch(can_use_ct_inv_match(options)) :-
> nb::NB_Global(._uuid = uuid, .options = options).
>
> I would make a few stylistic changes if I were writing it. I would drop
> the _uuid match because it is not used for anything. I would use a
> relation that just contains a bool, instead of a struct that contains a
> bool, because then there are fewer names to invent. I would usually
> drop the function, since it is only used in one place and it is a
> one-liner. So I'd end up with this:
>
> relation UseCtInvMatch[bool]
> UseCtInvMatch[use_ct_inv_match] :-
> nb::NB_Global(.options = options),
> var use_ct_inv_match = map_get_bool_def(options, "use_ct_inv_match",
> true).
>
> Or possibly I'd keep the map_get_bool_def(...) expression inside the
> brackets, I haven't decided what's really the best way:
>
> relation UseCtInvMatch[bool]
> UseCtInvMatch[map_get_bool_def(options, "use_ct_inv_match", true)] :-
> nb::NB_Global(.options = options).
>
> I'd also add a rule to handle the case where NB_Global doesn't exist.
> This is really out of paranoia. I don't know whether this can really
> happen in practice, but it costs only a few lines and avoids a problem
> if it does ever somehow happen:
>
> UseCtInvMatch[true] :-
> Unit(),
> not nb in nb::NB_Global().
>
> I noticed is that this global feature flag is getting copied inside
> every Switch. That wastes a little memory and I do not know of a
> benefit. So I'd drop it from Switch and join with UseCtInvMatch in the
> one place where it's needed.
>
> Finally, there's are lots of redundant strings in ovn_northd.dl. We can
> make that better with a little parameterization, something like this:
> @@ -2290,10 +2290,15 @@ for (Reject(lsuuid, pipeline, stage, acl,
> fair_meter, extra_match_, extra_action
> .actions = actions,
> .external_ids = stage_hint(acl._uuid))
> }
>
> /* build_acls */
> +for (UseCtInvMatch[use_ct_inv_match]) {
> +(var ct_inv_or, var and_not_ct_inv) = match (use_ct_inv_match) {
> +true -> ("ct.inv || ", "&& !ct.inv "),
> +false -> ("", ""),
> +} in
> for (sw in (.ls = ls))
> var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
> {
> /* Ingress and Egress ACL Table (Priority 0): Packets are
> allowed by
> * default. A related rule at priority 1 is added below if there
> @@ -2354,17 +2359,17 @@ var has_stateful = sw.has_stateful_acl or
> sw.has_lb_vip in
> *
> * This is enforced at a higher priority than ACLs can be
> defined. */
> Flow(.logical_datapath = ls._uuid,
> .stage= switch_stage(IN, ACL),
> .priority = 65535,
> - .__match = "ct.inv || (ct.est && ct.rpl &&
> ct_label.blocked == 1)",
> + .__match = ct_inv_or ++ "(ct.est && ct.rpl &&
> ct_label.blocked == 1)",
> .actions = "drop;",
> .external_ids = map_empty());
> Flow(.logical_datapath = ls._uuid,
> .stage= switch_stage(OUT, ACL),
> .priority = 65535,
> - .__match = "ct.inv || (ct.est && ct.rpl &&
>
Re: [ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.
On Mon, Mar 22, 2021 at 04:29:11PM +0530, num...@ovn.org wrote:
> From: Numan Siddique
>
> Presently we add 65535 priority lflows in the stages -
> 'ls_in_acl' and 'ls_out_acl' to drop packets which
> match on 'ct.inv'.
>
> This patch provides an option to not use this field in the
> logical flow matches for the following
> reasons:
>
> • Some of the smart NICs which support offloading datapath
>flows may not support this field. In such cases, almost
>all the datapath flows cannot be offloaded if ACLs/load balancers
>are configured on the logical switch datapath.
>
> • A recent commit in kernel ovs datapath sets the committed
>connection tracking entry to be liberal for out-of-window
>tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP
>packets will not be marked as invalid.
>
> There are still certain scenarios where a packet can be marked
> invalid. Like
> • If the tcp flags are not correct.
>
> • If there are checksum errors.
>
> In such cases, the packet will be delivered to the destination
> port. So CMS should evaluate their usecases before enabling
> this option.
>
> Signed-off-by: Numan Siddique
Thanks for writing dldog code! I have a few comments.
This looks correct:
function can_use_ct_inv_match(options: Map): bool {
map_get_bool_def(options, "use_ct_inv_match", true)
}
relation UseCtInvMatch(use_ct_inv_match: bool)
UseCtInvMatch(can_use_ct_inv_match(options)) :-
nb::NB_Global(._uuid = uuid, .options = options).
I would make a few stylistic changes if I were writing it. I would drop
the _uuid match because it is not used for anything. I would use a
relation that just contains a bool, instead of a struct that contains a
bool, because then there are fewer names to invent. I would usually
drop the function, since it is only used in one place and it is a
one-liner. So I'd end up with this:
relation UseCtInvMatch[bool]
UseCtInvMatch[use_ct_inv_match] :-
nb::NB_Global(.options = options),
var use_ct_inv_match = map_get_bool_def(options, "use_ct_inv_match",
true).
Or possibly I'd keep the map_get_bool_def(...) expression inside the
brackets, I haven't decided what's really the best way:
relation UseCtInvMatch[bool]
UseCtInvMatch[map_get_bool_def(options, "use_ct_inv_match", true)] :-
nb::NB_Global(.options = options).
I'd also add a rule to handle the case where NB_Global doesn't exist.
This is really out of paranoia. I don't know whether this can really
happen in practice, but it costs only a few lines and avoids a problem
if it does ever somehow happen:
UseCtInvMatch[true] :-
Unit(),
not nb in nb::NB_Global().
I noticed is that this global feature flag is getting copied inside
every Switch. That wastes a little memory and I do not know of a
benefit. So I'd drop it from Switch and join with UseCtInvMatch in the
one place where it's needed.
Finally, there's are lots of redundant strings in ovn_northd.dl. We can
make that better with a little parameterization, something like this:
@@ -2290,10 +2290,15 @@ for (Reject(lsuuid, pipeline, stage, acl,
fair_meter, extra_match_, extra_action
.actions = actions,
.external_ids = stage_hint(acl._uuid))
}
/* build_acls */
+for (UseCtInvMatch[use_ct_inv_match]) {
+(var ct_inv_or, var and_not_ct_inv) = match (use_ct_inv_match) {
+true -> ("ct.inv || ", "&& !ct.inv "),
+false -> ("", ""),
+} in
for (sw in (.ls = ls))
var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
{
/* Ingress and Egress ACL Table (Priority 0): Packets are allowed
by
* default. A related rule at priority 1 is added below if there
@@ -2354,17 +2359,17 @@ var has_stateful = sw.has_stateful_acl or
sw.has_lb_vip in
*
* This is enforced at a higher priority than ACLs can be
defined. */
Flow(.logical_datapath = ls._uuid,
.stage= switch_stage(IN, ACL),
.priority = 65535,
- .__match = "ct.inv || (ct.est && ct.rpl &&
ct_label.blocked == 1)",
+ .__match = ct_inv_or ++ "(ct.est && ct.rpl &&
ct_label.blocked == 1)",
.actions = "drop;",
.external_ids = map_empty());
Flow(.logical_datapath = ls._uuid,
.stage= switch_stage(OUT, ACL),
.priority = 65535,
- .__match = "ct.inv || (ct.est && ct.rpl &&
ct_label.blocked == 1)",
+ .__match = ct_inv_or ++ "(ct.est && ct.rpl &&
ct_label.blocked == 1)",
.actions = "drop;",
.external_ids = map_empty());
/* Ingress and
Re: [ovs-dev] [OVN Patch v15 1/3] ovn-libs: Add support for parallel processing
On Thu, Mar 25, 2021 at 3:01 PM Anton Ivanov
wrote:
>
>
>
> On 24/03/2021 15:31, Numan Siddique wrote:
> > On Mon, Mar 1, 2021 at 6:35 PM wrote:
> >>
> >> From: Anton Ivanov
> >>
> >> This adds a set of functions and macros intended to process
> >> hashes in parallel.
> >>
> >> The principles of operation are documented in the ovn-parallel-hmap.h
> >>
> >> If these one day go into the OVS tree, the OVS tree versions
> >> would be used in preference.
> >>
> >> Signed-off-by: Anton Ivanov
> >
> > Hi Anton,
> >
> > I tested the first 2 patches of this series and it crashes again for me.
> >
> > This time I ran tests on a 4 core machine - Intel(R) Xeon(R) CPU
> > E3-1220 v5 @ 3.00GHz
> >
> > The below trace is seen for both gcc and clang.
> >
> >
> > [Thread debugging using libthread_db enabled]
> > Using host libthread_db library "/lib64/libthread_db.so.1".
> > Core was generated by `ovn-northd -vjsonrpc
> > --ovnnb-db=unix:/mnt/mydisk/myhome/numan_alt/work/ovs_ovn/'.
> > Program terminated with signal SIGSEGV, Segmentation fault.
> > #0 0x7f27594ae212 in __new_sem_wait_slow.constprop.0 () from
> > /lib64/libpthread.so.0
> > [Current thread is 1 (Thread 0x7f2758c68640 (LWP 347378))]
> > Missing separate debuginfos, use: dnf debuginfo-install
> > glibc-2.32-3.fc33.x86_64 libcap-ng-0.8-1.fc33.x86_64
> > libevent-2.1.8-10.fc33.x86_64 openssl-libs-1.1.1i-1.fc33.x86_64
> > python3-libs-3.9.1-2.fc33.x86_64 unbound-libs-1.10.1-4.fc33.x86_64
> > zlib-1.2.11-23.fc33.x86_64
> > (gdb) bt
> > #0 0x7f27594ae212 in __new_sem_wait_slow.constprop.0 () from
> > /lib64/libpthread.so.0
> > #1 0x00422184 in wait_for_work (control=) at
> > ../lib/ovn-parallel-hmap.h:203
> > #2 build_lflows_thread (arg=0x2538420) at ../northd/ovn-northd.c:11855
> > #3 0x0049cd12 in ovsthread_wrapper (aux_=) at
> > ../lib/ovs-thread.c:383
> > #4 0x7f27594a53f9 in start_thread () from /lib64/libpthread.so.0
> > #5 0x7f2759142903 in clone () from /lib64/libc.so.6
> > -
> >
> > I'm not sure why you're not able to reproduce this issue.
>
> I can't. I have run it for days in a loop.
>
> One possibility is that for whatever reason your machine has slower IPC
> speeds compared to linear execution speeds. Thread debugging? AMD vs Intel?
> No idea.
>
> There is a race on-exit in the current code which I have found by inspection
> and which I have never been able to trigger. On my machines the workers
> always exit in time before the main thread has finished, so I cannot trigger
> this.
>
> Can you try this incremental fix to see if it fixes the problem for you. If
> that works, I will incorporate it and reissue the patch. If not - I will
> continue digging.
>
> diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c
> index e83ae23cb..3597f896f 100644
> --- a/lib/ovn-parallel-hmap.c
> +++ b/lib/ovn-parallel-hmap.c
> @@ -143,7 +143,8 @@ struct worker_pool *ovn_add_worker_pool(void
> *(*start)(void *)){
> }
>
> for (i = 0; i < pool_size; i++) {
> -ovs_thread_create("worker pool helper", start,
> _pool->controls[i]);
> +new_pool->controls[i].worker =
> +ovs_thread_create("worker pool helper", start,
> _pool->controls[i]);
> }
> ovs_list_push_back(_pools, _pool->list_node);
> }
> @@ -386,6 +387,9 @@ static void worker_pool_hook(void *aux OVS_UNUSED) {
> for (i = 0; i < pool->size ; i++) {
> sem_post(pool->controls[i].fire);
> }
> +for (i = 0; i < pool->size ; i++) {
> +pthread_join(pool->controls[i].worker, NULL);
> +}
> for (i = 0; i < pool->size ; i++) {
> sem_close(pool->controls[i].fire);
> sprintf(sem_name, WORKER_SEM_NAME, sembase, pool, i);
> diff --git a/lib/ovn-parallel-hmap.h b/lib/ovn-parallel-hmap.h
> index 8db61eaba..d62ca3da5 100644
> --- a/lib/ovn-parallel-hmap.h
> +++ b/lib/ovn-parallel-hmap.h
> @@ -82,6 +82,7 @@ struct worker_control {
> struct ovs_mutex mutex; /* Guards the data. */
> void *data; /* Pointer to data to be processed. */
> void *workload; /* back-pointer to the worker pool structure. */
> +pthread_t worker;
> };
>
> struct worker_pool {
>
I applied the above diff on top of patch 2 and did some tests. I see
a big improvement
with this. On my "Intel(R) Xeon(R) CPU E3-1220 v5 @ 3.00GHz" server,
I saw just one
crash only once when I ran the test suite multiple times.
On my work laptop (in which the tests used to hang earlier), all the
tests are passing now.
But I see a lot more consistent crashes here. For all single run of
whole testsuite (with make check -j5)
I observed around 7 crashes. Definitely an improvement when compared
to my previous runs with v14.
Here are the back traces details of the core dumps I observed -
https://gist.github.com/numansiddique/5cab90ec4a1ee6e1adbfd3cd90eccf5a
Crash 1 and Crash 2 are frequent. Let me know in case
Re: [ovs-dev] [PATCH ovn] tests: Add missing sync in "lb_force_snat_ip for Gateway Routers".
On Fri, Mar 26, 2021 at 8:36 AM Ben Pfaff wrote: > > On Fri, Mar 26, 2021 at 08:18:52AM +0530, num...@ovn.org wrote: > > From: Numan Siddique > > > > Signed-off-by: Numan Siddique > > Acked-by: Ben Pfaff Thanks. I applied to the main branch. Numan > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-nbctl: Fix comment.
On Fri, Mar 26, 2021 at 07:44:12AM +0530, Numan Siddique wrote: > On Thu, Mar 25, 2021 at 3:35 AM Ben Pfaff wrote: > > > > I guess this was a cut-and-paste error from ovs-ofctl. > > Acked-by: Numan Siddique Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] xml2nroff: Fix typo in generated nroff.
On Fri, Mar 26, 2021 at 07:45:35AM +0530, Numan Siddique wrote:
> On Thu, Mar 25, 2021 at 3:50 AM Ben Pfaff wrote:
> >
> > This is just a typo introduced in a previous fix. The typo shows up
> > at the top of generated manpages.
> >
> > Signed-off-by: Ben Pfaff
> > Fixes: 852316e8dc12 ("xml2nroff: Properly support .")
>
> Acked-by: Numan Siddique
Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Add missing sync in "lb_force_snat_ip for Gateway Routers".
On Fri, Mar 26, 2021 at 08:18:52AM +0530, num...@ovn.org wrote: > From: Numan Siddique > > Signed-off-by: Numan Siddique Acked-by: Ben Pfaff ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
On Thu, Mar 25, 2021 at 11:40:59PM +, svc.eng.git-m...@nutanix.com wrote:
> diff --git a/northd/lrouter.dl b/northd/lrouter.dl
> index 1a7cb2d23..1d8722282 100644
> --- a/northd/lrouter.dl
> +++ b/northd/lrouter.dl
> @@ -770,6 +770,36 @@ RouterStaticRoute(router, key, dsts) :-
> },
> var dsts = set_singleton(RouteDst{nexthop, src_ip, port,
> ecmp_symmetric_reply}).
>
> +/* compute route-route pairs for nexthop = "discard" routes */
> +function is_discard_route(nexthop: string): bool {
> +match (nexthop) {
> +"discard" -> true,
> +_ -> false
> +}
> +}
> +relation (lrsr: nb::Logical_Router_Static_Route,
> + key: route_key,
> + nexthop: string)
> +(.lrsr= lrsr,
> + .key = RouteKey{policy, ip_prefix, plen},
> + .nexthop = lrsr.nexthop) :-
> +lrsr in nb::Logical_Router_Static_Route(),
> +var policy = route_policy_from_string(lrsr.policy),
> +Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
> +is_discard_route(lrsr.nexthop).
> +
> +relation RouterDiscardRoute_(
> +router : Ref,
> +key : route_key,
> +nexthop : string)
> +
> +RouterDiscardRoute_(.router = router,
> +.key = route.key,
> +.nexthop = route.nexthop) :-
> +router in (.lr = nb::Logical_Router{.static_routes = routes}),
> +var route_id = FlatMap(routes),
> +route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
> route_id}).
Thanks so much for writing the ddlog support for this! I have a few
comments.
I first noticed that this is more complicated than necessary:
function is_discard_route(nexthop: string): bool {
match (nexthop) {
"discard" -> true,
_ -> false
}
}
It can be written as simply:
function is_discard_route(nexthop: string): bool {
nexthop == "discard"
}
Then, I noticed that is_discard_route() is only used in one place, so
that the last line in this:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
is_discard_route(lrsr.nexthop).
can be written as:
lrsr.nexthop == "discard".
and be a little clearer.
However, there's an optimization possibility here. ddlog is pretty
literal-minded and does things in order. Most routes won't be
"discard", so by putting that test first we can usually bypass the other
work. Now we have:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
lrsr.nexthop == "discard".
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
We can make things even better by noticing that we can put the "discard"
test right into the specification of Logical_Router_Static_Route. That
makes things even faster since ddlog can index it:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop == "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
At this point I noticed that the nexthop column wasn't good for anything
in DiscardRoute or RouterDiscardRoute_, because it's always the constant
string "discard". So we can get rid of it:
/* compute route-route pairs for nexthop = "discard" routes */
relation (lrsr: nb::Logical_Router_Static_Route,
key: route_key)
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen}) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop = "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
relation RouterDiscardRoute_(
router : Ref,
key : route_key)
RouterDiscardRoute_(.router = router,
.key = route.key) :-
router in (.lr = nb::Logical_Router{.static_routes = routes}),
var route_id = FlatMap(routes),
route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
route_id}).
Warning[message] :-
RouterStaticRoute_(.router = router, .key = key, .nexthop = nexthop),
not RouterStaticRoute(.router = router, .key = key),
var message = "No path for ${key.policy} static route
${key.ip_prefix}/${key.plen} with next hop ${nexthop}".
The overall
[ovs-dev] [PATCH ovn] tests: Add missing sync in "lb_force_snat_ip for Gateway Routers".
From: Numan Siddique Signed-off-by: Numan Siddique --- tests/ovn-northd.at | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 02f6ede847..47f2662e45 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2731,6 +2731,8 @@ check ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.4:8080 check ovn-nbctl lr-lb-add lr0 lb1 check ovn-nbctl set logical_router lr0 options:chassis=ch1 +check ovn-nbctl --wait=sb sync + ovn-sbctl dump-flows lr0 > lr0flows AT_CAPTURE_FILE([lr0flows]) -- 2.29.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] xml2nroff: Fix typo in generated nroff.
On Thu, Mar 25, 2021 at 3:50 AM Ben Pfaff wrote:
>
> This is just a typo introduced in a previous fix. The typo shows up
> at the top of generated manpages.
>
> Signed-off-by: Ben Pfaff
> Fixes: 852316e8dc12 ("xml2nroff: Properly support .")
Acked-by: Numan Siddique
Thanks
Numan
> ---
> build-aux/xml2nroff | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff
> index fc6de0dcbf19..9e781a396dac 100755
> --- a/build-aux/xml2nroff
> +++ b/build-aux/xml2nroff
> @@ -94,10 +94,10 @@ def manpage_to_nroff(xml_file, subst, include_path,
> version=None):
> . PP
> . I "\\$1"
> ..
> -build/''' % (build.nroff.text_to_nroff(program),
> - build.nroff.text_to_nroff(section),
> - build.nroff.text_to_nroff(title),
> - build.nroff.text_to_nroff(version))
> +''' % (build.nroff.text_to_nroff(program),
> + build.nroff.text_to_nroff(section),
> + build.nroff.text_to_nroff(title),
> + build.nroff.text_to_nroff(version))
>
> s += build.nroff.block_xml_to_nroff(doc.childNodes) + "\n"
>
> --
> 2.29.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-nbctl: Fix comment.
On Thu, Mar 25, 2021 at 3:35 AM Ben Pfaff wrote: > > I guess this was a cut-and-paste error from ovs-ofctl. Acked-by: Numan Siddique Thanks Numan > > Signed-off-by: Ben Pfaff > --- > utilities/ovn-nbctl.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 2c77f4ba7645..18405835699d 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -91,8 +91,7 @@ static int leader_only = true; > * are specified in the connetion method string. */ > static int shuffle_remotes = true; > > -/* --unixctl-path: Path to use for unixctl server, for "monitor" and "snoop" > - commands. */ > +/* --unixctl-path: Path to use for unixctl server socket, for daemon mode. */ > static char *unixctl_path; > > static unixctl_cb_func server_cmd_exit; > -- > 2.29.2 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Q on weird OvS+conntrack behavior
Hello, I am working on OvS-DPDK and conntrack and seeing weird behavior, maybe I am doing something wrong. ovs-vswitchd --version ovs-vswitchd (Open vSwitch) 2.13.3 DPDK 19.11.5 I have setup using https://docs.openvswitch.org/en/latest/tutorials/ovs-conntrack/ After initial syn/syn-ack/ack, flow state moves established state *root#ovs-appctl dpctl/dump-conntrack | grep 192.168.0.2tcp,orig=(src=192.168.0.2,dst=10.0.0.2,sport=1024,dport=2048),reply=(src=10.0.0.2,dst=192.168.0.2,sport=2048,dport=1024),protoinfo=(state=SYN_SENT)* *root# ovs-appctl dpctl/dump-conntrack | grep 192.168.0.2tcp,orig=(src=192.168.0.2,dst=10.0.0.2,sport=1024,dport=2048),reply=(src=10.0.0.2,dst=192.168.0.2,sport=2048,dport=1024),protoinfo=(state=ESTABLISHED)* Then I restart OvS, I still see conntrack entry still present so I wanted to know where conntrack entries are stored (note that connection has not received fin/fin-ack yet and restart happened before that). Second question, is there a way (an API?) to extract flows from conntrack and program OF tables? r *oot#ovs-ctl stop * Exiting ovs-vswitchd (24167) * Exiting ovsdb-server (24146)root#ovs-ctl start * Starting ovsdb-server * system ID not configured, please use --system-id * Configuring Open vSwitch system IDs * Starting ovs-vswitchd * Enabling remote OVSDB managersroot# root# ovs-appctl dpctl/dump-conntrack | grep 192.168.0.2tcp,orig=(src=192.168.0.2,dst=10.0.0.2,sport=1024,dport=2048),reply=(src=10.0.0.2,dst=192.168.0.2,sport=2048,dport=1024),protoinfo=(state=ESTABLISHED)* Thanks, Ravi ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs]: lib/db-ctl-base.c: Check that --all or records argument provided
On Thu, Mar 25, 2021 at 09:18:19PM +0200, Alexey Roytman wrote:
> From: Alexey Roytman
>
> ovn-nbctl and ovn-sbctl CLI utilities allow destroying the entire
> table or some records from the given table.
> However, either the --all option or the deleted records should be provided.
> This patch adds the test.
>
> Signed-off-by: Alexey Roytman
>
> ---
> lib/db-ctl-base.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index e95c77da2..81ade7abc 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -1823,6 +1823,11 @@ cmd_destroy(struct ctl_context *ctx)
> return;
> }
>
> +if (!delete_all && ctx->argc == 2) {
> +ctl_error(ctx, "either --all or records argument should be
> specified");
> +return;
> +}
I can see why this is worth warning about, but a fatal error seems like
overkill. I'd use VLOG_WARN here.
I'd also add a test.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 3/3] ovn-sbctl: Add daemon support.
Bleep bloop. Greetings Ben Pfaff, 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 has non-spaces leading whitespace WARNING: Line has trailing whitespace #1106 FILE: utilities/ovn-sbctl.8.xml:438: WARNING: Line has non-spaces leading whitespace WARNING: Line has trailing whitespace #1158 FILE: utilities/ovn-sbctl.8.xml:490: WARNING: Line has non-spaces leading whitespace WARNING: Line has trailing whitespace #1185 FILE: utilities/ovn-sbctl.8.xml:517: WARNING: Comment with 'xxx' marker #1548 FILE: utilities/ovn-sbctl.c:121: /* XXX add verification that table is empty */ Lines checked: 2104, Warnings: 7, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/3] ovn-nbctl: Refactor into infrastructure and northbound details.
Bleep bloop. Greetings Ben Pfaff, 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: Use ovs_abort() in place of abort() #606 FILE: utilities/ovn-dbctl.c:566: abort(); WARNING: Line has non-spaces leading whitespace WARNING: Line has trailing whitespace #821 FILE: utilities/ovn-dbctl.c:781: WARNING: Line lacks whitespace around operator #934 FILE: utilities/ovn-dbctl.c:894: argv[optind-1]); WARNING: Line lacks whitespace around operator #945 FILE: utilities/ovn-dbctl.c:905: argv[optind-1]); WARNING: Comment with 'xxx' marker #1381 FILE: utilities/ovn-nbctl.c:78: /* XXX add verification that table is empty */ Lines checked: 2781, Warnings: 5, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
From: Karthik Chandrashekar
Physical switches have the ability to specify "discard" or sometimes
"NULL interface" as a nexthop for routes. This can be used to prevent
routing loops in the network. Add a similar configuration for ovn
where nexthop accepts the string "discard". When the nexthop is discard
the action in the routing table will be to drop the packets.
Signed-off-by: Karthik Chandrashekar
v1 -> v2
* Add ddlog support
* Don't allow nexthop = "discard" when ECMP and BFD is set
---
northd/lrouter.dl | 30 +
northd/ovn-northd.c | 126 +-
northd/ovn_northd.dl | 10 +++
ovn-nb.xml| 4 +-
tests/ovn-nbctl.at| 25
tests/ovn.at | 94
utilities/ovn-nbctl.8.xml | 3 +-
utilities/ovn-nbctl.c | 67 +++-
8 files changed, 287 insertions(+), 72 deletions(-)
diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 1a7cb2d23..1d8722282 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -770,6 +770,36 @@ RouterStaticRoute(router, key, dsts) :-
},
var dsts = set_singleton(RouteDst{nexthop, src_ip, port,
ecmp_symmetric_reply}).
+/* compute route-route pairs for nexthop = "discard" routes */
+function is_discard_route(nexthop: string): bool {
+match (nexthop) {
+"discard" -> true,
+_ -> false
+}
+}
+relation (lrsr: nb::Logical_Router_Static_Route,
+ key: route_key,
+ nexthop: string)
+(.lrsr= lrsr,
+ .key = RouteKey{policy, ip_prefix, plen},
+ .nexthop = lrsr.nexthop) :-
+lrsr in nb::Logical_Router_Static_Route(),
+var policy = route_policy_from_string(lrsr.policy),
+Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
+is_discard_route(lrsr.nexthop).
+
+relation RouterDiscardRoute_(
+router : Ref,
+key : route_key,
+nexthop : string)
+
+RouterDiscardRoute_(.router = router,
+.key = route.key,
+.nexthop = route.nexthop) :-
+router in (.lr = nb::Logical_Router{.static_routes = routes}),
+var route_id = FlatMap(routes),
+route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
route_id}).
+
Warning[message] :-
RouterStaticRoute_(.router = router, .key = key, .nexthop = nexthop),
not RouterStaticRoute(.router = router, .key = key),
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 57df62b92..a6357f02c 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7938,6 +7938,7 @@ struct parsed_route {
uint32_t hash;
const struct nbrec_logical_router_static_route *route;
bool ecmp_symmetric_reply;
+bool is_discard_route;
};
static uint32_t
@@ -7957,20 +7958,23 @@ parsed_routes_add(struct ovs_list *routes,
/* Verify that the next hop is an IP address with an all-ones mask. */
struct in6_addr nexthop;
unsigned int plen;
-if (!ip46_parse_cidr(route->nexthop, , )) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "bad 'nexthop' %s in static route"
- UUID_FMT, route->nexthop,
- UUID_ARGS(>header_.uuid));
-return NULL;
-}
-if ((IN6_IS_ADDR_V4MAPPED() && plen != 32) ||
-(!IN6_IS_ADDR_V4MAPPED() && plen != 128)) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "bad next hop mask %s in static route"
- UUID_FMT, route->nexthop,
- UUID_ARGS(>header_.uuid));
-return NULL;
+bool is_discard_route = !strcmp(route->nexthop, "discard");
+if (!is_discard_route) {
+if (!ip46_parse_cidr(route->nexthop, , )) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "bad 'nexthop' %s in static route"
+ UUID_FMT, route->nexthop,
+ UUID_ARGS(>header_.uuid));
+return NULL;
+}
+if ((IN6_IS_ADDR_V4MAPPED() && plen != 32) ||
+(!IN6_IS_ADDR_V4MAPPED() && plen != 128)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "bad next hop mask %s in static route"
+ UUID_FMT, route->nexthop,
+ UUID_ARGS(>header_.uuid));
+return NULL;
+}
}
/* Parse ip_prefix */
@@ -7984,13 +7988,15 @@ parsed_routes_add(struct ovs_list *routes,
}
/* Verify that ip_prefix and nexthop have same address familiy. */
-if (IN6_IS_ADDR_V4MAPPED() != IN6_IS_ADDR_V4MAPPED()) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "Address family doesn't match between 'ip_prefix' %s"
- " and 'nexthop' %s in static
[ovs-dev] [PATCH ovn 3/3] ovn-sbctl: Add daemon support.
Also rewrite the manpage and convert it to XML for consistency with
ovn-nbctl, and add tests.
Signed-off-by: Ben Pfaff
---
NEWS | 2 +
manpages.mk | 17 -
tests/ovn-sbctl.at| 76 +++--
utilities/automake.mk | 7 +-
utilities/ovn-dbctl.c | 24 +-
utilities/ovn-dbctl.h | 3 +-
utilities/ovn-nbctl.c | 1 +
utilities/ovn-sbctl.8.in | 317 --
utilities/ovn-sbctl.8.xml | 583 +
utilities/ovn-sbctl.c | 670 +++---
10 files changed, 785 insertions(+), 915 deletions(-)
delete mode 100644 utilities/ovn-sbctl.8.in
create mode 100644 utilities/ovn-sbctl.8.xml
diff --git a/NEWS b/NEWS
index 530c5d42fe85..5048433148b8 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,8 @@ Post-v21.03.0
(This may take testing and tuning to be effective.) This version of OVN
requires DDLog 0.36.
- Introduce ovn-controller incremetal processing engine statistics
+ - ovn-sbctl can now run as a daemon (long-lived, background process).
+See ovn-sbctl(8) for details.
OVN v21.03.0 - 12 Mar 2021
-
diff --git a/manpages.mk b/manpages.mk
index 44e544681424..3334b38f943d 100644
--- a/manpages.mk
+++ b/manpages.mk
@@ -10,20 +10,3 @@ lib/common-syn.man:
lib/common.man:
lib/ovs.tmac:
-utilities/ovn-sbctl.8: \
- utilities/ovn-sbctl.8.in \
- lib/common.man \
- lib/db-ctl-base.man \
- lib/ovs.tmac \
- lib/ssl-bootstrap.man \
- lib/ssl.man \
- lib/table.man \
- lib/vlog.man
-utilities/ovn-sbctl.8.in:
-lib/common.man:
-lib/db-ctl-base.man:
-lib/ovs.tmac:
-lib/ssl-bootstrap.man:
-lib/ssl.man:
-lib/table.man:
-lib/vlog.man:
diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
index 2712cc15490c..9334762fd313 100644
--- a/tests/ovn-sbctl.at
+++ b/tests/ovn-sbctl.at
@@ -1,9 +1,14 @@
AT_BANNER([ovn-sbctl])
+OVS_START_SHELL_HELPERS
# OVN_SBCTL_TEST_START
m4_define([OVN_SBCTL_TEST_START],
- [dnl Create databases (ovn-nb, ovn-sb).
- AT_KEYWORDS([ovn])
+ [AT_KEYWORDS([ovn])
+ AT_CAPTURE_FILE([ovsdb-server.log])
+ AT_CAPTURE_FILE([ovn-northd.log])
+ ovn_sbctl_test_start $1])
+ovn_sbctl_test_start() {
+ dnl Create databases (ovn-nb, ovn-sb).
for daemon in ovn-nb ovn-sb; do
AT_CHECK([ovsdb-tool create $daemon.db
$abs_top_srcdir/${daemon}.ovsschema])
done
@@ -15,27 +20,54 @@ m4_define([OVN_SBCTL_TEST_START],
AT_CHECK([[sed < stderr '
/vlog|INFO|opened log file/d
/ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d']])
- AT_CAPTURE_FILE([ovsdb-server.log])
dnl Start ovn-northd.
AT_CHECK([ovn-northd --detach --no-chdir --pidfile --log-file
--ovnnb-db=unix:$OVS_RUNDIR/ovnnb_db.sock
--ovnsb-db=unix:$OVS_RUNDIR/ovnsb_db.sock], [0], [], [stderr])
on_exit "kill `cat ovn-northd.pid`"
AT_CHECK([[sed < stderr '
/vlog|INFO|opened log file/d']])
- AT_CAPTURE_FILE([ovn-northd.log])
-])
+
+ AS_CASE([$1],
+ [daemon],
+ [export OVN_SB_DAEMON=$(ovn-sbctl --pidfile --detach --no-chdir
--log-file -vsocket_util:off)
+on_exit "kill `cat ovn-sbctl.pid`"],
+ [direct], [],
+ [*], [AT_FAIL_IF(:)])
+}
# OVN_SBCTL_TEST_STOP
-m4_define([OVN_SBCTL_TEST_STOP],
- [AT_CHECK([check_logs "$1"])
- OVS_APP_EXIT_AND_WAIT([ovn-northd])
- OVS_APP_EXIT_AND_WAIT_BY_TARGET([$OVS_RUNDIR/ovnnb_db.ctl],
[$OVS_RUNDIR/ovnnb_db.pid])
- OVS_APP_EXIT_AND_WAIT_BY_TARGET([$OVS_RUNDIR/ovnsb_db.ctl],
[$OVS_RUNDIR/ovnsb_db.pid])])
+m4_define([OVN_SBCTL_TEST_STOP], [ovn_sbctl_test_stop])
+ovn_sbctl_test_stop() {
+ AT_CHECK([check_logs "$1"])
+ OVS_APP_EXIT_AND_WAIT([ovn-northd])
+ OVS_APP_EXIT_AND_WAIT_BY_TARGET([$OVS_RUNDIR/ovnnb_db.ctl],
[$OVS_RUNDIR/ovnnb_db.pid])
+ OVS_APP_EXIT_AND_WAIT_BY_TARGET([$OVS_RUNDIR/ovnsb_db.ctl],
[$OVS_RUNDIR/ovnsb_db.pid])
+}
+OVS_END_SHELL_HELPERS
+
+# OVN_SBCTL_TEST(NAME, TITLE, COMMANDS)
+m4_define([OVN_SBCTL_TEST],
+ [OVS_START_SHELL_HELPERS
+$1() {
+ $3
+}
+OVS_END_SHELL_HELPERS
+
+AT_SETUP([ovn-sbctl - $2 - direct])
+OVN_SBCTL_TEST_START direct
+$1
+OVN_SBCTL_TEST_STOP
+AT_CLEANUP
+
+AT_SETUP([ovn-sbctl - $2 - daemon])
+OVN_SBCTL_TEST_START daemon
+$1
+OVN_SBCTL_TEST_STOP
+AT_CLEANUP])
dnl -
-AT_SETUP([ovn-sbctl - chassis commands])
-OVN_SBCTL_TEST_START
+OVN_SBCTL_TEST([ovn_sbctl_chassis_commands], [ovn-sbctl - chassis commands], [
ovn_init_db ovn-sb
AT_CHECK([ovn-sbctl chassis-add ch0 geneve 1.2.3.4])
@@ -61,16 +93,14 @@ AT_CHECK([ovn-sbctl -f csv -d bare --no-headings --columns
ip,type list encap |
1.2.3.5,vxlan
])
-OVN_SBCTL_TEST_STOP
as ovn-sb
OVS_APP_EXIT_AND_WAIT([ovsdb-server])
-AT_CLEANUP
+as
+])
dnl -
-AT_SETUP([ovn-sbctl])
-OVN_SBCTL_TEST_START
-
+OVN_SBCTL_TEST([ovn_sbctl_commands], [ovn-sbctl], [
[ovs-dev] [PATCH ovn 2/3] ovn-nbctl: Refactor into infrastructure and northbound details.
In an upcoming commit, this will allow adding daemon mode to ovn-sbctl without having a lot of duplicated code. Signed-off-by: Ben Pfaff --- utilities/automake.mk |5 +- utilities/ovn-dbctl.c | 1215 utilities/ovn-dbctl.h | 60 ++ utilities/ovn-nbctl.c | 1362 - 4 files changed, 1412 insertions(+), 1230 deletions(-) create mode 100644 utilities/ovn-dbctl.c create mode 100644 utilities/ovn-dbctl.h diff --git a/utilities/automake.mk b/utilities/automake.mk index c4a6d248c274..50c0cfded018 100644 --- a/utilities/automake.mk +++ b/utilities/automake.mk @@ -71,7 +71,10 @@ utilities/ovn-lib: $(top_builddir)/config.status # ovn-nbctl bin_PROGRAMS += utilities/ovn-nbctl -utilities_ovn_nbctl_SOURCES = utilities/ovn-nbctl.c +utilities_ovn_nbctl_SOURCES = \ +utilities/ovn-dbctl.c \ +utilities/ovn-dbctl.h \ +utilities/ovn-nbctl.c utilities_ovn_nbctl_LDADD = lib/libovn.la $(OVSDB_LIBDIR)/libovsdb.la $(OVS_LIBDIR)/libopenvswitch.la # ovn-sbctl diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c new file mode 100644 index ..730e84f5e770 --- /dev/null +++ b/utilities/ovn-dbctl.c @@ -0,0 +1,1215 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include "ovn-dbctl.h" + +#include + +#include "command-line.h" +#include "daemon.h" +#include "db-ctl-base.h" +#include "fatal-signal.h" +#include "jsonrpc.h" +#include "memory.h" +#include "openvswitch/poll-loop.h" +#include "openvswitch/vlog.h" +#include "ovn-util.h" +#include "ovsdb-idl.h" +#include "process.h" +#include "simap.h" +#include "stream-ssl.h" +#include "svec.h" +#include "table.h" +#include "timer.h" +#include "unixctl.h" +#include "util.h" + +VLOG_DEFINE_THIS_MODULE(ovn_dbctl); + +/* --db: The database server to contact. */ +static const char *db; + +/* --oneline: Write each command's output as a single line? */ +static bool oneline; + +/* --dry-run: Do not commit any changes. */ +static bool dry_run; + +/* --wait=TYPE: Wait for configuration change to take effect? */ +static enum nbctl_wait_type wait_type = NBCTL_WAIT_NONE; + +static bool print_wait_time = false; + +/* --timeout: Time to wait for a connection to 'db'. */ +static unsigned int timeout; + +/* Format for table output. */ +static struct table_style table_style = TABLE_STYLE_DEFAULT; + +/* The IDL we're using and the current transaction, if any. This is for use by + * ovn_dbctl_exit() only, to allow it to clean up. Other code should use its + * context arguments. */ +static struct ovsdb_idl *the_idl; +static struct ovsdb_idl_txn *the_idl_txn; + +/* --leader-only, --no-leader-only: Only accept the leader in a cluster. */ +static int leader_only = true; + +/* --shuffle-remotes, --no-shuffle-remotes: Shuffle the order of remotes that + * are specified in the connetion method string. */ +static int shuffle_remotes = true; + +/* --unixctl-path: Path to use for unixctl server, for "monitor" and "snoop" + commands. */ +static char *unixctl_path; + +static unixctl_cb_func server_cmd_exit; +static unixctl_cb_func server_cmd_run; + +static struct option *get_all_options(void); +static bool has_option(const struct ovs_cmdl_parsed_option *, size_t n, + int option); +static void dbctl_client(const struct ovn_dbctl_options *dbctl_options, + const char *socket_name, + const struct ovs_cmdl_parsed_option *, size_t n, + int argc, char *argv[]); +static bool will_detach(const struct ovs_cmdl_parsed_option *, size_t n); +static void apply_options_direct(const struct ovn_dbctl_options *dbctl_options, + const struct ovs_cmdl_parsed_option *, + size_t n, struct shash *local_options); +static char * OVS_WARN_UNUSED_RESULT run_prerequisites( +const struct ovn_dbctl_options *dbctl_options, +struct ctl_command[], size_t n_commands, struct ovsdb_idl *); +static char * OVS_WARN_UNUSED_RESULT do_dbctl( +const struct ovn_dbctl_options *dbctl_options, +const char *args, struct ctl_command *, size_t n, +struct ovsdb_idl *, const struct timer *, bool *retry); +static char * OVS_WARN_UNUSED_RESULT main_loop( +const struct ovn_dbctl_options *, const char *args, +struct ctl_command *commands, size_t n_commands, +struct ovsdb_idl *idl, const struct timer *); +static void
[ovs-dev] [PATCH ovn 1/3] ovn-nbctl: Improve manpage.
This rearranges the manpage into a more logical order, documents some options that weren't documented, adds some sections such as Environment and Exit Status that a manpage should have, puts the headings at reasonable levels instead of all at the top level, and adds a little more explanatory text in a few places. Signed-off-by: Ben Pfaff --- utilities/ovn-nbctl.8.xml | 670 ++ 1 file changed, 392 insertions(+), 278 deletions(-) diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml index 2cab592ce347..51fc986dcf9a 100644 --- a/utilities/ovn-nbctl.8.xml +++ b/utilities/ovn-nbctl.8.xml @@ -7,9 +7,327 @@ ovn-nbctl [options] command [arg...] Description -This utility can be used to manage the OVN northbound database. -General Commands + + The ovn-nbctl program configures the + OVN_Northbound database by providing a high-level interface + to its configuration database. See ovn-nb(5) for + comprehensive documentation of the database schema. + + + + ovn-nbctl connects to an ovsdb-server process + that maintains an OVN_Northbound configuration database. Using this + connection, it queries and possibly applies changes to the database, + depending on the supplied commands. + + + + ovn-nbctl can perform any number of commands in a single + run, implemented as a single atomic transaction against the database. + + + + The ovn-nbctl command line begins with global options (see + OPTIONS below for details). The global options are followed + by one or more commands. Each command should begin with -- + by itself as a command-line argument, to separate it from the following + commands. (The -- before the first command is optional.) + The command itself starts with command-specific options, if any, followed + by the command name and any arguments. + + +Daemon Mode + + + When it is invoked in the most ordinary way, ovn-nbctl + connects to an OVSDB server that hosts the northbound database, retrieves + a partial copy of the database that is complete enough to do its work, + sends a transaction request to the server, and receives and processes the + server's reply. In common interactive use, this is fine, but if the + database is large, the step in which ovn-nbctl retrieves a + partial copy of the database can take a long time, which yields poor + performance overall. + + + + To improve performance in such a case, ovn-nbctl offers a + "daemon mode," in which the user first starts ovn-nbctl + running in the background and afterward uses the daemon to execute + operations. Over several ovn-nbctl command invocations, + this performs better overall because it retrieves a copy of the database + only once at the beginning, not once per program run. + + + + Use the --detach option to start an ovn-nbctl + daemon. With this option, ovn-nbctl prints the name of a + control socket to stdout. The client should save this name in + environment variable OVN_NB_DAEMON. Under the Bourne shell + this might be done like this: + + + + export OVN_NB_DAEMON=$(ovn-nbctl --pidfile --detach) + + + + When OVN_NB_DAEMON is set, ovn-nbctl + automatically and transparently uses the daemon to execute its commands. + + + + When the daemon is no longer needed, kill it and unset the environment + variable, e.g.: + + + + kill $(cat $OVN_RUNDIR/ovn-nbctl.pid) + unset OVN_NB_DAEMON + + + + When using daemon mode, an alternative to the OVN_NB_DAEMON + environment variable is to specify a path for the Unix socket. When + starting the ovn-nbctl daemon, specify the -u option with a + full path to the location of the socket file. Here is an exmple: + + + + ovn-nbctl --detach -u /tmp/mysock.ctl + + + + Then to connect to the running daemon, use the -u option + with the full path to the socket created when the daemon was started: + + + + ovn-nbctl -u /tmp/mysock.ctl show + + + + Daemon mode is experimental. + + +Daemon Commands + + + Daemon mode is internally implemented using the same mechanism used by + ovs-appctl. One may also use ovs-appctl + directly with the following commands: + + + + +run [options] command +[arg...] [-- [options] +command [arg...] ...] + + +Instructs the daemon process to run one or more ovn-nbctl +commands described above and reply with the results of running these +commands. Accepts the --no-wait, --wait, +--timeout, --dry-run, --oneline, +and the options described under Table Formatting Options +in addition to the the command-specific options. + +
[ovs-dev] [PATCH ovn 0/3] Add ovn-sbctl daemon mode
A daemon mode for ovn-sbctl can make benchmarks much faster. For me, it made the benchmark provided by Numan drop from several minutes to about 100 seconds (without ddlog) in one case. I do not know whether it will make real use of ovn faster, but it's definitely useful if you want to run tests. Ben Pfaff (3): ovn-nbctl: Improve manpage. ovn-nbctl: Refactor into infrastructure and northbound details. ovn-sbctl: Add daemon support. NEWS |2 + manpages.mk | 17 - tests/ovn-sbctl.at| 76 ++- utilities/automake.mk | 12 +- utilities/ovn-dbctl.c | 1227 + utilities/ovn-dbctl.h | 61 ++ utilities/ovn-nbctl.8.xml | 670 ++ utilities/ovn-nbctl.c | 1363 - utilities/ovn-sbctl.8.in | 317 - utilities/ovn-sbctl.8.xml | 583 utilities/ovn-sbctl.c | 670 -- 11 files changed, 2582 insertions(+), 2416 deletions(-) create mode 100644 utilities/ovn-dbctl.c create mode 100644 utilities/ovn-dbctl.h delete mode 100644 utilities/ovn-sbctl.8.in create mode 100644 utilities/ovn-sbctl.8.xml -- 2.29.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] ovn-northd-ddlog scale issues
OK. I guess I'll have to try it myself.
On Thu, Mar 25, 2021 at 12:59:00PM +0530, Numan Siddique wrote:
> Hi Ben,
>
> With your memory fixes applied, I still see many test failures due to
> memory leaks
> when configured as:
>
> ./configure --enable-Werror --enable-sparse CFLAGS="-g -fsanitize=address"
>
> You can find more details about the memory leaks here -
> https://gist.github.com/numansiddique/e701777da977a89e24b49f159bb30d5d
>
> Thanks
> Numan
>
>
>
> On Wed, Mar 24, 2021 at 9:19 PM Ben Pfaff wrote:
> >
> > Thanks a lot for the report! I need a new test case so I'll give this
> > one a shot when I can.
> >
> > On Wed, Mar 24, 2021 at 04:03:07PM +0100, Dumitru Ceara wrote:
> > > Hi Ben,
> > >
> > > We discussed a bit about this during one of the recent IRC OVN meetings,
> > > but I didn't get around to properly reporting this until now.
> > >
> > > I've tried running ovn-northd-ddlog against some large OVN NB/DB
> > > databases extracted from one of our scale testing runs:
> > >
> > > http://people.redhat.com/~dceara/ovn-northd-ddlog-tests/20210324/existing-nb-sb/
> > >
> > > It seems that ovn-northd-ddlog gets stuck in a busy loop and uses a lot
> > > of memory:
> > >
> > > 775734 root 10 -10 81.6g 80.8g 22396 S 99.7 64.2 3:50.79
> > > ovn-northd-ddlog
> > >
> > > ovn-northd-ddlog is stuck in an (infinite?) loop in
> > > ddlog_transaction_commit_dump_changes():
> > >
> > > (gdb) bt
> > > #0 0x7f2762167e92 in pthread_cond_wait@@GLIBC_2.3.2 () from
> > > /lib64/libpthread.so.0
> > > #1 0x00891eb3 in std::thread::park ()
> > > #2 0x00530963 in crossbeam_channel::context::Context::wait_until
> > > ()
> > > #3 0x00530aa1 in
> > > crossbeam_channel::context::Context::with::{{closure}} ()
> > > #4 0x00531f5c in
> > > crossbeam_channel::flavors::list::Channel::recv ()
> > > #5 0x0075b35d in crossbeam_channel::channel::Receiver::recv ()
> > > #6 0x00512156 in
> > > differential_datalog::program::RunningProgram::flush ()
> > > #7 0x0050de67 in
> > > differential_datalog::program::RunningProgram::transaction_commit ()
> > > #8 0x0093f17d in > > differential_datalog::ddlog::DDlog>::transaction_commit_dump_changes ()
> > > #9 0x0040cc40 in ddlog_transaction_commit_dump_changes ()
> > > #10 0x0040a758 in ddlog_commit (ddlog=) at
> > > northd/ovn-northd-ddlog.c:435
> > > #11 northd_parse_update (update=0x3dc0598, ctx=0x3d71880) at
> > > northd/ovn-northd-ddlog.c:435
> > > #12 northd_run (ctx=0x3d71880) at northd/ovn-northd-ddlog.c:512
> > > #13 0x00408edd in main (argc=, argv= > > out>) at northd/ovn-northd-ddlog.c:1203
> > >
> > > For comparison, running the C version of ovn-northd I get:
> > >
> > > 2021-03-24T14:48:06.556Z|00033|timeval|WARN|Unreasonably long 11290ms
> > > poll interval (10916ms user, 334ms system)
> > > 2021-03-24T14:48:14.678Z|00050|timeval|WARN|Unreasonably long 8122ms poll
> > > interval (8046ms user, 48ms system)
> > >
> > > But after the northd iteration ends, memory usage is OK:
> > >
> > > 777567 root 10 -10 1657308 1.6g 3496 S 0.0 1.2 0:49.08
> > > ovn-northd
> > >
> > > The behavior above is consistent both with current OVN master and also
> > > when cherry-picking the ddlog-related patches that are pending in
> > > patchwork:
> > > http://patchwork.ozlabs.org/project/ovn/list/?series=233075
> > > http://patchwork.ozlabs.org/project/ovn/list/?series=232480
> > > http://patchwork.ozlabs.org/project/ovn/list/?series=233079
> > > http://patchwork.ozlabs.org/project/ovn/list/?series=233080
> > >
> > > I didn't try out the changes from the following series though as I
> > > understand they need a v2:
> > > http://patchwork.ozlabs.org/project/ovn/list/?series=232040
> > >
> > > Regards,
> > > Dumitru
> > >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovs]: lib/db-ctl-base.c: Check that --all or records argument provided
From: Alexey Roytman
ovn-nbctl and ovn-sbctl CLI utilities allow destroying the entire
table or some records from the given table.
However, either the --all option or the deleted records should be provided.
This patch adds the test.
Signed-off-by: Alexey Roytman
---
lib/db-ctl-base.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index e95c77da2..81ade7abc 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -1823,6 +1823,11 @@ cmd_destroy(struct ctl_context *ctx)
return;
}
+if (!delete_all && ctx->argc == 2) {
+ctl_error(ctx, "either --all or records argument should be specified");
+return;
+}
+
if (delete_all) {
const struct ovsdb_idl_row *row;
const struct ovsdb_idl_row *next_row;
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/2] testsuite: add test cases for ingress_policing parameters
On Fri, Mar 12, 2021 at 12:59:17PM +0100, Simon Horman wrote:
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -70,3 +70,50 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded
> flows : [[1-9]]"], [0], [i
>
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst -
> offloads disabled])
> +AT_KEYWORDS([ingress_policing])
> +OVS_TRAFFIC_VSWITCHD_START()
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
> +AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
> +AT_CHECK([cat other_config.txt],[0],
> +[other_config: {hw-offload="false"}
> +])
> +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |grep rate| awk
> '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' >
> tc_ovs-p0.txt ],[0],[])
Uhh.. I wanted to say "just use tc -json ... | jq ..." to avoid all this
parsing, but the action police doesn't support it yet. :-(
> +AT_CHECK([cat tc_ovs-p0.txt],[0],
> +[rate 100Kbit burst 1280b
This ties back to my previous email. Is 1280b really what is expected
here? (for both flavors)
If 100K was kept as 100K, seems 'K' is what is wanted.
Then, the burst would be 1250b.
While reading the code on this, now noticed that in
netdev_dpdk_policer_construct() it uses 1000 for both.
Anyhow, good that this is now getting asserted.
Other than this and comments already made by Tonghao Zhang, I have no
further comments.
Thanks,
Marcelo
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] netdev-linux: correct unit of burst parameter
On Thu, Mar 25, 2021 at 03:51:11PM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman wrote:
> > From: "Yong.Xu"
> >
> > Correct calculation of burst parameter used when configuring TC policer
> > action for ingress port-based policing in the case where TC offload is in
> > use. This now matches the value calculated for the case where TC offload is
> > not in use.
> >
> > The division by 8 is to convert from bits to bytes.
> > Its unclear why 64 was previously used.
>
> Yeah.. I have the feeling that it might be related to kernel's:
> /* Avoid doing 64 bit divide */
> #define PSCHED_SHIFT6
> #define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT)
> #define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT)
> but I can't confirm it.
>
> >
> > Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload")
> > Signed-off-by: Yong.Xu
> > [simon: reworked changelog]
> > Signed-off-by: Simon Horman
> > Signed-off-by: Louis Peens
> > ---
> > lib/netdev-linux.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 15b25084b..f87a20075 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2572,7 +2572,7 @@ exit:
> > static struct tc_police
> > tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
> > {
> > -unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
> > +unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
> > unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8;
>
> I know that the patch is not changing this but, while at this, why for
> bsize the 'k' is 1024, while for bps it's 1000?
>
> AFAICT it backtracks to
> netdev_set_policing(iface->netdev,
> MIN(UINT32_MAX, iface->cfg->ingress_policing_rate),
> MIN(UINT32_MAX,
> iface->cfg->ingress_policing_burst));
> in iface_configure_qos() and I don't see a reason for them being
> different.
>
> qos.rst states:
> ``ingress_policing_rate``
> the maximum rate (in Kbps) that this VM should be allowed to send
>
> ``ingress_policing_burst``
> a parameter to the policing algorithm to indicate the maximum amount of data
> (in Kb) that this interface can send beyond the policing rate.
>
> Both with capital K. So if the 64 was bothering, the 1024 is likely
> doing so as well.
While vswitchd/vswitch.xml says:
Maximum rate for data received on this interface, in kbps. Data
Maximum burst size for data received on this interface, in kb. The
default burst size if set to 0 is 8000 kbit. This value
I'm not sure which one has preference here.
>
> Nevertheless, as the patch is fixing the /64, patch LGTM.
>
> > struct tc_police police;
> > struct tc_ratespec rate;
> > --
> > 2.20.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] netdev-linux: correct unit of burst parameter
On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman wrote:
> From: "Yong.Xu"
>
> Correct calculation of burst parameter used when configuring TC policer
> action for ingress port-based policing in the case where TC offload is in
> use. This now matches the value calculated for the case where TC offload is
> not in use.
>
> The division by 8 is to convert from bits to bytes.
> Its unclear why 64 was previously used.
Yeah.. I have the feeling that it might be related to kernel's:
/* Avoid doing 64 bit divide */
#define PSCHED_SHIFT6
#define PSCHED_TICKS2NS(x) ((s64)(x) << PSCHED_SHIFT)
#define PSCHED_NS2TICKS(x) ((x) >> PSCHED_SHIFT)
but I can't confirm it.
>
> Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload")
> Signed-off-by: Yong.Xu
> [simon: reworked changelog]
> Signed-off-by: Simon Horman
> Signed-off-by: Louis Peens
> ---
> lib/netdev-linux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 15b25084b..f87a20075 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2572,7 +2572,7 @@ exit:
> static struct tc_police
> tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
> {
> -unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
> +unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
> unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8;
I know that the patch is not changing this but, while at this, why for
bsize the 'k' is 1024, while for bps it's 1000?
AFAICT it backtracks to
netdev_set_policing(iface->netdev,
MIN(UINT32_MAX, iface->cfg->ingress_policing_rate),
MIN(UINT32_MAX, iface->cfg->ingress_policing_burst));
in iface_configure_qos() and I don't see a reason for them being
different.
qos.rst states:
``ingress_policing_rate``
the maximum rate (in Kbps) that this VM should be allowed to send
``ingress_policing_burst``
a parameter to the policing algorithm to indicate the maximum amount of data
(in Kb) that this interface can send beyond the policing rate.
Both with capital K. So if the 64 was bothering, the 1024 is likely
doing so as well.
Nevertheless, as the patch is fixing the /64, patch LGTM.
> struct tc_police police;
> struct tc_ratespec rate;
> --
> 2.20.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 0/2] netdev-linux: correct unit of burst parameter
On Thu, Mar 18, 2021 at 02:47:32PM +0100, Simon Horman wrote: > On Fri, Mar 12, 2021 at 12:59:15PM +0100, Simon Horman wrote: > > Hi, > > > > this short series corrects the unit of the burst parameter used to > > configure TC policer actions in the case case where ingress rate limiting > > is configured when TC flow offload is enabled. > > > > Patch: 1/2 > > Change since v1: minor update to changelog > > > > Adds tests for the configuration of rate limiting. > > > > Patches 2/2 > > New in v2 > > Hi Marcello, > > could I trouble you to review this? Hi Simon, Sorry the delay.. Disclaimer: while I probably should be, I'm not that involved with this part of the code. But yes :-) Marcelo ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] expr: Combine multiple ipv4 with wildcard mask.
On 19/03/2021 13:01, Dumitru Ceara wrote: > On 3/10/21 2:29 PM, gmingchen(陈供明) wrote: >> From: Gongming Chen >> Thanks for the patch. Looks like a lot of thought went into the algorithm and it has been interesting to review. Do you know if there are any well-known algorithms to do this? It seems like a common problem? If there was, it may be better to use it as we could reference standard documentation. >> This patch merges ipv4 addresses with wildcard masks, and replaces this >> ipv4 addresses with the combined ip/mask. This will greatly reduce the >> entries in the ovs security group flow table, especially when the host >> size is large. >> >> Analysis in the simplest scenario, a network 1.1.1.0/24 network,create 253 >> ports(1.1.1.2-1.1.1.254). >> Only focus on the number of ip addresses, the original 253 addresses will >> be combined into 13 addresses. >> 1.1.1.2/31 >> 1.1.1.4/30 >> 1.1.1.8/29 >> 1.1.1.16/28 >> 1.1.1.32/27 >> 1.1.1.64/26 >> 1.1.1.128/26 >> 1.1.1.192/27 >> 1.1.1.224/28 >> 1.1.1.240/29 >> 1.1.1.248/30 >> 1.1.1.252/31 >> 1.1.1.254 >> >> Some scenes are similar to the following: >> 1.1.1.2, 1.1.1.6 >> After the combine: >> 1.1.1.2/255.255.255.251 >> You can use ovn-match-ip utility to match ip. >> such as: >> ovs-ofctl dump-flows br-int | ovn-match-ip 1.1.1.6 >> 1.1.1.2/255.255.255.251 will show. >> >> Simple description of the algorithm. >> There are two situations >> 1. Combine once >> such as: >> 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1 >> Combined into: 1.1.1.0/31, 1.0.1.0/31 >> 2. Combine multiple times >> 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1 >> Combined into: 1.0.1.0/255.254.255.254 >> >> Considering the actual scene and simplicity, the first case is used to >> combine once. >> >> ...00... >> ...01... >> ...10... >> ...11... >> "..." means the same value omitted. >> Obviously, the above value can be expressed as ...00.../11100111. This >> continuous interval that can be represented by one or several wildcard >> masks is called a segment. >> Only if all 2<> exist, can they be combined into 00...(n)/00...( n) >> >> First sort all the values by size. Iterate through each value. >> 1. Find a new segment, where two values differ only by 1 bit, such as >> ...0... and ...1... >> diff = ip_next ^ ip >> if (diff & (diff-1)) == 0 >> new_segment = true >> The first non-zero place in the high direction of ip is the end of the >> segment(segment_end). >> For example...100... and...101..., the segment_end is ...111... >> >> 2. Count the number of consecutive and less than continuous_size in the >> segment. >> diff = ip_next - ip >> if (diff & (diff-1)) == 0 && ip_next <= segment_end >> continuous_size++ >> >> 3. Combine different ip intervals in the segment according to >> continuous_size. >> In continuous_size, from the highest bit of 1 to the lowest bit of 1, in >> the order of segment start, each bit that is 1 is a different ip interval >> that can be combined with a wildcard mask. >> For example, 000, 001, 010: >> continuous_size: 3 (binary 11), segment_start: 000 >> mask: ~(1 << 1 - 1) = 110; ~(1 << 0 - 1) = 111; >> Combined to: 000/110, 010/111 >> >> 4. The ip that cannot be recorded in a segment will not be combined. >> >> Signed-off-by: Gongming Chen >> --- > > Hi Gongming, > > Sorry for the delayed review. > > I have a few general remarks/concerns and some specific comments inline. > First, the general remarks. > > I'm wondering if it would make more sense for this wildcard combination > of IPs to be done outside OVN, in the CMS, for the following reasons: > > - it's IPv4 specific. > - the CMS usually has more information about when it is matching on IP > sets and can probably optimize the match it uses in the ACL. > - the code in expr_const_sets_add() used to be a relatively direct > translation from sets of constants to sets of port names/IPs; this > changes with the optimization your proposing. > - the new utility, ovn-match-ip, would be useful but I'm worried that > users will often forget to use it. I'd like to understand more about the benefits of this change, what problem is it solving? I agree with Dumitru that it will make debugging quite a bit more difficult (even with the new tool) so it would be good to understand the tradeoff? > > I'm curious about your and the maintainers' opinion on these points. > > Another general remark is to increment the patch version when submitting > a new revision. From what I can see this specific patch is at v7. When > sending a v8 you could add "-v8" when generating the patch, e.g.: > > git format-patch --subject-prefix="PATCH ovn" -v8 -M origin/master > > Would generate a patch with subject: > > "Subject: [PATCH ovn v8] " > > This usually helps when reviewing patches that go through multiple > iterations. > > More specific comments inline. > >> debian/ovn-common.install | 1 + >> lib/expr.c| 219 ++ >>
[ovs-dev] [PATCH ovn] lflow: Avoid parsing lflow->pipeline for non-local flows.
Spotted during code inspection.
Signed-off-by: Dumitru Ceara
---
controller/lflow.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index a3d84aff4..680b8cca1 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -716,15 +716,15 @@ consider_logical_flow__(const struct sbrec_logical_flow
*lflow,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
{
-/* Determine translation of logical table IDs to physical table IDs. */
-bool ingress = !strcmp(lflow->pipeline, "ingress");
-
if (!get_local_datapath(l_ctx_in->local_datapaths, dp->tunnel_key)) {
VLOG_DBG("lflow "UUID_FMT" is not for local datapath, skip",
UUID_ARGS(>header_.uuid));
return true;
}
+/* Determine translation of logical table IDs to physical table IDs. */
+bool ingress = !strcmp(lflow->pipeline, "ingress");
+
/* Determine translation of logical table IDs to physical table IDs. */
uint8_t first_ptable = (ingress
? OFTABLE_LOG_INGRESS_PIPELINE
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 6/9] tests: Amend release stale port binding test for RBAC
On Wed, Mar 24, 2021 at 2:32 PM Frode Nordahl
wrote:
>
> On Wed, Mar 24, 2021 at 1:54 PM Numan Siddique wrote:
> > I applied the patches 6 and 7 to the main branch.
> >
> > There are some issues with patch 9. I didn't apply patch 8 as it
> > seems related to patch 9.
> >
> > If I configure like below and run "make check" it fails for me. Can
> > you please take a look.
> >
> > $mkdir _gcc
> > $cd _gcc
> > $../configure --enable-Werror --enable-sparse --with-ovs-source=...
> > $cd ..
> > $make -C _gcc check
> > make[2]: Entering directory /tmp/ovn/_gcc'
> > make[2]: 'tests/atlocal' is up to date.
> > make[2]: 'tests/testpki-cacert.pem' is up to date.
> > make[2]: 'tests/testpki-test-cert.pem' is up to date.
> > make[2]: 'tests/testpki-test-privkey.pem' is up to date.
> > make[2]: 'tests/testpki-test-req.pem' is up to date.
> > make[2]: 'tests/testpki-test2-cert.pem' is up to date.
> > make[2]: 'tests/testpki-test2-privkey.pem' is up to date.
> > make[2]: 'tests/testpki-test2-req.pem' is up to date.
> > cp /tmp/ovn/_gcc/tests/pki/main-cert.pem tests/testpki-main-cert.pem
> > cp: cannot stat '/tmp/ovn/_gcc/tests/pki/main-cert.pem': No such file
> > or directory
> > make[2]: *** [Makefile:3512: tests/testpki-main-cert.pem] Error 1
> > make[2]: Leaving directory '/tmp/ovn/_gcc'
>
> Thank you for finding this issue, as it hid itself from me when
> checking with a plain `make distcheck`.
>
> I'll investigate and put up a v3.
Do you have any more details to share about your environment? I
created a clean container and ran your steps with patch 8 and 9 and it
succeeds with no issues here:
https://pastebin.ubuntu.com/p/WXryXktqCh/
There could still be an issue here, but I need some more detail to
figure out what is happening on your end.
--
Frode Nordahl
> --
> Frode Nordahl
>
>
> > Thanks
> > Numan
> >
> >
> > >
> > > --
> > > Frode Nordahl
> > >
> > > > Thanks.
> > > >
> > > > On 3/5/21 7:16 AM, Frode Nordahl wrote:
> > > > > The current version of the test attempts to simulate chassis
> > > > > registration prior to starting `ovn-controller`, however it does
> > > > > not set the `hostname` field.
> > > > >
> > > > > The RBAC role for `ovn-controller` does not allow for a chassis to
> > > > > change its own name or hostname, which makes sense as this is used
> > > > > for authentication.
> > > > >
> > > > > Update the test to set the `hostname` field when simulating chassis
> > > > > registration so that `ovn-controller` does not attempt to update it
> > > > > and subsequently make the test fail.
> > > > >
> > > > > Fixes b6b3823d4 ("ovn-controller: Fix I-P for SB Port_Binding and OVS
> > > > > Interface")
> > > > >
> > > > > Signed-off-by: Frode Nordahl
> > > > > ---
> > > > > tests/ovn.at | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > > > index bec593dcc..ca9623fee 100644
> > > > > --- a/tests/ovn.at
> > > > > +++ b/tests/ovn.at
> > > > > @@ -21572,7 +21572,7 @@ ovn-nbctl --wait=sb lsp-add ls1 lsp1
> > > > >
> > > > > # Simulate the fact that lsp1 had been previously bound on hv1.
> > > > > ovn-sbctl --id=@e create encap chassis_name=hv1 ip="192.168.0.1"
> > > > > type="geneve" \
> > > > > --- --id=@c create chassis name=hv1 encaps=@e \
> > > > > +-- --id=@c create chassis hostname=hv1 name=hv1 encaps=@e \
> > > > > -- set Port_Binding lsp1 chassis=@c
> > > > >
> > > > > as hv1
> > > > >
> > > >
> > > ___
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>
>
>
> --
> Frode Nordahl
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v4] binding: Fix the crashes seen when port binding type changes.
On 3/23/21 4:55 PM, num...@ovn.org wrote:
> From: Numan Siddique
>
> When a port binding type changes from type 'A' to type 'B', then
> there are many code paths in the existing binding.c which results
> in crashes due to use-after-free or NULL references.
>
> Below crashes are seen when a container lport is changed to a normal
> lport and then deleted.
>
> ***
> (gdb) bt
>0 in raise () from /lib64/libc.so.6
>1 in abort () from /lib64/libc.so.6
>2 ovs_abort_valist ("%s: assertion %s failed in %s()") at lib/util.c:419
>3 vlog_abort_valist ("%s: assertion %s failed in %s()") at lib/vlog.c:1249
>4 vlog_abort ("%s: assertion %s failed in %s()") at lib/vlog.c:1263
>5 ovs_assert_failure (where="lib/ovsdb-idl.c:4653",
> function="ovsdb_idl_txn_write__",
> condition="row->new_datum != NULL") at lib/util.c:86
>6 ovsdb_idl_txn_write__ () at lib/ovsdb-idl.c:4695
>7 ovsdb_idl_txn_write_clone () at lib/ovsdb-idl.c:4757
>8 sbrec_port_binding_set_chassis () at lib/ovn-sb-idl.c:25946
>9 release_lport () at controller/binding.c:971
> 10 release_local_binding_children () at controller/binding.c:1039
> 11 release_local_binding () at controller/binding.c:1056
> 12 consider_iface_release (iface_rec=..
> iface_id="bb43e818-b2ee-4329-b67e-218556580056") at controller/binding.c:1880
> 13 binding_handle_ovs_interface_changes () at controller/binding.c:1998
> 14 runtime_data_ovs_interface_handler () at
> controller/ovn-controller.c:1481
> 15 engine_compute () at lib/inc-proc-eng.c:306
> 16 engine_run_node () at lib/inc-proc-eng.c:352
> 17 engine_run () at lib/inc-proc-eng.c:377
> 18 main () at controller/ovn-controller.c:2826
>
> The present code creates a 'struct local_binding' instance for a
> container lport and adds this object to the parent local binding
> children list. And if the container lport is changed to a normal
> vif, then there is no way to access the local binding object created
> earlier. This patch fixes these type of issues by refactoring the
> 'local binding' code of binding.c. This patch now creates only one
> instance of 'struct local_binding' for every OVS interface with
> external_ids:iface-id set. A new structure 'struct binding_lport' is
> added which is created for a VIF, container and virtual port bindings
> and is stored in 'binding_lports' shash. 'struct local_binding' now
> maintains a list of binding_lports which it maps to.
>
> When a container lport is changed to a normal lport, we now can
> easily access the 'binding_lport' object of the container lport
> fron the 'binding_lports' shash.
>
> Reported-by: Dumitru Ceara
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1936328
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1936331
>
> Co-authored-by: Dumitru Ceara
> Signed-off-by: Dumitru Ceara
> Signed-off-by: Numan Siddique
> ---
Hi Numan,
>
> v3 -> v4
> ---
> * -> Addressed some of the review comments from Dumitru.
> Added another test case to test the scenarion when a container
> port changes its parent from one to another port.
>
> v2 -> v3
>
> * Fixed a crash seen when a container port is changed to normal port
>and then deleted in the same transaction. Added the relevant test case
>for this.
>
> v1 -> v2
>
> * Fixed a crash seen when 2 container ports are changed to normal ports
>in the same transaction. Added the relevant test case for this.
>
> controller/binding.c| 973 +++-
> controller/binding.h| 32 +-
> controller/ovn-controller.c | 25 +-
> controller/physical.c | 13 +-
> tests/ovn.at| 304 +++
> 5 files changed, 949 insertions(+), 398 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 4e6c756969..0e3e4aad91 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -597,6 +597,23 @@ remove_local_lport_ids(const struct sbrec_port_binding
> *pb,
> }
> }
>
> +/* Corresponds to each Port_Binding.type. */
> +enum en_lport_type {
> +LP_UNKNOWN,
> +LP_VIF,
> +LP_CONTAINER,
> +LP_PATCH,
> +LP_L3GATEWAY,
> +LP_LOCALNET,
> +LP_LOCALPORT,
> +LP_L2GATEWAY,
> +LP_VTEP,
> +LP_CHASSISREDIRECT,
> +LP_VIRTUAL,
> +LP_EXTERNAL,
> +LP_REMOTE
> +};
> +
> /* Local bindings. binding.c module binds the logical port (represented by
> * Port_Binding rows) and sets the 'chassis' column when it sees the
> * OVS interface row (of type "" or "internal") with the
> @@ -608,134 +625,116 @@ remove_local_lport_ids(const struct
> sbrec_port_binding *pb,
> * 'struct local_binding' is used. A shash of these local bindings is
> * maintained with the 'external_ids:iface-id' as the key to the shash.
> *
> - * struct local_binding (defined in binding.h) has 3 main fields:
> - *- type
> + * struct local_binding
Re: [ovs-dev] [PATCH ovn] Fix connection string in case of changes in the ovndb-servers.ocf RA
On Thu, Mar 25, 2021 at 3:02 PM Michele Baldessari wrote:
>
> Currently if you update the master_ip attribute, pacemaker will restart
> the resource but the ovn master role will still listen to the previous
> old ip address. The reason for this is the following piece of code:
>
> conn=`ovn-nbctl get NB_global . connections`
> if [ "$conn" == "[]" ]; then
> ovn-nbctl -- --id=@conn_uuid create Connection \
> target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTEN_ON_IP}" \
> inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
> connections=@conn_uuid
> fi
>
> Once the connection is set the first time 'get NB_global . connections'
> will always return the UUID of the connection so we're never changing
> the connection data.
>
> Let's set the connection info whenever the connection is not "[]"
>
> Tested as follows:
> 1) Started with the following listening ip addresses (resource is configured
> with master_ip=172.17.1.44):
> tcp LISTEN 0 10172.17.1.44:6641 0.0.0.0:*
> users:(("ovsdb-server",pid=150360,fd=15)) ino:18609895 sk:4d <->
> tcp LISTEN 0 10172.17.1.44:6642 0.0.0.0:*
> users:(("ovsdb-server",pid=150379,fd=15)) ino:18609038 sk:4e <->
>
> 2) Updated the resource master_ip with:
> pcs resource update ovndb_servers master_ip=4.5.6.7
>
> 3) Observed the new listening ip addresses on the master node:
> [root@controller-3 stdouts]# ss -natulpe |grep 664[12]
> tcp LISTEN 0 104.5.6.7:6641 0.0.0.0:*
> users:(("ovsdb-server",pid=516770,fd=15)) ino:11641860 sk:4d <->
> tcp LISTEN 0 104.5.6.7:6642 0.0.0.0:*
> users:(("ovsdb-server",pid=516789,fd=15)) ino:11642885 sk:4e <->
>
> Previously we'd observe ovsdb-server listening to the old IP even after
> the resource change.
>
> Signed-off-by: Michele Baldessari
Thanks Michele for the patch. There was compilation issue due to tab
being used.
I fixed it and applied the patch to the main branch. Please let me
know if it needs backports for the released branches.
Thanks
Numan
> ---
> utilities/ovndb-servers.ocf | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/utilities/ovndb-servers.ocf b/utilities/ovndb-servers.ocf
> index 7351c7d64a94..896d590ecc78 100755
> --- a/utilities/ovndb-servers.ocf
> +++ b/utilities/ovndb-servers.ocf
> @@ -259,6 +259,9 @@ ovsdb_server_notify() {
> ovn-nbctl -- --id=@conn_uuid create Connection \
> target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTEN_ON_IP}" \
> inactivity_probe=$INACTIVE_PROBE -- set NB_Global . connections=@conn_uuid
> + else
> +CONN_UID=$(sed -e 's/^\[//' -e 's/\]$//' <<< ${conn})
> +ovn-nbctl set connection "${CONN_UID}"
> target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTEN_ON_IP}"
> fi
>
> conn=`ovn-sbctl get SB_global . connections`
> @@ -267,6 +270,9 @@ inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
> connections=@conn_uuid
> ovn-sbctl -- --id=@conn_uuid create Connection \
> target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${LISTEN_ON_IP}" \
> inactivity_probe=$INACTIVE_PROBE -- set SB_Global . connections=@conn_uuid
> +else
> +CONN_UID=$(sed -e 's/^\[//' -e 's/\]$//' <<< ${conn})
> +ovn-sbctl set connection "${CONN_UID}"
> target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${LISTEN_ON_IP}"
> fi
>
> else
> --
> 2.30.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] pinctrl: Don't send gARPs for localports
On Thu, Mar 25, 2021 at 2:42 PM Daniel Alvarez Sanchez wrote: > > On Thu, Mar 25, 2021 at 8:58 AM Dumitru Ceara wrote: > > > On 3/24/21 6:23 PM, Daniel Alvarez Sanchez wrote: > > > Ports of type 'localport' are present on every hypervisor and > > > ovn-controller is sending gARPs for them which makes upstream > > > switches to see its MAC address flapping. > > > > > > In order to avoid this behavior, the current patch is skipping > > > localports when sending gARP/RARP packets. > > > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939470 > > > > > > Signed-off-by: Daniel Alvarez Sanchez > > > > 0-day robot was complaining because this misses: > > > > Co-authored-by: Dumitru Ceara > > > > But I guess that, if the patch is accepted, this can be added at commit > > time. > > > > Right, if you think it's best I can send a v3 replacing your 'Signed-off' > by 'Co-authored'. Thanks Daniel and Dumitru. I added the Co-authored-by tag and applied the patch to main branch and branch-21.03. I suppose we should also backport to other branches. I'll backport them in some time. Thanks Numan > > thanks! > daniel > > > > > Regards, > > Dumitru > > > > > Signed-off-by: Dumitru Ceara > > > --- > > > > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Fix connection string in case of changes in the ovndb-servers.ocf RA
Bleep bloop. Greetings Michele Baldessari, 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 has non-spaces leading whitespace #52 FILE: utilities/ovndb-servers.ocf:262: else Lines checked: 70, Warnings: 1, Errors: 0 build: /bin/python3 ./build-aux/dpdkstrip.py | \ sed \ -e 's,[@]PKIDIR[@],/usr/local/var/lib/openvswitch/pki,g' \ -e 's,[@]LOGDIR[@],/usr/local/var/log/ovn,g' \ -e 's,[@]DBDIR[@],/usr/local/etc/ovn,g' \ -e 's,[@]PYTHON3[@],/bin/python3,g' \ -e 's,[@]OVN_RUNDIR[@],/usr/local/var/run/ovn,g' \ -e 's,[@]OVSBUILDDIR[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR,g' \ -e 's,[@]VERSION[@],21.03.90,g' \ -e 's,[@]OVSVERSION[@],2.15.90,g' \ -e 's,[@]localstatedir[@],/usr/local/var,g' \ -e 's,[@]pkgdatadir[@],/usr/local/share/ovn,g' \ -e 's,[@]sysconfdir[@],/usr/local/etc,g' \ -e 's,[@]bindir[@],/usr/local/bin,g' \ -e 's,[@]sbindir[@],/usr/local/sbin,g' \ -e 's,[@]abs_builddir[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace,g' \ -e 's,[@]abs_top_srcdir[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace,g' \ > rhel/ovn-fedora.spec.tmp mv rhel/ovn-fedora.spec.tmp rhel/ovn-fedora.spec utilities/ovndb-servers.ocf See above for files that use tabs for indentation. Please use spaces instead. make[1]: *** [check-tabs] Error 1 make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace' make: *** [all] Error 2 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] Fix connection string in case of changes in the ovndb-servers.ocf RA
Currently if you update the master_ip attribute, pacemaker will restart
the resource but the ovn master role will still listen to the previous
old ip address. The reason for this is the following piece of code:
conn=`ovn-nbctl get NB_global . connections`
if [ "$conn" == "[]" ]; then
ovn-nbctl -- --id=@conn_uuid create Connection \
target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTEN_ON_IP}" \
inactivity_probe=$INACTIVE_PROBE -- set NB_Global . connections=@conn_uuid
fi
Once the connection is set the first time 'get NB_global . connections'
will always return the UUID of the connection so we're never changing
the connection data.
Let's set the connection info whenever the connection is not "[]"
Tested as follows:
1) Started with the following listening ip addresses (resource is configured
with master_ip=172.17.1.44):
tcp LISTEN 0 10172.17.1.44:6641 0.0.0.0:*
users:(("ovsdb-server",pid=150360,fd=15)) ino:18609895 sk:4d <->
tcp LISTEN 0 10172.17.1.44:6642 0.0.0.0:*
users:(("ovsdb-server",pid=150379,fd=15)) ino:18609038 sk:4e <->
2) Updated the resource master_ip with:
pcs resource update ovndb_servers master_ip=4.5.6.7
3) Observed the new listening ip addresses on the master node:
[root@controller-3 stdouts]# ss -natulpe |grep 664[12]
tcp LISTEN 0 104.5.6.7:6641 0.0.0.0:*
users:(("ovsdb-server",pid=516770,fd=15)) ino:11641860 sk:4d <->
tcp LISTEN 0 104.5.6.7:6642 0.0.0.0:*
users:(("ovsdb-server",pid=516789,fd=15)) ino:11642885 sk:4e <->
Previously we'd observe ovsdb-server listening to the old IP even after
the resource change.
Signed-off-by: Michele Baldessari
---
utilities/ovndb-servers.ocf | 6 ++
1 file changed, 6 insertions(+)
diff --git a/utilities/ovndb-servers.ocf b/utilities/ovndb-servers.ocf
index 7351c7d64a94..896d590ecc78 100755
--- a/utilities/ovndb-servers.ocf
+++ b/utilities/ovndb-servers.ocf
@@ -259,6 +259,9 @@ ovsdb_server_notify() {
ovn-nbctl -- --id=@conn_uuid create Connection \
target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTEN_ON_IP}" \
inactivity_probe=$INACTIVE_PROBE -- set NB_Global . connections=@conn_uuid
+ else
+CONN_UID=$(sed -e 's/^\[//' -e 's/\]$//' <<< ${conn})
+ovn-nbctl set connection "${CONN_UID}"
target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTEN_ON_IP}"
fi
conn=`ovn-sbctl get SB_global . connections`
@@ -267,6 +270,9 @@ inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
connections=@conn_uuid
ovn-sbctl -- --id=@conn_uuid create Connection \
target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${LISTEN_ON_IP}" \
inactivity_probe=$INACTIVE_PROBE -- set SB_Global . connections=@conn_uuid
+else
+CONN_UID=$(sed -e 's/^\[//' -e 's/\]$//' <<< ${conn})
+ovn-sbctl set connection "${CONN_UID}"
target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${LISTEN_ON_IP}"
fi
else
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [OVN Patch v15 1/3] ovn-libs: Add support for parallel processing
On 24/03/2021 15:31, Numan Siddique wrote:
On Mon, Mar 1, 2021 at 6:35 PM wrote:
From: Anton Ivanov
This adds a set of functions and macros intended to process
hashes in parallel.
The principles of operation are documented in the ovn-parallel-hmap.h
If these one day go into the OVS tree, the OVS tree versions
would be used in preference.
Signed-off-by: Anton Ivanov
Hi Anton,
I tested the first 2 patches of this series and it crashes again for me.
This time I ran tests on a 4 core machine - Intel(R) Xeon(R) CPU
E3-1220 v5 @ 3.00GHz
The below trace is seen for both gcc and clang.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Core was generated by `ovn-northd -vjsonrpc
--ovnnb-db=unix:/mnt/mydisk/myhome/numan_alt/work/ovs_ovn/'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x7f27594ae212 in __new_sem_wait_slow.constprop.0 () from
/lib64/libpthread.so.0
[Current thread is 1 (Thread 0x7f2758c68640 (LWP 347378))]
Missing separate debuginfos, use: dnf debuginfo-install
glibc-2.32-3.fc33.x86_64 libcap-ng-0.8-1.fc33.x86_64
libevent-2.1.8-10.fc33.x86_64 openssl-libs-1.1.1i-1.fc33.x86_64
python3-libs-3.9.1-2.fc33.x86_64 unbound-libs-1.10.1-4.fc33.x86_64
zlib-1.2.11-23.fc33.x86_64
(gdb) bt
#0 0x7f27594ae212 in __new_sem_wait_slow.constprop.0 () from
/lib64/libpthread.so.0
#1 0x00422184 in wait_for_work (control=) at
../lib/ovn-parallel-hmap.h:203
#2 build_lflows_thread (arg=0x2538420) at ../northd/ovn-northd.c:11855
#3 0x0049cd12 in ovsthread_wrapper (aux_=) at
../lib/ovs-thread.c:383
#4 0x7f27594a53f9 in start_thread () from /lib64/libpthread.so.0
#5 0x7f2759142903 in clone () from /lib64/libc.so.6
-
I'm not sure why you're not able to reproduce this issue.
I can't. I have run it for days in a loop.
One possibility is that for whatever reason your machine has slower IPC speeds
compared to linear execution speeds. Thread debugging? AMD vs Intel? No idea.
There is a race on-exit in the current code which I have found by inspection
and which I have never been able to trigger. On my machines the workers always
exit in time before the main thread has finished, so I cannot trigger this.
Can you try this incremental fix to see if it fixes the problem for you. If
that works, I will incorporate it and reissue the patch. If not - I will
continue digging.
diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c
index e83ae23cb..3597f896f 100644
--- a/lib/ovn-parallel-hmap.c
+++ b/lib/ovn-parallel-hmap.c
@@ -143,7 +143,8 @@ struct worker_pool *ovn_add_worker_pool(void *(*start)(void
*)){
}
for (i = 0; i < pool_size; i++) {
-ovs_thread_create("worker pool helper", start,
_pool->controls[i]);
+new_pool->controls[i].worker =
+ovs_thread_create("worker pool helper", start,
_pool->controls[i]);
}
ovs_list_push_back(_pools, _pool->list_node);
}
@@ -386,6 +387,9 @@ static void worker_pool_hook(void *aux OVS_UNUSED) {
for (i = 0; i < pool->size ; i++) {
sem_post(pool->controls[i].fire);
}
+for (i = 0; i < pool->size ; i++) {
+pthread_join(pool->controls[i].worker, NULL);
+}
for (i = 0; i < pool->size ; i++) {
sem_close(pool->controls[i].fire);
sprintf(sem_name, WORKER_SEM_NAME, sembase, pool, i);
diff --git a/lib/ovn-parallel-hmap.h b/lib/ovn-parallel-hmap.h
index 8db61eaba..d62ca3da5 100644
--- a/lib/ovn-parallel-hmap.h
+++ b/lib/ovn-parallel-hmap.h
@@ -82,6 +82,7 @@ struct worker_control {
struct ovs_mutex mutex; /* Guards the data. */
void *data; /* Pointer to data to be processed. */
void *workload; /* back-pointer to the worker pool structure. */
+pthread_t worker;
};
struct worker_pool {
All the test cases passed for me. So maybe something's wrong when
ovn-northd exits.
IMHO, these crashes should be addressed before these patches can be considered.
Thanks
Numan
---
lib/automake.mk | 2 +
lib/ovn-parallel-hmap.c | 455
lib/ovn-parallel-hmap.h | 301 ++
3 files changed, 758 insertions(+)
create mode 100644 lib/ovn-parallel-hmap.c
create mode 100644 lib/ovn-parallel-hmap.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 250c7aefa..781be2109 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -13,6 +13,8 @@ lib_libovn_la_SOURCES = \
lib/expr.c \
lib/extend-table.h \
lib/extend-table.c \
+ lib/ovn-parallel-hmap.h \
+ lib/ovn-parallel-hmap.c \
lib/ip-mcast-index.c \
lib/ip-mcast-index.h \
lib/mcast-group-index.c \
diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c
new file mode 100644
index 0..e83ae23cb
--- /dev/null
+++ b/lib/ovn-parallel-hmap.c
@@ -0,0 +1,455 @@
+/*
+ * Copyright (c) 2020 Red
Re: [ovs-dev] [PATCH V4 00/14] Netdev vxlan-decap offload
On 3/25/2021 11:16 AM, Ivan Malov wrote:
External email: Use caution opening links or attachments
Hello,
You know, now you mention it, will action COUNT always go before any
other actions in OvS ("tunnel set" and "tunnel match" flows)? If this is
the case, then the byte count in the counter of "tunnel set" flow should
be "*before* decapsulation" and the byte count in the counter of "tunnel
match" flow should be "*after* decapsulation". Is my understanding right?
Specifically for MLX5 PMD it doesn't matter (this WA won't defect the
stats), but your understanding is correct, and this is how it is done in
this series.
On 25/03/2021 12:10, Eli Britstein wrote:
Hello,
Note that MLX5 PMD has a bug that the tnl_pop private actions must be
first. In OVS it is not.
Fixing this issue is scheduled for 21.05 (and stable 20.11.2).
Meanwhile, tests were done with a workaround for it.
See
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Felibritstein%2Fdpdk-stable%2Fpull%2F1data=04%7C01%7Celibr%40nvidia.com%7C4107e762202a4165d18008d8ef6ea88d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637522605863267715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=N%2B5G3EXiqvSwUiwCv%2F%2Ba4AT6CNBZGV%2F2SPhi1LuF83k%3Dreserved=0
Also, any other comments on this series?
Thanks,
Eli
On 3/17/2021 8:35 AM, Eli Britstein wrote:
VXLAN decap in OVS-DPDK configuration consists of two flows:
F1: in_port(ens1f0),eth(),ipv4(),udp(), actions:tnl_pop(vxlan_sys_4789)
F2: tunnel(),in_port(vxlan_sys_4789),eth(),ipv4(), actions:ens1f0_0
F1 is a classification flow. It has outer headers matches and it
classifies the packet as a VXLAN packet, and using tnl_pop action the
packet continues processing in F2.
F2 is a flow that has matches on tunnel metadata as well as on the
inner
packet headers (as any other flow).
In order to fully offload VXLAN decap path, both F1 and F2 should be
offloaded. As there are more than one flow in HW, it is possible that
F1 is done by HW but F2 is not. Packet is received by SW, and should be
processed starting from F2 as F1 was already done by HW.
Rte_flows are applicable only on physical port IDs. Keeping the
original
physical in port on which the packet is received on enables applying
vport flows (e.g. F2) on that physical port.
This patch-set makes use of [1] introduced in DPDK 20.11, that adds API
for tunnel offloads.
v2-v1:
- Tracking original in_port, and applying vport on that physical port
instead of all PFs.
v3-v2:
- Traversing ports using a new API instead of flow_dump.
- Refactor packet state recover logic, with bug fix for error
pop_header.
- One ref count for netdev in non-tunnel case.
- Rename variables, comments, rebase.
v4-v3:
- Extract orig_in_port from physdev for flow modify.
- Miss handling fixes.
Travis:
v1:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Felibritstein%2FOVS%2Fbuilds%2F756418552data=04%7C01%7Celibr%40nvidia.com%7C4107e762202a4165d18008d8ef6ea88d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637522605863267715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=naQh0J1BfXE62kUJ5lrsUGVZjA3hOT6hHIOwwnrypDk%3Dreserved=0
v2:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Felibritstein%2FOVS%2Fbuilds%2F758382963data=04%7C01%7Celibr%40nvidia.com%7C4107e762202a4165d18008d8ef6ea88d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637522605863267715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=h%2Fz8UbZgQOHAFUWed35AQ1HCkohzeKfhxnIVeN9zToQ%3Dreserved=0
v3:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Felibritstein%2FOVS%2Fbuilds%2F761089087data=04%7C01%7Celibr%40nvidia.com%7C4107e762202a4165d18008d8ef6ea88d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637522605863267715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=dL3iEXINyDjzOEpyQJ7PgGy03eJZQSJWvqHyfJpmREk%3Dreserved=0
v4:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Felibritstein%2FOVS%2Fbuilds%2F763146966data=04%7C01%7Celibr%40nvidia.com%7C4107e762202a4165d18008d8ef6ea88d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637522605863267715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=b4Yd2RflTgRU76v5ntkkbpXJsAExFCGuZ3t4HMxnzlk%3Dreserved=0
GitHub Actions:
v1:
Re: [ovs-dev] [PATCH V4 00/14] Netdev vxlan-decap offload
Hello,
You know, now you mention it, will action COUNT always go before any
other actions in OvS ("tunnel set" and "tunnel match" flows)? If this is
the case, then the byte count in the counter of "tunnel set" flow should
be "*before* decapsulation" and the byte count in the counter of "tunnel
match" flow should be "*after* decapsulation". Is my understanding right?
On 25/03/2021 12:10, Eli Britstein wrote:
Hello,
Note that MLX5 PMD has a bug that the tnl_pop private actions must be
first. In OVS it is not.
Fixing this issue is scheduled for 21.05 (and stable 20.11.2).
Meanwhile, tests were done with a workaround for it.
See https://github.com/elibritstein/dpdk-stable/pull/1
Also, any other comments on this series?
Thanks,
Eli
On 3/17/2021 8:35 AM, Eli Britstein wrote:
VXLAN decap in OVS-DPDK configuration consists of two flows:
F1: in_port(ens1f0),eth(),ipv4(),udp(), actions:tnl_pop(vxlan_sys_4789)
F2: tunnel(),in_port(vxlan_sys_4789),eth(),ipv4(), actions:ens1f0_0
F1 is a classification flow. It has outer headers matches and it
classifies the packet as a VXLAN packet, and using tnl_pop action the
packet continues processing in F2.
F2 is a flow that has matches on tunnel metadata as well as on the inner
packet headers (as any other flow).
In order to fully offload VXLAN decap path, both F1 and F2 should be
offloaded. As there are more than one flow in HW, it is possible that
F1 is done by HW but F2 is not. Packet is received by SW, and should be
processed starting from F2 as F1 was already done by HW.
Rte_flows are applicable only on physical port IDs. Keeping the original
physical in port on which the packet is received on enables applying
vport flows (e.g. F2) on that physical port.
This patch-set makes use of [1] introduced in DPDK 20.11, that adds API
for tunnel offloads.
v2-v1:
- Tracking original in_port, and applying vport on that physical port
instead of all PFs.
v3-v2:
- Traversing ports using a new API instead of flow_dump.
- Refactor packet state recover logic, with bug fix for error pop_header.
- One ref count for netdev in non-tunnel case.
- Rename variables, comments, rebase.
v4-v3:
- Extract orig_in_port from physdev for flow modify.
- Miss handling fixes.
Travis:
v1: https://travis-ci.org/github/elibritstein/OVS/builds/756418552
v2: https://travis-ci.org/github/elibritstein/OVS/builds/758382963
v3: https://travis-ci.org/github/elibritstein/OVS/builds/761089087
v4: https://travis-ci.org/github/elibritstein/OVS/builds/763146966
GitHub Actions:
v1: https://github.com/elibritstein/OVS/actions/runs/515334647
v2: https://github.com/elibritstein/OVS/actions/runs/554986007
v3: https://github.com/elibritstein/OVS/actions/runs/613226225
v4: https://github.com/elibritstein/OVS/actions/runs/658415274
[1] https://mails.dpdk.org/archives/dev/2020-October/187314.html
Eli Britstein (11):
netdev-offload: Add HW miss packet state recover API
netdev-dpdk: Introduce DPDK tunnel APIs
netdev-offload: Introduce an API to traverse ports
netdev-dpdk: Add flow_api support for netdev vxlan vports
netdev-offload-dpdk: Implement HW miss packet recover for vport
dpif-netdev: Add HW miss packet state recover logic
netdev-offload-dpdk: Change log rate limits
netdev-offload-dpdk: Support tunnel pop action
netdev-offload-dpdk: Refactor offload rule creation
netdev-offload-dpdk: Support vports flows offload
netdev-dpdk-offload: Add vxlan pattern matching function
Ilya Maximets (2):
netdev-offload: Allow offloading to netdev without ifindex.
netdev-offload: Disallow offloading to unrelated tunneling vports.
Sriharsha Basavapatna (1):
dpif-netdev: Provide orig_in_port in metadata for tunneled packets
Documentation/howto/dpdk.rst | 1 +
NEWS | 2 +
lib/dpif-netdev.c | 97 +++--
lib/netdev-dpdk.c | 118 +
lib/netdev-dpdk.h | 106 -
lib/netdev-offload-dpdk.c | 782 ++
lib/netdev-offload-provider.h | 5 +
lib/netdev-offload-tc.c | 8 +
lib/netdev-offload.c | 47 +-
lib/netdev-offload.h | 10 +
lib/packets.h | 8 +-
11 files changed, 1052 insertions(+), 132 deletions(-)
--
Ivan M
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] pinctrl: Don't send gARPs for localports
On Thu, Mar 25, 2021 at 8:58 AM Dumitru Ceara wrote: > On 3/24/21 6:23 PM, Daniel Alvarez Sanchez wrote: > > Ports of type 'localport' are present on every hypervisor and > > ovn-controller is sending gARPs for them which makes upstream > > switches to see its MAC address flapping. > > > > In order to avoid this behavior, the current patch is skipping > > localports when sending gARP/RARP packets. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939470 > > > > Signed-off-by: Daniel Alvarez Sanchez > > 0-day robot was complaining because this misses: > > Co-authored-by: Dumitru Ceara > > But I guess that, if the patch is accepted, this can be added at commit > time. > Right, if you think it's best I can send a v3 replacing your 'Signed-off' by 'Co-authored'. thanks! daniel > > Regards, > Dumitru > > > Signed-off-by: Dumitru Ceara > > --- > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V4 00/14] Netdev vxlan-decap offload
Hello, Note that MLX5 PMD has a bug that the tnl_pop private actions must be first. In OVS it is not. Fixing this issue is scheduled for 21.05 (and stable 20.11.2). Meanwhile, tests were done with a workaround for it. See https://github.com/elibritstein/dpdk-stable/pull/1 Also, any other comments on this series? Thanks, Eli On 3/17/2021 8:35 AM, Eli Britstein wrote: VXLAN decap in OVS-DPDK configuration consists of two flows: F1: in_port(ens1f0),eth(),ipv4(),udp(), actions:tnl_pop(vxlan_sys_4789) F2: tunnel(),in_port(vxlan_sys_4789),eth(),ipv4(), actions:ens1f0_0 F1 is a classification flow. It has outer headers matches and it classifies the packet as a VXLAN packet, and using tnl_pop action the packet continues processing in F2. F2 is a flow that has matches on tunnel metadata as well as on the inner packet headers (as any other flow). In order to fully offload VXLAN decap path, both F1 and F2 should be offloaded. As there are more than one flow in HW, it is possible that F1 is done by HW but F2 is not. Packet is received by SW, and should be processed starting from F2 as F1 was already done by HW. Rte_flows are applicable only on physical port IDs. Keeping the original physical in port on which the packet is received on enables applying vport flows (e.g. F2) on that physical port. This patch-set makes use of [1] introduced in DPDK 20.11, that adds API for tunnel offloads. v2-v1: - Tracking original in_port, and applying vport on that physical port instead of all PFs. v3-v2: - Traversing ports using a new API instead of flow_dump. - Refactor packet state recover logic, with bug fix for error pop_header. - One ref count for netdev in non-tunnel case. - Rename variables, comments, rebase. v4-v3: - Extract orig_in_port from physdev for flow modify. - Miss handling fixes. Travis: v1: https://travis-ci.org/github/elibritstein/OVS/builds/756418552 v2: https://travis-ci.org/github/elibritstein/OVS/builds/758382963 v3: https://travis-ci.org/github/elibritstein/OVS/builds/761089087 v4: https://travis-ci.org/github/elibritstein/OVS/builds/763146966 GitHub Actions: v1: https://github.com/elibritstein/OVS/actions/runs/515334647 v2: https://github.com/elibritstein/OVS/actions/runs/554986007 v3: https://github.com/elibritstein/OVS/actions/runs/613226225 v4: https://github.com/elibritstein/OVS/actions/runs/658415274 [1] https://mails.dpdk.org/archives/dev/2020-October/187314.html Eli Britstein (11): netdev-offload: Add HW miss packet state recover API netdev-dpdk: Introduce DPDK tunnel APIs netdev-offload: Introduce an API to traverse ports netdev-dpdk: Add flow_api support for netdev vxlan vports netdev-offload-dpdk: Implement HW miss packet recover for vport dpif-netdev: Add HW miss packet state recover logic netdev-offload-dpdk: Change log rate limits netdev-offload-dpdk: Support tunnel pop action netdev-offload-dpdk: Refactor offload rule creation netdev-offload-dpdk: Support vports flows offload netdev-dpdk-offload: Add vxlan pattern matching function Ilya Maximets (2): netdev-offload: Allow offloading to netdev without ifindex. netdev-offload: Disallow offloading to unrelated tunneling vports. Sriharsha Basavapatna (1): dpif-netdev: Provide orig_in_port in metadata for tunneled packets Documentation/howto/dpdk.rst | 1 + NEWS | 2 + lib/dpif-netdev.c | 97 +++-- lib/netdev-dpdk.c | 118 + lib/netdev-dpdk.h | 106 - lib/netdev-offload-dpdk.c | 782 ++ lib/netdev-offload-provider.h | 5 + lib/netdev-offload-tc.c | 8 + lib/netdev-offload.c | 47 +- lib/netdev-offload.h | 10 + lib/packets.h | 8 +- 11 files changed, 1052 insertions(+), 132 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] pinctrl: Don't send gARPs for localports
On 3/24/21 6:23 PM, Daniel Alvarez Sanchez wrote: > Ports of type 'localport' are present on every hypervisor and > ovn-controller is sending gARPs for them which makes upstream > switches to see its MAC address flapping. > > In order to avoid this behavior, the current patch is skipping > localports when sending gARP/RARP packets. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939470 > > Signed-off-by: Daniel Alvarez Sanchez 0-day robot was complaining because this misses: Co-authored-by: Dumitru Ceara But I guess that, if the patch is accepted, this can be added at commit time. Regards, Dumitru > Signed-off-by: Dumitru Ceara > --- ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 ovn 0/4] northd: build_lrouter_nat_defrag_and_lb refactor
On Mon, Mar 8, 2021 at 7:30 PM Lorenzo Bianconi wrote: > > Changes since v1: > - fix compilation errors with -Werror and clang compiler > (just repost patches from 5/8 to 8/8) Thanks Lorenzo. I applied these patches to the main branch. Numan > > Lorenzo Bianconi (4): > northd: introduce build_lrouter_out_undnat_flow routine > northd: introduce build_lrouter_out_snat_flow routine > northd: introduce build_lrouter_ingress_flow routine > northd: introduce lrouter_check_nat_entry routine > > northd/ovn-northd.c | 545 +++- > 1 file changed, 291 insertions(+), 254 deletions(-) > > -- > 2.29.2 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3] ovn-controller: Add 'local_ip' option to tunnel ports for IPsec case
On 24/03/2021 11:12, Numan Siddique wrote:
> On Tue, Mar 16, 2021 at 2:32 AM Mark Michelson wrote:
>>
>> LGMT
>>
>> Acked-by: Mark Michelson
>
> Thank you Mark G (and Mark M for the reviews).
>
> I applied this patch to master.
>
> Numan
>
Thank everyone.
>>
>> On 2/16/21 6:55 AM, Mark Gray wrote:
>>> If a chassis has multiple interfaces, 'ovn-encap-ip' can be used
>>> to specify the IP address of the interface that is used for tunnel
>>> traffic. OVN uses that IP address to configure the 'remote_ip' of
>>> a tunnel port. OVS tunnel ports also accept 'options:local_ip', which,
>>> according to the OVS documentation specifies "the tunnel destination
>>> IP that received packets must match. Default is to match all addresses".
>>> OVN does not set 'local_ip'.
>>>
>>> 'ovs-monitor-ipsec' is an OVS daemon that is used to configure and IPsec
>>> IKE daemon on the host. In order to correctly specify an IPsec
>>> connection, it requires the source and destination IP address of
>>> that connection. In the OVN case, as 'local_ip' is not specified, it
>>> is unable to infer the IP address of both sides of a tunnel and, therefore,
>>> cannot setup an IPsec connection.
>>>
>>> This patch configures 'local_ip' on tunnel ports when IPsec has
>>> been enabled. This allows for OVS/OVN IPsec to work when 'ovn-encap-ip'
>>> is not specified as the chassis default gateway interface.
>>>
>>> This patch also adds some unit tests. The OVS daemon 'ovs-monitor-ipsec'
>>> requires a number of options to be configured on OVS tunnel ports in order
>>> to function correctly. These unit tests ensure that these options are
>>> configured correctly when IPsec has been enabled through the northbound
>>> database.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1924041
>>> Signed-off-by: Mark Gray
>>> ---
>>>
>>> v2: Updated topic filter to "PATCH ovn"
>>> v3: Rebased due to 0-day bot warning
>>>
>>> controller/chassis.c | 5 +++
>>> controller/encaps.c | 26 ++-
>>> tests/automake.mk| 3 +-
>>> tests/ovn-ipsec.at | 104 +++
>>> tests/testsuite.at | 1 +
>>> 5 files changed, 137 insertions(+), 2 deletions(-)
>>> create mode 100644 tests/ovn-ipsec.at
>>>
>>> diff --git a/controller/chassis.c b/controller/chassis.c
>>> index 310132d09d2e..9b0a36cf076f 100644
>>> --- a/controller/chassis.c
>>> +++ b/controller/chassis.c
>>> @@ -279,6 +279,11 @@ chassis_parse_ovs_config(const struct
>>> ovsrec_open_vswitch_table *ovs_table,
>>> return false;
>>> }
>>>
>>> +/* 'ovn-encap-ip' can accept a comma-delimited list of IP addresses
>>> instead
>>> + * of a single IP address. Although this is undocumented, it can be
>>> used
>>> + * to enable certain hardware-offloaded use cases in which a host has
>>> + * multiple NICs and is assigning SR-IOV VFs to a guest (as logical
>>> ports).
>>> + */
>>> if (!chassis_parse_ovs_encap_ip(encap_ips, _cfg->encap_ip_set)) {
>>> sset_destroy(_cfg->encap_type_set);
>>> return false;
>>> diff --git a/controller/encaps.c b/controller/encaps.c
>>> index 7eac4bb064ac..fc93bf1eeb87 100644
>>> --- a/controller/encaps.c
>>> +++ b/controller/encaps.c
>>> @@ -59,6 +59,7 @@ struct tunnel_ctx {
>>>
>>> struct ovsdb_idl_txn *ovs_txn;
>>> const struct ovsrec_bridge *br_int;
>>> +const struct sbrec_chassis *this_chassis;
>>> };
>>>
>>> struct chassis_node {
>>> @@ -176,6 +177,28 @@ tunnel_add(struct tunnel_ctx *tc, const struct
>>> sbrec_sb_global *sbg,
>>>
>>> /* Add auth info if ipsec is enabled. */
>>> if (sbg->ipsec) {
>>> +const struct sbrec_chassis *this_chassis = tc->this_chassis;
>>> +const char *local_ip = NULL;
>>> +
>>> +/* Determine 'ovn-encap-ip' of the local chassis as this will be
>>> the
>>> + * tunnel port's 'local_ip'. We do not support the case in which
>>> + * 'ovn-encap-ip' holds multiple comma-delimited IP addresses.
>>> + */
>>> +for (int i = 0; i < this_chassis->n_encaps; i++) {
>>> +if (local_ip && strcmp(local_ip, this_chassis->encaps[i]->ip))
>>> {
>>> +VLOG_ERR("ovn-encap-ip has been configured as a list. This
>>> "
>>> + "is unsupported for IPsec.");
>>> +/* No need to loop further as we know this condition has
>>> been
>>> + * hit */
>>> +break;
>>> +} else {
>>> +local_ip = this_chassis->encaps[i]->ip;
>>> +}
>>> +}
>>> +
>>> +if (local_ip) {
>>> +smap_add(, "local_ip", local_ip);
>>> +}
>>> smap_add(, "remote_name", new_chassis_id);
>>> }
>>>
>>> @@ -310,7 +333,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
>>> struct tunnel_ctx tc = {
>>> .chassis = SHASH_INITIALIZER(),
>>> .port_names = SSET_INITIALIZER(_names),
>>> -
Re: [ovs-dev] ovn-northd-ddlog scale issues
Hi Ben,
With your memory fixes applied, I still see many test failures due to
memory leaks
when configured as:
./configure --enable-Werror --enable-sparse CFLAGS="-g -fsanitize=address"
You can find more details about the memory leaks here -
https://gist.github.com/numansiddique/e701777da977a89e24b49f159bb30d5d
Thanks
Numan
On Wed, Mar 24, 2021 at 9:19 PM Ben Pfaff wrote:
>
> Thanks a lot for the report! I need a new test case so I'll give this
> one a shot when I can.
>
> On Wed, Mar 24, 2021 at 04:03:07PM +0100, Dumitru Ceara wrote:
> > Hi Ben,
> >
> > We discussed a bit about this during one of the recent IRC OVN meetings,
> > but I didn't get around to properly reporting this until now.
> >
> > I've tried running ovn-northd-ddlog against some large OVN NB/DB
> > databases extracted from one of our scale testing runs:
> >
> > http://people.redhat.com/~dceara/ovn-northd-ddlog-tests/20210324/existing-nb-sb/
> >
> > It seems that ovn-northd-ddlog gets stuck in a busy loop and uses a lot
> > of memory:
> >
> > 775734 root 10 -10 81.6g 80.8g 22396 S 99.7 64.2 3:50.79
> > ovn-northd-ddlog
> >
> > ovn-northd-ddlog is stuck in an (infinite?) loop in
> > ddlog_transaction_commit_dump_changes():
> >
> > (gdb) bt
> > #0 0x7f2762167e92 in pthread_cond_wait@@GLIBC_2.3.2 () from
> > /lib64/libpthread.so.0
> > #1 0x00891eb3 in std::thread::park ()
> > #2 0x00530963 in crossbeam_channel::context::Context::wait_until ()
> > #3 0x00530aa1 in
> > crossbeam_channel::context::Context::with::{{closure}} ()
> > #4 0x00531f5c in
> > crossbeam_channel::flavors::list::Channel::recv ()
> > #5 0x0075b35d in crossbeam_channel::channel::Receiver::recv ()
> > #6 0x00512156 in
> > differential_datalog::program::RunningProgram::flush ()
> > #7 0x0050de67 in
> > differential_datalog::program::RunningProgram::transaction_commit ()
> > #8 0x0093f17d in > differential_datalog::ddlog::DDlog>::transaction_commit_dump_changes ()
> > #9 0x0040cc40 in ddlog_transaction_commit_dump_changes ()
> > #10 0x0040a758 in ddlog_commit (ddlog=) at
> > northd/ovn-northd-ddlog.c:435
> > #11 northd_parse_update (update=0x3dc0598, ctx=0x3d71880) at
> > northd/ovn-northd-ddlog.c:435
> > #12 northd_run (ctx=0x3d71880) at northd/ovn-northd-ddlog.c:512
> > #13 0x00408edd in main (argc=, argv=)
> > at northd/ovn-northd-ddlog.c:1203
> >
> > For comparison, running the C version of ovn-northd I get:
> >
> > 2021-03-24T14:48:06.556Z|00033|timeval|WARN|Unreasonably long 11290ms poll
> > interval (10916ms user, 334ms system)
> > 2021-03-24T14:48:14.678Z|00050|timeval|WARN|Unreasonably long 8122ms poll
> > interval (8046ms user, 48ms system)
> >
> > But after the northd iteration ends, memory usage is OK:
> >
> > 777567 root 10 -10 1657308 1.6g 3496 S 0.0 1.2 0:49.08
> > ovn-northd
> >
> > The behavior above is consistent both with current OVN master and also
> > when cherry-picking the ddlog-related patches that are pending in
> > patchwork:
> > http://patchwork.ozlabs.org/project/ovn/list/?series=233075
> > http://patchwork.ozlabs.org/project/ovn/list/?series=232480
> > http://patchwork.ozlabs.org/project/ovn/list/?series=233079
> > http://patchwork.ozlabs.org/project/ovn/list/?series=233080
> >
> > I didn't try out the changes from the following series though as I
> > understand they need a v2:
> > http://patchwork.ozlabs.org/project/ovn/list/?series=232040
> >
> > Regards,
> > Dumitru
> >
> ___
> 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
