Re: [ovs-dev] [PATCH v3] ovsdb: Don't iterate over rows on empty mutation.
On 3/6/24 22:40, Mike Pattrick wrote: > Previously when an empty mutation was used to count the number of rows > in a table, OVSDB would iterate over all rows twice. First to perform an > RBAC check, and then to perform the no-operation. > > This change adds a short circuit to mutate operations with no conditions > and an empty mutation set, returning immediately. One notable change in > functionality is not performing the RBAC check in this condition, as no > mutation actually takes place. > > Reported-by: Terry Wilson > Reported-at: https://issues.redhat.com/browse/FDP-359 > Signed-off-by: Mike Pattrick > --- > v2: Added additional non-rbac tests, and support for conditional > counting without the rbac check > v3: Changed a struct to a size_t. > --- > ovsdb/execution.c| 23 +- > ovsdb/mutation.h | 6 + > tests/ovsdb-execution.at | 51 > tests/ovsdb-rbac.at | 23 ++ > 4 files changed, 102 insertions(+), 1 deletion(-) Thanks, Mike! Applied. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-dpdk: Dump packets that fail Tx preparation.
On 3/8/24 12:21, Kevin Traynor wrote: > On 07/03/2024 19:39, Ilya Maximets wrote: >> It's hard to debug situations where driver rejects packets for some >> reason. Dumping out the mbuf should help with that. >> >> Sample output looks like this: >> >> |netdev_dpdk(pmd-c03/id:8)|DBG|ovs-p1: First invalid packet: >> dump mbuf at 0x1180bce140, iova=0x2cb7ce400, buf_len=2176 >> pkt_len=64, ol_flags=0x2, nb_segs=1, port=65535, ptype=0 >> segment at 0x1180bce140, data=0x1180bce580, len=90, off=384, refcnt=1 >> Dump data at [0x1180bce580], len=64 >> : 33 33 00 00 00 16 AA 27 91 F9 4D 96 86 DD 60 00 | >> 33.'..M...`. >> 0010: 00 00 00 24 00 01 00 00 00 00 00 00 00 00 00 00 | >> ...$ >> 0020: 00 00 00 00 00 00 FF 02 00 00 00 00 00 00 00 00 | >> >> 0030: 00 00 00 00 00 16 3A 00 05 02 00 00 01 00 8F 00 | >> ..:. >> >> Signed-off-by: Ilya Maximets > > Acked-by: Kevin Traynor > >> --- >> >> Version 2: >> * Only dumping the first invalid packet as we don't actually >> know if the other ones correct or not. [Kevin] >> >> Also, we may want to have this backported to 3.3 and maybe 3.2, >> as I suspect those will be branches where will will actually have >> to debug offloading rejection issues. We already have a few. >> For example: https://github.com/openvswitch/ovs-issues/issues/321 >> for which I actually prepared this patch in the first place. >> >> This is a debug-only change that should not have any impact >> on end users. >> >> Thoughts? >> > > I think it's reasonable to backport it. It is non-intrusive for the > normal case and will be helpful for debug. Thanks, Kevin and Eelco! Applied and backported down to 3.2. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] bfd: Improve state change log message.
On 3/8/24 11:22, Eelco Chaudron wrote: > > > On 5 Mar 2024, at 21:37, Timothy Redaelli wrote: > >> A log message like this one: >> >> 2024-01-09T06:45:17.201Z|00071|bfd(handler2)|INFO|ovn-0af536-0: BFD state >> change: down->up "Neighbor Signaled Session Down"->"Neighbor Signaled Session >> Down". >> >> can be hard to read since '->' usually represents a status change, but >> in this case the diagnostic code stays constant. Update the log message to >> avoid such ambiguity. The log message for the above event become: >> >> 2024-01-09T06:45:16.211Z|00026|bfd(handler3)|INFO|ovn-0af536-0: BFD state >> change: (bfd.SessionState: down, bfd.LocalDiag: "Neighbor Signaled Session >> Down") -> (bfd.SessionState: up, bfd.LocalDiag: "Neighbor Signaled Session >> Down") >> >> Reported-by: Alex Stupnikov >> Reported-at: https://bugzilla.redhat.com/2258496 >> Signed-off-by: Timothy Redaelli > > Thanks for following up, the change looks good to me! > > Acked-by: Eelco Chaudron > Thanks, Timothy and Eelco! Applied and backported down to 2.17 as previously discussed. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Fix "SSL db: implementation" test with openssl > 3.2.0.
On 3/8/24 11:37, Eelco Chaudron wrote: > > > On 5 Mar 2024, at 19:55, Timothy Redaelli wrote: > >> In OpenSSL 3.2.0 (81b741f) all the "alert" error messages was updated to > > was -> were > >> replace "sslv3" with "ssl/tls". >> >> This commit updates the "SSL db: implementation" test to support both the >> pre-openssl 3.2.0 error message: "sslv3 alert certificate unknown" and the >> post-openssl 3.2.0 error message: "ssl/tls alert certificate unknown". >> >> Signed-off-by: Timothy Redaelli >> --- > > Other than the small grammer correction this patch look good to me. So > assuming the change is applied on commit; > > Acked-by: Eelco Chaudron Thanks, Timothy and Eelco! I fixed the issues and applied the patch to all branches down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-2.17] conntrack: Fix flush not flushing all elements.
On Wed, Mar 06, 2024 at 07:06:39PM +, Simon Horman wrote:
> From: Xavier Simonart
>
> On netdev datapath, when a ct element was cleaned, the cmap
> could be shrinked, potentially causing some elements to be skipped
> in the flush iteration.
>
> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.")
> Signed-off-by: Xavier Simonart
> Acked-by: Mike Pattrick
> Signed-off-by: Simon Horman
Applied to branch-2.17:
- conntrack: Fix flush not flushing all elements.
https://github.com/openvswitch/ovs/commit/ee8485227f3e
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.0] conntrack: Fix flush not flushing all elements.
On Wed, Mar 06, 2024 at 07:00:50PM +, Simon Horman wrote:
> From: Xavier Simonart
>
> On netdev datapath, when a ct element was cleaned, the cmap
> could be shrinked, potentially causing some elements to be skipped
> in the flush iteration.
>
> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.")
> Signed-off-by: Xavier Simonart
> Acked-by: Mike Pattrick
> Signed-off-by: Simon Horman
Applied to branch-3.0:
- conntrack: Fix flush not flushing all elements.
https://github.com/openvswitch/ovs/commit/70af47501470
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.1] conntrack: Fix flush not flushing all elements.
On Wed, Mar 06, 2024 at 06:21:36PM +, 'Simon Horman' wrote:
> From: Xavier Simonart
>
> On netdev datapath, when a ct element was cleaned, the cmap
> could be shrinked, potentially causing some elements to be skipped
> in the flush iteration.
>
> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.")
> Signed-off-by: Xavier Simonart
> Acked-by: Mike Pattrick
> Signed-off-by: Simon Horman
Applied to branch-3.1:
- conntrack: Fix flush not flushing all elements.
https://github.com/openvswitch/ovs/commit/c56f0b4c4986
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.2 0/2] conntrack: Fix flush not flushing all elements.
On Wed, Mar 06, 2024 at 06:11:08PM +, 'Simon Horman' wrote: > Backport to branch-3.2 of: > > - conntrack: Fix flush not flushing all elements. > https://github.com/openvswitch/ovs/commit/6c082a8310d5 > > Including dependency which is not present in branch-3.2. Applied to branch-3.2: - conntrack: Fix flush not flushing all elements. https://github.com/openvswitch/ovs/commit/41a8b58aa92b - conntrack: Remove nat_conn introducing key directionality. https://github.com/openvswitch/ovs/commit/ad92b0da17f4 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/3] northd: Add nexhop id in ct_label.label.
Hi Lorenzo,
Just a couple of small comments below.
On 3/7/24 08:19, Lorenzo Bianconi wrote:
Introduce the nexthop identifier in the ct_label.label field for
ecmp-symmetric replies connections. This field will be used by
ovn-controller to track ct entries and to flush them if requested by the
CMS (e.g. removing the related static routes).
Signed-off-by: Lorenzo Bianconi
---
northd/northd.c | 11 +--
tests/ovn.at| 4 ++--
tests/system-ovn.at | 48 -
3 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 3770f9f94..e85339704 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10600,15 +10600,22 @@ add_ecmp_symmetric_reply_flows(struct lflow_table
*lflows,
* ds_put_cstr() call. The previous contents are needed.
*/
ds_put_cstr(, " && !ct.rpl && (ct.new || ct.est)");
+struct ds nexthop_label = DS_EMPTY_INITIALIZER;
+int id = smap_get_int(_route->options, "nexthop-id", -1);
+if (id > 0) {
+ds_put_format(_label, "ct_label.label = %d;", id);
+}
As mentioned in my review of patch 1, this should use the SB
ECMP_nexthop nexthop-id instead of the NB static route nexthop-id.
ds_put_format(,
"ct_commit { ct_label.ecmp_reply_eth = eth.src; "
-" %s = %" PRId64 ";}; "
+" %s = %" PRId64 "; %s }; "
"next;",
-ct_ecmp_reply_port_match, out_port->sb->tunnel_key);
+ct_ecmp_reply_port_match, out_port->sb->tunnel_key,
+ds_cstr(_label));
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100,
ds_cstr(), ds_cstr(),
_route->header_,
lflow_ref);
+ds_destroy(_label);
/* Bypass ECMP selection if we already have ct_label information
* for where to route the packet.
diff --git a/tests/ovn.at b/tests/ovn.at
index d26c95054..d5ee7a1f3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -29181,7 +29181,7 @@ AT_CHECK([
for hv in 1 2; do
grep table=17 hv${hv}flows | \
grep "priority=100" | \
-grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]]))"
+grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]],load:0x[[0-9]]->NXM_NX_CT_LABEL\\[[96..127\\]]))"
grep table=25 hv${hv}flows | \
grep "priority=200" | \
@@ -29306,7 +29306,7 @@ AT_CHECK([
for hv in 1 2; do
grep table=17 hv${hv}flows | \
grep "priority=100" | \
-grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]]))"
+grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]],load:0x[[0-9]]->NXM_NX_CT_LABEL\\[[96..127\\]]))"
grep table=25 hv${hv}flows | \
grep "priority=200" | \
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 2411b0267..146bf70e2 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6121,19 +6121,20 @@ NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2
10.0.0.2 | FORMAT_PING], \
# and just ensure that the known ethernet address is present.
AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \
sed -e 's/zone=[[0-9]]*/zone=/' |
-sed -e 's/mark=[[0-9]]*/mark=/'], [0], [dnl
-icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=,type=0,code=0),zone=,mark=,labels=0x4010204
-tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=,dport=),reply=(src=10.0.0.2,dst=172.16.0.1,sport=,dport=),zone=,mark=,labels=0x4010204,protoinfo=(state=)
+sed -e 's/mark=[[0-9]]*/mark=/' |
+sed -e 's/labels=0x[[0-9]]/labels=0x?/'], [0], [dnl
+icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=,type=0,code=0),zone=,mark=,labels=0x?04010204
+tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=,dport=),reply=(src=10.0.0.2,dst=172.16.0.1,sport=,dport=),zone=,mark=,labels=0x?04010204,protoinfo=(state=)
])
# Ensure datapaths show conntrack states as expected
# Like with conntrack entries, we shouldn't try to predict
# port binding tunnel keys. So omit them from expected labels.
ovs-appctl dpctl/dump-flows | grep
'ct_state(+new-est-rpl+trk).*ct(.*label=0x4010204/.*)'
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep
'ct_state(+new-est-rpl+trk).*ct(.*label=0x4010204/.*)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk)' |
grep '4010204' -c],
Re: [ovs-dev] [PATCH ovn 2/3] northd: Add nexhop id in ct_label.label.
Hi Lorenzo,
Just a couple of small comments below.
On 3/7/24 08:19, Lorenzo Bianconi wrote:
Introduce the nexthop identifier in the ct_label.label field for
ecmp-symmetric replies connections. This field will be used by
ovn-controller to track ct entries and to flush them if requested by the
CMS (e.g. removing the related static routes).
Signed-off-by: Lorenzo Bianconi
---
northd/northd.c | 11 +--
tests/ovn.at| 4 ++--
tests/system-ovn.at | 48 -
3 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 3770f9f94..e85339704 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10600,15 +10600,22 @@ add_ecmp_symmetric_reply_flows(struct lflow_table
*lflows,
* ds_put_cstr() call. The previous contents are needed.
*/
ds_put_cstr(, " && !ct.rpl && (ct.new || ct.est)");
+struct ds nexthop_label = DS_EMPTY_INITIALIZER;
+int id = smap_get_int(_route->options, "nexthop-id", -1);
+if (id > 0) {
+ds_put_format(_label, "ct_label.label = %d;", id);
+}
As mentioned in my review of patch 1, this should use the SB
ECMP_nexthop nexthop-id instead of the NB static route nexthop-id.
ds_put_format(,
"ct_commit { ct_label.ecmp_reply_eth = eth.src; "
-" %s = %" PRId64 ";}; "
+" %s = %" PRId64 "; %s }; "
"next;",
-ct_ecmp_reply_port_match, out_port->sb->tunnel_key);
+ct_ecmp_reply_port_match, out_port->sb->tunnel_key,
+ds_cstr(_label));
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100,
ds_cstr(), ds_cstr(),
_route->header_,
lflow_ref);
+ds_destroy(_label);
/* Bypass ECMP selection if we already have ct_label information
* for where to route the packet.
diff --git a/tests/ovn.at b/tests/ovn.at
index d26c95054..d5ee7a1f3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -29181,7 +29181,7 @@ AT_CHECK([
for hv in 1 2; do
grep table=17 hv${hv}flows | \
grep "priority=100" | \
-grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]]))"
+grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]],load:0x[[0-9]]->NXM_NX_CT_LABEL\\[[96..127\\]]))"
grep table=25 hv${hv}flows | \
grep "priority=200" | \
@@ -29306,7 +29306,7 @@ AT_CHECK([
for hv in 1 2; do
grep table=17 hv${hv}flows | \
grep "priority=100" | \
-grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]]))"
+grep -c
"ct(commit,zone=NXM_NX_REG11\\[[0..15\\]],.*exec(move:NXM_OF_ETH_SRC\\[[\\]]->NXM_NX_CT_LABEL\\[[32..79\\]],load:0x[[0-9]]->NXM_NX_CT_MARK\\[[16..31\\]],load:0x[[0-9]]->NXM_NX_CT_LABEL\\[[96..127\\]]))"
grep table=25 hv${hv}flows | \
grep "priority=200" | \
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 2411b0267..146bf70e2 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6121,19 +6121,20 @@ NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2
10.0.0.2 | FORMAT_PING], \
# and just ensure that the known ethernet address is present.
AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \
sed -e 's/zone=[[0-9]]*/zone=/' |
-sed -e 's/mark=[[0-9]]*/mark=/'], [0], [dnl
-icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=,type=0,code=0),zone=,mark=,labels=0x4010204
-tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=,dport=),reply=(src=10.0.0.2,dst=172.16.0.1,sport=,dport=),zone=,mark=,labels=0x4010204,protoinfo=(state=)
+sed -e 's/mark=[[0-9]]*/mark=/' |
+sed -e 's/labels=0x[[0-9]]/labels=0x?/'], [0], [dnl
+icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=,type=0,code=0),zone=,mark=,labels=0x?04010204
+tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=,dport=),reply=(src=10.0.0.2,dst=172.16.0.1,sport=,dport=),zone=,mark=,labels=0x?04010204,protoinfo=(state=)
])
# Ensure datapaths show conntrack states as expected
# Like with conntrack entries, we shouldn't try to predict
# port binding tunnel keys. So omit them from expected labels.
ovs-appctl dpctl/dump-flows | grep
'ct_state(+new-est-rpl+trk).*ct(.*label=0x4010204/.*)'
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep
'ct_state(+new-est-rpl+trk).*ct(.*label=0x4010204/.*)' -c], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk)' |
grep '4010204' -c],
Re: [ovs-dev] [PATCH ovn 1/3] northd: Introduce ECMP_Nexthop table in SB db.
Hi Lorenzo,
I have some comments below.
On 3/7/24 08:19, Lorenzo Bianconi wrote:
Introduce ECMP_Nexthop table in the SB db in order to track active
ecmp-symmetric-reply connections and flush stale ones.
Signed-off-by: Lorenzo Bianconi
---
northd/en-northd.c | 4 ++
northd/inc-proc-northd.c | 8 +++-
northd/northd.c | 98
northd/northd.h | 3 ++
ovn-sb.ovsschema | 15 +-
ovn-sb.xml | 19
tests/ovn-northd.at | 4 ++
7 files changed, 147 insertions(+), 4 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 4479b4aff..8d2ab481f 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -76,6 +76,8 @@ northd_get_input_data(struct engine_node *node,
EN_OVSDB_GET(engine_get_input("NB_chassis_template_var", node));
input_data->nbrec_mirror_table =
EN_OVSDB_GET(engine_get_input("NB_mirror", node));
+input_data->nbrec_static_route_table =
+EN_OVSDB_GET(engine_get_input("NB_logical_router_static_route", node));
input_data->sbrec_datapath_binding_table =
EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
@@ -101,6 +103,8 @@ northd_get_input_data(struct engine_node *node,
EN_OVSDB_GET(engine_get_input("SB_chassis_template_var", node));
input_data->sbrec_mirror_table =
EN_OVSDB_GET(engine_get_input("SB_mirror", node));
+input_data->sbrec_ecmp_nexthop_table =
+EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node));
struct ed_type_lb_data *lb_data =
engine_get_input_data("lb_data", node);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index e1073812c..1c58da0bf 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -61,7 +61,8 @@ static unixctl_cb_func chassis_features_list;
NB_NODE(meter, "meter") \
NB_NODE(bfd, "bfd") \
NB_NODE(static_mac_binding, "static_mac_binding") \
-NB_NODE(chassis_template_var, "chassis_template_var")
+NB_NODE(chassis_template_var, "chassis_template_var") \
+NB_NODE(logical_router_static_route, "logical_router_static_route")
enum nb_engine_node {
#define NB_NODE(NAME, NAME_STR) NB_##NAME,
@@ -101,7 +102,8 @@ static unixctl_cb_func chassis_features_list;
SB_NODE(fdb, "fdb") \
SB_NODE(static_mac_binding, "static_mac_binding") \
SB_NODE(chassis_template_var, "chassis_template_var") \
-SB_NODE(logical_dp_group, "logical_dp_group")
+SB_NODE(logical_dp_group, "logical_dp_group") \
+SB_NODE(ecmp_nexthop, "ecmp_nexthop")
enum sb_engine_node {
#define SB_NODE(NAME, NAME_STR) SB_##NAME,
@@ -180,6 +182,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _nb_mirror, NULL);
engine_add_input(_northd, _nb_static_mac_binding, NULL);
engine_add_input(_northd, _nb_chassis_template_var, NULL);
+engine_add_input(_northd, _nb_logical_router_static_route, NULL);
engine_add_input(_northd, _sb_chassis, NULL);
engine_add_input(_northd, _sb_mirror, NULL);
@@ -192,6 +195,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _sb_fdb, NULL);
engine_add_input(_northd, _sb_static_mac_binding, NULL);
engine_add_input(_northd, _sb_chassis_template_var, NULL);
+engine_add_input(_northd, _sb_ecmp_nexthop, NULL);
engine_add_input(_northd, _global_config,
northd_global_config_handler);
diff --git a/northd/northd.c b/northd/northd.c
index 4b39137e7..3770f9f94 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -16654,6 +16654,101 @@ sync_mirrors(struct ovsdb_idl_txn *ovnsb_txn,
shash_destroy(_mirrors);
}
+struct sb_ecmp_nexthop_entry {
+struct hmap_node hmap_node;
+const struct sbrec_ecmp_nexthop *sb_ecmp_nexthop;
+};
+
+static struct sb_ecmp_nexthop_entry *
+sb_ecmp_nexthop_lookup(const struct hmap *map, const char *nexthop)
+{
+uint32_t hash = hash_string(nexthop, 0);
+struct sb_ecmp_nexthop_entry *enh_e;
+
+HMAP_FOR_EACH_WITH_HASH (enh_e, hmap_node, hash, map) {
+if (!strcmp(enh_e->sb_ecmp_nexthop->nexthop, nexthop)) {
+return enh_e;
+}
+}
+return NULL;
+}
+
+#define NEXTHOP_IDS_LEN65535
+static void
+sync_ecmp_symmetric_reply_nexthop(struct ovsdb_idl_txn *ovnsb_txn,
+const struct nbrec_logical_router_static_route_table *nbrec_sr_table,
+const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nextop_table)
+{
+unsigned long *nexthop_ids = bitmap_allocate(NEXTHOP_IDS_LEN);
+struct hmap sb_only = HMAP_INITIALIZER(_only);
+const struct sbrec_ecmp_nexthop *sb_ecmp_nexthop;
+struct sb_ecmp_nexthop_entry *enh_e;
+int id;
+
+SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sb_ecmp_nexthop,
+ sbrec_ecmp_nextop_table) {
+uint32_t hash =
Re: [ovs-dev] [PATCH ovn 0/3] Introduce ECMP_nexthop monitor in ovn-controller
Hi Lorenzo, I haven't had time to review patch 3 yet, but I have made some comments on patches 1 and 2 for now. On 3/7/24 08:19, Lorenzo Bianconi wrote: Reported-at: https://issues.redhat.com/browse/FDP-56 Lorenzo Bianconi (3): northd: Introduce ECMP_Nexthop table in SB db. northd: Add nexhop id in ct_label.label. ofctrl: Introduce ecmp_nexthop_monitor. controller/ofctrl.c | 102 ++ controller/ofctrl.h | 2 + controller/ovn-controller.c | 2 + northd/en-northd.c | 4 + northd/inc-proc-northd.c| 8 +- northd/northd.c | 109 ++- northd/northd.h | 3 + ovn-sb.ovsschema| 15 +- ovn-sb.xml | 19 +++ tests/ovn-northd.at | 4 + tests/ovn.at| 4 +- tests/system-ovn-kmod.at| 263 tests/system-ovn.at | 52 --- 13 files changed, 557 insertions(+), 30 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] utilities: Make database connection optional for ovn-detrace.
Thanks Ales, it looks good to me.
Acked-by: Mark Michelson
On 3/5/24 01:21, Ales Musil wrote:
The ovn-detrace required both connections (SB and NB) to provide some
output. However, it is a valid scenario to have only one database
available, usually the SB. With that in mind make the connection
optional and print warning into stderr that the output will not
contain information from DB that is not available.
Reported-at: https://issues.redhat.com/browse/FDP-68
Signed-off-by: Ales Musil
---
utilities/ovn_detrace.py.in | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/utilities/ovn_detrace.py.in b/utilities/ovn_detrace.py.in
index 364f11a71..12df6e52a 100755
--- a/utilities/ovn_detrace.py.in
+++ b/utilities/ovn_detrace.py.in
@@ -37,6 +37,9 @@ except Exception:
argv0 = sys.argv[0]
version = "@VERSION@"
+DB_CONNECTION_ERR = ('The connection to {0} DB is not available,'
+ ' {0} information will be missing from the detrace.')
+
def usage():
print("""\
@@ -77,6 +80,11 @@ def chassis_str(chassis):
ch = chassis[0]
return 'chassis-name "%s", chassis-str "%s"' % (ch.name, ch.hostname)
+
+class ConnectionException(Exception):
+pass
+
+
class OVSDB(object):
STREAM_TIMEOUT_MS = 1000
@@ -111,7 +119,7 @@ class OVSDB(object):
os.strerror(error)))
strm = None
if not strm:
-raise Exception('Unable to connect to %s' % self.remote)
+raise ConnectionException()
rpc = jsonrpc.Connection(strm)
req = jsonrpc.Message.create_request('get_schema', [schema_name])
@@ -167,6 +175,8 @@ class CookieHandlerByUUUID(CookieHandler):
super(CookieHandlerByUUUID, self).__init__(db, table, printer)
def get_records(self, cookie):
+if not self._db:
+return []
# Adjust cookie to include leading zeroes if needed.
cookie = cookie.zfill(8)
return self._db.find_rows_by_partial_uuid(self._table, cookie)
@@ -488,8 +498,19 @@ def main():
if ovs and not ovs_db:
ovs_db = 'unix:%s/db.sock' % ovs_rundir
-ovsdb_ovnsb = OVSDB(ovnsb_db, 'OVN_Southbound', leader_only=leader_only)
-ovsdb_ovnnb = OVSDB(ovnnb_db, 'OVN_Northbound', leader_only=leader_only)
+try:
+ovsdb_ovnsb = OVSDB(ovnsb_db, 'OVN_Southbound',
+leader_only=leader_only)
+except ConnectionException:
+print(DB_CONNECTION_ERR.format('SB'), file=sys.stderr)
+ovsdb_ovnsb = None
+
+try:
+ovsdb_ovnnb = OVSDB(ovnnb_db, 'OVN_Northbound',
+leader_only=leader_only)
+except ConnectionException:
+print(DB_CONNECTION_ERR.format('NB'), file=sys.stderr)
+ovsdb_ovnnb = None
printer = Printer()
cookie_handlers = get_cookie_handlers(ovsdb_ovnnb, ovsdb_ovnsb, printer)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH 0/4] net: openvswitch: Add sample multicasting.
On 3/7/24 22:29, Ilya Maximets wrote:
> On 3/7/24 21:59, Adrian Moreno wrote:
>>
>>
>> On 3/7/24 17:54, Ilya Maximets wrote:
>>> On 3/7/24 16:18, Adrian Moreno wrote:
** Background ** Currently, OVS supports several packet
sampling mechanisms (sFlow, per-bridge IPFIX, per-flow IPFIX).
These end up being translated into a userspace action that
needs to be handled by ovs-vswitchd's handler threads only to
be forwarded to some third party application that will somehow
process the sample and provide observability on the datapath.
The fact that sampled traffic share netlink sockets and
handler thread time with upcalls, apart from being a
performance bottleneck in the sample extraction itself, can
severely compromise the datapath, yielding this solution unfit
for highly loaded production systems.
Users are left with little options other than guessing what
sampling rate will be OK for their traffic pattern and system
load and dealing with the lost accuracy.
** Proposal ** In this RFC, I'd like to request feedback on an
attempt to fix this situation by adding a flag to the userspace
action to indicate the upcall should be sent to a netlink
multicast group instead of unicasted to ovs-vswitchd.
This would allow for other processes to read samples directly,
freeing the netlink sockets and handler threads to process
packet upcalls.
** Notes on tc-offloading ** I am aware of the efforts being
made to offload the sample action with the help of psample. I
did consider using psample to multicast the samples. However, I
found a limitation that I'd like to discuss: I would like to
support OVN-driven per-flow (IPFIX) sampling because it allows
OVN to insert two 32-bit values (obs_domain_id and
ovs_point_id) that can be used to enrich the sample with "high
level" controller metadata (see debug_drop_domain_id NBDB
option in ovn-nb(5)).
The existing fields in psample_metadata are not enough to
carry this information. Would it be possible to extend this
struct to make room for some extra "application-specific"
metadata?
** Alternatives ** An alternative approach that I'm
considering (apart from using psample as explained above) is to
use a brand-new action. This lead to a cleaner separation of
concerns with existing userspace action (used for slow paths
and OFP_CONTROLLER actions) and cleaner statistics. Also,
ovs-vswitchd could more easily make the layout of this new
userdata part of the public API, allowing third party sample
collectors to decode it.
I am currently exploring this alternative but wanted to send
the RFC to get some early feedback, guidance or ideas.
>>>
>>>
>>> Hi, Adrian. Thanks for the patches!
>>>
>>
>> Thanks for the quick feedback. Also adding Dumitru who I missed to
>> include in the original CC list.
>>
>>> Though I'm not sure if broadcasting is generally the best
>>> approach. These messages contain opaque information that is not
>>> actually parsable by any other entity than a process that
>>> created the action. And I don't think the structure of these
>>> opaque fields should become part of uAPI in neither kernel nor
>>> OVS in userspace.
>>>
>>
>> I understand this can be cumbersome, specially given the opaque
>> field is currently also used for some purely-internal OVS actions
>> (e.g: CONTROLLER).
>>
>> However, for features such as OVN-driven per-flow sampling, where
>> OVN-generated identifiers are placed in obs_domain_id and
>> obs_point_id, it would be _really_ useful if this opaque value
>> could be somehow decoded by external programs.
>>
>> Two ideas come to mind to try to alleviate the potential
>> maintainability issues: - As I suggested, using a new action maybe
>> makes things easier. By splitting the current "user_action_cookie"
>> in two, one for internal actions and one for "observability"
>> actions, we could expose the latter in the OVS userspace API
>> without having to expose the former. - Exposing functions in OVS
>> that decode the opaque value. Third party applications could link
>> against, say, libopenvswitch.so and use it to extract
>> obs_{domain,point}_ids.
>
> Linking with OVS libraries is practically the same as just exposing
> the internal structure, because once the external application is
> running it either must have the same library version as the process
> that installs the action, or it may not be able to parse the
> message.
>
> Any form of exposing to an external application will freeze the
> opaque arguments and effectively make them a form of uAPI.
>
> The separate action with a defined uAPI solves this problem by just
> creating a new uAPI, but I'm not sure why it is needed.
>
>>
>> What do you think?
>>
>>> The userspace() action
Re: [ovs-dev] [PATCH v2] netdev-dpdk: Dump packets that fail Tx preparation.
On 07/03/2024 19:39, Ilya Maximets wrote:
> It's hard to debug situations where driver rejects packets for some
> reason. Dumping out the mbuf should help with that.
>
> Sample output looks like this:
>
> |netdev_dpdk(pmd-c03/id:8)|DBG|ovs-p1: First invalid packet:
> dump mbuf at 0x1180bce140, iova=0x2cb7ce400, buf_len=2176
> pkt_len=64, ol_flags=0x2, nb_segs=1, port=65535, ptype=0
> segment at 0x1180bce140, data=0x1180bce580, len=90, off=384, refcnt=1
> Dump data at [0x1180bce580], len=64
> : 33 33 00 00 00 16 AA 27 91 F9 4D 96 86 DD 60 00 | 33.'..M...`.
> 0010: 00 00 00 24 00 01 00 00 00 00 00 00 00 00 00 00 | ...$
> 0020: 00 00 00 00 00 00 FF 02 00 00 00 00 00 00 00 00 |
> 0030: 00 00 00 00 00 16 3A 00 05 02 00 00 01 00 8F 00 | ..:.
>
> Signed-off-by: Ilya Maximets
Acked-by: Kevin Traynor
> ---
>
> Version 2:
> * Only dumping the first invalid packet as we don't actually
> know if the other ones correct or not. [Kevin]
>
> Also, we may want to have this backported to 3.3 and maybe 3.2,
> as I suspect those will be branches where will will actually have
> to debug offloading rejection issues. We already have a few.
> For example: https://github.com/openvswitch/ovs-issues/issues/321
> for which I actually prepared this patch in the first place.
>
> This is a debug-only change that should not have any impact
> on end users.
>
> Thoughts?
>
I think it's reasonable to backport it. It is non-intrusive for the
normal case and will be helpful for debug.
> lib/netdev-dpdk.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 45f61930d..9444c53b1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2664,6 +2664,35 @@ netdev_dpdk_prep_hwol_batch(struct netdev_dpdk *dev,
> struct rte_mbuf **pkts,
> return cnt;
> }
>
> +static void
> +netdev_dpdk_mbuf_dump(const char *prefix, const char *message,
> + const struct rte_mbuf *mbuf)
> +{
> +static struct vlog_rate_limit dump_rl = VLOG_RATE_LIMIT_INIT(5, 5);
> +char *response = NULL;
> +FILE *stream;
> +size_t size;
> +
> +if (VLOG_DROP_DBG(_rl)) {
> +return;
> +}
> +
> +stream = open_memstream(, );
> +if (!stream) {
> +VLOG_ERR("Unable to open memstream for mbuf dump: %s.",
> + ovs_strerror(errno));
> +return;
> +}
> +
> +rte_pktmbuf_dump(stream, mbuf, rte_pktmbuf_pkt_len(mbuf));
> +
> +fclose(stream);
> +
> +VLOG_DBG(prefix ? "%s: %s:\n%s" : "%s%s:\n%s",
> + prefix ? prefix : "", message, response);
> +free(response);
> +}
> +
> /* Tries to transmit 'pkts' to txq 'qid' of device 'dev'. Takes ownership of
> * 'pkts', even in case of failure.
> *
> @@ -2680,6 +2709,8 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int
> qid,
> VLOG_WARN_RL(, "%s: Output batch contains invalid packets. "
> "Only %u/%u are valid: %s", netdev_get_name(>up),
> nb_tx_prep, cnt, rte_strerror(rte_errno));
> +netdev_dpdk_mbuf_dump(netdev_get_name(>up),
> + "First invalid packet", pkts[nb_tx_prep]);
> }
>
> while (nb_tx != nb_tx_prep) {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Skip EDNS test if the scapy version doesn't support it.
On 3/8/24 09:26, Ales Musil wrote:
> On Thu, Mar 7, 2024 at 4:19 PM Dumitru Ceara wrote:
>
>> EDNS0ClientSubnet is quite a recent addition to scapy and not all
>> distros might already have a new enough version of scapy available.
>>
>> Fixes: b7fe2c8b1b08 ("pinctrl: dns: Ignore additional records.")
>> Signed-off-by: Dumitru Ceara
>> ---
>>
>
> Hi Dumitru,
> thank you for the patch, I have one small comment down below.
>
>
>> tests/ovn-macros.at | 6 ++
>> tests/ovn.at| 2 +-
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
>> index 269023ca81..a70d2ccaa7 100644
>> --- a/tests/ovn-macros.at
>> +++ b/tests/ovn-macros.at
>> @@ -1030,6 +1030,12 @@ m4_define([TAG_UNSTABLE], [
>> AT_SKIP_IF([test X"$SKIP_UNSTABLE" = Xyes])
>> ])
>>
>> +m4_define([OVN_CHECK_SCAPY_EDNS_CLIENT_SUBNET_SUPPORT],
>> +[
>> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>> +AT_SKIP_IF([! echo "from scapy.layers.dns import EDNS0ClientSubnet" |
>> python &> /dev/null])
>>
>
> &> isn't POSIX compliant, we should use &2>1 >/dev/null.
>
You're right, "2>&1" is the right way. I fixed it up.
>
>> +])
>> +
>> m4_define([OFTABLE_PHY_TO_LOG], [0])
>> m4_define([OFTABLE_LOG_INGRESS_PIPELINE], [8])
>> m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37])
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index d26c950543..e87df6c8dc 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -11498,7 +11498,7 @@ AT_CLEANUP
>>
>> OVN_FOR_EACH_NORTHD([
>> AT_SETUP([dns lookup : EDNS])
>> -AT_SKIP_IF([test $HAVE_SCAPY = no])
>> +OVN_CHECK_SCAPY_EDNS_CLIENT_SUBNET_SUPPORT()
>> ovn_start
>>
>> check ovn-nbctl ls-add ls \
>> --
>> 2.39.3
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>
> With the above addressed:
>
> Acked-by: Ales Musil
>
Thanks! Applied to main and all branches down to 22.03.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 5 Mar 2024, at 16:44, Aaron Conole wrote: > From: Kevin Sprague > > During normal operations, it is useful to understand when a particular flow > gets removed from the system. This can be useful when debugging performance > issues tied to ofproto flow changes, trying to determine deployed traffic > patterns, or while debugging dynamic systems where ports come and go. > > Prior to this change, there was a lack of visibility around flow expiration. > The existing debugging infrastructure could tell us when a flow was added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the revalidator > determines that the flow should be removed. Additionally, we track the > reason for the flow eviction and provide that information as well. With > this change, we can track the complete flow lifecycle for the netlink > datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and > the revalidator USDT, letting us watch as flows are added and removed from > the kernel datapath. > > This change only enables this information via USDT probe, so it won't be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py) > which serves as a demonstration of how the new USDT probe might be used > going forward. > > Acked-by: Han Zhou > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks Aaron for following this up. One small nit, but none blocking, maybe can fix it on commit. Acked-by: Eelco Chaudron > --- > v8 -> v9: Reorganized the flow delete reasons enum > Updated flow_reval_monitor to use pahole to extract fields > Added the purge reason with a proper USDT point > Updated documentation > Dropped all the outstanding ACKs > > v9 -> v10: Reorder the usdt arguments >Rewrite delete comment >Added python docstrings to functions >Use ternary assignment >Clean up example output comment >Capitolize 'F' in Flow >Snip out the comments including help text >Shrink try / except blocks for the USDT probes >refactor the print()/sys.exit() into a single call that uses >sys.stderr >Re-run black & flake8 > > > Documentation/topics/usdt-probes.rst | 43 + > ofproto/ofproto-dpif-upcall.c| 44 +- > utilities/automake.mk| 3 + > utilities/usdt-scripts/flow_reval_monitor.py | 978 +++ > 4 files changed, 1062 insertions(+), 6 deletions(-) > create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..b9a6c54b29 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,8 +214,10 @@ Available probes in ``ovs_vswitchd``: > - dpif_recv:recv_upcall > - main:poll_block > - main:run_start > +- revalidate:flow_result > - revalidate_ukey\_\_:entry > - revalidate_ukey\_\_:exit > +- revalidator_sweep\_\_:flow_result > - udpif_revalidator:start_dump > - udpif_revalidator:sweep_done > > @@ -443,6 +445,47 @@ sweep phase was completed. > - ``utilities/usdt-scripts/reval_monitor.py`` > > > +probe revalidate:flow_result > + > + > +**Description**: > +This probe is triggered when the revalidator has executed on a particular > +flow key to make a determination whether to evict a flow, and the cause > +for eviction. The revalidator runs periodically, and this probe will only > +be triggered when a flow is flagged for revalidation. > + > +**Arguments**: > + > +- *arg0*: ``(struct udpif *) udpif`` > +- *arg1*: ``(struct udpif_key *) ukey`` > +- *arg2*: ``(enum reval_result) result`` > +- *arg3*: ``(enum flow_del_reason) del_reason`` > + > +**Script references**: > + > +- ``utilities/usdt-scripts/flow_reval_monitor.py`` > + > + > +probe revalidator_sweep\_\_:flow_result > +~~~ > + > +**Description**: > +This probe is placed in the path of the revalidator sweep, and is executed > +under the condition that a flow entry is in an unexpected state, or the > +flows were asked to be purged due to a user action. > + > +**Arguments**: > + > +- *arg0*: ``(struct udpif *) udpif`` > +- *arg1*: ``(struct udpif_key *) ukey`` > +- *arg2*: ``(enum reval_result) result`` > +- *arg3*: ``(enum flow_del_reason) del_reason`` > + > +**Script references**: > + > +- ``utilities/usdt-scripts/flow_reval_monitor.py`` > + > + > Adding your own probes > -- > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 9a5c5c29ce..d881956366 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -269,6
Re: [ovs-dev] [PATCH v6 2/2] netlink-conntrack: Optimize flushing ct zone.
On 3/4/24 09:22, Felix Huettner via dev wrote:
> Previously the kernel did not provide a netlink interface to flush/list
> only conntrack entries matching a specific zone. With [1] and [2] it is now
> possible to flush and list conntrack entries filtered by zone. Older
> kernels not yet supporting this feature will ignore the filter.
> For the list request that means just returning all entries (which we can
> then filter in userspace as before).
> For the flush request that means deleting all conntrack entries.
>
> The implementation is now identical to the windows one, so we combine
> them.
>
> These significantly improves the performance of flushing conntrack zones
> when the conntrack table is large. Since flushing a conntrack zone is
> normally triggered via an openflow command it blocks the main ovs thread
> and thereby also blocks new flows from being applied. Using this new
> feature we can reduce the flushing time for zones by around 93%.
>
> In combination with OVN the creation of a Logical_Router (which causes
> the flushing of a ct zone) could block other operations, e.g. the
> failover of Logical_Routers (as they cause new flows to be created).
> This is visible from a user perspective as a ovn-controller that is idle
> (as it waits for vswitchd) and vswitchd reporting:
> "blocked 1000 ms waiting for main to quiesce" (potentially with ever
> increasing times).
>
> The following performance tests where run in a qemu vm with 500.000
> conntrack entries distributed evenly over 500 ct zones using `ovstest
> test-netlink-conntrack flush zone=`.
>
> | flush zone with 1000 entries | flush zone with no entry |
> +-+--+-+--|
> | with the patch| without | with the patch| without |
> +--+--+--+--+--+--|
> | v6.8-rc4 | v6.7.1 | v6.8-rc4 | v6.8-rc4 | v6.7.1 | v6.8-rc4 |
> +-+--+--+--+--+--+--|
> | Min | 0.260 | 3.946 | 3.497 | 0.228 | 3.462 | 3.212 |
> | Median | 0.319 | 4.237 | 4.349 | 0.298 | 4.460 | 4.010 |
> | 90%ile | 0.335 | 4.367 | 4.522 | 0.325 | 4.662 | 4.572 |
> | 99%ile | 0.348 | 4.495 | 4.773 | 0.340 | 4.931 | 6.003 |
> | Max | 0.362 | 4.543 | 5.054 | 0.348 | 5.390 | 6.396 |
> | Mean| 0.320 | 4.236 | 4.331 | 0.296 | 4.430 | 4.071 |
> | Total | 80.02 | 1058| 1082| 73.93 | 1107| 1017|
>
> [1]:
> https://github.com/torvalds/linux/commit/eff3c558bb7e61c41b53e4c8130e514a5a4df9ba
> [2]:
> https://github.com/torvalds/linux/commit/fa173a1b4e3fd1ab5451cbc57de6fc624c824b0a
>
> Co-Authored-By: Luca Czesla
> Signed-off-by: Luca Czesla
> Co-Authored-By: Max Lamprecht
> Signed-off-by: Max Lamprecht
> Signed-off-by: Felix Huettner
> ---
> v5->v6: none
> v4->v5: none
> v3->v4:
> - combine the flush logic with windows implementation
> v2->v3:
> - update description to include upstream fix (Thanks to Ilya for finding
> that issue)
> v1->v2:
> - fixed wrong signed-off-by
>
> lib/netlink-conntrack.c | 52 -
> tests/system-traffic.at | 8 +++
> 2 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 492bfcffb..d9015d9f0 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> @@ -141,6 +141,9 @@ nl_ct_dump_start(struct nl_ct_dump_state **statep, const
> uint16_t *zone,
>
> nl_msg_put_nfgenmsg(>buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK,
> IPCTNL_MSG_CT_GET, NLM_F_REQUEST);
> +if (zone) {
> +nl_msg_put_be16(>buf, CTA_ZONE, htons(*zone));
> +}
> nl_dump_start(>dump, NETLINK_NETFILTER, >buf);
> ofpbuf_clear(>buf);
>
> @@ -263,11 +266,9 @@ out:
> return err;
> }
>
> -#ifdef _WIN32
> -int
> -nl_ct_flush_zone(uint16_t flush_zone)
> +static int
> +nl_ct_flush_zone_with_cta_zone(uint16_t flush_zone)
> {
> -/* Windows can flush a specific zone */
> struct ofpbuf buf;
> int err;
>
> @@ -282,24 +283,63 @@ nl_ct_flush_zone(uint16_t flush_zone)
>
> return err;
> }
> +
> +#ifdef _WIN32
> +int
> +nl_ct_flush_zone(uint16_t flush_zone)
> +{
> +return nl_ct_flush_zone_with_cta_zone(flush_zone);
> +}
> #else
> +
> +static bool
> +netlink_flush_supports_zone(void)
> +{
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +static bool supported = false;
> +
> +if (ovsthread_once_start()) {
> +if (ovs_kernel_is_version_or_newer(6, 8)) {
> +supported = true;
> +} else {
> +VLOG_INFO("disabling conntrack flush by zone. "
> + "Not supported in Linux kernel");
> +}
> +ovsthread_once_done();
> +}
> +return
Re: [ovs-dev] [PATCH] tests: Fix "SSL db: implementation" test with openssl > 3.2.0.
On 5 Mar 2024, at 19:55, Timothy Redaelli wrote: > In OpenSSL 3.2.0 (81b741f) all the "alert" error messages was updated to was -> were > replace "sslv3" with "ssl/tls". > > This commit updates the "SSL db: implementation" test to support both the > pre-openssl 3.2.0 error message: "sslv3 alert certificate unknown" and the > post-openssl 3.2.0 error message: "ssl/tls alert certificate unknown". > > Signed-off-by: Timothy Redaelli > --- Other than the small grammer correction this patch look good to me. So assuming the change is applied on commit; Acked-by: Eelco Chaudron > tests/ovsdb-server.at | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index b8ccc4c8e..35447a52e 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -936,8 +936,10 @@ AT_CHECK_UNQUOTED( >[ignore]) > # The error message for being unable to negotiate a shared ciphersuite > # is 'sslv3 alert handshake failure'. This is not the clearest message. > +# In openssl 3.2.0 all the error messages was updated to replace "sslv3" with was -> were > +# "ssl/tls". > AT_CHECK_UNQUOTED( > - [grep "sslv3 alert handshake failure" output], [0], > + [grep -E "(sslv3|ssl/tls) alert handshake failure" output], [0], >[stdout], >[ignore]) > OVSDB_SERVER_SHUTDOWN([" > -- > 2.43.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 v1 12/12] documentation: Document ovs-flowviz.
On 3/8/24 11:02, Adrian Moreno wrote: > > > On 2/20/24 07:29, Adrian Moreno wrote: >> >> >> On 2/19/24 23:53, Ilya Maximets wrote: >>> On 2/19/24 09:14, Adrian Moreno wrote: Add a page for flow visualization with a few key concepts and examples. Signed-off-by: Adrian Moreno --- Documentation/automake.mk | 3 +- Documentation/topics/flow-visualization.rst | 469 >>> >>> Hi, Adrian. This doc looks a lot like a man page. Maybe it should it be >>> a man page instead? i.e. be in Documentation/ref/ and structured a little >>> more like a man page? >>> >>> I suppose it can also be split in two docs, the man page and a topic >>> document with more comprehensive usage examples. What do you think? >>> >> >> That's a good idea. Will do. >> > > I already was half-way in re-writing the doc in troff when I found that there > are some manpages in rST format. What is the current preferred way of writing > manpages? rST is preferred. All rST pages are going to Documentation/ref and they are also visible as part of html documentation on the website. Other types of man pages are only available in dist-docs. > Side question: why do we have manpages written in two different formats? Because it's hard to re-write them. We also have a few pages in xml. troff was first, then xml, then rST, AFAIK. But neither transition was complete, so we have pages in 3 different formats. Pages that include parts of other pages are harder to translate, we also have extra tooling around xml for database schema pages that will need a re-write while translating to rST. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] bfd: Improve state change log message.
On 5 Mar 2024, at 21:37, Timothy Redaelli wrote: > A log message like this one: > > 2024-01-09T06:45:17.201Z|00071|bfd(handler2)|INFO|ovn-0af536-0: BFD state > change: down->up "Neighbor Signaled Session Down"->"Neighbor Signaled Session > Down". > > can be hard to read since '->' usually represents a status change, but > in this case the diagnostic code stays constant. Update the log message to > avoid such ambiguity. The log message for the above event become: > > 2024-01-09T06:45:16.211Z|00026|bfd(handler3)|INFO|ovn-0af536-0: BFD state > change: (bfd.SessionState: down, bfd.LocalDiag: "Neighbor Signaled Session > Down") -> (bfd.SessionState: up, bfd.LocalDiag: "Neighbor Signaled Session > Down") > > Reported-by: Alex Stupnikov > Reported-at: https://bugzilla.redhat.com/2258496 > Signed-off-by: Timothy Redaelli Thanks for following up, the change looks good to me! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-dpdk: Dump packets that fail Tx preparation.
On 7 Mar 2024, at 20:39, Ilya Maximets wrote: > It's hard to debug situations where driver rejects packets for some > reason. Dumping out the mbuf should help with that. > > Sample output looks like this: > > |netdev_dpdk(pmd-c03/id:8)|DBG|ovs-p1: First invalid packet: > dump mbuf at 0x1180bce140, iova=0x2cb7ce400, buf_len=2176 > pkt_len=64, ol_flags=0x2, nb_segs=1, port=65535, ptype=0 > segment at 0x1180bce140, data=0x1180bce580, len=90, off=384, refcnt=1 > Dump data at [0x1180bce580], len=64 > : 33 33 00 00 00 16 AA 27 91 F9 4D 96 86 DD 60 00 | 33.'..M...`. > 0010: 00 00 00 24 00 01 00 00 00 00 00 00 00 00 00 00 | ...$ > 0020: 00 00 00 00 00 00 FF 02 00 00 00 00 00 00 00 00 | > 0030: 00 00 00 00 00 16 3A 00 05 02 00 00 01 00 8F 00 | ..:. > > Signed-off-by: Ilya Maximets This debug addition looks good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 12/12] documentation: Document ovs-flowviz.
On 2/20/24 07:29, Adrian Moreno wrote: On 2/19/24 23:53, Ilya Maximets wrote: On 2/19/24 09:14, Adrian Moreno wrote: Add a page for flow visualization with a few key concepts and examples. Signed-off-by: Adrian Moreno --- Documentation/automake.mk | 3 +- Documentation/topics/flow-visualization.rst | 469 Hi, Adrian. This doc looks a lot like a man page. Maybe it should it be a man page instead? i.e. be in Documentation/ref/ and structured a little more like a man page? I suppose it can also be split in two docs, the man page and a topic document with more comprehensive usage examples. What do you think? That's a good idea. Will do. I already was half-way in re-writing the doc in troff when I found that there are some manpages in rST format. What is the current preferred way of writing manpages? Side question: why do we have manpages written in two different formats? I didn't fully read the doc, but see some typos and minor issues that caught my eye below. Best regards, Ilya Maximets. Documentation/topics/index.rst | 1 + 3 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 Documentation/topics/flow-visualization.rst diff --git a/Documentation/automake.mk b/Documentation/automake.mk index 47d2e336a..f1339e876 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -45,7 +45,7 @@ DOC_SOURCE = \ Documentation/topics/fuzzing/ovs-fuzzing-infrastructure.rst \ Documentation/topics/fuzzing/ovs-fuzzers.rst \ Documentation/topics/fuzzing/security-analysis-of-ovs-fuzzers.rst \ - Documentation/topics/testing.rst \ + Documentation/topics/flow-visualization.rst \ Documentation/topics/integration.rst \ Documentation/topics/language-bindings.rst \ Documentation/topics/networking-namespaces.rst \ @@ -55,6 +55,7 @@ DOC_SOURCE = \ Documentation/topics/ovsdb-replication.rst \ Documentation/topics/porting.rst \ Documentation/topics/record-replay.rst \ + Documentation/topics/testing.rst \ Documentation/topics/tracing.rst \ Documentation/topics/usdt-probes.rst \ Documentation/topics/userspace-checksum-offloading.rst \ diff --git a/Documentation/topics/flow-visualization.rst b/Documentation/topics/flow-visualization.rst new file mode 100644 index 0..366275c54 --- /dev/null +++ b/Documentation/topics/flow-visualization.rst @@ -0,0 +1,469 @@ +.. + 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. + + Convention for heading levels in Open vSwitch documentation: + + === Heading 0 (reserved for the title in a document) + --- Heading 1 + ~~~ Heading 2 + +++ Heading 3 + ''' Heading 4 + + Avoid deeper levels because they do not render well. + += +ovs-flowviz: Datapath and Openflow flow visualization * OpenFlow += + +When troubleshooting networking issues with OVS, we typically end up looking +at OpenFlow or datapath flow dumps. These dumps tend to be quite dense and +difficult to reason about. + +``ovs-flowviz`` is a utility script that helps visualizing OpenFlow and +datapath flows to make it easier to understand what is going on. + + +Installing ovs-flowviz +-- + +``ovs-flowviz`` is part of the openvswitch python package. To install it, run: +:: + + $ pip install openvswitch[flowviz] + +Or, if you are working with the OVS tree: +:: + + $ cd python && pip install .[flowviz] + +Running the tool + +Here is the basic usage of the tool: +:: + + $ ovs-flowviz --help + Usage: ovs-flowviz [OPTIONS] COMMAND [ARGS]... + + OpenvSwitch flow visualization utility. + + It reads openflow and datapath flows (such as the output of ovs-ofctl dump- + flows or ovs-appctl dpctl/dump-flows) and prints them in different formats. + + Options: + -c, --config PATH Use config file [default: /home/amorenoz/src/ovs/pyth We probbaly shouldn't list your home directory in the docs. :) + on/venv/lib64/python3.12/site- + packages/ovs/flowviz/ovs-flowviz.conf] Or venv for that matter. Uups! I just C-P the "--help" output where the default value is obtained from the current setup. I'll fix that. + --style TEXT
Re: [ovs-dev] [PATCH v6 1/2] util: Support checking for kernel versions.
On 4 Mar 2024, at 9:22, Felix Huettner via dev wrote: > Extract checking for a given kernel version to a separate function. > It will be used also in the next patch. > > Signed-off-by: Felix Huettner Thanks for fixing the version check. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
On Wed, Mar 6, 2024 at 8:24 PM Naveen Yerramneni <
naveen.yerramn...@nutanix.com> wrote:
>
>
> > On 18-Dec-2023, at 8:53 PM, Dumitru Ceara wrote:
> >
> > On 12/18/23 16:17, Naveen Yerramneni wrote:
> >>
> >>
> >>> On 18-Dec-2023, at 7:26 PM, Dumitru Ceara wrote:
> >>>
> >>> On 11/30/23 16:32, Dumitru Ceara wrote:
> On 11/30/23 15:54, Naveen Yerramneni wrote:
> >
> >
> >> On 30-Nov-2023, at 6:06 PM, Dumitru Ceara
> wrote:
> >>
> >> On 11/30/23 09:45, Naveen Yerramneni wrote:
> >>>
> >>>
> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara
> wrote:
>
> On 11/29/23 07:45, naveen.yerramneni wrote:
> > This functionality can be enabled at the logical switch level:
> > - "other_config:fdb_local" can be used to enable/disable this
> > functionality, it is disabled by default.
> > - "other_config:fdb_local_idle_timeout" sepcifies idle timeout
> > for locally learned fdb flows, default timeout is 300 secs.
> >
> > If enabled, below lflow is added for each port that has unknown
> addr set.
> > - table=2 (ls_in_lookup_fdb), priority=100, match=(inport ==
> ),
> > action=(commit_fdb_local(timeout=); next;
> >
> > New OVN action: "commit_fdb_local". This sets following OVS
> action.
> > -
> learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
> >
> NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
> >
> > This is useful when OVN is managing VLAN network that has
> multiple ports
> > set with unknown addr and localnet_learn_fdb is enabled. With
> this config,
> > if there is east-west traffic flowing between VMs part of same
> VLAN
> > deployed on different hypervisors then, MAC addrs of the source
> and
> > destination VMs keeps flapping between VM port and localnet port
> in
> > Southbound FDB table. Enabling fdb_local config makes fdb table
> local to
> > the chassis and avoids MAC flapping.
> >
> > Signed-off-by: Naveen Yerramneni
> > ---
>
> Hi Naveen,
>
> Thanks a lot for the patch!
>
> Just a note, we already have a fix for the east-west traffic that
> causes
> FDB flapping when localnet is used:
>
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>
> In general, however, I think it's a very good idea to move the
> FDB away
> from the Southbound and make it local to each hypervisor. That
> reduces
> load on the Southbound among other things.
>
> >>>
> >>> Hi Dumitru,
> >>>
> >>> Thanks for informing about the patches.
> >>> Yes, local FDB reduces load on southbound.
> >>>
> >>>
> > include/ovn/actions.h | 7 +++
> > lib/actions.c | 94
> > northd/northd.c | 26 ++
> > ovn-nb.xml| 14 ++
> > tests/ovn.at | 108
> ++
> > utilities/ovn-trace.c | 2 +
> > 6 files changed, 251 insertions(+)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 49cfe0624..85ac92cd3 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -127,6 +127,7 @@ struct collector_set_ids;
> > OVNACT(CHK_LB_AFF,ovnact_result) \
> > OVNACT(SAMPLE,ovnact_sample) \
> > OVNACT(MAC_CACHE_USE, ovnact_null)\
> > +OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
> >
> > /* enum ovnact_type, with a member OVNACT_ for each
> action. */
> > enum OVS_PACKED_ENUM ovnact_type {
> > @@ -514,6 +515,12 @@ struct ovnact_commit_lb_aff {
> > uint16_t timeout;
> > };
> >
> > +/* OVNACT_COMMIT_FBD_LOCAL. */
> > +struct ovnact_commit_fdb_local{
> > +struct ovnact ovnact;
> > +uint16_t timeout; /* fdb_local flow timeout */
> > +};
> > +
> > /* Internal use by the helpers below. */
> > void ovnact_init(struct ovnact *, enum ovnact_type, size_t len);
> >
[ovs-dev] [PATCH] ofproto: Fix mod flow table name not to take effect.
From: "zhouyuhao.philozhou"
When mod a flow table's name with table's prefix name, there
will be no change. Because when check whether the new and old
name are the same, only compare the length of the new name.
Case:
table 10: "good"
There will be no change if mod the table's name with "g" "go" "goo".
Signed-off-by: zhouyuhao.philozhou
---
ofproto/ofproto.c | 4 +++-
tests/ofproto.at | 12
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 122a06f30..bf7ed91b1 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char *name,
int level)
if (level >= table->name_level) {
if (name) {
if (name[0]) {
-if (!table->name || strncmp(name, table->name, len)) {
+if (!table->name
+|| strncmp(name, table->name, len)
+|| len != strlen(table->name)) {
free(table->name);
table->name = xmemdup0(name, len);
}
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 2889f81fb..09c57b292 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -2523,6 +2523,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features
br0 |grep '^ table'],
table 253:
])
+# Make sure that the new name is old table's name prefix can also take effect.
+AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 3 name:thr])
+AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'],
+ [0], [dnl
+ table 0 ("zero"):
+ table 1 ("one"): ditto
+ table 2: ditto
+ table 3 ("thr"): ditto
+ tables 4...252: ditto
+ table 253:
+])
+
OVS_VSWITCHD_STOP
AT_CLEANUP
--
2.39.3 (Apple Git-146)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC] bridge: Retry tunnel port addition for conflict.
On 3/7/24 5:39 AM, Ilya Maximets wrote:
On 2/27/24 20:14, Han Zhou wrote:
For kernel datapath, when a new tunnel port is created in the same
transaction in which an old tunnel port with the same tunnel
configuration is deleted, the new tunnel port creation will fail and
left in an error state. This can be easily reproduced in OVN by
triggering a chassis deletion and addition with the same encap in the
same SB DB transaction, such as:
ovn-sbctl chassis-add aa geneve 1.2.3.4
ovn-sbctl chassis-del aa -- chassis-add bb 1.2.3.4
ovs-vsctl show | grep error
error: "could not add network device ovn-bb-0 to ofproto (File exists)"
Related logs in OVS:
—
2024-02-23T05:41:49.978Z|405933|bridge|INFO|bridge br-int: deleted interface
ovn-aa-0 on port 113
2024-02-23T05:41:49.989Z|405935|tunnel|WARN|ovn-bb-0: attempting to add tunnel
port with same config as port 'ovn-aa-0' (::->1.2.3.4, key=flow, legacy_l2,
dp port=9)
2024-02-23T05:41:49.989Z|405936|ofproto|WARN|br-int: could not add port
ovn-bb-0 (File exists)
2024-02-23T05:41:49.989Z|405937|bridge|WARN|could not add network device
ovn-bb-0 to ofproto (File exists)
—
Hi, Han. Thanks for the patch!
Depending on when there are other OVSDB changes, it may take a long time
for the device to be added successfully, triggered by the next OVS
iteration.
(note: native tunnel ports do not have this problem)
I don't think this is correct. The code path is common for both system
and native tunnels. I can reproduce the issues in a sandbox with:
$ make -j8 sandbox SANDBOXFLAGS="\-\-dummy='system'"
[tutorial]$ ovs-vsctl add-port br0 tunnel_port \
-- set Interface tunnel_port \
type=geneve options:remote_ip=flow options:key=123
[tutorial]$ ovs-vsctl del-port tunnel_port \
-- add-port br0 tunnel_port2 \
-- set Interface tunnel_port2 \
type=geneve options:remote_ip=flow options:key=123
ovs-vsctl: Error detected while setting up 'tunnel_port2':
could not add network device tunnel_port2 to ofproto (File exists).
See ovs-vswitchd log for details.
The same should work in a testsuite as well, i.e. we should be able to
create a test for this scenario.
Note: The --dummy=system prevents OVS from replacing tunnel ports with
dummy ones.
The problem is how the tunnel port deletion and creation are handled. In
bridge_reconfigure(), port deletion is handled before creation, to avoid
such resource conflict. However, for kernel tunnel ports, the real clean
up is performed at the end of the bridge_reconfigure() in the:
bridge_run__()->ofproto_run()->...->ofproto_dpif:port_destruct()
We cannot call bridge_run__() at an earlier point before all
reconfigurations are done, so this patch tries a generic approach to
just re-run the bridge_reconfigure() when there are any port creations
encountered "File exists" error, which indicates a possible resource
conflict may have happened due to a later deleted port, and retry may
succeed.
Signed-off-by: Han Zhou
---
This is RFC because I am not sure if there is a better way to solve the problem
more specifically by executing the port_destruct for the old port before trying
to create the new port. The fix may be more complex though.
I don't think re-trying is a good approach in general. We should likely
just destroy the tnl_port structure right away, similarly to how we clean
up stp, lldp and bundles in ofproto_port_unregister(). Maybe we can add
a new callback similar to bundle_remove() and call tnl_port_del() from it?
(I didn't try, so I'm not 100% sure this will not cause any issues.)
What do you think?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Ilya and Han,
We hit this issue some days ago. As our analysis, it was introduced by
commit fe83f81df977 ("netdev: Remove netdev from global shash when the
user is changing interface configuration.")
Reproduce:
ovs-vsctl add-port br-int vxlan0 \
-- set interface vxlan0 type=vxlan options:remote_ip=10.10.10.1
sleep 2
ovs-vsctl --if-exists del-port vxlan0 \
-- add-port br-int vxlan1 \
-- set interface vxlan1 type=vxlan options:remote_ip=10.10.10.1
ovs-vsctl: Error detected while setting up 'vxlan1': could not add
network device vxlan1 to ofproto (File exists).
vswitchd log:
bridge|INFO|bridge br-int: added interface vxlan0 on port 1106
bridge|INFO|bridge br-int: deleted interface vxlan0 on port 1106
tunnel|WARN|vxlan1: attempting to add tunnel port with same config as
port 'vxlan0' (::->10.10.10.1, key=0, legacy_l2, dp port=122)
ofproto|WARN|br-int: could not add port vxlan1 (File exists)
bridge|WARN|could not add network device vxlan1 to ofproto (File exists)
CallTrace:
bridge_reconfigure
bridge_del_ports
port_destroy
iface_destroy__
netdev_remove
Re: [ovs-dev] [PATCH ovn] tests: Skip EDNS test if the scapy version doesn't support it.
On Thu, Mar 7, 2024 at 4:19 PM Dumitru Ceara wrote:
> EDNS0ClientSubnet is quite a recent addition to scapy and not all
> distros might already have a new enough version of scapy available.
>
> Fixes: b7fe2c8b1b08 ("pinctrl: dns: Ignore additional records.")
> Signed-off-by: Dumitru Ceara
> ---
>
Hi Dumitru,
thank you for the patch, I have one small comment down below.
> tests/ovn-macros.at | 6 ++
> tests/ovn.at| 2 +-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 269023ca81..a70d2ccaa7 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -1030,6 +1030,12 @@ m4_define([TAG_UNSTABLE], [
> AT_SKIP_IF([test X"$SKIP_UNSTABLE" = Xyes])
> ])
>
> +m4_define([OVN_CHECK_SCAPY_EDNS_CLIENT_SUBNET_SUPPORT],
> +[
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
> +AT_SKIP_IF([! echo "from scapy.layers.dns import EDNS0ClientSubnet" |
> python &> /dev/null])
>
&> isn't POSIX compliant, we should use &2>1 >/dev/null.
> +])
> +
> m4_define([OFTABLE_PHY_TO_LOG], [0])
> m4_define([OFTABLE_LOG_INGRESS_PIPELINE], [8])
> m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index d26c950543..e87df6c8dc 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11498,7 +11498,7 @@ AT_CLEANUP
>
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([dns lookup : EDNS])
> -AT_SKIP_IF([test $HAVE_SCAPY = no])
> +OVN_CHECK_SCAPY_EDNS_CLIENT_SUBNET_SUPPORT()
> ovn_start
>
> check ovn-nbctl ls-add ls \
> --
> 2.39.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With the above addressed:
Acked-by: Ales Musil
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ofproto: Fix mod flow table name not to take effect.
From: "zhouyuhao.philozhou"
When mod a flow table's name with table's prefix name, there
will be no change. Because when check whether the new and old
name are the same, only compare the length of the new name.
Case:
table 10: "good"
There will be no change if mod the table's name with "g" "go" "goo".
Signed-off-by: zhouyuhao.philozhou
---
ofproto/ofproto.c | 4 +++-
tests/ofproto.at | 12
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 122a06f30..bf7ed91b1 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char *name,
int level)
if (level >= table->name_level) {
if (name) {
if (name[0]) {
-if (!table->name || strncmp(name, table->name, len)) {
+if (!table->name
+|| strncmp(name, table->name, len)
+|| len != strlen(table->name)) {
free(table->name);
table->name = xmemdup0(name, len);
}
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 2889f81fb..b68881a27 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -2418,6 +2418,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features
br0 |grep '^ table'],
table 253:
])
+# Make sure that the new name is old table's name prefix can also take effect.
+AT_CHECK([ovs-ofctl -O OpenFlow13 mod-table br0 3 name:thr])
+AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features br0 |grep '^ table'],
+ [0], [dnl
+ table 0 ("zero"):
+ table 1 ("one"): ditto
+ table 2: ditto
+ table 3 ("thr"): ditto
+ tables 4...252: ditto
+ table 253:
+])
+
# Set some table names via OVSDB.
AT_CHECK(
[ovs-vsctl \
--
2.39.3 (Apple Git-146)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
