Re: [ovs-dev] [PATCH ovn] extend-table: Fix table ID double allocation after OVS restart.
On Thu, Jul 28, 2022 at 1:34 PM Numan Siddique wrote:
>
> On Wed, Jul 27, 2022 at 6:18 PM Han Zhou wrote:
> >
> > There were problems observed occasionally after OVS restart, the
> > OVS flow bundle installation from ovn-controller was failed because of
> > "GROUP_EXISTS" error, which end up with missing flows/groups/meters
> > in OVS until ovn-controller is restarted.
> >
>
> Hi Han,
>
> Can you please check this bugzilla and see if its the same issue ?
>
Yes.
> https://bugzilla.redhat.com/show_bug.cgi?id=2112111
>
> Looks to me it is. If so, can you please also add - "Reported-at" tag
> referencing this BZ (for our tracking purpose).
Ack.
>
> Please see a few small nits.
>
> > Example error logs in OVS:
> > 2022-07-08T01:38:22.837Z|00676|connmgr|INFO|br-int<->unix#0: sending
OFPGMFC_GROUP_EXISTS error reply to OFPT_BUNDLE_ADD_MESSAGE message
> > 2022-07-08T01:38:22.913Z|00677|connmgr|INFO|br-int<->unix#0: sending
OFPBFC_MSG_FAILED error reply to OFPT_BUNDLE_CONTROL message
> >
> > The root cause is that with ovn-ofctrl-wait-before-clear set, ofctrl
> > module would call ovn_extend_table_clear() to clear the existing group
> > table AFTER ovn-controll finished computing the desired
>
> s/ovn-controll/ovn-controller
Ack.
>
> > flows/groups/meters in the state S_CLEAR. However, the function
> > ovn_extend_table_clear() clears the bitmap of the group IDs, while the
> > IDs are still being used by desired group table. This is not a problem
>
> s/by desired group/by the group
I meant to emphasize it is the "desired" group table that still uses the
bitmap although the "existing" group table is cleared.
I rephrased it to avoid the confusion.
>
> > if a recompute happens soon, the desired group table will be cleared
> > first and IDs will be reallocated and the bitmap will reflect the actual
> > allocations. However, if there are any group creation changes (related
> > to LB, ECMP, etc.) happen before the recompute, new IDs may be allocated
> > to be conflict with existing IDs because the cleared bitmap status
> > doesn't reflect the real IDs being used. The conflict IDs finally causes
> > the "GROUP_EXIST" error replied by OVS when ovn-controller tries to
> > install the desired groups to OVS. Even worse, because the group
> > modifications are now wrapped in a bundle with flow modifications, it
> > would end up with not only missing groups but also missing flows.
> >
> > Both desired table and existing table share the same bitmap, which is to
> > avoid reallocating an ID that is still exists in OVS, but the current
>
> s/that is still/that still
>
Ack.
> > logic seems to have an assumption that the "existing" table entries are
> > deleted always AFTER the "desired" entries. This assumption is not true
> > after the introduction of ovn-ofctrl-wait-before-clear feature.
> >
> > The fix here is to introduce a reference between the desired and
> > existing entries, so that when deleting an entry in either of the tables
> > it knows if the ID is still in use by its peer and decide if it is the
> > right time to clear the bit from the bitmap, without depending on the
> > order of deletion.
> >
> > Fixes: 896adfd2d8b3 ("ofctrl: Support ovn-ofctrl-wait-before-clear to
reduce down time during upgrade.")
> > Signed-off-by: Han Zhou
> > ---
> > lib/extend-table.c | 67 +
> > lib/extend-table.h | 17 ---
> > tests/ovn-controller.at | 26 +++-
> > 3 files changed, 73 insertions(+), 37 deletions(-)
> >
> > diff --git a/lib/extend-table.c b/lib/extend-table.c
> > index 4c3c4fac2..453b4468a 100644
> > --- a/lib/extend-table.c
> > +++ b/lib/extend-table.c
> > @@ -40,13 +40,17 @@ ovn_extend_table_init(struct ovn_extend_table
*table)
> > }
> >
> > static struct ovn_extend_table_info *
> > -ovn_extend_table_info_alloc(const char *name, uint32_t id, bool
is_new_id,
> > +ovn_extend_table_info_alloc(const char *name, uint32_t id,
> > +struct ovn_extend_table_info *peer,
> > uint32_t hash)
> > {
> > struct ovn_extend_table_info *e = xmalloc(sizeof *e);
> > e->name = xstrdup(name);
> > e->table_id = id;
> > -e->new_table_id = is_new_id;
> > +e->peer = peer;
> > +if (peer) {
> > +peer->peer = e;
> > +}
> > e->hmap_node.hash = hash;
> > hmap_init(>references);
> > return e;
> > @@ -184,9 +188,10 @@ ovn_extend_table_clear(struct ovn_extend_table
*table, bool existing)
> > /* Clear the target table. */
> > HMAP_FOR_EACH_SAFE (g, hmap_node, target) {
> > hmap_remove(target, >hmap_node);
> > -/* Don't unset bitmap for desired group_info if the group_id
> > - * was not freshly reserved. */
> > -if (existing || g->new_table_id) {
> > +if (g->peer) {
> > +g->peer->peer = NULL;
> > +} else {
> > +/* Don't unset bitmap if the peer table is still using it.
*/
>
> This
Re: [ovs-dev] [PATCH ovn] extend-table: Fix table ID double allocation after OVS restart.
On Wed, Jul 27, 2022 at 6:18 PM Han Zhou wrote:
>
> There were problems observed occasionally after OVS restart, the
> OVS flow bundle installation from ovn-controller was failed because of
> "GROUP_EXISTS" error, which end up with missing flows/groups/meters
> in OVS until ovn-controller is restarted.
>
Hi Han,
Can you please check this bugzilla and see if its the same issue ?
https://bugzilla.redhat.com/show_bug.cgi?id=2112111
Looks to me it is. If so, can you please also add - "Reported-at" tag
referencing this BZ (for our tracking purpose).
Please see a few small nits.
> Example error logs in OVS:
> 2022-07-08T01:38:22.837Z|00676|connmgr|INFO|br-int<->unix#0: sending
> OFPGMFC_GROUP_EXISTS error reply to OFPT_BUNDLE_ADD_MESSAGE message
> 2022-07-08T01:38:22.913Z|00677|connmgr|INFO|br-int<->unix#0: sending
> OFPBFC_MSG_FAILED error reply to OFPT_BUNDLE_CONTROL message
>
> The root cause is that with ovn-ofctrl-wait-before-clear set, ofctrl
> module would call ovn_extend_table_clear() to clear the existing group
> table AFTER ovn-controll finished computing the desired
s/ovn-controll/ovn-controller
> flows/groups/meters in the state S_CLEAR. However, the function
> ovn_extend_table_clear() clears the bitmap of the group IDs, while the
> IDs are still being used by desired group table. This is not a problem
s/by desired group/by the group
> if a recompute happens soon, the desired group table will be cleared
> first and IDs will be reallocated and the bitmap will reflect the actual
> allocations. However, if there are any group creation changes (related
> to LB, ECMP, etc.) happen before the recompute, new IDs may be allocated
> to be conflict with existing IDs because the cleared bitmap status
> doesn't reflect the real IDs being used. The conflict IDs finally causes
> the "GROUP_EXIST" error replied by OVS when ovn-controller tries to
> install the desired groups to OVS. Even worse, because the group
> modifications are now wrapped in a bundle with flow modifications, it
> would end up with not only missing groups but also missing flows.
>
> Both desired table and existing table share the same bitmap, which is to
> avoid reallocating an ID that is still exists in OVS, but the current
s/that is still/that still
> logic seems to have an assumption that the "existing" table entries are
> deleted always AFTER the "desired" entries. This assumption is not true
> after the introduction of ovn-ofctrl-wait-before-clear feature.
>
> The fix here is to introduce a reference between the desired and
> existing entries, so that when deleting an entry in either of the tables
> it knows if the ID is still in use by its peer and decide if it is the
> right time to clear the bit from the bitmap, without depending on the
> order of deletion.
>
> Fixes: 896adfd2d8b3 ("ofctrl: Support ovn-ofctrl-wait-before-clear to reduce
> down time during upgrade.")
> Signed-off-by: Han Zhou
> ---
> lib/extend-table.c | 67 +
> lib/extend-table.h | 17 ---
> tests/ovn-controller.at | 26 +++-
> 3 files changed, 73 insertions(+), 37 deletions(-)
>
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index 4c3c4fac2..453b4468a 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -40,13 +40,17 @@ ovn_extend_table_init(struct ovn_extend_table *table)
> }
>
> static struct ovn_extend_table_info *
> -ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id,
> +ovn_extend_table_info_alloc(const char *name, uint32_t id,
> +struct ovn_extend_table_info *peer,
> uint32_t hash)
> {
> struct ovn_extend_table_info *e = xmalloc(sizeof *e);
> e->name = xstrdup(name);
> e->table_id = id;
> -e->new_table_id = is_new_id;
> +e->peer = peer;
> +if (peer) {
> +peer->peer = e;
> +}
> e->hmap_node.hash = hash;
> hmap_init(>references);
> return e;
> @@ -184,9 +188,10 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
> bool existing)
> /* Clear the target table. */
> HMAP_FOR_EACH_SAFE (g, hmap_node, target) {
> hmap_remove(target, >hmap_node);
> -/* Don't unset bitmap for desired group_info if the group_id
> - * was not freshly reserved. */
> -if (existing || g->new_table_id) {
> +if (g->peer) {
> +g->peer->peer = NULL;
> +} else {
> +/* Don't unset bitmap if the peer table is still using it. */
This comment is confusing to me. The comment says don't unset bitmap
but the function bitmap_set0() unsets the bit right ?
Maybe the comment should be in the "if" condition ?
With these small comments addressed
Acked-by: Numan Siddique
Numan
> bitmap_set0(table->table_ids, g->table_id);
> }
> ovn_extend_table_info_destroy(g);
> @@ -209,11 +214,15 @@ void
> ovn_extend_table_remove_existing(struct
[ovs-dev] [PATCH] system-offloads-traffic: Fix waiting for netcat indefinitely.
$NC_EOF_OPT should be used to avoid some netcat implementations
to wait indefinitely.
This fixes the check-offloads testsuite hanging in Ubuntu 22.04.
Fixes: 5660b89a309d ("dpif-netlink: Offloading meter to tc police action")
Signed-off-by: Ilya Maximets
---
tests/system-offloads-traffic.at | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 834d444f0..1e1012965 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -203,7 +203,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 del-flows br0])
AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "priority=10,in_port=ovs-p0,udp
actions=meter:1,normal"])
AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "priority=1 actions=normal"])
-NS_CHECK_EXEC([at_ns0], [echo "mark" | nc -u 10.1.1.2 5678 -p 6789])
+NS_CHECK_EXEC([at_ns0], [echo "mark" | nc $NC_EOF_OPT -u 10.1.1.2 5678 -p
6789])
AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" | DUMP_CLEAN_SORTED],
[0], [dnl
in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:0,
bytes:0, used:0.001s, actions:meter(0),3
])
@@ -211,7 +211,7 @@
in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:0, bytes:0
sleep 1
for i in `seq 10`; do
-NS_CHECK_EXEC([at_ns0], [echo "mark" | nc -u 10.1.1.2 5678 -p 6789])
+NS_CHECK_EXEC([at_ns0], [echo "mark" | nc $NC_EOF_OPT -u 10.1.1.2 5678 -p
6789])
done
AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" | DUMP_CLEAN_SORTED],
[0], [dnl
@@ -252,7 +252,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 del-flows br0])
AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "priority=10,in_port=ovs-p0,udp
actions=meter:1,normal"])
AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "priority=1 actions=normal"])
-NS_CHECK_EXEC([at_ns0], [echo "mark" | nc -u 10.1.1.2 5678 -p 6789])
+NS_CHECK_EXEC([at_ns0], [echo "mark" | nc $NC_EOF_OPT -u 10.1.1.2 5678 -p
6789])
AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" | DUMP_CLEAN_SORTED],
[0], [dnl
in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:0,
bytes:0, used:0.001s, actions:meter(0),3
])
@@ -260,7 +260,7 @@
in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:0, bytes:0
sleep 1
for i in `seq 10`; do
-NS_CHECK_EXEC([at_ns0], [echo "mark" | nc -u 10.1.1.2 5678 -p 6789])
+NS_CHECK_EXEC([at_ns0], [echo "mark" | nc $NC_EOF_OPT -u 10.1.1.2 5678 -p
6789])
done
AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" | DUMP_CLEAN_SORTED],
[0], [dnl
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 2/2] openvswitch: Fix overreporting of drops in dropwatch
Currently queue_userspace_packet will call kfree_skb for all frames, whether or not an error occurred. This can result in a single dropped frame being reported as multiple drops in dropwatch. This patch will consume the skbs instead. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2109957 Signed-off-by: Mike Pattrick --- net/openvswitch/datapath.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 029f9c3e1c28..3eee4b0a2005 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -561,8 +561,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, out: if (err) skb_tx_error(skb); - kfree_skb(user_skb); - kfree_skb(nskb); + consume_skb(user_skb); + consume_skb(nskb); + return err; } -- 2.31.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 1/2] openvswitch: Fix double reporting of drops in dropwatch
Frames sent to userspace can be reported as dropped in
ovs_dp_process_packet, however, if they are dropped in the netlink code
then netlink_attachskb will report the same frame as dropped.
This patch checks for error codes which indicate that the frame has
already been freed.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2109946
Signed-off-by: Mike Pattrick
---
net/openvswitch/datapath.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 7e8a39a35627..029f9c3e1c28 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -252,10 +252,20 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct
sw_flow_key *key)
upcall.mru = OVS_CB(skb)->mru;
error = ovs_dp_upcall(dp, skb, key, , 0);
- if (unlikely(error))
- kfree_skb(skb);
- else
+ switch (error) {
+ case 0:
+ fallthrough;
+ case -EAGAIN:
+ fallthrough;
+ case -ERESTARTSYS:
+ fallthrough;
+ case -EINTR:
consume_skb(skb);
+ break;
+ default:
+ kfree_skb(skb);
+ break;
+ }
stats_counter = >n_missed;
goto out;
}
--
2.31.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netlink: Fix incorrect bit shift in compat mode.
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior in
lib/dpif-netlink.c:1077:40: runtime error:
left shift of 1 by 31 places cannot be represented in type 'int'
#0 0x73fc31 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1077:40
#1 0x73fc31 in dpif_netlink_port_add lib/dpif-netlink.c:1132:17
#2 0x2c1745 in dpif_port_add lib/dpif.c:597:13
#3 0x07b279 in port_add ofproto/ofproto-dpif.c:3957:17
#4 0x01b209 in ofproto_port_add ofproto/ofproto.c:2124:13
#5 0xfdbfce in iface_do_create vswitchd/bridge.c:2066:13
#6 0xfdbfce in iface_create vswitchd/bridge.c:2109:13
#7 0xfdbfce in bridge_add_ports__ vswitchd/bridge.c:1173:21
#8 0xfb5319 in bridge_add_ports vswitchd/bridge.c:1189:5
#9 0xfb5319 in bridge_reconfigure vswitchd/bridge.c:901:9
#10 0xfae0f9 in bridge_run vswitchd/bridge.c:3334:9
#11 0xfe67dd in main vswitchd/ovs-vswitchd.c:129:9
#12 0x4b6d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f)
#13 0x4b6e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f)
#14 0x562594eed024 in _start (vswitchd/ovs-vswitchd+0x787024)
Fixes: 526df7d8543f ("tunnel: Provide framework for tunnel extensions for
VXLAN-GBP and others")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..89e1d4325 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -1074,7 +1074,7 @@ dpif_netlink_port_add_compat(struct dpif_netlink *dpif,
struct netdev *netdev,
ext_ofs = nl_msg_start_nested(, OVS_TUNNEL_ATTR_EXTENSION);
for (i = 0; i < 32; i++) {
-if (tnl_cfg->exts & (1 << i)) {
+if (tnl_cfg->exts & (UINT32_C(1) << i)) {
nl_msg_put_flag(, i);
}
}
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] test-ovsdb: Fix false-positive leaks from LeakSanitizer.
LeakSanitizer for some reason reports these json objects as leaked,
even though we do have references to them at the moment ovs_fatal()
called from check_ovsdb_error().
Previously it complained only with -O2, but with newer versions of
clang/llvm it started complaining even with -O1. For example, negative
ovsdb parsing tests are failing on ubuntu 22.04 with clang 14 if built
with ASan and detect_leaks=1.
Fix that by destroying the json object before aborting the process.
And we may also build with default -O2 in CI with that change.
Alternative implementation might be to just pass the json to destroy
to every check_ovsdb_error() call, but indirect registering of the
pointer seems a bit less invasive.
Signed-off-by: Ilya Maximets
---
.ci/linux-build.sh | 6 ++---
tests/test-ovsdb.c | 61 ++
2 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index c396ec1e8..22e138df3 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -245,15 +245,13 @@ fi
if [ "$ASAN" ]; then
# This will override default option configured in tests/atlocal.in.
export ASAN_OPTIONS='detect_leaks=1'
-# -O2 generates few false-positive memory leak reports in test-ovsdb
-# application, so lowering optimizations to -O1 here.
-CFLAGS_ASAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=address"
+CFLAGS_ASAN="-fno-omit-frame-pointer -fno-common -fsanitize=address"
CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}"
fi
if [ "$UBSAN" ]; then
# Use the default options configured in tests/atlocal.in, in UBSAN_OPTIONS.
-CFLAGS_UBSAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=undefined"
+CFLAGS_UBSAN="-fno-omit-frame-pointer -fno-common -fsanitize=undefined"
CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}"
fi
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 343833151..965b0f913 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -300,11 +300,24 @@ print_and_free_ovsdb_error(struct ovsdb_error *error)
free(string);
}
+static struct json **json_to_destroy;
+
+static void
+destroy_on_ovsdb_error(struct json **json)
+{
+json_to_destroy = json;
+}
+
static void
check_ovsdb_error(struct ovsdb_error *error)
{
if (error) {
char *s = ovsdb_error_to_string_free(error);
+
+if (json_to_destroy) {
+json_destroy(*json_to_destroy);
+json_to_destroy = NULL;
+}
ovs_fatal(0, "%s", s);
}
}
@@ -487,6 +500,8 @@ do_diff_data(struct ovs_cmdl_context *ctx)
struct json *json;
struct ovsdb_datum new, old, diff, reincarnation;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_type_from_json(, json));
json_destroy(json);
@@ -562,6 +577,8 @@ do_parse_atomic_type(struct ovs_cmdl_context *ctx)
enum ovsdb_atomic_type type;
struct json *json;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_atomic_type_from_json(, json));
json_destroy(json);
@@ -574,6 +591,8 @@ do_parse_base_type(struct ovs_cmdl_context *ctx)
struct ovsdb_base_type base;
struct json *json;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_base_type_from_json(, json));
json_destroy(json);
@@ -587,6 +606,8 @@ do_parse_type(struct ovs_cmdl_context *ctx)
struct ovsdb_type type;
struct json *json;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_type_from_json(, json));
json_destroy(json);
@@ -601,6 +622,8 @@ do_parse_atoms(struct ovs_cmdl_context *ctx)
struct json *json;
int i;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_base_type_from_json(, json));
json_destroy(json);
@@ -630,6 +653,8 @@ do_parse_atom_strings(struct ovs_cmdl_context *ctx)
struct json *json;
int i;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_base_type_from_json(, json));
json_destroy(json);
@@ -675,6 +700,8 @@ do_parse_data__(int argc, char *argv[],
struct json *json;
int i;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(argv[1]));
check_ovsdb_error(ovsdb_type_from_json(, json));
json_destroy(json);
@@ -706,6 +733,8 @@ do_parse_data_strings(struct ovs_cmdl_context *ctx)
struct json *json;
int i;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
check_ovsdb_error(ovsdb_type_from_json(, json));
json_destroy(json);
@@ -746,6 +775,8 @@ do_sort_atoms(struct ovs_cmdl_context *ctx)
size_t n_atoms;
int i;
+destroy_on_ovsdb_error();
+
json = unbox_json(parse_json(ctx->argv[1]));
Re: [ovs-dev] [PATCH 4/4] debian: Fix incorrect linkage of the python C extension.
On Thu, Jul 28, 2022 at 5:00 PM Frode Nordahl wrote: > > > This is only a suggestion to avoid adding too many -Wl options as they > > > stack up if you are not careful. > > > > I see. Though, it is probably not a big concern for this particular > > use-case. > > > > I can replace '-Wl,-Bstatic -lopenvswitch -Wl,-Bdynamic' with > > '-l:libopenvswitch.a' and send a new version, or replace on commit, > > if you think that will be better. I don't have a strong preference. > > > > What do you think? > > > > Frode, do you have any comments on this? > > David, thank you for sharing the `-l:` syntax, I was not aware of that. > > I have no strong opinions about what to use, if it helps to tip the > scale, I have to admit in this specific use-case where we only have > two of these, the longer syntax appears more clear on the tin to see > what's going on. We could switch to `-l:` if we need to add more of > these for example? Ok for me, so let's go with the current form. The whole series lgtm. With the nit on patch 2 commitlog, for the series, Reviewed-by: David Marchand -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/4] debian: Fix incorrect linkage of the python C extension.
On Thu, Jul 28, 2022 at 3:17 PM Ilya Maximets wrote: > > On 7/27/22 23:02, David Marchand wrote: > > On Mon, Jul 25, 2022 at 2:29 PM Ilya Maximets wrote: > >> > >> Current version of debian/rules simply passes the libopenvswitch.a > >> as a command line argument via LDFLAGS, but that doesn't actually > >> lead to this library being statically linked into python extension, > >> which is a shared library. Instead, the build "succeeds", but the > >> resulted extension is not usable, because most of the symbols are > >> missing: > >> > >> from ovs import _json > >> > >> ImportError: > >> > >> /usr/lib/python3/dist-packages/ovs/_json.cpython-310-x86_64-linux-gnu.so: > >> undefined symbol: json_parser_finish > >> > >> '-lopenvswitch' with a path to a static library should be passed > >> instead to make it actually statically linked. But even that is not > >> enough as all the libraries that libopenvswitch.a was built with also > >> has to be passed. Otherwise, we'll have unresolved symbols like ssl, > >> cap-ng, etc. > >> > >> The most convenient way to get all the required libraries and cflags > >> seems to be by using pkg-config. > >> > >> Setting several environment variables for pkg-config, so it can find > >> the libopenvswitch.pc in non-standard directory, not skip default > >> locations and also report them with the right base directory. > >> > >> Extra '-Wl,-Bstatic -lopenvswitch -Wl,-Bdynamic' is added before all > >> the libs to ensure static linking of libopenvswitch even if the > >> dynamic library is available in a system. > > > > Did you consider the simple :filename form to tell ld to link an archive? > > Here that would translate to a simple -l:libopenvswitch.a > > Yes, I looked into that. There shouldn't be any difference for this > particular use case. -l:filename syntax is supported since binutils 2.18 > that was released about 11 years ago, not sure if that's a priblem or > not. It's certainly not a problem for current debian packaging, since it > will likely not work on systems that old anyway. > > > > > This is only a suggestion to avoid adding too many -Wl options as they > > stack up if you are not careful. > > I see. Though, it is probably not a big concern for this particular use-case. > > I can replace '-Wl,-Bstatic -lopenvswitch -Wl,-Bdynamic' with > '-l:libopenvswitch.a' and send a new version, or replace on commit, > if you think that will be better. I don't have a strong preference. > > What do you think? > > Frode, do you have any comments on this? David, thank you for sharing the `-l:` syntax, I was not aware of that. I have no strong opinions about what to use, if it helps to tip the scale, I have to admit in this specific use-case where we only have two of these, the longer syntax appears more clear on the tin to see what's going on. We could switch to `-l:` if we need to add more of these for example? -- Frode Nordahl > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] controller: physical: fix regression for container ports
Oops, sorry about that. This should be backported to 22.06.
Acked-By: Ihar Hrachyshka wrote:
>
> After commit 'd07e5f99d ("Introduce match_outport_dp_and_port_keys
> in physical.c")' it is not longer possible to ping a parent port from
> container one.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2105901
> Fixes: d07e5f99d ("Introduce match_outport_dp_and_port_keys in physical.c")
> Signed-off-by: Lorenzo Bianconi
> ---
> controller/physical.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 816a557e7..a92534a03 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -929,7 +929,7 @@ put_local_common_flows(uint32_t dp_key,
> * unnecessary.
> */
> ofpbuf_clear(ofpacts_p);
> -match_outport_dp_and_port_keys(, dp_key, port_key);
> +match_outport_dp_and_port_keys(, dp_key,
> parent_pb->tunnel_key);
> match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0,
> MLF_NESTED_CONTAINER, MLF_NESTED_CONTAINER);
>
> --
> 2.37.1
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] controller: physical: fix regression for container ports
After commit 'd07e5f99d ("Introduce match_outport_dp_and_port_keys
in physical.c")' it is not longer possible to ping a parent port from
container one.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2105901
Fixes: d07e5f99d ("Introduce match_outport_dp_and_port_keys in physical.c")
Signed-off-by: Lorenzo Bianconi
---
controller/physical.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/controller/physical.c b/controller/physical.c
index 816a557e7..a92534a03 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -929,7 +929,7 @@ put_local_common_flows(uint32_t dp_key,
* unnecessary.
*/
ofpbuf_clear(ofpacts_p);
-match_outport_dp_and_port_keys(, dp_key, port_key);
+match_outport_dp_and_port_keys(, dp_key, parent_pb->tunnel_key);
match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0,
MLF_NESTED_CONTAINER, MLF_NESTED_CONTAINER);
--
2.37.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/4] debian: Fix incorrect linkage of the python C extension.
On 7/27/22 23:02, David Marchand wrote: > On Mon, Jul 25, 2022 at 2:29 PM Ilya Maximets wrote: >> >> Current version of debian/rules simply passes the libopenvswitch.a >> as a command line argument via LDFLAGS, but that doesn't actually >> lead to this library being statically linked into python extension, >> which is a shared library. Instead, the build "succeeds", but the >> resulted extension is not usable, because most of the symbols are >> missing: >> >> from ovs import _json >> >> ImportError: >> /usr/lib/python3/dist-packages/ovs/_json.cpython-310-x86_64-linux-gnu.so: >> undefined symbol: json_parser_finish >> >> '-lopenvswitch' with a path to a static library should be passed >> instead to make it actually statically linked. But even that is not >> enough as all the libraries that libopenvswitch.a was built with also >> has to be passed. Otherwise, we'll have unresolved symbols like ssl, >> cap-ng, etc. >> >> The most convenient way to get all the required libraries and cflags >> seems to be by using pkg-config. >> >> Setting several environment variables for pkg-config, so it can find >> the libopenvswitch.pc in non-standard directory, not skip default >> locations and also report them with the right base directory. >> >> Extra '-Wl,-Bstatic -lopenvswitch -Wl,-Bdynamic' is added before all >> the libs to ensure static linking of libopenvswitch even if the >> dynamic library is available in a system. > > Did you consider the simple :filename form to tell ld to link an archive? > Here that would translate to a simple -l:libopenvswitch.a Yes, I looked into that. There shouldn't be any difference for this particular use case. -l:filename syntax is supported since binutils 2.18 that was released about 11 years ago, not sure if that's a priblem or not. It's certainly not a problem for current debian packaging, since it will likely not work on systems that old anyway. > > This is only a suggestion to avoid adding too many -Wl options as they > stack up if you are not careful. I see. Though, it is probably not a big concern for this particular use-case. I can replace '-Wl,-Bstatic -lopenvswitch -Wl,-Bdynamic' with '-l:libopenvswitch.a' and send a new version, or replace on commit, if you think that will be better. I don't have a strong preference. What do you think? Frode, do you have any comments on this? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] revalidator: add a USDT probe after evaluation when flows are deleted.
On 28 Jul 2022, at 13:57, Eelco Chaudron wrote:
> On 27 Jul 2022, at 16:48, Kevin Sprague wrote:
>
>> 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
>> revaldiator 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/filter_probe.py) that
>> serves
>> as a demonstration of how the new USDT probe might be used going forward.
>>
>> Signed-off-by: Kevin Sprague
>
> See comments below, but it's not a full review, as unfortunately, I was
> running out of time...
>
> //Eelco
>
>> ---
>> Documentation/topics/usdt-probes.rst | 21 +
>> ofproto/ofproto-dpif-upcall.c | 40 +-
>> utilities/automake.mk | 2 +
>> utilities/usdt-scripts/filter_probe.py | 549 +
>> 4 files changed, 606 insertions(+), 6 deletions(-)
>> create mode 100755 utilities/usdt-scripts/filter_probe.py
>>
>> diff --git a/Documentation/topics/usdt-probes.rst
>> b/Documentation/topics/usdt-probes.rst
>> index 7ce19aaed..a977dc006 100644
>> --- a/Documentation/topics/usdt-probes.rst
>> +++ b/Documentation/topics/usdt-probes.rst
>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
>> - dpif_recv:recv_upcall
>> - main:poll_block
>> - main:run_start
>> +- revalidate:flow_result
>>
>>
>> dpif_netlink_operate\_\_:op_flow_del
>> @@ -294,6 +295,7 @@ DPIF_OP_FLOW_PUT operation as part of the dpif
>> ``operate()`` callback.
>>
>> **Script references**:
>>
>> +- ``utilities/usdt-scripts/filter_probe.py``
>> - ``utilities/usdt-scripts/upcall_cost.py``
>>
>>
>> @@ -357,6 +359,25 @@ See also the ``main:run_start`` probe above.
>>
>> - ``utilities/usdt-scripts/bridge_loop.bt``
>
> Need extra CR/LF here.
>>
>> +probe revalidate:flow_result
>> +~
>> +
>> +**Description**:
>> +This probe is triggered when the revalidator decides whether or not to
>> +revalidate a flow. ``reason`` is an enum that denotes that either the flow
>> +is being kept, or the reason why the flow is being deleted. The
>> +``filter_probe.py`` script uses this probe to notify users when flows
>> +matching user-provided criteria are deleted.
>> +
>> +**Arguments**:
>> +
>> +- *arg0*: ``(enum flow_del_reason) reason``
>> +- *arg1*: ``(struct udpif *) udpif``
>> +- *arg2*: ``(struct udpif_key *) ukey``
>> +
>> +**Script references**:
>> +
>> +- ``utilities/usdt-scripts/filter_probe.py``
>>
>> Adding your own probes
>> --
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 57f94df54..efee8c0a4 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -31,6 +31,7 @@
>> #include "openvswitch/list.h"
>> #include "netlink.h"
>> #include "openvswitch/ofpbuf.h"
>> +#include "openvswitch/usdt-probes.h"
>> #include "ofproto-dpif-ipfix.h"
>> #include "ofproto-dpif-sflow.h"
>> #include "ofproto-dpif-xlate.h"
>> @@ -260,6 +261,17 @@ enum ukey_state {
>> };
>> #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +enum flow_del_reason {
>> +FDR_FLOW_LIVE = 0,
>> +FDR_FLOW_TIME_OUT, /* the flow went unused and was deleted. */
>
> Start with a capital T.
>
>> +FDR_TOO_EXPENSIVE,
>> +FDR_FLOW_WILDCARDED,
>> +FDR_BAD_ODP_FIT,
>> +FDR_ASSOCIATED_OFPROTO,
>> +FDR_XLATION_ERROR,
>> +FDR_AVOID_CACHING,
>
> I think it would make sense to add a description to each delete reason so
> that when people use the USDT script, they can easily understand why the flow
> got deleted.
>
>> +};
>> +
>> /* 'udpif_key's are responsible for tracking the little bit of state udpif
>> * needs to do flow expiration which can't be pulled directly from the
>> * datapath. They may be created by any handler or revalidator thread at
>> any
>> @@ -2202,7 +2214,8 @@ populate_xcache(struct udpif *udpif, struct udpif_key
>> *ukey,
>> static enum reval_result
>> revalidate_ukey__(struct udpif
Re: [ovs-dev] [PATCH v2] revalidator: add a USDT probe after evaluation when flows are deleted.
On 27 Jul 2022, at 16:48, Kevin Sprague wrote:
> 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
> revaldiator 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/filter_probe.py) that serves
> as a demonstration of how the new USDT probe might be used going forward.
>
> Signed-off-by: Kevin Sprague
See comments below, but it's not a full review, as unfortunately, I was running
out of time...
//Eelco
> ---
> Documentation/topics/usdt-probes.rst | 21 +
> ofproto/ofproto-dpif-upcall.c | 40 +-
> utilities/automake.mk | 2 +
> utilities/usdt-scripts/filter_probe.py | 549 +
> 4 files changed, 606 insertions(+), 6 deletions(-)
> create mode 100755 utilities/usdt-scripts/filter_probe.py
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index 7ce19aaed..a977dc006 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
> - dpif_recv:recv_upcall
> - main:poll_block
> - main:run_start
> +- revalidate:flow_result
>
>
> dpif_netlink_operate\_\_:op_flow_del
> @@ -294,6 +295,7 @@ DPIF_OP_FLOW_PUT operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> +- ``utilities/usdt-scripts/filter_probe.py``
> - ``utilities/usdt-scripts/upcall_cost.py``
>
>
> @@ -357,6 +359,25 @@ See also the ``main:run_start`` probe above.
>
> - ``utilities/usdt-scripts/bridge_loop.bt``
Need extra CR/LF here.
>
> +probe revalidate:flow_result
> +~
> +
> +**Description**:
> +This probe is triggered when the revalidator decides whether or not to
> +revalidate a flow. ``reason`` is an enum that denotes that either the flow
> +is being kept, or the reason why the flow is being deleted. The
> +``filter_probe.py`` script uses this probe to notify users when flows
> +matching user-provided criteria are deleted.
> +
> +**Arguments**:
> +
> +- *arg0*: ``(enum flow_del_reason) reason``
> +- *arg1*: ``(struct udpif *) udpif``
> +- *arg2*: ``(struct udpif_key *) ukey``
> +
> +**Script references**:
> +
> +- ``utilities/usdt-scripts/filter_probe.py``
>
> Adding your own probes
> --
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 57f94df54..efee8c0a4 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -31,6 +31,7 @@
> #include "openvswitch/list.h"
> #include "netlink.h"
> #include "openvswitch/ofpbuf.h"
> +#include "openvswitch/usdt-probes.h"
> #include "ofproto-dpif-ipfix.h"
> #include "ofproto-dpif-sflow.h"
> #include "ofproto-dpif-xlate.h"
> @@ -260,6 +261,17 @@ enum ukey_state {
> };
> #define N_UKEY_STATES (UKEY_DELETED + 1)
>
> +enum flow_del_reason {
> +FDR_FLOW_LIVE = 0,
> +FDR_FLOW_TIME_OUT, /* the flow went unused and was deleted. */
Start with a capital T.
> +FDR_TOO_EXPENSIVE,
> +FDR_FLOW_WILDCARDED,
> +FDR_BAD_ODP_FIT,
> +FDR_ASSOCIATED_OFPROTO,
> +FDR_XLATION_ERROR,
> +FDR_AVOID_CACHING,
I think it would make sense to add a description to each delete reason so that
when people use the USDT script, they can easily understand why the flow got
deleted.
> +};
> +
> /* 'udpif_key's are responsible for tracking the little bit of state udpif
> * needs to do flow expiration which can't be pulled directly from the
> * datapath. They may be created by any handler or revalidator thread at any
> @@ -2202,7 +2214,8 @@ populate_xcache(struct udpif *udpif, struct udpif_key
> *ukey,
> static enum reval_result
> revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey,
>uint16_t tcp_flags, struct ofpbuf *odp_actions,
> - struct recirc_refs *recircs, struct xlate_cache *xcache)
> +
[ovs-dev] [PATCH RFC] ofproto-dpif-xlate: Optimize the clone for patch ports
When the packet was traveling through patch port boundary
OvS would check if any of the actions is reversible,
if not it would clone the packet. However, the check
was only at the first level of the second bridge.
That caused some issues when the packet had gone
through more actions, some of them might have been
irreversible.
To have all the information add context that will track
if we have hit any irreversible action. At the end
we can wrap the irreversible patch port traverse
in clone.
Signed-off-by: Ales Musil
---
include/openvswitch/ofp-actions.h | 69 +++
lib/netlink.c | 28 +
lib/netlink.h | 2 +
ofproto/ofproto-dpif-xlate.c | 200 --
tests/ofproto-dpif.at | 38 +-
5 files changed, 272 insertions(+), 65 deletions(-)
diff --git a/include/openvswitch/ofp-actions.h
b/include/openvswitch/ofp-actions.h
index 7b57e49ad..1a09f751f 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -251,6 +251,75 @@ ofpact_find_type_flattened(const struct ofpact *a, enum
ofpact_type type,
return NULL;
}
+static inline bool
+ofpact_is_reversible(const struct ofpact *a)
+{
+switch (a->type) {
+case OFPACT_CT:
+case OFPACT_METER:
+case OFPACT_NAT:
+case OFPACT_OUTPUT_TRUNC:
+case OFPACT_ENCAP:
+case OFPACT_DECAP:
+case OFPACT_DEC_NSH_TTL:
+return false;
+case OFPACT_BUNDLE:
+case OFPACT_CLEAR_ACTIONS:
+case OFPACT_CLONE:
+case OFPACT_CONJUNCTION:
+case OFPACT_CONTROLLER:
+case OFPACT_CT_CLEAR:
+case OFPACT_DEBUG_RECIRC:
+case OFPACT_DEBUG_SLOW:
+case OFPACT_DEC_MPLS_TTL:
+case OFPACT_DEC_TTL:
+case OFPACT_ENQUEUE:
+case OFPACT_EXIT:
+case OFPACT_FIN_TIMEOUT:
+case OFPACT_GOTO_TABLE:
+case OFPACT_GROUP:
+case OFPACT_LEARN:
+case OFPACT_MULTIPATH:
+case OFPACT_NOTE:
+case OFPACT_OUTPUT:
+case OFPACT_OUTPUT_REG:
+case OFPACT_POP_MPLS:
+case OFPACT_POP_QUEUE:
+case OFPACT_PUSH_MPLS:
+case OFPACT_PUSH_VLAN:
+case OFPACT_REG_MOVE:
+case OFPACT_RESUBMIT:
+case OFPACT_SAMPLE:
+case OFPACT_SET_ETH_DST:
+case OFPACT_SET_ETH_SRC:
+case OFPACT_SET_FIELD:
+case OFPACT_SET_IP_DSCP:
+case OFPACT_SET_IP_ECN:
+case OFPACT_SET_IP_TTL:
+case OFPACT_SET_IPV4_DST:
+case OFPACT_SET_IPV4_SRC:
+case OFPACT_SET_L4_DST_PORT:
+case OFPACT_SET_L4_SRC_PORT:
+case OFPACT_SET_MPLS_LABEL:
+case OFPACT_SET_MPLS_TC:
+case OFPACT_SET_MPLS_TTL:
+case OFPACT_SET_QUEUE:
+case OFPACT_SET_TUNNEL:
+case OFPACT_SET_VLAN_PCP:
+case OFPACT_SET_VLAN_VID:
+case OFPACT_STACK_POP:
+case OFPACT_STACK_PUSH:
+case OFPACT_STRIP_VLAN:
+case OFPACT_UNROLL_XLATE:
+case OFPACT_WRITE_ACTIONS:
+case OFPACT_WRITE_METADATA:
+case OFPACT_CHECK_PKT_LARGER:
+case OFPACT_DELETE_FIELD:
+default:
+return true;
+}
+}
+
#define OFPACT_FIND_TYPE_FLATTENED(A, TYPE, END) \
ofpact_get_##TYPE##_nullable( \
ofpact_find_type_flattened(A, OFPACT_##TYPE, END))
diff --git a/lib/netlink.c b/lib/netlink.c
index 6215282d6..8bcf56953 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -981,3 +981,31 @@ nl_attr_find_nested(const struct nlattr *nla, uint16_t
type)
{
return nl_attr_find__(nl_attr_get(nla), nl_attr_get_size(nla), type);
}
+
+/* Wraps existing Netlink attributes with their data into Netlink attribute
+ * making them nested. The offset species where the Netlink attributes start
+ * within the buffer. The tailing data are moved further to make space for the
+ * nested header. */
+void
+nl_msg_wrap_unspec(struct ofpbuf *buf, uint16_t type, uint32_t offset,
+ size_t size)
+{
+nl_msg_reserve(buf, NLA_HDRLEN);
+
+uint8_t *src = (uint8_t *) buf->data + offset;
+uint8_t *dst = src + NLA_HDRLEN;
+uint32_t tail_data_len = buf->size - offset;
+
+memmove(dst, src, tail_data_len);
+
+/* Reset size so we can add header. */
+nl_msg_reset_size(buf, offset);
+uint32_t nested_offset = nl_msg_start_nested(buf, type);
+
+/* Move past the size of nested data and end the nested header. */
+nl_msg_reset_size(buf, buf->size + size);
+nl_msg_end_non_empty_nested(buf, nested_offset);
+
+/* Move the buffer back to the end after all data. */
+nl_msg_reset_size(buf, buf->size + (tail_data_len - size));
+}
diff --git a/lib/netlink.h b/lib/netlink.h
index e9050c31b..6a6ce20c3 100644
--- a/lib/netlink.h
+++ b/lib/netlink.h
@@ -248,5 +248,7 @@ const struct nlattr *nl_attr_find(const struct ofpbuf *,
size_t hdr_len,
const struct nlattr
Re: [ovs-dev] [PATCH ovn] tests: enable and fix vif-provider test scenario
Hello, Ihar,
On Thu, Jul 28, 2022 at 1:28 AM Ihar Hrachyshka wrote:
>
> Signed-off-by: Ihar Hrachyshka
> ---
> controller/test-vif-plug.c | 2 +-
> tests/ovn-vif-plug.at | 2 +-
> tests/testsuite.at | 1 +
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/controller/test-vif-plug.c b/controller/test-vif-plug.c
> index 01ff37d8f..d7094199a 100644
> --- a/controller/test-vif-plug.c
> +++ b/controller/test-vif-plug.c
> @@ -36,7 +36,7 @@ test_vif_plug(struct ovs_cmdl_context *ctx OVS_UNUSED)
> ovs_assert(
> sset_contains(
> vif_plug_get_maintained_iface_options(vif_plug_class),
> -"plug-dummy-option"));
> +"vif-plug-dummy-option"));
>
> struct vif_plug_port_ctx_in ctx_in = {
> .op_type = PLUG_OP_CREATE,
> diff --git a/tests/ovn-vif-plug.at b/tests/ovn-vif-plug.at
> index 86b0b4b84..d4c225e90 100644
> --- a/tests/ovn-vif-plug.at
> +++ b/tests/ovn-vif-plug.at
> @@ -4,5 +4,5 @@
> AT_BANNER([OVN unit tests - vif-plug])
>
> AT_SETUP([unit test -- plugging infrastructure tests])
> -AT_CHECK([ovstest test-plug run], [0], [])
> +AT_CHECK([ovstest test-vif-plug run], [0], [])
> AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 479e786bd..d3f00e1bf 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -39,3 +39,4 @@ m4_include([tests/ovn-controller-vtep.at])
> m4_include([tests/ovn-ic.at])
> m4_include([tests/checkpatch.at])
> m4_include([tests/ovn-ipsec.at])
> +m4_include([tests/ovn-vif-plug.at])
> --
> 2.34.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Looks like the unit tests were never enabled, thank you for catching that!
I would reword the subject/commit message to use the words "unit test"
instead of test scenario, as when you say test scenario I think about
the functional test named "ovn-controller - VIF plugging" located in
tests/ovn.at:31396-31515.
With a updated commit message:
Acked-by: Frode Nordahl
--
Frode Nordahl
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
